myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Leonardo Uribe" <lu4...@gmail.com>
Subject Re: [tomahawk] extensionsfilter refactoring
Date Wed, 13 Aug 2008 20:48:30 GMT
On Wed, Aug 13, 2008 at 4:12 AM, simon.kitching@chello.at <
simon.kitching@chello.at> wrote:

> Matthias Wessendorf schrieb:
>
>  Simon,
>>
>> On Wed, Aug 13, 2008 at 10:53 AM, Simon Kitching <skitching@apache.org>
>> 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.

 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).

regards

Leonardo Uribe

Mime
View raw message