Sling
  1. Sling
  2. SLING-2411

ResourceDecorator(Resource, HttpServletRequest) doesn't get called consistently

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: JCR Resource 2.1.0, API 2.3.0
    • Component/s: API, JCR
    • Labels:
      None

      Description

      The ResourceDecorator currently has two methods:
      decorate(Resource)
      decorate(Resource, HttpServletRequest)

      The JcrResourceResolver uses the latter method when resolve(HttpServletRequest,String) is invoked. In all other cases, the former method is used. This behavior is correct.

      However, the JcrResourceResolver (and any future ResourceResolver implementation for that matter) really doesn't have any control over how it is invoked. For example, at present <sling:include> and <sling:forward> call resolve(String), not resolve(HttpServletRequest,String) (see http://s.apache.org/lL5). But even if we did a code audit within Sling and fixed all the cases like this, we don't have any control over downstream applications which may or may not invoke the two-argument resolve() method.

        Activity

        Gavin made changes -
        Workflow re-open possible,doc-test-required [ 12788454 ] no-reopen-closed,doc-test-required [ 12792977 ]
        Gavin made changes -
        Workflow no-reopen-closed,doc-test-required [ 12766935 ] re-open possible,doc-test-required [ 12788454 ]
        Gavin made changes -
        Workflow Copy of no-reopen-closed,doc-test-required [ 12762794 ] no-reopen-closed,doc-test-required [ 12766935 ]
        Gavin made changes -
        Workflow no-reopen-closed,doc-test-required [ 12652704 ] Copy of no-reopen-closed,doc-test-required [ 12762794 ]
        Carsten Ziegeler made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Justin Edelson made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Justin Edelson added a comment -

        deprecation done in r1245269

        Show
        Justin Edelson added a comment - deprecation done in r1245269
        Justin Edelson made changes -
        Component/s API [ 12311944 ]
        Component/s JCR [ 12311946 ]
        Justin Edelson made changes -
        Assignee Justin Edelson [ justinedelson ]
        Fix Version/s JCR Resource 2.1.0 [ 12316202 ]
        Fix Version/s API 2.2.6 [ 12319514 ]
        Hide
        Justin Edelson added a comment -

        setting fix versions

        Show
        Justin Edelson added a comment - setting fix versions
        Hide
        Justin Edelson added a comment -

        Vote to deprecate has passed. See thread here: http://sling.markmail.org/thread/fozcoxaylos72c2w

        Show
        Justin Edelson added a comment - Vote to deprecate has passed. See thread here: http://sling.markmail.org/thread/fozcoxaylos72c2w
        Hide
        Julian Sedding added a comment -

        Please don't let the many lines of new code distract you. They are mainly generic implementations of ResourceResolverWrapper and IteratorWrapper. The "real" changes are the addition of RequestScopedResourceResolverWrapper and changes in SlingMainServlet and ResourceDecoratorTracker.

        Show
        Julian Sedding added a comment - Please don't let the many lines of new code distract you. They are mainly generic implementations of ResourceResolverWrapper and IteratorWrapper. The "real" changes are the addition of RequestScopedResourceResolverWrapper and changes in SlingMainServlet and ResourceDecoratorTracker.
        Julian Sedding made changes -
        Field Original Value New Value
        Attachment SLING-2411-jsedding.patch [ 12514269 ]
        Hide
        Julian Sedding added a comment -

        I attached a patch to illustrate what I have in mind. Currently, I am not 100% satisfied with the patch, but maybe someone has a good idea, how to address the following issues.

        1. The RequestScopedResourceResolverWrapper class needs to be exported from the bundle, if it should be available for use by e.g. the ResourceDecoratorTracker. Should it be in the API bundle or exported by the engine bundle (that's how it's done in the patch)? My gut feeling is that the ResourceDecorator(Tracker) does not actually belong into the jcr.resources bundle. This should become more evident when the ResourceResolver is decoupled from the JCR-ResourceProvider (see SLING-2396).

        2. I'm not certain whether it can be dangerous/undesired that RequestScopedResourceResolverWrapper redirects map(String) to map(Request, String).

        Eventually, if we decide to go down this route, it may make sense to allow registering ResourceResolverDecorator services, in a similar way to the current ResourceDecorator services. Curiously, I belive that the ResourceDecorator mechanism could be fairly easily implemented as a ResourceResolverDecorator.

        Show
        Julian Sedding added a comment - I attached a patch to illustrate what I have in mind. Currently, I am not 100% satisfied with the patch, but maybe someone has a good idea, how to address the following issues. 1. The RequestScopedResourceResolverWrapper class needs to be exported from the bundle, if it should be available for use by e.g. the ResourceDecoratorTracker. Should it be in the API bundle or exported by the engine bundle (that's how it's done in the patch)? My gut feeling is that the ResourceDecorator(Tracker) does not actually belong into the jcr.resources bundle. This should become more evident when the ResourceResolver is decoupled from the JCR-ResourceProvider (see SLING-2396 ). 2. I'm not certain whether it can be dangerous/undesired that RequestScopedResourceResolverWrapper redirects map(String) to map(Request, String). Eventually, if we decide to go down this route, it may make sense to allow registering ResourceResolverDecorator services, in a similar way to the current ResourceDecorator services. Curiously, I belive that the ResourceDecorator mechanism could be fairly easily implemented as a ResourceResolverDecorator.
        Hide
        Justin Edelson added a comment -

        Sorry for any confusion I may have caused. I had intended to send out a VOTE email to the dev list on deprecating the two-argument method following the creation of this issue, but it looks like I never hit send.

        The patch is just what I said above - a strawman. IF we are not going to deprecate the method, then I think we need to do something like the strawman. Julian's proposal sounds interesting and I'm curious to see it.

        Show
        Justin Edelson added a comment - Sorry for any confusion I may have caused. I had intended to send out a VOTE email to the dev list on deprecating the two-argument method following the creation of this issue, but it looks like I never hit send. The patch is just what I said above - a strawman. IF we are not going to deprecate the method, then I think we need to do something like the strawman. Julian's proposal sounds interesting and I'm curious to see it.
        Hide
        Felix Meschberger added a comment -

        > Yes, as I said, it was just a quick thought - but then using a thread local inside the resolver doesn't look right either

        Oh ! Sorry. I didn't fully read this issue and proposal. No, I don't like this thread local implementation proposal either.

        Considering this and considering the request is not always available, I think I favor an approach already mentioned: We deprecate the method with the request argument (and might even define that null is always passed in). If a decorator then requires access to the request, it is the task of that decorate to provide the request, for example in a thread local provided by a filter.

        Or Sling provides such a (component level) filter which provides service API to get the current thread's request, response, and resource. This can be implemented in a separate bundle, is not intrusive in existing code and would provide actual data for all.

        Show
        Felix Meschberger added a comment - > Yes, as I said, it was just a quick thought - but then using a thread local inside the resolver doesn't look right either Oh ! Sorry. I didn't fully read this issue and proposal. No, I don't like this thread local implementation proposal either. Considering this and considering the request is not always available, I think I favor an approach already mentioned: We deprecate the method with the request argument (and might even define that null is always passed in). If a decorator then requires access to the request, it is the task of that decorate to provide the request, for example in a thread local provided by a filter. Or Sling provides such a (component level) filter which provides service API to get the current thread's request, response, and resource. This can be implemented in a separate bundle, is not intrusive in existing code and would provide actual data for all.
        Hide
        Carsten Ziegeler added a comment -

        > A ResourceResolver is not bound to a request and thus using it for the factory sounds wrong.
        Yes, as I said, it was just a quick thought - but then using a thread local inside the resolver doesn't look right either

        Show
        Carsten Ziegeler added a comment - > A ResourceResolver is not bound to a request and thus using it for the factory sounds wrong. Yes, as I said, it was just a quick thought - but then using a thread local inside the resolver doesn't look right either
        Hide
        Julian Sedding added a comment - - edited

        > A ResourceResolver is not bound to a request and thus using it for the factory sounds wrong.
        I agree with this statement. Taking this a step further, maybe it would be worthwhile to factor out the "resolve" and "map" methods into a different service (something Vidar may already be doing in his work to decouple JCR from the ResourceResolverFactory). The methods "resolve" and "map" seem to be inherently about requests: they provide the plumbing to get from the "external" address (i.e. URI) of a resource to the "internal" address (i.e. location in Sling's resource tree). The ResourceResolver javadocs suggest this as well, regarding "resolve": "This kind of method is intended to resolve request URLs to resources."

        Actually, this may be a topic for the list.

        While the changes are fairly minimal in the patch, I don't like the approach. I believe the ThreadLocals shouldn't be necessary. Rather, I considered wrapping the ResourceResolver, so it can piggyback the request (of course this is optional and not applicable to e.g. an administrative resource resolver). The ResourceDecoratorTracker could then check if the ResourceResolver is an instance of the respective wrapper, cast it and get the piggybacked request. The natural place for this wrapping to happen would be in the SlingMainServlet, just before the call to processRequest.

        On the other hand, I'm not too keen to put more and more functionality into the SlingMainServlet. So an alternative might be to wrap the Request (e.g. using a Filter) and let the Request#getResourceResolver return a wrapped ResourceResolver as explained above. I'm not yet sure how feasible this approach is, because ideally the ResourceResolver would be wrapped before the RequestData#initResource is called.

        I've started working on a patch and hope to get this finished at the weekend. I'm interested in reading your thoughts.

        Show
        Julian Sedding added a comment - - edited > A ResourceResolver is not bound to a request and thus using it for the factory sounds wrong. I agree with this statement. Taking this a step further, maybe it would be worthwhile to factor out the "resolve" and "map" methods into a different service (something Vidar may already be doing in his work to decouple JCR from the ResourceResolverFactory). The methods "resolve" and "map" seem to be inherently about requests: they provide the plumbing to get from the "external" address (i.e. URI) of a resource to the "internal" address (i.e. location in Sling's resource tree). The ResourceResolver javadocs suggest this as well, regarding "resolve": "This kind of method is intended to resolve request URLs to resources." Actually, this may be a topic for the list. While the changes are fairly minimal in the patch, I don't like the approach. I believe the ThreadLocals shouldn't be necessary. Rather, I considered wrapping the ResourceResolver, so it can piggyback the request (of course this is optional and not applicable to e.g. an administrative resource resolver). The ResourceDecoratorTracker could then check if the ResourceResolver is an instance of the respective wrapper, cast it and get the piggybacked request. The natural place for this wrapping to happen would be in the SlingMainServlet, just before the call to processRequest. On the other hand, I'm not too keen to put more and more functionality into the SlingMainServlet. So an alternative might be to wrap the Request (e.g. using a Filter) and let the Request#getResourceResolver return a wrapped ResourceResolver as explained above. I'm not yet sure how feasible this approach is, because ideally the ResourceResolver would be wrapped before the RequestData#initResource is called. I've started working on a patch and hope to get this finished at the weekend. I'm interested in reading your thoughts.
        Hide
        Felix Meschberger added a comment -

        A ResourceResolver is not bound to a request and thus using it for the factory sounds wrong.

        Show
        Felix Meschberger added a comment - A ResourceResolver is not bound to a request and thus using it for the factory sounds wrong.
        Hide
        Carsten Ziegeler added a comment -

        Just a thought, what about passing the request into the resource resolver factory when it is created? The created resolver can then just keep the request object

        Using a thread local has the potential surprise that all resource resolvers running in that thread are using the request object, for example long running admin resolvers from a used service etc. This can be wanted or unexpected. Not sure actually

        Show
        Carsten Ziegeler added a comment - Just a thought, what about passing the request into the resource resolver factory when it is created? The created resolver can then just keep the request object Using a thread local has the potential surprise that all resource resolvers running in that thread are using the request object, for example long running admin resolvers from a used service etc. This can be wanted or unexpected. Not sure actually
        Hide
        Justin Edelson added a comment -

        strawman patch here: http://codereview.appspot.com/5653043

        creates a ThreadLocal to store the request and then the ResourceDecoratorTracker tries to use this if necessary.

        Show
        Justin Edelson added a comment - strawman patch here: http://codereview.appspot.com/5653043 creates a ThreadLocal to store the request and then the ResourceDecoratorTracker tries to use this if necessary.
        Justin Edelson created issue -

          People

          • Assignee:
            Justin Edelson
            Reporter:
            Justin Edelson
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development