On Thu, 2008-08-14 at 13:54 +0200, Simon Kitching wrote:
> Leonardo Uribe schrieb:
> >
> >
> > On Wed, Aug 13, 2008 at 3:48 PM, Leonardo Uribe <[EMAIL PROTECTED]
> > <mailto:[EMAIL PROTECTED]>> wrote:
> >
> >
> >
> > On Wed, Aug 13, 2008 at 4:12 AM, [EMAIL PROTECTED]
> > <mailto:[EMAIL PROTECTED]> <[EMAIL PROTECTED]
> > <mailto:[EMAIL PROTECTED]>> wrote:
> >
> > Matthias Wessendorf schrieb:
> >
> > Simon,
> >
> > On Wed, Aug 13, 2008 at 10:53 AM, Simon Kitching
> > <[EMAIL PROTECTED] <mailto:[EMAIL PROTECTED]>> wrote:
> >
> >
> > Hi,
> >
> > Just a reminder of the email I posted a few weeks ago:
> > I'm still very
> > concerned about the recent ExtensionsFilter refactoring.
> >
> > The new code now parses the web.xml to extract some
> > configuration
> > information. This just feels wrong to me.
> > The new code also is based around a custom
> > FacesContext wrapper. I'm worried
> > about possible interactions between this and other
> > libraries that also wrap
> > FacesContext. And I'm worried about the portability of
> > this code to the
> > upcoming JSF2.0.
> >
> > I know this stuff is needed to get tomahawk useable
> > for portlets. But it's
> > very important not to break existing users.
> >
> > What I would like to see is:
> > * separate out the resource-serving stuff into a
> > separate utilities module,
> > that can be called from either a filter or a FacesContext.
> > And call the common code from both ExtensionsFilter and
> > TomahawkFacesContextWrapper.
> > * have the TomahawkFacesContextFactory create a
> > FacesContext wrapper only in
> > the case where the filter is not configured, ie
> > look in the request-scope for a flag placed there by
> > the filter. That means
> > there is no chance of breaking existing setups where
> > the filter is defined explicitly.
> > * ideally, also have some way of turning off the
> > faces-context wrapping even
> > when the filter is not present.
> > * don't parse the web.xml at all. I know you said
> > earlier that it is needed,
> > but I just don't see why. In any case, I think we *must*
> > find some other solution; the current approach is
> > really hard to maintain.
> >
> > Until this is fixed, I would definitely vote -1 on any
> > tomahawk release.
> > It's a major issue.
> >
> >
> >
> > was there a jira issue for it ?
> > or can you point me to the svn rev?
> >
> >
> > The best thing to do is to look at these classes:
> > org.apache.myfaces.webapp.filter.ExtensionsFilter
> > org.apache.myfaces.webapp.filter.TomahawkFacesContextFactory
> > org.apache.myfaces.webapp.filter.TomahawkFacesContextWrapper
> > org.apache.myfaces.webapp.filter.ExtensionsFilterConfig [1]
> >
> > The svn history of these files references MYFACES-434.
> > There is no specific jira issue about my concerns; it is
> > really an architecture/design disagreement rather than a
> > specific bug.
> >
> > [1] ExtensionsFilterConfig.getExtensionsFilterConfig is
> > invoked from TomahawkFacesContextWrapper.
> >
> > Regards, Simon
> >
> >
> > Hi
> >
> > I'll do a simple review (just to make clear the discussion).
> > ExtensionsFilter has the following responsibilities (all we know
> > this but sometimes is not very clear):
> >
> > 1. Wrap a multipart request (used for t:inputFileUpload), so the
> > request could be correctly decoded.
> >
> > 1.1 For do this, it receives some params from web.xml like this:
> >
> > <filter>
> > <filter-name>extensionsFilter</filter-name>
> >
> >
> > <filter-class>org.apache.myfaces.webapp.filter.ExtensionsFilter</filter-class>
> > <init-param>
> > <param-name>uploadMaxFileSize</param-name>
> > <param-value>100m</param-value>
> > </init-param>
> > <init-param>
> > <param-name>uploadThresholdSize</param-name>
> > <param-value>100k</param-value>
> > </init-param>
> > </filter>
> >
> > 1.2 So, ExtensionsFilter must wrap the request that goes to faces
> > pages using a filter-mapping like this:
> >
> > <filter-mapping>
> > <filter-name>extensionsFilter</filter-name>
> > <url-pattern>*.jsf</url-pattern>
> > </filter-mapping>
> > <filter-mapping>
> > <filter-name>extensionsFilter</filter-name>
> > <url-pattern>/faces/*</url-pattern>
> > </filter-mapping>
> >
> > 2. ExtensionsFilter must serve resources (javascript, css...) of
> > some tomahawk components.
> >
> > 3. If a buffered instance of AddResource is configured,
> > ExtensionsFilter must buffer and add the resource reference
> > to the head of jsf pages (for example when it is used
> > DefaultAddResource)
> >
> > The changes proposed on MYFACES-434 was the following:
> >
> > a. Use a TomahawkFacesContextWrapper to do tasks 1 and 3 of
> > ExtensionsFilter. It must be read the config params to know
> > how to wrap the request from web.xml, because this params are on
> > the filter init-param part.
> >
> > b. Add a ServeResourcePhaseListener for do the task 2 of
> > ExtensionsFilter (In portlets we could not be ExtensionsFilter
> > working).
> >
> > c. Wrap the portlet request properly when it is multipart content,
> > so t:inputFileUpload could work on portlets
> > (to be done, Scott told me to wait some time).
> >
> > Actually, ExtensionsFilter only does task 2
> > (ServeResourcePhaseListener was added on faces-config.xml) and
> > tasks 1 and 3
> > are done by TomahawkFacesContextWrapper.
> >
> > Now the problem.
> >
> > It could be some problems when you mix 1.1 libraries with 1.2
> > code that wraps FacesContext (I don't remember the myfaces
> > issue, but the local example of this problem is when you mix
> > orchestra(1.1) myfaces core(1.2) with tomahawk(1.1.7-SNAPSHOT)).
> >
> > In order to anticipate the events, It could be good to remain the
> > old feature (if ExtensionsFilter is configured and
> > is working, do not use TomahawkFacesContextWrapper). Sounds good
> > the suggestions, but in my opinion, there are not
> > blocking issues for a release.
> >
> >
> > I have added the param
> > org.apache.myfaces.DISABLE_TOMAHAWK_FACES_CONTEXT_WRAPPER, and if
> > ExtensionsFilter is used before, do not use
> > TomahawkFacesContextWrapper. If someone wants to implement the other
> > suggestions go ahead, but remember do not break anything or reduce the
> > features available right now. It takes a lot of effort and time make
> > things work.
> >
> >
> >
> > But I don't see a solution for get the params for initialize the
> > multipart request wrapper different than parse the web.xml file
> > I'm open to suggestions, but any solution must remain the actual
> > behavior(it works and it is easier to users, since
> > the params has been configured there for a long time).
> >
>
> Thanks for the very clear description above.
>
> And thanks for the changes you have made so far.
>
> I know very little about portlets, but assume that a portlet engine
> typically is deployed as a servlet within a servlet container, and then
> internally invokes the relevant portlets, providing the PortletContext
> object etc? And that what we are talking about here with respect to
> extracting the ExtensionFilter config params is this kind of setup?
>
> The docs for PortletContext that I have found say:
> <quote>
> There is one context per "portlet application" per Java Virtual Machine.
> (A "portlet application" is a collection of portlets, servlets, and
> content installed under a specific subset of the server URL namespace,
> such as |/catalog|. They are possibly installed via a |.war| file.) As a
> web application, a portlet application also has a servlet context. The
> portlet context leverages most of its functionality from the servlet
> context of the portlet application.
>
> Attibutes stored in the context are global for /all/ users and /all/
> components in the portlet application.
> </quote>
>
> So somePortletContext.getAttribute() accesses "application scope" data
> common to all portlets.
> And ServletContext.getAttribute() is also "application scope" data.
>
> Aren't they the *same* application scope in the setup we are talking
> about here, ie if the ExtensionsFilter calls
> ServletContext.setAttribute("foo", "bar");
> then when something calls
> PortletContext.getAttribute("foo");
> doesn't that return "bar"?
Actually, after some more thought I wonder why we need to share config
at all.
The use cases are:
(1) user does new install of portlet-only webapp
(2) user does new install of mixed portlet/normal webapp
(3) user adds portlets to existing normal webapp
(4) user upgrades existing normal webapp to new tomahawk version,
and doesn't use portlets at all.
Note: #4 is the most common case.
In case (1), there is absolutely no sense in requiring the user to
define a <filter> block. Instead we should have some other way of
configuring the fileUpload and similar behaviour. Probably via context
init parameters.
So if we are then going to have to define some way of doing global
config (ie not via filter-specific params) then it also makes sense to
use that for (2) and (3). In both cases, this is not a simple "upgrade"
like #4. Instead, the user is configuring something that never used to
work before. So the fact that they need to change the way that tomahawk
is configured seems no real issue.
For #4 we do need to be hackwards-compatible with configuration; a user
simply upgrading tomahawk in an existing working webapp shouldn't need
to modify their config. But #4 can be solved by simply *not* ripping out
the ExtensionsFilter code, as is currently the case. Instead, if the
filter continues to do the work, and the TomahawkFacesContextFactory
simply does nothing when it detects that the filter has already run,
then there is no problem. The support code can internally be refactored
into a common util that both Filter and FacesContext can call, as long
as the filter still does the same tasks it currently does now, and the
FacesContext wrapper does NOT get created.
Or have I missed something?
Regards, Simon
