Pluto
  1. Pluto
  2. PLUTO-478

Portlet Dispatching loses wrappers

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0
    • Fix Version/s: 2.0.0
    • Component/s: portlet container
    • Labels:
      None

      Description

      When you dispatch using a wrapped request/response object, pluto doesn't preserve the wrapping when it executes the dispatch. I.e. it upwraps the request/response and dispatches on that. This prevents portlets from filtering request/responses to/from dispatched/servlet entities.

      It would be nice if we added a TCK test for this case as well. The spec is clear that one can use a wrapped request/response to dispatch to. Though it doesn't specifically state that this must be preserved, it not only is the reasonable interpreation/expectation but is what clients will be counting on. Hence for the sake of interoperability, having a TCK test will catch this problem early.

        Activity

        Michael Freedman created issue -
        Michael Freedman made changes -
        Field Original Value New Value
        Description When you dispatch using a wrapped request/response object, pluto doesn't preserve the wrapping when it executes the dispatch. I.e. it upwraps the request/response and dispatches on that. This prevents portlets from filtering request/responses to/from dispatched/servlet entities. When you dispatch using a wrapped request/response object, pluto doesn't preserve the wrapping when it executes the dispatch. I.e. it upwraps the request/response and dispatches on that. This prevents portlets from filtering request/responses to/from dispatched/servlet entities.

        It would be nice if we added a TCK test for this case as well. The spec is clear that one can use a wrapped request/response to dispatch to. Though it doesn't specifically state that this must be preserved, it not only is the reasonable interpreation/expectation but is what clients will be counting on. Hence for the sake of interoperability, having a TCK test will catch this problem early.
        Hide
        Craig Doremus added a comment -

        Hi Mike. Since you know about this issue, can you offer a suggestion on how to fix it? Better yet, can you supply a patch?

        Show
        Craig Doremus added a comment - Hi Mike. Since you know about this issue, can you offer a suggestion on how to fix it? Better yet, can you supply a patch?
        Hide
        Michael Freedman added a comment -

        More specifically: In the version of the code in which this bug was logged, the problem is in PortletRequestDispatcherImpl. Both include() and forward() unwrap the the incoming request/response objects before calling include/forward internal. Each relies on the utility methods in InternalImplConverter – its these methods that actually do the unwrapping.

        Show
        Michael Freedman added a comment - More specifically: In the version of the code in which this bug was logged, the problem is in PortletRequestDispatcherImpl. Both include() and forward() unwrap the the incoming request/response objects before calling include/forward internal. Each relies on the utility methods in InternalImplConverter – its these methods that actually do the unwrapping.
        Hide
        Michael Freedman added a comment -

        Is there an updated status on this bug? I am back full force on working on the Portlet 2.0 bridge and this is a showstopper.

        Show
        Michael Freedman added a comment - Is there an updated status on this bug? I am back full force on working on the Portlet 2.0 bridge and this is a showstopper.
        Hide
        Ate Douma added a comment -

        Hi Mike,

        I just did look at this "feature" and I can see this "unwrapping" is done for two different reasons:
        a) to be able to mark the request/response internal state as being included/forwarded for which the Portlet API requires different handling
        b) to be able to do the actual servlet dispatch using the ServletRequestDispatcher which only can take a ServletRequest & ServletResponse as parameters

        I think it's not difficult to come up with a solution for a) by using only temporarily "unwrapping" just to mark the internal state.
        Note though, this then will leave the door open for actual wrappers to "break" the Portlet API requirements when overriding some of the specific methods "incorrectly".

        But, the second reason b) really is a killer: how would you do the actual servlet dispatch which requires the underlying ServletRequest/ServletResponse without unwrapping?
        Maybe wrapping the (possibly itself wrapped) PortletRequest/PortletResponse with yet another HttpServletRequestWrapper/HttpServletResponseWrapper and delegate all their methods back to the PortletRequest/PortletResponse?
        I'm not sure that's 100% guaranteed to work on all (or at least some) web containers which, like Tomcat, sometimes do their own "unwrapping" to get to their own internal state...
        And what should be returned for the following method then: ServletRequest HttpServletRequestWrapper.getRequest()?

        I'm willing to try to solve this one, but right now I don't see a "solid" solution yet. But if you have any ideas what might work (without resorting to webcontainer/vendor specific solutions) I'm all ears!

        Show
        Ate Douma added a comment - Hi Mike, I just did look at this "feature" and I can see this "unwrapping" is done for two different reasons: a) to be able to mark the request/response internal state as being included/forwarded for which the Portlet API requires different handling b) to be able to do the actual servlet dispatch using the ServletRequestDispatcher which only can take a ServletRequest & ServletResponse as parameters I think it's not difficult to come up with a solution for a) by using only temporarily "unwrapping" just to mark the internal state. Note though, this then will leave the door open for actual wrappers to "break" the Portlet API requirements when overriding some of the specific methods "incorrectly". But, the second reason b) really is a killer: how would you do the actual servlet dispatch which requires the underlying ServletRequest/ServletResponse without unwrapping? Maybe wrapping the (possibly itself wrapped) PortletRequest/PortletResponse with yet another HttpServletRequestWrapper/HttpServletResponseWrapper and delegate all their methods back to the PortletRequest/PortletResponse? I'm not sure that's 100% guaranteed to work on all (or at least some) web containers which, like Tomcat, sometimes do their own "unwrapping" to get to their own internal state... And what should be returned for the following method then: ServletRequest HttpServletRequestWrapper.getRequest()? I'm willing to try to solve this one, but right now I don't see a "solid" solution yet. But if you have any ideas what might work (without resorting to webcontainer/vendor specific solutions) I'm all ears!
        Hide
        Michael Freedman added a comment -

        As the Pluto (internal) Request/response objects are already HTTServlet... wrappers and these already have special code in them to flag when they are called in the incldued/forwarded state – I would expect the implementation to be something along the lines of:

        These internal request/response objects would remember the origin dispatched objects (if these are wrapped objects). And then for any method that the spec says the portlet is supposed to provide the answer/function from doing the equivalent portlet behavior you would have the internal request/response object call the wrapped object to do the work. It also probably needs to set an internal flag so that if it gets delegated back to it can detect it should do the work vs execute the wrapper again.

        Show
        Michael Freedman added a comment - As the Pluto (internal) Request/response objects are already HTTServlet... wrappers and these already have special code in them to flag when they are called in the incldued/forwarded state – I would expect the implementation to be something along the lines of: These internal request/response objects would remember the origin dispatched objects (if these are wrapped objects). And then for any method that the spec says the portlet is supposed to provide the answer/function from doing the equivalent portlet behavior you would have the internal request/response object call the wrapped object to do the work. It also probably needs to set an internal flag so that if it gets delegated back to it can detect it should do the work vs execute the wrapper again.
        Ate Douma made changes -
        Assignee Ate Douma [ adouma ]
        Hide
        Ate Douma added a comment -

        All fixed and implemented with the recent big bang refactoring and final commits r753592 and r753593

        Show
        Ate Douma added a comment - All fixed and implemented with the recent big bang refactoring and final commits r753592 and r753593
        Ate Douma made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Mark Thomas made changes -
        Workflow jira [ 12427306 ] Default workflow, editable Closed status [ 12564873 ]
        Mark Thomas made changes -
        Workflow Default workflow, editable Closed status [ 12564873 ] jira [ 12585966 ]

          People

          • Assignee:
            Ate Douma
            Reporter:
            Michael Freedman
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development