As was discussed earlier on this thread, see also:
http://www.nabble.com/More-required-Pluto-2.0-SPI-and-implementation-refactoring-issues-td21973310.html
as well as earlier this week, see also:
http://www.nabble.com/Re%3A--unsure----RT--Cleaning-up-our-packages-p22305490.html
I've just committed the intermediate state of my work implementing the proposed
changed (and some).
Because of this, Pluto trunk is *BROKEN* temporarily until the proposed
refactoring is completed.
As I also mentioned on the commit message, the current state is as follows:
This commit is a big start of the new SPI implementation.
Especially all the Portlet Developer API (e.g. the Portlet API layer of the container) is mostly complete now (but untested...), taking care
of all the spec requirements/constraints/validation
before handing over the actual processing to the internal container and portal
backend (e.g. through the request/response -Context SPI)
Still very much *not* implemented (at all) is:
- request/response -Context SPI (see: DefaultPortletRequestContextService)
- PortletAppDescriptorerviceImpl.mergeWebDescriptor(...)
- rewrite of Portal/PortletURLProvider and EventProvider for Pluto PortalDriver
Everyone willing to chime in and/or help out with completing the above work,
please shout!
I'm happy to coordinate and delegate tasks :)
And, of course, please all developers review the current API/SPI changes and
implementations so far.
Any feedback (positive/negative) is very much welcome!
Be warned, not yet much javadoc in place so you'll need some time in code reading and especially SPI usage within the container classes to
see what goes on or is needed for the implementation.
Regards,
Ate
Ate Douma wrote:
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
