Uploaded image for project: 'Sling'
  1. Sling
  2. SLING-7792

Resource Resolver should return more than one resolved path if available

Details

    Description

      The current ResourceResolver#map methods return a single "path mapped from the (resource) path". However, it is possible than a given path can be mapped to multiple others while using features such as sling:alias and sling:vanityUrl.

      In order to support that scenario, it is require to implement new maps method for ResourceResolver which returns a collection of "resolved path". This collection must contain the resources mapped through /etc/map, sling:alias and sling:vanityUrl.

      The current API suggests to implement a second method to be consistent/symmetric with the existing map operations

      @Nonnull java.util.Collection<java.lang.String> maps(@Nonnull java.lang.String resourcePath)
      @Nonnull java.util.Collection<java.lang.String> maps(@Nonnull javax.servlet.http.HttpServletRequest request, @Nonnull java.lang.String resourcePath)

      Attachments

        1. console.png
          21 kB
          Robert Munteanu

        Issue Links

          Activity

            How will the client code select which path to use if you resolve to multiple candidates and return just their paths?

            We probably need to see the whole picture of how you intend to use this to understand better, but if there are multiple candidates I think the resolver should provide values such as which one are favorites, or why each candidate is present in the result set, to be able to make a sensible decision about which path to actually use.

            Or maybe it's just a question of assigning priorities to the various mapping mechanisms, if their precedence is currently undefined (I' don't know if that's the case).

            bdelacretaz Bertrand Delacretaz added a comment - How will the client code select which path to use if you resolve to multiple candidates and return just their paths? We probably need to see the whole picture of how you intend to use this to understand better, but if there are multiple candidates I think the resolver should provide values such as which one are favorites, or why each candidate is present in the result set, to be able to make a sensible decision about which path to actually use. Or maybe it's just a question of assigning priorities to the various mapping mechanisms, if their precedence is currently undefined (I' don't know if that's the case).

            As this is based on an offline discussion between myself, acollign and cziegeler, I'll add some more details.

            The background is that for a given resource client code needs to know all possible variations so that they are added as requirements for an authentication handler. In case multiple mappings are found, e.g. the page has an alias but there are also matching entries under /etc/map the current ResourceResolver#map is not enough, as it only returns one entry.

            rombert Robert Munteanu added a comment - As this is based on an offline discussion between myself, acollign and cziegeler , I'll add some more details. The background is that for a given resource client code needs to know all possible variations so that they are added as requirements for an authentication handler. In case multiple mappings are found, e.g. the page has an alias but there are also matching entries under /etc/map the current ResourceResolver#map is not enough, as it only returns one entry.

            The background is that for a given resource client code needs to know all possible variations so that they are added as requirements for an authentication handler.

            Ah ok, I see the idea now, thanks for clarifying.

            Maybe getMapppingCandidates expresses this better than maps as a method name?

            bdelacretaz Bertrand Delacretaz added a comment - The background is that for a given resource client code needs to know all possible variations so that they are added as requirements for an authentication handler. Ah ok, I see the idea now, thanks for clarifying. Maybe getMapppingCandidates expresses this better than maps as a method name?

            I like that better than maps, indeed.

            rombert Robert Munteanu added a comment - I like that better than maps , indeed.
            jsedding Julian Sedding added a comment -

            I think providing this information is valuable. I'm reluctant to just add it to ResourceResolver, however. It doesn't seem like a method that would get used a lot. Maybe we could introduce a Mapper interface (better names welcome!), which can provide detailed information about mappings and resolutions. The methods ResourceResolver#map and ResourceResolver#resolve would then become mere convenience methods, but the canonical source of truth for mappings would be the Mapper.

            Background: for quite some time I have come to the conclusion that the ResourceResolver conflates two responsibilities:

            • accessing the Resource tree
            • mapping/resolving URLs to/from the Resource tree

            The current implementation shows that these ideas are conflated, which IMHO is why the mapping/resolution code is not very modular. In an ideal future, we could get to a point where mapping/resolution is pluggable (via OSGi services) and the current implementations (i.e. /etc/maps, sling:alias, sling:vanityPaths) are hooked into this extension point. The Mapper would be the service that provides a compound view of all applied mappings/resolutions.

            jsedding Julian Sedding added a comment - I think providing this information is valuable. I'm reluctant to just add it to ResourceResolver , however. It doesn't seem like a method that would get used a lot. Maybe we could introduce a Mapper interface (better names welcome!), which can provide detailed information about mappings and resolutions. The methods ResourceResolver#map and ResourceResolver#resolve would then become mere convenience methods, but the canonical source of truth for mappings would be the Mapper . Background: for quite some time I have come to the conclusion that the ResourceResolver conflates two responsibilities: accessing the Resource tree mapping/resolving URLs to/from the Resource tree The current implementation shows that these ideas are conflated, which IMHO is why the mapping/resolution code is not very modular. In an ideal future, we could get to a point where mapping/resolution is pluggable (via OSGi services) and the current implementations (i.e. /etc/maps, sling:alias, sling:vanityPaths) are hooked into this extension point. The Mapper would be the service that provides a compound view of all applied mappings/resolutions.

            Sounds good to me and I agree a separate service makes sense.
            In that case we should probably put it into a separate package which will allows us to evolve it easier

            cziegeler Carsten Ziegeler added a comment - Sounds good to me and I agree a separate service makes sense. In that case we should probably put it into a separate package which will allows us to evolve it easier

            Draft API pull request at https://github.com/apache/sling-org-apache-sling-api/pull/6 . Wanted to get the proposal out since I'll be travelling tomorrow.

            rombert Robert Munteanu added a comment - Draft API pull request at https://github.com/apache/sling-org-apache-sling-api/pull/6 . Wanted to get the proposal out since I'll be travelling tomorrow.

            Thanks rombert, I think the api for the mapping service itself looks good. If a couple of comments

            • in the javadocs it's better to reference the full qualified service if it's in another package, this way there is no "import" statement between the packages required
            • the current ResourceResolver.map() method is bound to the resource resolver and therefore to a user, which means different users might get different results. The new service does not have the notation of a user, so I assume we'll use a service user which then means that the results might be different
            • as the map() method is frequently used and due to the above reason (bound to a user), I don't think we should deprecate it
            cziegeler Carsten Ziegeler added a comment - Thanks rombert , I think the api for the mapping service itself looks good. If a couple of comments in the javadocs it's better to reference the full qualified service if it's in another package, this way there is no "import" statement between the packages required the current ResourceResolver.map() method is bound to the resource resolver and therefore to a user, which means different users might get different results. The new service does not have the notation of a user, so I assume we'll use a service user which then means that the results might be different as the map() method is frequently used and due to the above reason (bound to a user), I don't think we should deprecate it

            What about also implementing the reverse (i.e. resolveAll) which should return all potential resource candidates for a certain request. That is sometimes useful as well (compare with https://www.mail-archive.com/dev@sling.apache.org/msg56451.html).

            kwin Konrad Windszus added a comment - What about also implementing the reverse (i.e. resolveAll) which should return all potential resource candidates for a certain request. That is sometimes useful as well (compare with https://www.mail-archive.com/dev@sling.apache.org/msg56451.html ).

            I agree with Carsten about the current mapping methods being bound to a ResourceResolver, if the new ResourceMapper is standalone it might provide different results.

            Maybe the mappiig methods need a ResourceResolver parameter? Or a Request object to establish that context?

            bdelacretaz Bertrand Delacretaz added a comment - I agree with Carsten about the current mapping methods being bound to a ResourceResolver, if the new ResourceMapper is standalone it might provide different results. Maybe the mappiig methods need a ResourceResolver parameter? Or a Request object to establish that context?
            sseifert Stefan Seifert added a comment -

            how do we get a ResourceMapper instance? adapting from ResourceResolver or osgi service?
            the javadocs should give a hint how to achieve it.

            sseifert Stefan Seifert added a comment - how do we get a ResourceMapper instance? adapting from ResourceResolver or osgi service? the javadocs should give a hint how to achieve it.

            > how do we get a ResourceMapper instance? ...

            Great question, and if it's by adapting a ResourceResolver that would provide the context mentioned in my previous comment.

            bdelacretaz Bertrand Delacretaz added a comment - > how do we get a ResourceMapper instance? ... Great question, and if it's by adapting a ResourceResolver that would provide the context mentioned in my previous comment.

            I like Stefan and Bertrand's idea of adapting the ResourceResolver to a ResourceMapper, as it makes it very clear which user the mappings are based on. I will un-deprecate the map methods in ResourceResolver and instruct the user to access the ones on the mapper in case more results are needed.

            kwin - I think resolveAll would sit well in the ResourceResolver, not in a new service.

            rombert Robert Munteanu added a comment - I like Stefan and Bertrand's idea of adapting the ResourceResolver to a ResourceMapper , as it makes it very clear which user the mappings are based on. I will un-deprecate the map methods in ResourceResolver and instruct the user to access the ones on the mapper in case more results are needed. kwin - I think resolveAll would sit well in the ResourceResolver , not in a new service.

            I've updated the API PR to reflect the discussion from this Jira issue and dev@sling . I'll start working on the implementation part in parallel, but more comments are always welcome.

            rombert Robert Munteanu added a comment - I've updated the API PR to reflect the discussion from this Jira issue and dev@sling . I'll start working on the implementation part in parallel, but more comments are always welcome.

            cziegeler - we could use your opinion regarding the ResourceProvider overlay test issue, see https://lists.apache.org/thread.html/45f7055091070c378bf7e40904d5f697bcb57809fe061a6b5865802f@%3Cdev.sling.apache.org%3E .

            Other than that, I think this are working as expected now, see https://github.com/apache/sling-org-apache-sling-api/pull/6 for the final API and also https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/9 , but I want to refactor the impl a bit more as right now the ResourceResolver exposes a bit too much as the ResourceMapper needs it.

            rombert Robert Munteanu added a comment - cziegeler - we could use your opinion regarding the ResourceProvider overlay test issue, see https://lists.apache.org/thread.html/45f7055091070c378bf7e40904d5f697bcb57809fe061a6b5865802f@%3Cdev.sling.apache.org%3E . Other than that, I think this are working as expected now, see https://github.com/apache/sling-org-apache-sling-api/pull/6 for the final API and also https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/9 , but I want to refactor the impl a bit more as right now the ResourceResolver exposes a bit too much as the ResourceMapper needs it.

            I think the test is wrong - the API of the ResourceProvider states that if there is more than one provider with the same root path, the one with the highest service ranking is used.
            As both providers are registered with the same bundle id, the one registered first has the lower service id and therefore, it should one.
            I think we should change the test to explicitely use service ranking and register the second one with a higher ranking.

            cziegeler Carsten Ziegeler added a comment - I think the test is wrong - the API of the ResourceProvider states that if there is more than one provider with the same root path, the one with the highest service ranking is used. As both providers are registered with the same bundle id, the one registered first has the lower service id and therefore, it should one. I think we should change the test to explicitely use service ranking and register the second one with a higher ranking.

            Thanks for the confirmation cziegeler. For the record, sseifert@pro-vision.de already merged the changes required.

            rombert Robert Munteanu added a comment - Thanks for the confirmation cziegeler . For the record, sseifert@pro-vision.de already merged the changes required.
            rombert Robert Munteanu added a comment - Fixed, with sling-org-apache-sling-api commit 64e2a90 sling-org-apache-sling-resourceresolver commit bdfa9e3 sling-org-apache-sling-resourceresolver commit b01bea8 sling-org-apache-sling-resourceresolver commit def05b6

            Reopening, seems I did not preserve some backwards compatibility scenarios

            [ERROR] Tests run: 62, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 37.627 s <<< FAILURE! - in org.apache.sling.launchpad.webapp.integrationtest.resourceresolver.ResourceResolverGeneralTest
            [ERROR] testMapResourceAliasJcrContent(org.apache.sling.launchpad.webapp.integrationtest.resourceresolver.ResourceResolverGeneralTest)  Time elapsed: 0.591 s  <<< FAILURE!
            org.junit.ComparisonFailure: expected:</[testAlias/]_jcr_content> but was:</[]_jcr_content>
            	at org.apache.sling.launchpad.webapp.integrationtest.resourceresolver.ResourceResolverGeneralTest.testMapResourceAliasJcrContent(ResourceResolverGeneralTest.java:2360)
            
            [ERROR] testMapResourceAlias(org.apache.sling.launchpad.webapp.integrationtest.resourceresolver.ResourceResolverGeneralTest)  Time elapsed: 0.661 s  <<< FAILURE!
            org.junit.ComparisonFailure: expected:</[testAlias/]child> but was:</[]child>
            	at org.apache.sling.launchpad.webapp.integrationtest.resourceresolver.ResourceResolverGeneralTest.testMapResourceAlias(ResourceResolverGeneralTest.java:2334)
            
            rombert Robert Munteanu added a comment - Reopening, seems I did not preserve some backwards compatibility scenarios [ERROR] Tests run: 62, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 37.627 s <<< FAILURE! - in org.apache.sling.launchpad.webapp.integrationtest.resourceresolver.ResourceResolverGeneralTest [ERROR] testMapResourceAliasJcrContent(org.apache.sling.launchpad.webapp.integrationtest.resourceresolver.ResourceResolverGeneralTest) Time elapsed: 0.591 s <<< FAILURE! org.junit.ComparisonFailure: expected:</[testAlias/]_jcr_content> but was:</[]_jcr_content> at org.apache.sling.launchpad.webapp.integrationtest.resourceresolver.ResourceResolverGeneralTest.testMapResourceAliasJcrContent(ResourceResolverGeneralTest.java:2360) [ERROR] testMapResourceAlias(org.apache.sling.launchpad.webapp.integrationtest.resourceresolver.ResourceResolverGeneralTest) Time elapsed: 0.661 s <<< FAILURE! org.junit.ComparisonFailure: expected:</[testAlias/]child> but was:</[]child> at org.apache.sling.launchpad.webapp.integrationtest.resourceresolver.ResourceResolverGeneralTest.testMapResourceAlias(ResourceResolverGeneralTest.java:2334)
            rombert Robert Munteanu added a comment - Fixed edge case in sling-org-apache-sling-resourceresolver commit 9ff4836

            I've also updated the web console in sling-org-apache-sling-resourceresolver commit 69c3a3e.

            rombert Robert Munteanu added a comment - I've also updated the web console in sling-org-apache-sling-resourceresolver commit 69c3a3e .

            People

              rombert Robert Munteanu
              acollign Alex Collignon
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Time Tracking

                  Estimated:
                  Original Estimate - Not Specified
                  Not Specified
                  Remaining:
                  Remaining Estimate - 0h
                  0h
                  Logged:
                  Time Spent - 2h
                  2h