Hi all,
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
Pluto 2.0 trunk.
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.
The currently identified areas and elements which need changing are:
*** 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.
So far, nothing wrong yet.
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.
This is a rather big change, but really required.
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
classes will make this much easier and transparent to deal with.
(side note: I actually wrote a testcase for this and this spec requirement is
broken in most other open-source portlet containers as well!)
*** RequestDispatcher path query string parameter handling
PLT.19.4.1 specifies that "The request dispatching mechanism is responsible for
aggregating query string parameters when forwarding or
including requests".
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
implementations are split up as described above.
*** PortletURLProvider SPI
This PortalCallbackService provided *factory* SPI really has too much
responsibilities:
- used as a *stateful* instance by:
- PortletContainerImpl doAction and fireEvent for generating a (Portal)
redirect URL
- BaseURLImpl toString method for generating a PortletURL
- 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.
*** new: PortletRequestStateService SPI
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.
*** PropertyManager SPI
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.
*** new: PortletResponseStateProvider SPI
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
completed.
*** EventProvider SPI
The EventProvider SPI also has "mixed" responsibilities.
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)
*** new: EventCoordinationService SPI
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.
*** FilterManager implementation and usage
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.
*** PortletURLGenerationListener implementation and usage
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.
*** Servlet request/response and PortletWindow parameters to SPI methods
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.
I'm looking forward to feedback and discussion of the above,
Regards,
Ate
