Re: More required Pluto 2.0 SPI and implementation refactoring issues

[Eric Dalquist]

This list is great. Even in Pluto 1.1 I've run into a lot of issues, primarily around request/response handling, that would be resolved by these refactoring. I can try to help with some of the refactoring but I have a conference for JASIG the first week of march that I have to get a uPortal RC put together for. At the very least I'll continue to be paying attention on the list and to Jira to see where I can jump in and contribute.

-Eric

Ate Douma wrote:

Hi all,


Pluto 2.0 trunk.





The currently identified areas and elements which need changing are:



So far, nothing wrong yet.


This is a rather big change, but really required.


classes will make this much easier and transparent to deal with.




*** RequestDispatcher path query string parameter handling

including requests".





implementations are split up as described above.


*** PortletURLProvider SPI

- used as a *stateful* instance by:

  - BaseURLImpl toString method for generating a PortletURL







*** new: PortletRequestStateService SPI



*** PropertyManager SPI





*** new: PortletResponseStateProvider SPI





completed.


*** EventProvider SPI
The EventProvider SPI also has "mixed" responsibilities.









*** new: EventCoordinationService SPI



*** FilterManager implementation and usage



*** PortletURLGenerationListener implementation and usage






*** Servlet request/response and PortletWindow parameters to SPI methods


----------------------------------





I'm looking forward to feedback and discussion of the above,

Regards,

Ate

David Taylor and myself are busy trying to get all JSR-286 features properly implemented in the new Jetspeed-2.2, based upon the current However, now that we're getting close to the "meat" of all this, we've encountered again several issues in both the current Pluto container implementation classes as well in the design and usage of some of the Pluto SPI interfaces to support proper integration in Jetspeed-2 and other portals. Although we could redefine and re-implement some of the below described issues separately for Jetspeed-2 itself, we want to leverage and improve as much as possible of the Pluto container SPI and implementation itself so everybody can benefit from it. This means we will have to go through some additional Pluto refactoring iterations to straighten this out. *** InternalPortletRequest/Response implementations (and subclasses thereof) extending HttpServletRequest/ResponseWrapper This solution (dating back from Pluto 1.0 implementation) has a very tricky but serious flaw. By using a single instance HttpServletRequestWrapper instance for both the PortletRequest and dispatched ServletRequest, a dispatched servlet retrieving the current PortletRequest (or Response) using HttpServletRequest.getAttribute("javax.portlet.request") as specified by the Portlet specification (PLT.19.3.2), will actually return the *current* HttpServletRequestWrapper itself again. But, as the InternalPortletRequestImpl (which is the real implementation class) also maintains internal instance state concerning its dispatched state and based upon that decides how overlapping methods need to behave, the PortletRequest object retrieved like this from within a servlet environment actually behaves as a dispatched ServletRequest. This is *not* compliant with the Portlet specification, even if the current JSR-286 TCK doesn't (properly) test against this. The only solution to solve this is *not* using a piggy back solution for the dispatched ServletRequest/Response objects, but use independent instances for the PortletRequest/Response and wrap these within the dispatched ServletRequest/Response objects. On the bright side, doing so will result in a much more readable/maintainable solution as the current implementation has to maintain some tricky state flags to keep track of its "identity". Getting rid of all that and moving the dispatched servlet specific handling in separate (side note: I actually wrote a testcase for this and this spec requirement is broken in most other open-source portlet containers as well!) PLT.19.4.1 specifies that "The request dispatching mechanism is responsible for aggregating query string parameters when forwarding or This requirement has been implemented very literally in Pluto: a PortletRequestDispatcher path query string is "stored" in the request instance and on subsequent access to the parameter map from within the invoked servlet parsed and merged on top of the portlet parametermap. This "manual" processing and merging of query string parameters however breaks the Servlet spec requirements! It goes wrong if from within the dispatched servlet a subsequent dispatch is performed with additional query string parameters. In the current solution, this possible usage hasn't been taken into account, with as result you'll *always* receive the same parametermap on subsequent (nested) dispatches (for example from within a JSP including another JSP). But, even if the servletrequest.getRequestDispatcher(path) would be overridden to "fix" this problem, that still won't solve it for two reasons: - ServletContext.getRequestDispatcher(path) *cannot* be overridden, therefore still leaving a "hole" in the solution - there is no "returned from dispatch" callback mechanism to "revert" the current parametermap after a dispatch Therefore, "manual" parsing and merging of dispatcher query string parameters *cannot* be used to implement this spec requirement. However, the correct way to implement this is actually extremely simple: just let the servlet container handle it all by itself! There is no need to "store", parse and merge dispatcher query string parameters, and in the servletrequest(wrapper).getParameterMap(), just return super.getParameterMap() which the container will have "injected" with the additional query string parameters merged already. Jetspeed-2 has used this solution from the beginning (with some additional fancy cache handling) and it just works as expected. Changing to this solution will dramatically simplify the current implementation, especially after the PortletRequest and ServletRequest This PortalCallbackService provided *factory* SPI really has too much responsibilities: - PortletContainerImpl doAction and fireEvent for generating a (Portal) redirect URL - used by PortletRequestImpl and ResourceURLImpl for accessing the *readonly* request scoped public render parameters - used by PortletRequestImpl both parsing and storing request dispatcher query string parameters - used by PortletContainerImpl as "back door" mechanism to store the final PortalURL after doAction/fireEvent (inside the Pluto *Driver* PortalRequestContext, although it seems its not even actually used (anymore)) As result, the SPI definition itself leads to tricky usage patterns which causes very difficult to maintain implementation code. I propose to trim down this interface to only provide the factory responsibility for new PortletURL generations, split off the readonly request state related methods to a new interface (see below), and drop the remaining (also no longer needed) methods. I propose this new SPI interface for providing readonly access to *all* types of request parameters and related state, including request properties (see below). Besides providing a pluggable solution for accessing all the request parameters (private, public, resource), it will also allow "fixing" the current implemented ResourceRequest.getResourceID() handling and the not yet implemented ResourceRequest.getCacheability(). ResourceRequest.getResourceID() is currently implemented using a *hard coded* request parameter, which simply isn't usable as generic solution. The current PropertyManager SPI also has too many responsibilities: it is used both for accessing the (readonly) PortletRequest properties (headers and cookies) as well as for storing new PortletResponse properties (headers, cookies and DOM elements for MARKUP_HEAD_ELEMENT contributions). Note: the current default implementation doesn't actually do anything when setting properties! I propose to move the readonly access for the current PortletRequest headers and cookies to the new PortletRequestStateService SPI (see above), and to provide a new SPI for handling the PortletResponse properties and all other PortletResponse state, see below. The current PortletResponse (and derivate) implementations "write" the response state directly to the portal provided servlet response, or use the above described PropertyManager SPI. For the generated content output, this solution assumes (requires) that the portal provided servlet response properly deals with the required buffering, content type, etc. handling, but this is not appropriately captured in a SPI contract. In addition, essential JSR-286 features like CacheControl, PortletWindow title setting nextPossiblePortletModes, etc. are not yet implemented or in an unexpected way (RenderResponse.setTitle using PortalCallback.setTitle). In my view, all these response "state" parameters need to be dealt with as a single set as the aggregating portal will have to deal with then as a a single set anyway. For example, Once response caching is properly implemented, *all* these response state parameters need to be cached as one set. For that, one single SPI should be used instead of requiring a generic servlet response wrapper implementation by the Portal without any formal contract as well as having to "merge" with some other SPI callback method results too. I propose adding a new PortletResponseStateProvider SPI to take over the set* methods of the current PropertyManager SPI as well as for storing response content (and related attributes like contentType), PortletWindow title, nextPossiblePortletModes and the CacheControl. All PortletResponse (and dispatched Servletresponse) methods dealing with writing content state should delegate to this new SPI. The aggregating portal then can retrieve and process the resulting response state as a single entity after the portlet invocation is As a "-Provider" it is used to generate PortletEvent objects on behalf of the ActionResponse and EventResponse implementations which is subsequently stores in an internal list. But, it is also responsible for dispatching (coordinating) these registered PortletEvents after the container processAction or fireEvent invocation. The problem is that the EventProvider can only fireEvents which have been registered by the portlet itself. Container initiated events (as described by JSR-286) cannot be handled through this SPI, and Pluto currently doesn't have any support for them either. As the logic for event coordination (firing) is a very portal (and possibly policy based) functionality, while the creation of events by a portlet is very "standards" based, these two responsibilities shouldn't be "mixed" in one interface. I propose to remove the fireEvents handling from the EventProvider SPI and introduce a new SPI for handling the event coordination. The EventProvider SPI needs to be extended to allow retrieving the "registered" event queue after the portlet invocation by the container. (site note: the EventProvider as provided by the Pluto Portal Driver needs some major internal refactoring as well as its implementation is rather inefficient right now) As described above, PortletEvent coordination should be dealt with separately, which this new SPI will provide. It should as a minimum allow firing events based on a (also new to be defined) PortletEventQueue, as provided by the EventProvider or created directly by the container or possibly even the Portal itself. The current implementation and usage of the FilterManager is very inefficient: a new FilterManagerImpl and FilterChainImpl is created on the fly for each container invocation, while the behavior and context of a PortletFilter really is deployment descriptor based. This implementation needs to be refactoring to a much more performant solution. This issue is similar to the issue with the FilterManager implementation: on each PortletURL creation all portlet deployment descriptor defined PortletURLGenerationListener classes are recreated on the fly while these are expected to be stateless singleton instances anyway. *** missing ContainerRuntimeOption implementations (including a required one) JSR-286, PLT.10.4, describes the optional *and* the one required ContainerRuntimeOptions. *None* of these are supported or handled correctly by Pluto right now, not even the required (but unfortunately TCK untested) actionScopedRequestAttributes. The escapeXML option is a rather simple feature which should be easy to "fix" (some part of the implementation is already provided, but somehow not completed). The renderHeaders option really is a Portal scoped feature so Pluto container itself doesn't need (nor in my view should) provide handling for this, Jetspeed-2 of course will provide this. For the servletDefaultSessionScope option, Portals Bridges already provides (and was basis for this spec feature ;) ) a generic proxy based solution which I think can easily be merged as default solution into the Pluto Container, but should be "pluggable" through a new SPI also. The *required* actionScopedRequestAttributes option really needs to be provided, but this also is a feature which needs to be hooked into the container through the portal. So, this also needs both a new SPI and a (lightweight) implementation. Almost all SPI methods require passing in the initial (Portal provided) servletrequest and/or response objects, and additionally the current PortletWindow. I propose changing all these (where feasible) to take only the current InternalPortletRequest/Reponse object(s) as parameters as through those the initial servlet request/response and the PortletWindow instances are already retrievable by the SPI implementations. This will will allow more generic and extendable implementations of them and also largely simplifies the method signatures and parameter passing around. The above list in my view now covers most if not all critical *design* issues for Pluto container SPI and implementation. Once we have "fixed" the above, I think the Pluto container will finally be stabilized out enough as basis for an actual 2.0 release. Sure enough, there also will be implementation bugs to deal with, but those should (hopefully) no longer require major SPI and implementation refactoring. Besides David and myself, I hope also some other Portals committers and community members will be able to help out with this on short notice because our goal is to get this "done" before the upcoming ApacheCon, end of march in Amsterdam.

smime.p7s
Description: S/MIME Cryptographic Signature