geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jakob Korherr <jakob.korh...@gmail.com>
Subject Re: ordering tck failures in geronimo
Date Wed, 08 Dec 2010 13:30:26 GMT
Hi guys,

I really see no reason why Geronimo should implement its own
faces-config merging and ordering algorithm or even have to think
about relying on MyFaces internals. IMHO getFacesConfigData() should
be removed from FacesConfigurationProvider, because this algorithm
does not have to be customizable and thus should be removed from the
SPI.

I created MYFACES-2998 with a more detailed description and a patch
for this problem. Please review it and if there are no objections, I
will commit the patch soon.

I hope this will make your lifes a lot easier!

Regards,
Jakob

2010/12/8 Ivan <xhhsld@gmail.com>:
> Thanks for tacking this, Leonardo.
> I am not sure I understand the changes. Moving methods to the
> FacesConfigurationProvider is required, but it seems to me that it is not
> enough. With this spi, do I still need to provide those sorting/merging/...
> methods ?
> The only way I could see now is that, a. extend the wrapper class b. Use
> FacesConfigurationProviderFactory to get an instance of
> FacesConfigurationProvider. But that seems a little strange to me ...
>
> I also moved this thread from geronimo-tck to dev-geronimo, as current
> discussion is not related to tck, and no related message in current email.
> thanks.
>
> 2010/12/8 Leonardo Uribe <lu4242@gmail.com>
>>
>> Hi
>>
>> 2010/12/7 David Jencks <david_jencks@yahoo.com>
>>>
>>> On Dec 7, 2010, at 10:12 AM, Leonardo Uribe wrote:
>>>
>>> Hi
>>>
>>> 2010/12/6 Ivan <xhhsld@gmail.com>
>>>
>>>>     So, at least could you please help to add the export the
>>>> org.apache.myfaces.config package in 2.0.3, that will make my life easier
>>>> now. And remove it once those methods are moved to
>>>> FacesConfigurationProvider in the future.
>>>
>>>
>>> Instead do that, it is better to add them now. (I committed it on
>>> MYFACES-2945) It is a change already studied, so I don't see any problem
>>> doing it. I did some small changes too, so it could be good if you try it to
>>> see if everything is ok (I'll wait for your response) .
>>>
>>> 2010/12/7 David Jencks <david_jencks@yahoo.com>
>>>>
>>>>
>>>> I have not compared the myfaces classes with the openejb jaxb tree for
>>>> faces-config.xml, so I have no idea how plausible this idea might be.  Would
>>>> it be possible to annotate the myfaces object tree representing
>>>> faces-config.xml so that it could be read in by jaxb as well as by digester?
>>>> For use in geronimo, I wonder if putting some or all of this code in a
>>>> fragment bundle hosted by the myfaces bundle would reduce the number of
>>>> exports needed.
>>>
>>> I don't think so, because do that requires to add a dependency to jaxb on
>>> myfaces impl, and we have one xml library dependency already
>>> (commons-digester).
>>>
>>> Classes containing annotations that aren't available at runtime should
>>> still load fine, so this would be a compile time dependency but optional at
>>> runtime.  It would possibly make the commons-digester dependency optional at
>>> runtime as well (at least with non-myfaces additional code that uses jaxb)
>>
>> I have never tried, but what I understand is only annotations with
>> @Retention(RetentionPolicy.SOURCE) are discarded by the compiler. But if it
>> is possible to add jaxb as an optional dependency, yes, myfaces could
>> include those annotations. Anyway in this moment it is just a possibility,
>> so I will not believe it until I see it. Note JSF 2.0 is JDK 1.5 compatible.
>> That means we should take care on keep MyFaces working in 1.5.
>>
>> regards,
>>
>> Leonardo Uribe
>>
>>>
>>> thanks
>>> david jencks
>>>
>>> regards,
>>>
>>> Leonardo URibe
>>>
>>>>
>>>> thanks
>>>> david jencks
>>>>
>>>> For item b, I created a sub classes of
>>>> DefaultFacesConfigurationProvider, and just overrides those get*** methods,
>>>> the class is look like :
>>>> --->
>>>> public class GeronimoFacesConfigurationProvider extends
>>>> DefaultFacesConfigurationProvider {
>>>>
>>>>     private FacesConfig annotationsFacesConfig;
>>>>
>>>>     private List<FacesConfig> classloaderFacesConfigs;
>>>>
>>>>     private List<FacesConfig> contextSpecifiedFacesConfigs;
>>>>
>>>>     private FacesConfig webAppFacesConfig;
>>>>
>>>>     private FacesConfig standardFacesConfig;
>>>>
>>>>     public GeronimoFacesConfigurationProvider(FacesConfig
>>>> standardFacesConfig, FacesConfig webAppFacesConfig, FacesConfig
>>>> annotationsFacesConfig, List<FacesConfig> classloaderFacesConfigs,
>>>>             List<FacesConfig> contextSpecifiedFacesConfigs)
{
>>>>         this.annotationsFacesConfig = annotationsFacesConfig;
>>>>         this.classloaderFacesConfigs = classloaderFacesConfigs;
>>>>         this.contextSpecifiedFacesConfigs =
>>>> contextSpecifiedFacesConfigs;
>>>>         this.webAppFacesConfig = webAppFacesConfig;
>>>>         this.standardFacesConfig = standardFacesConfig;
>>>>     }
>>>>
>>>>     @Override
>>>>     protected FacesConfig getAnnotationsFacesConfig(ExternalContext
>>>> ectx, boolean metadataComplete) {
>>>>         return annotationsFacesConfig;
>>>>     }
>>>>
>>>>     @Override
>>>>     protected List<FacesConfig>
>>>> getClassloaderFacesConfig(ExternalContext externalContext) {
>>>>         return classloaderFacesConfigs;
>>>>     }
>>>>
>>>>     @Override
>>>>     protected List<FacesConfig>
>>>> getContextSpecifiedFacesConfig(ExternalContext externalContext) {
>>>>         return contextSpecifiedFacesConfigs;
>>>>     }
>>>>
>>>>     @Override
>>>>     protected FacesConfig getStandardFacesConfig(ExternalContext
>>>> externalContext) {
>>>>         return standardFacesConfig;
>>>>     }
>>>>
>>>>     @Override
>>>>     protected FacesConfig getWebAppFacesConfig(ExternalContext
>>>> externalContext) {
>>>>         return webAppFacesConfig;
>>>>     }
>>>>
>>>> }
>>>> <---
>>>> Thoughts ?
>>>> thanks.
>>>>
>>>> 2010/12/7 Leonardo Uribe <lu4242@gmail.com>
>>>>>
>>>>> Hi
>>>>>
>>>>> First of all, I want to note that if the default algorithm for
>>>>> FacesConfigResourceProvider is not able to find a.faces-config.xml and
>>>>> c.faces-config.xml, that means the Thread Context Class Loader needs
to be
>>>>> fixed, because is not taking into account jar files under WEB-INF/lib.
>>>>>
>>>>> 2010/12/6 Ivan <xhhsld@gmail.com>
>>>>>>
>>>>>>
>>>>>> 2010/12/6 David Jencks <david_jencks@yahoo.com>
>>>>>>>
>>>>>>> On Dec 5, 2010, at 7:44 PM, Ivan wrote:
>>>>>>>
>>>>>>> > I am wondering whether the myfaces bundle could also export
the
>>>>>>> > spi.impl package, I hope to take advantage of some codes
there, e.g.
>>>>>>> > DefaultFacesConfigurationProvider, it contains some sort
algorithm.
>>>>>>> > thanks.
>>>>>>> >
>>>>>>>
>>>>>
>>>>> Why export spi.impl package? the idea to have an spi api and impl is
>>>>> allow impl package to change and let api stable to prevent break code
later,
>>>>> right? If something should be exposed from DefaultFacesConfigurationProvider
>>>>> (for example adding some methods on FacesConfigurationProvider), it should
>>>>> be discussed first.
>>>>>
>>>>> I agree to expose this two:
>>>>>
>>>>> +
>>>>> org.apache.myfaces.config.impl.digester.elements;version="${project.version}",
>>>>> +
>>>>> org.apache.myfaces.config.impl.digester;version="${project.version}
>>>>> because theorically the code there will not change (only between
>>>>> different versions of JSF), but the other ones:
>>>>>
>>>>> +
>>>>> org.apache.myfaces.spi.impl;version="${project.version}",
>>>>> +
>>>>> org.apache.myfaces.config;version="${project.version}",
>>>>>
>>>>> needs some argumentation first.
>>>>>
>>>>>>>
>>>>>>> I'd say if you need to expose the default implementation of the
spi
>>>>>>> then there is something wrong with the interface design.  If
the sorting is
>>>>>>> universal then perhaps it should not be in a class implemented
by a service
>>>>>>> provider?
>>>>>>
>>>>>
>>>>> Ok, as long as I undersand there is no reason why expose sorting
>>>>> algorithm to the container. Also, allow a different parser for FacesConfig
>>>>> was discussed before. I remember on MYFACES-2945 that it was proposed
an
>>>>> interface with this methods:
>>>>>
>>>>> public abstract class FacesConfigurationProvider
>>>>> {
>>>>>     public abstract FacesConfig getStandardFacesConfig(ExternalContext
>>>>> ectx);
>>>>>
>>>>>     public abstract FacesConfig
>>>>> getMetaInfServicesFacesConfig(ExternalContext ectx);
>>>>>
>>>>>     public abstract FacesConfig
>>>>> getAnnotationsFacesConfig(ExternalContext ectx, boolean metadataComplete);
>>>>>
>>>>>     public abstract List<FacesConfig>
>>>>> getClassloaderFacesConfig(ExternalContext ectx);
>>>>>
>>>>>     public abstract List<FacesConfig>
>>>>> getContextSpecifiedFacesConfig(ExternalContext ectx);
>>>>>
>>>>>     public abstract FacesConfig getWebAppFacesConfig(ExternalContext
>>>>> ectx);
>>>>>
>>>>> }
>>>>>
>>>>> But finally the final form was this:
>>>>>
>>>>> public abstract class FacesConfigurationProvider
>>>>> {
>>>>>
>>>>>     public abstract FacesConfigData getFacesConfigData(ExternalContext
>>>>> ectx);
>>>>>
>>>>> }
>>>>>
>>>>> And this comment was added:
>>>>>
>>>>> "...After some attempts, the structure of the solution is not too
>>>>> different from the previous patch, because after all in
>>>>> DefaultFacesConfigurationProvider it is necessary to put all previous
code
>>>>> plus ordering and feeding of FacesConfig files...."
>>>>>
>>>>> Note the first form allows to define an custom parser, because to
>>>>> retrieve FacesConfig you should locate them first and then parse them,
but
>>>>> the second one does not.
>>>>>
>>>>>>    Yes, in Geronimo, we have a sepearted algorthim for sorting.
>>>>>> Currently, I still use the implementation from MyFaces to do it,
maybe in
>>>>>> the future I could move them to Geronimo side or it will be abstracted
from
>>>>>> current default spi provider. I also need to provide a mock external
context
>>>>>> to make it work. or I might need to copy some codes from myfaces.
>>>>>>   Another thing is that, I use the digester xml parser from MyFaces
to
>>>>>> parse all the faces configuration files, while we using jaxb tree
in the
>>>>>> past. This could be avoid, just did not want to add some codes to
convert
>>>>>> two FacesConfig objects.
>>>>>
>>>>> The idea behind refactor org.apache.myfaces.config.element package is
>>>>> give some base classes from myfaces, to allow use a different parser.
Those
>>>>> changes were committed, but to allow that it is still necessary to commit
>>>>> some methods on FacesConfigurationProvider to do the "bridge".
>>>>>
>>>>> regards,
>>>>>
>>>>> Leonardo Uribe
>>>>>
>>>>>>   Thanks.
>>>>>>
>>>>>>>
>>>>>>> thanks
>>>>>>> david jencks
>>>>>>>
>>>>>>> >
>>>>>>> > --
>>>>>>> > Ivan
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Ivan
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Ivan
>>>>
>>>
>>>
>>
>
>
>
> --
> Ivan
>



-- 
Jakob Korherr

blog: http://www.jakobk.com
twitter: http://twitter.com/jakobkorherr
work: http://www.irian.at

Mime
View raw message