Pluto
  1. Pluto
  2. PLUTO-516

Pluto's PorletSessionImpl#setAttribute sometimes sets var into APPLICATION_SCOPE without explanation

    Details

      Description

      as per the confusions around:
      https://issues.apache.org/jira/browse/PB-84

      and:
      http://www.nabble.com/StringIndexOutOfBoundsException-on-apache-bridges-td20055052.html#a20079383

      PorletSessionImpl#setAttribute will sometimes store attributes into APPLICATION_SCOPE, going against the portlet spec. There is also no explanation in the source code as to why.

      Please add explanation or correct the behavior.

        Activity

        Hide
        Ate Douma added a comment -

        I agree this seems to be bug in the current Pluto implementation of the portlet api.
        I've traced the origin of code in question back to PLUTO-263 and specifically the third patch attached to that issue.

        Will investigate further and see if we can fix this one quickly

        Show
        Ate Douma added a comment - I agree this seems to be bug in the current Pluto implementation of the portlet api. I've traced the origin of code in question back to PLUTO-263 and specifically the third patch attached to that issue. Will investigate further and see if we can fix this one quickly
        Hide
        Ate Douma added a comment -

        I discovered the bug and have a fix for it which I will commit shortly.

        As I already thought, this was caused by a wrong implementation of PLT.10.4.3 - Runtime Option javax.portlet.servletDefaultSessionScope!

        That spec rule says that servlets included or forwarded from a portlet should get access to the APPLICATION_SCOPE session attributes.
        Now, what went wrong (and I think this most likely is at the bases of all those other container implementations showing the same error, see PB-84)
        is that the PortletRequest implementations actually are HttpServletRequestWrappers which are passed on to the target servlet directly.
        And, for the HttpServletRequest.getSession() method handling (on top of the PortletRequest implementation), the PortletSession is returned, not the underlying HttpSession of the request.
        Now, if you do that and need to implement PLT.10.4.3, you have a problem... which was "solved" by setting a flag in the PortletSession telling if it was included/forwarded to a servlet
        in which case it would switch the default attribute scope from PORTLET_SCOPE to APPLICATION_SCOPE.
        All is is still valid if you only access the dispatched HttpServletRequest/PortletRequest using only the HttpServletRequest methods.
        But... what everyone seems to have forgotten is the following spec defined usage:

        PortletRequest portletRequest = (PortletRequest)httpServletRequest.getAttribute("javax.portlet.PortletRequest");

        Now, as in the above example portletRequest==httpServletRequest, calling portletRequest.getPortletSession().setAttribute("name","value") then leads to this bug
        of using APPLICATION_SCOPE instead of the spec required PORTLET_SCOPE.

        The solution luckily is simple: not returning the PortletSession for the HttpServletRequest.getSession() method but simply the actually wrapped HttpSession directly.
        Then, the internal flag within PortletSession to let it know it is included/forwarded also isn't needed anymore and the code actually becomes much simpler

        I've run my fix against the Portlet 2.0 TCK and it nicely passes the relevant tests for this functionality, so I'm pretty sure this fix is OK.

        As said, I will commit this fix for both the trunk and the 2.0-spi-refactoring branch where all the real action is right now.

        Although the 2.0-spi-refactoring branch isn't completed yet, we're getting close and AFAIK its already better at passing the Portlet 2.0 TCK than the trunk (both still have a few errors though).
        As our intention is to swap the trunk with this branch as soon as we're ready (enough) and we are giving more attention now to fixing there instead of the trunk,
        I would personally no longer use of the trunk for development anymore ...
        Our expatiation and hope is to swap the trunk with this branch end of this week or latest early next week.

        Show
        Ate Douma added a comment - I discovered the bug and have a fix for it which I will commit shortly. As I already thought, this was caused by a wrong implementation of PLT.10.4.3 - Runtime Option javax.portlet.servletDefaultSessionScope! That spec rule says that servlets included or forwarded from a portlet should get access to the APPLICATION_SCOPE session attributes. Now, what went wrong (and I think this most likely is at the bases of all those other container implementations showing the same error, see PB-84 ) is that the PortletRequest implementations actually are HttpServletRequestWrappers which are passed on to the target servlet directly. And, for the HttpServletRequest.getSession() method handling (on top of the PortletRequest implementation), the PortletSession is returned, not the underlying HttpSession of the request. Now, if you do that and need to implement PLT.10.4.3, you have a problem... which was "solved" by setting a flag in the PortletSession telling if it was included/forwarded to a servlet in which case it would switch the default attribute scope from PORTLET_SCOPE to APPLICATION_SCOPE. All is is still valid if you only access the dispatched HttpServletRequest/PortletRequest using only the HttpServletRequest methods. But... what everyone seems to have forgotten is the following spec defined usage: PortletRequest portletRequest = (PortletRequest)httpServletRequest.getAttribute("javax.portlet.PortletRequest"); Now, as in the above example portletRequest==httpServletRequest, calling portletRequest.getPortletSession().setAttribute("name","value") then leads to this bug of using APPLICATION_SCOPE instead of the spec required PORTLET_SCOPE. The solution luckily is simple: not returning the PortletSession for the HttpServletRequest.getSession() method but simply the actually wrapped HttpSession directly. Then, the internal flag within PortletSession to let it know it is included/forwarded also isn't needed anymore and the code actually becomes much simpler I've run my fix against the Portlet 2.0 TCK and it nicely passes the relevant tests for this functionality, so I'm pretty sure this fix is OK. As said, I will commit this fix for both the trunk and the 2.0-spi-refactoring branch where all the real action is right now. Although the 2.0-spi-refactoring branch isn't completed yet, we're getting close and AFAIK its already better at passing the Portlet 2.0 TCK than the trunk (both still have a few errors though). As our intention is to swap the trunk with this branch as soon as we're ready (enough) and we are giving more attention now to fixing there instead of the trunk, I would personally no longer use of the trunk for development anymore ... Our expatiation and hope is to swap the trunk with this branch end of this week or latest early next week.
        Hide
        Ate Douma added a comment -

        Fix committed

        Show
        Ate Douma added a comment - Fix committed

          People

          • Assignee:
            Ate Douma
            Reporter:
            Antony Stubbs
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development