Sling
  1. Sling
  2. SLING-1596

Reduce coupling between RequestData and SlingHttpServletRequestImpl

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: Engine 2.0.6
    • Fix Version/s: None
    • Component/s: Engine
    • Labels:
      None

      Description

      As discussed in http://markmail.org/thread/ldayz27ehldyvzr4 the tight coupling between RequestData and SlingHttpServletRequestImpl makes it impossible to use request classes that just implement SlingHttpServletRequest.

      I need this for example for SLING-550, where servlets run outside of the container's request/response cycle.

      I'll attach a patch that reduces coupling by grabbing the RequestData from a request attribute instead of relying on the SlingHttpServletRequestImpl class to provide it. All tests including integration pass with this patch.

      1. SLING-1596.patch
        4 kB
        Bertrand Delacretaz

        Issue Links

          Activity

          Hide
          Felix Meschberger added a comment -

          Some comments to the patch:

          • You should check the actual type of the request attribute (in the getRequestData method) instead of simply casting. If some rogue
            script would set that request attribute, this would cause improperly handled issues
          • Theoretically with SLING-500/SLING-1603 the RequestData request attribute may be set multiple times. Thus setting it must be
            be accompanied with storing any previous value and resetting the previous value at the end of the request

          Finally, we should clearly see, where the RequestData.unwrap methods are used and fix that use (the unwrap methods are internal to the bundle, so this search should be complete).

          Show
          Felix Meschberger added a comment - Some comments to the patch: You should check the actual type of the request attribute (in the getRequestData method) instead of simply casting. If some rogue script would set that request attribute, this would cause improperly handled issues Theoretically with SLING-500 / SLING-1603 the RequestData request attribute may be set multiple times. Thus setting it must be be accompanied with storing any previous value and resetting the previous value at the end of the request Finally, we should clearly see, where the RequestData.unwrap methods are used and fix that use (the unwrap methods are internal to the bundle, so this search should be complete).
          Hide
          Carsten Ziegeler added a comment -

          Ok, maybe I don't see the real problem behind this....why not creating http servlet request/response and then pass this through the sling main servlet?

          Show
          Carsten Ziegeler added a comment - Ok, maybe I don't see the real problem behind this....why not creating http servlet request/response and then pass this through the sling main servlet?
          Hide
          Alexander Klimetschek added a comment -

          > Usually request wrappers path through request attributes, but what if not? Do we really want to disallow this?

          Most likely the wrapper would be written by someone who wants to make it work with Sling, so I think it is no problem to require the wrapper to do that.

          Show
          Alexander Klimetschek added a comment - > Usually request wrappers path through request attributes, but what if not? Do we really want to disallow this? Most likely the wrapper would be written by someone who wants to make it work with Sling, so I think it is no problem to require the wrapper to do that.
          Hide
          Alexander Klimetschek added a comment -

          > Currently Sling cannot process a request that is not a SlingHttpServletRequestImpl, even if its is a SlingHttpServletRequest. That's much uglier than exposing an implementation object as an attribute, IMHO.

          Agreed. Also, placing implementation objects into request attributes is a very common case, eg. for passing through context information. It doesn't affect servlet code (if the name is unique, which is the case by using the fully qualified class name). I don't know of any servlet that iterates over request attributes and fails if there is some unexpected object inside...

          Show
          Alexander Klimetschek added a comment - > Currently Sling cannot process a request that is not a SlingHttpServletRequestImpl, even if its is a SlingHttpServletRequest. That's much uglier than exposing an implementation object as an attribute, IMHO. Agreed. Also, placing implementation objects into request attributes is a very common case, eg. for passing through context information. It doesn't affect servlet code (if the name is unique, which is the case by using the fully qualified class name). I don't know of any servlet that iterates over request attributes and fails if there is some unexpected object inside...
          Hide
          Bertrand Delacretaz added a comment -

          > I think we should not do this...

          How do you suggest fixing this then?

          Currently Sling cannot process a request that is not a SlingHttpServletRequestImpl, even if its is a SlingHttpServletRequest. That's much uglier than exposing an implementation object as an attribute, IMHO.

          Show
          Bertrand Delacretaz added a comment - > I think we should not do this... How do you suggest fixing this then? Currently Sling cannot process a request that is not a SlingHttpServletRequestImpl, even if its is a SlingHttpServletRequest. That's much uglier than exposing an implementation object as an attribute, IMHO.
          Hide
          Carsten Ziegeler added a comment - - edited

          Usually request wrappers pass through request attributes, but what if not? Do we really want to disallow this?

          Show
          Carsten Ziegeler added a comment - - edited Usually request wrappers pass through request attributes, but what if not? Do we really want to disallow this?
          Hide
          Carsten Ziegeler added a comment -

          I think we should not do this - request data is internal stuff - like the request impl is and we should not expose this through a request attribute or any other means

          Show
          Carsten Ziegeler added a comment - I think we should not do this - request data is internal stuff - like the request impl is and we should not expose this through a request attribute or any other means
          Hide
          Alexander Klimetschek added a comment -

          Makes sense, had that issue once as well. Storing the RequestData as a request attribute makes the code a lot simpler. I think you no longer need the unwrap() methods at all, since a wrapper can be expected to pass through getAttribute calls that don't match locally (and I guess the wrappers in Sling only pass that call through anyway).

          Show
          Alexander Klimetschek added a comment - Makes sense, had that issue once as well. Storing the RequestData as a request attribute makes the code a lot simpler. I think you no longer need the unwrap() methods at all, since a wrapper can be expected to pass through getAttribute calls that don't match locally (and I guess the wrappers in Sling only pass that call through anyway).

            People

            • Assignee:
              Bertrand Delacretaz
              Reporter:
              Bertrand Delacretaz
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:

                Development