You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shale.apache.org by Craig McClanahan <cr...@apache.org> on 2006/09/29 09:04:05 UTC
Re: [jira] Commented: (SHALE-295) collect web archives with explicit configured faces-config.xml pointing to them
On 9/28/06, Mario Ivankovits (JIRA) <ji...@apache.org> wrote:
>
> [
> http://issues.apache.org/struts/browse/SHALE-295?page=comments#action_38300]
>
> Mario Ivankovits commented on SHALE-295:
> ----------------------------------------
>
> No, you misunderstood me. BTW: I am a hibernate etc. user, I know what it
> means to have tons of dependencies. (We have 91 of them - excluding our own
> app)
>
> Crag, did you look at the patch?
Yes, I did.
Its a very simple one.
No, I totally disagree.
There is *zero* documentation on what the changes attempt to accomplish.
Besides that, there seem to be a large number of changes that are (from the
viewpoint of an external person) totally gratuitous -- such as changing data
types from List<...> to Collection<...>. The amount of such changes makes
the ultimate intent of what you are trying to do totally invisible to me.
If you can't even describe to the initial author of this code what you're
trying to do, and how, it's not going to go very far :-).
You will see, that it uses explicitResources() (your method in
> LifeCycleListener2) to get a list of URLs of all explicitely configures
> faces-config.xml.
> Then, it simply gets the jarfile from the urlConnection.
>
> No additional scanning takes places.
>
> Its just, that shale-tiger not only recognizes META-INF/faces-config.xmlas faces-app, but also those jars which contain a
> faces-config.xml, assumed, that the pointer to the faces-config.xml is
> configured through CONFIG_FILES configuration.
> This is code already running in LifeCycleListener2, now, its just reused
> for a different thing, there is no performance penalty.
As far as I understood your patch, you were planning to remove the condition
about jars that had a META-INF/faces-conifg.xml file. Is that not the
acase?
If you were intending to remove this restriction, and replace it with a test
for whether the JAR includes a class with a package configured in a config
parameter, then I'm likely to remain -1 on this approach, for the reasons
stated in earlier responses on the threads related to this change. Please
explain to me how, if I happen to have an app with hibernate (and its
dependencies) *and* declare a set of packges with your proposed config
parameter, you will eliminate the need to analize the Hibernate JAR itself,
or any of its non-JSF-related dependencies.
AFAICT, your proposed approach makes the environment worse, rather than
better.
Craig
> collect web archives with explicit configured faces-config.xml pointing to
> them
> >
> -------------------------------------------------------------------------------
> >
> > Key: SHALE-295
> > URL: http://issues.apache.org/struts/browse/SHALE-295
> > Project: Shale
> > Issue Type: Improvement
> > Components: Tiger
> > Reporter: Mario Ivankovits
> > Attachments: explicit_jars.diff
> >
> >
> > The patch will also collect jars in WEB-INF/lib which have a
> faces-config.xml pointing to them through the configuration
> javax.faces.CONFIG_FILES
> > That way, you no longer need to have a "marker" faces-config.xml in
> META-INF.
> > The patch is not heavily tested yet.
>
> --
> This message is automatically generated by JIRA.
> -
> If you think it was sent incorrectly contact one of the administrators:
> http://issues.apache.org/struts/secure/Administrators.jspa
> -
> For more information on JIRA, see: http://www.atlassian.com/software/jira
>
>
>
Re: [jira] Commented: (SHALE-295) collect web archives with explicit configured faces-config.xml pointing to them
Posted by Craig McClanahan <cr...@apache.org>.
On 9/29/06, Mario Ivankovits <ma...@ops.co.at> wrote:
>
> Hi!
> >> I had a look at the JSF-RI impl, there you will find in
> >> ConfigureListener the method "getContextURLForPath" which will be used
> >> to lookup the configuration files configured in
> >> javax.faces.CONFIG_FILES. This method use
> >> ServletContext.getResource(path) - the same as shale-tiger use, and for
> >> sure myfaces-impl.
> > Right. That looks for a static resource in the webapp, not a resource
> > buried inside a JAR file,. because it uses ServletContext.getResource()
> > instead of ClassLoader.getResource().
> Ok, now I am a idiot :-)
> This is a perfect example of "test your patch before post it" ...
>
> Craig, please close this jira as invalid, it just wont work.
Will do.
You are
> right with this.
> Thanks for your patience, if it will ever happen you'll have two free
> beer (or three ;-) )
Be happy to raise up beers with you next time we're on the same continent
:-).
Craig
Ciao,
> Mario
>
>
Re: [jira] Commented: (SHALE-295) collect web archives with explicit
configured faces-config.xml pointing to them
Posted by Mario Ivankovits <ma...@ops.co.at>.
Hi!
>> I had a look at the JSF-RI impl, there you will find in
>> ConfigureListener the method "getContextURLForPath" which will be used
>> to lookup the configuration files configured in
>> javax.faces.CONFIG_FILES. This method use
>> ServletContext.getResource(path) - the same as shale-tiger use, and for
>> sure myfaces-impl.
> Right. That looks for a static resource in the webapp, not a resource
> buried inside a JAR file,. because it uses ServletContext.getResource()
> instead of ClassLoader.getResource().
Ok, now I am a idiot :-)
This is a perfect example of "test your patch before post it" ...
Craig, please close this jira as invalid, it just wont work. You are
right with this.
Thanks for your patience, if it will ever happen you'll have two free
beer (or three ;-) )
Ciao,
Mario
Re: [jira] Commented: (SHALE-295) collect web archives with explicit configured faces-config.xml pointing to them
Posted by Craig McClanahan <cr...@apache.org>.
On 9/29/06, Mario Ivankovits <ma...@ops.co.at> wrote:
>
> Hi Craig,
> or should I say - good morning :-)
Yep, at the moment :-).
>> javax.faces.CONFIG_FILES=/content/app/conf/faces-config.xml
> >> ,/content/app2/conf/faces-config.xml
> >
> > OK, so lets continue the "twenty questions" game for a minute, and
> > ask, why
> > would you have a faces-config.xml file nested like this? JSF is going
> to
> > ignore it as well.
> Why will JSF ignore it? I dont understand what you mean. (I checked the
> JSR-RI source, please read on)
> Our application works using such a configuration like a charme.
> We use the myfaces JSF impl, but as far as I remember, this worked with
> JSF-RI too.
See below.
> The whole point of the META-INF convention in the spec
> > is so that you do not have to explicitly configure things like this.
> Yes, I know this, but we decided to use the javax.faces.CONFIG_FILES way.
I think I was assuming from your description that the actual
faces-config.xml file was buried inside a JAR file inside WEB-INF/lib. If
that's not the case, then the discussion below about whats supported is not
relevant.
One thing to note, however, is that if your faces-config.xml resource *is* a
static resource under the content directory in your web application, then it
is accessible to web clients that do a GET on the appropriate URL. Some
people (including me :-) would consider this a security hole.
> Second thing to note is that the spec defines this context init
> > parameter as
> > defining *context-relative* resource paths to faces-config.xmlresources,
> > not to classpath resources inside a JAR.
> I cant find a pointer to what you say about the spec, however, the way
> how your explicitResource() method works is the same as the one how
> myfaces lookup the faces-config.xml. The patch didn't change the way how
> to lookup resources pointed to by javax.faces.CONFIG_FILES.
See Section 10.1.3, first bullet:
javax.faces.CONFIG_FILES -- Comma-delimited list of context relative
resource paths under which the JSF implementation will look for
application
configuration resources (see Section 10.4.3 "Application Configuration
Resource Format") before loading a configuration resource named
"/WEB-INF/faces-config.xml" (if such a resource exists).
The RI class that reads this init parameter (
com.sun.faces.config.ConfigureListener obeys this definition ... it treats
all the paths as context relative, not class loader relative.
> Any application that relies on the
> > latter (which I assume the MyFaces impl must be supporting?) is not
> > going to
> > be portable.
> I had a look at the JSF-RI impl, there you will find in
> ConfigureListener the method "getContextURLForPath" which will be used
> to lookup the configuration files configured in
> javax.faces.CONFIG_FILES. This method use
> ServletContext.getResource(path) - the same as shale-tiger use, and for
> sure myfaces-impl.
Right. That looks for a static resource in the webapp, not a resource
buried inside a JAR file,. because it uses ServletContext.getResource()
instead of ClassLoader.getResource().
The two most important JSF implementations behaves the same way, also
> shale-tiger uses this strategy to lookup javax.faces.CONFIG_FILES
> configured faces-config.xml files.
>
> So my patch simply completes the webArchives lookup strategy.
Ciao,
> Mario
>
>
Craig
Re: [jira] Commented: (SHALE-295) collect web archives with explicit
configured faces-config.xml pointing to them
Posted by Mario Ivankovits <ma...@ops.co.at>.
Hi Craig,
or should I say - good morning :-)
>> javax.faces.CONFIG_FILES=/content/app/conf/faces-config.xml
>> ,/content/app2/conf/faces-config.xml
>
> OK, so lets continue the "twenty questions" game for a minute, and
> ask, why
> would you have a faces-config.xml file nested like this? JSF is going to
> ignore it as well.
Why will JSF ignore it? I dont understand what you mean. (I checked the
JSR-RI source, please read on)
Our application works using such a configuration like a charme.
We use the myfaces JSF impl, but as far as I remember, this worked with
JSF-RI too.
> The whole point of the META-INF convention in the spec
> is so that you do not have to explicitly configure things like this.
Yes, I know this, but we decided to use the javax.faces.CONFIG_FILES way.
> Second thing to note is that the spec defines this context init
> parameter as
> defining *context-relative* resource paths to faces-config.xml resources,
> not to classpath resources inside a JAR.
I cant find a pointer to what you say about the spec, however, the way
how your explicitResource() method works is the same as the one how
myfaces lookup the faces-config.xml. The patch didn't change the way how
to lookup resources pointed to by javax.faces.CONFIG_FILES.
> Any application that relies on the
> latter (which I assume the MyFaces impl must be supporting?) is not
> going to
> be portable.
I had a look at the JSF-RI impl, there you will find in
ConfigureListener the method "getContextURLForPath" which will be used
to lookup the configuration files configured in
javax.faces.CONFIG_FILES. This method use
ServletContext.getResource(path) - the same as shale-tiger use, and for
sure myfaces-impl.
The two most important JSF implementations behaves the same way, also
shale-tiger uses this strategy to lookup javax.faces.CONFIG_FILES
configured faces-config.xml files.
So my patch simply completes the webArchives lookup strategy.
Ciao,
Mario
Re: [jira] Commented: (SHALE-295) collect web archives with explicit configured faces-config.xml pointing to them
Posted by Craig McClanahan <cr...@apache.org>.
On 9/29/06, Mario Ivankovits <ma...@ops.co.at> wrote:
>
> Hi Craig!
>
> Ok, lets concentrate only on the patch. Please, forget the discussion
> about speedup, this has nothing to do with the patch.
>
> I do not want to overload CONFIG_FILES nor change its meaning.
> Say you have a web.xml with the following configuration:
>
> javax.faces.CONFIG_FILES=/content/app/conf/faces-config.xml
> ,/content/app2/conf/faces-config.xml
OK, so lets continue the "twenty questions" game for a minute, and ask, why
would you have a faces-config.xml file nested like this? JSF is going to
ignore it as well. The whole point of the META-INF convention in the spec
is so that you do not have to explicitly configure things like this.
Second thing to note is that the spec defines this context init parameter as
defining *context-relative* resource paths to faces-config.xml resources,
not to classpath resources inside a JAR. Any application that relies on the
latter (which I assume the MyFaces impl must be supporting?) is not going to
be portable.
Craig
The first faces-config.xml is located in myapp.jar and the second one is
> located in myapp2.jar.
> Both jars do NOT have a META-INF/faces-config.xml due to the use of
> javax.faces.CONFIG_FILES
>
> shale-tiger ignores those jars due to this fact.
>
> The patch do nothing else than to lookup those jars and add them to the
> list of webArchives.
>
>
> Just for safety I changed the List to a TreeSet. It is possible that
> such a jar contains a META-INF/faces-config.xml and also some additional
> special config.xml pointing to with javax.faces.CONFIG_FILES. To not
> parse them twice is the reason why I changed it to a Set.
>
>
> You see, no special tricks with javax.faces.CONFIG_FILES, nor any other
> style of class scanning.
>
>
> Ciao,
> Mario
>
>
Re: [jira] Commented: (SHALE-295) collect web archives with explicit
configured faces-config.xml pointing to them
Posted by Mario Ivankovits <ma...@ops.co.at>.
Hi Craig!
Ok, lets concentrate only on the patch. Please, forget the discussion
about speedup, this has nothing to do with the patch.
I do not want to overload CONFIG_FILES nor change its meaning.
Say you have a web.xml with the following configuration:
javax.faces.CONFIG_FILES=/content/app/conf/faces-config.xml,/content/app2/conf/faces-config.xml
The first faces-config.xml is located in myapp.jar and the second one is
located in myapp2.jar.
Both jars do NOT have a META-INF/faces-config.xml due to the use of
javax.faces.CONFIG_FILES
shale-tiger ignores those jars due to this fact.
The patch do nothing else than to lookup those jars and add them to the
list of webArchives.
Just for safety I changed the List to a TreeSet. It is possible that
such a jar contains a META-INF/faces-config.xml and also some additional
special config.xml pointing to with javax.faces.CONFIG_FILES. To not
parse them twice is the reason why I changed it to a Set.
You see, no special tricks with javax.faces.CONFIG_FILES, nor any other
style of class scanning.
Ciao,
Mario
Re: [jira] Commented: (SHALE-295) collect web archives with explicit configured faces-config.xml pointing to them
Posted by Craig McClanahan <cr...@apache.org>.
On 9/29/06, Mario Ivankovits <ma...@ops.co.at> wrote:
>
> Hi!
> > Yes, I did.
> >
> > Its a very simple one.
> >
> >
> > No, I totally disagree.
> >
> > There is *zero* documentation on what the changes attempt to accomplish.
> There is a one-liner above the code, maybe not the best english, but hey
> .... its not zero ;-)
Looking from the perspective of someone reviewing the commit notification
related to this patch (if it were applied), this is *effectively* zero
because it does not establish any motivation for why the change should be
made in the first place.
> Besides that, there seem to be a large number of changes that are
> > (from the
> > viewpoint of an external person) totally gratuitous -- such as
> > changing data
> > types from List<...> to Collection<...>.
> This was required to being able to use a TreeSet instead of a list to
> avoid duplicate adding of jars.
Totally unnecessary, since the outer loop is iterating over the jar files
that exist in WEB-INF/lib and will never process the same jar twice.
> As far as I understood your patch, you were planning to remove the
> > condition
> > about jars that had a META-INF/faces-conifg.xml file. Is that not the
> > acase?
> I didnt remove anything, I just *added* a functionality.
> Nothing changed to the previous behaviour.
>
> This patch and the discussion "[tiger] speedup of startup and extension
> to view annotation" are two completely different topics.
Then it should be posted on a completely separate JIRA issue. I am
complaining about the fact that SHALE-295 makes no attempt to explain *why*
the proposed patch should be applied, or *what* the proposed patch is
supposed to accomplish. Wihtout those two things, there is no way I'm going
to apply it.
Now to the patch: We heavily use the javax.faces.CONFIG_FILES context
> parameter to configure which faces-config.xml to load.
> As it is currently, shale-tiger will ignore our jars as we do not have a
> META-INF/faces-config.xml, instead we placed ours e.g. in
> content/APP/webconf/faces-config.xml
> Now by checking also the jars containing faces-config.xml configured by
> CONFIG_FILES we can make even such a configuration work with shale-tiger.
> Using your explicitResources() method will not increase or decrease
> speedup. This is not what the patch tried to address.
>
> I hope now its clear what I mean and what this patch do.
The javax.faces.CONFIG_FILES context init parameter is defined to be a
comma-separated list of faces-config.xml resoures. You seem to be proposing
to overload it as also pointing at jar files to be processed, or package
name wildcards to be matched. Those uses are contrary to the documented
behavior of this context init paramater (in the JSF Specification).
Therefore, at a minimum, a different paramter name MUST be used.
Even if a different parameter name is used to specify package names to look
at, you still haven't convinced me that the suggested changes will not
*hurt* startup performance, rather than *improve* it.
Ciao,
> Mario
>
>
Craig
Re: [jira] Commented: (SHALE-295) collect web archives with explicit
configured faces-config.xml pointing to them
Posted by Mario Ivankovits <ma...@ops.co.at>.
Hi!
> Yes, I did.
>
> Its a very simple one.
>
>
> No, I totally disagree.
>
> There is *zero* documentation on what the changes attempt to accomplish.
There is a one-liner above the code, maybe not the best english, but hey
.... its not zero ;-)
> Besides that, there seem to be a large number of changes that are
> (from the
> viewpoint of an external person) totally gratuitous -- such as
> changing data
> types from List<...> to Collection<...>.
This was required to being able to use a TreeSet instead of a list to
avoid duplicate adding of jars.
> As far as I understood your patch, you were planning to remove the
> condition
> about jars that had a META-INF/faces-conifg.xml file. Is that not the
> acase?
I didnt remove anything, I just *added* a functionality.
Nothing changed to the previous behaviour.
This patch and the discussion "[tiger] speedup of startup and extension
to view annotation" are two completely different topics.
Now to the patch: We heavily use the javax.faces.CONFIG_FILES context
parameter to configure which faces-config.xml to load.
As it is currently, shale-tiger will ignore our jars as we do not have a
META-INF/faces-config.xml, instead we placed ours e.g. in
content/APP/webconf/faces-config.xml
Now by checking also the jars containing faces-config.xml configured by
CONFIG_FILES we can make even such a configuration work with shale-tiger.
Using your explicitResources() method will not increase or decrease
speedup. This is not what the patch tried to address.
I hope now its clear what I mean and what this patch do.
Ciao,
Mario