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 Fri, 10 Dec 2010 18:53:32 GMT
Hi guys,

As discussed, I just committed a modified version of the proposed patch.

Furthermore I added the custom SPI impl that I used for testing to the
issue [1] which may help for the real Geronimo impl.

It would be great if you guys could check if this solution works for
you, and if so, we can close this issue and start the 2.0.3 release!

Regards,
Jakob

[1] https://issues.apache.org/jira/secure/attachment/12466010/GeronimoFacesConfigurationMerger.java

2010/12/10 Jakob Korherr <jakob.korherr@gmail.com>:
> Hi Leo,
>
> OK great. That's fine with me.
>
> I will apply the appropriate changes and commit the patch!
>
> Regards,
> Jakob
>
> 2010/12/10 Leonardo Uribe <lu4242@gmail.com>:
>> Hi Jakob
>>
>> I think it is better do not include it as parameter and instead add some
>> comment on the javadoc,
>> saying the information to take into account is retrieved from
>> FacesConfigurationProvider. The fact
>> of use FacesConfigurationProvider is more an implementation detail than a
>> requeriment, so if
>> somebody wants to do not use it is valid usage.
>>
>> regards,
>>
>> Leonardo Uribe
>>
>> 2010/12/10 Jakob Korherr <jakob.korherr@gmail.com>
>>>
>>> Hi,
>>>
>>> OK, great! I added another comment to MYFACES-2945 explaining why I
>>> did it this way.
>>>
>>> Please tell me your final opinion after reading this comment and I'll
>>> commit an appropriate version of the proposed patch!
>>>
>>> Regards,
>>> Jakob
>>>
>>> 2010/12/9 Leonardo Uribe <lu4242@gmail.com>:
>>> > Hi
>>> >
>>> > I think it is not necessary to pass FacesConfigurationProvider as param
>>> > for
>>> > getFacesConfigData, because in theory we don't do anything with it
>>> > before
>>> > pass it and wrappers will not do anything with it later. I think it
>>> > looks
>>> > good to load it using
>>> >
>>> > FacesConfigurationProviderFactory.getFacesConfigurationProvider(ExternalContext).
>>> >
>>> > The other parts of the patch looks good.
>>> >
>>> > regards,
>>> >
>>> > Leonardo Uribe
>>> >
>>> > 2010/12/9 Jakob Korherr <jakob.korherr@gmail.com>
>>> >>
>>> >> Hi guys,
>>> >>
>>> >> I just uploaded a patch for a FacesConfigurationMerger SPI:
>>> >> MYFACES-2945-FacesConfigurationMerger.patch
>>> >>
>>> >> Furthermore I added a quick code sample as comment on the MYFACES-2945
>>> >> issue about how Geronimo can use this new SPI.
>>> >>
>>> >> Please take a look at the patch and if there are no objections, I will
>>> >> commit it soon!
>>> >>
>>> >> Regards,
>>> >> Jakob
>>> >>
>>> >> 2010/12/9 Jakob Korherr <jakob.korherr@gmail.com>:
>>> >> > Hi,
>>> >> >
>>> >> > I called it ugly, because of its implementation code in
>>> >> > DefaultFacesConfigurationProvider: The method is already inside
of a
>>> >> > FacesConfigurationProvider, but it does this:
>>> >> >
>>> >> > FacesConfigurationProvider provider =
>>> >> > FacesConfigurationProviderFactory.
>>> >> >            getFacesConfigurationProviderFactory(_externalContext).
>>> >> >                getFacesConfigurationProvider(_externalContext);
>>> >> >
>>> >> > ...and then calls all the other methods of FacesConfigurationProvider
>>> >> > on this provider instance.
>>> >> >
>>> >> > As I said, I know this is this way, because
>>> >> > FacesConfigurationProvider
>>> >> > can be wrapped, but IMHO it is really ugly.
>>> >> >
>>> >> >
>>> >> > The method used on MYFACES-2998 was a first approach to solve this
>>> >> > problem in a better way. However, I really like those two of your
>>> >> > suggestions:
>>> >> >
>>> >> > 1) Leo #2: Create another SPI interface for getFacesConfigData()
>>> >> > (please suggest a name for it, maybe
>>> >> > FacesConfigurationMergerProvider?) and remove this method form
>>> >> > FacesConfigurationProvider.
>>> >> > 2) Ivan: In a few words: let getFacesConfigData() on
>>> >> > FacesConfigurationProvider, but provide an
>>> >> > AbstractFacesConfigurationProvider which implements the merging
and
>>> >> > sorting to let custom SPI impls take advantage of it.
>>> >> >
>>> >> > At first, I really liked Ivan's proposal, but after thinking more
>>> >> > about it, it is not consistent with what we have right now (we
do not
>>> >> > provide any other Abstract-SPI impl) and we would have the huge
>>> >> > merging and sorting code all in the SPI(-api) package, but IMO
it
>>> >> > should really go into o.a.m.config.
>>> >> >
>>> >> > Thus I think that a new FacesConfigurationMergerProvider-SPI as
Leo
>>> >> > proposed is the best choice here. Note that for this SPI it is
good
>>> >> > practice to use other SPI impls.
>>> >> >
>>> >> > I will provide a patch for it soon and then we can discuss it
>>> >> > further!
>>> >> >
>>> >> > Regards,
>>> >> > Jakob
>>> >> >
>>> >> > 2010/12/9 Ivan <xhhsld@gmail.com>:
>>> >> >> my 2 cents, it seems for me less urgly ...
>>> >> >> a. For the FacesConfigurationProvider , it is better to have
only
>>> >> >> one
>>> >> >> method
>>> >> >> getFacesConfigData()
>>> >> >> b. Create another abstract class AbstractFacesConfigurationProvider
>>> >> >> which
>>> >> >> extends FacesConfigurationProvider, and define those proctected
>>> >> >> methods
>>> >> >> of
>>> >> >> get***, also place those sorting/merging codes there.
>>> >> >> c. In the DefaultFacesConfigurationProvider, it only implements
>>> >> >> those
>>> >> >> get***
>>> >> >> methods.
>>> >> >> thanks.
>>> >> >>
>>> >> >> 2010/12/9 Leonardo Uribe <lu4242@gmail.com>
>>> >> >>>
>>> >> >>> Hi
>>> >> >>>
>>> >> >>> I agree with Jakob about faces-config merging and ordering
>>> >> >>> algorithm
>>> >> >>> should not be exposed by MyFaces. Why is it not enough?.
At this
>>> >> >>> point
>>> >> >>> it is
>>> >> >>> not clear the reasons. Note in this moment ordering and
sorting
>>> >> >>> algoritm it
>>> >> >>> is not being exposed by FacesConfigurationProvider interface.
>>> >> >>>
>>> >> >>> Other different thing is
>>> >> >>> FacesConfigurationProvider.getFacesConfigData().
>>> >> >>> The intention of this method is provide a way to get a
Serializable
>>> >> >>> object
>>> >> >>> that represents all config information required to initialize
>>> >> >>> MyFaces
>>> >> >>> and
>>> >> >>> allow container to store it temporally somewere. In this
way it is
>>> >> >>> possible
>>> >> >>> to deploy and undeploy an application more quickly, because
if
>>> >> >>> "nothing
>>> >> >>> changes"(no added dependencies, no update from faces-config.xml
>>> >> >>> files
>>> >> >>> or
>>> >> >>> classes) this information is always the same.
>>> >> >>>
>>> >> >>> On MYFACES-2945 and previous discussions it was proposed
two
>>> >> >>> different
>>> >> >>> options:
>>> >> >>>
>>> >> >>> 1. Add getFacesConfigData()
>>> >> >>> 2. Add a set of methods to retrieve FacesConfig objects
instead.
>>> >> >>>
>>> >> >>>     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);
>>> >> >>>
>>> >> >>> The first option has the advantage that it fill the initial
>>> >> >>> requeriment
>>> >> >>> without add more complexity to the problem. The second
one seems to
>>> >> >>> be
>>> >> >>> more
>>> >> >>> structured and opens the possibility to do other things
like how
>>> >> >>> override
>>> >> >>> MyFaces parsing for faces-config.xml files like it is being
>>> >> >>> discussed.
>>> >> >>> If
>>> >> >>> the container parse faces-config.xml files, with the previous
>>> >> >>> methods
>>> >> >>> it is
>>> >> >>> possible to prevent parse the same files twice.
>>> >> >>>
>>> >> >>> My first intention, as is shown on MYFACES-2945 was that
>>> >> >>> FacesConfigurationProvider does not included getFacesConfigData(),
>>> >> >>> because
>>> >> >>> it is possible to fill the initial objective with these
methods,
>>> >> >>> but
>>> >> >>> finally
>>> >> >>> it was agreed the first option looks better, right?
>>> >> >>>
>>> >> >>> Now I see that on MYFACES-2998 that fact is questioned:
>>> >> >>>
>>> >> >>> JK>> Unfortunately it also provides a method that
should combine
>>> >> >>> all
>>> >> >>> these
>>> >> >>> data: getFacesConfigData().
>>> >> >>> JK>> This method is here due to refactorings, but
IMHO this is the
>>> >> >>> last
>>> >> >>> place where it should be put,
>>> >> >>> JK>> because it gets "its own implementation" via
its Factory and
>>> >> >>> then
>>> >> >>> calls all of the above methods on
>>> >> >>> JK>> it. I know this was introduced to support wrapping
the default
>>> >> >>> impl,
>>> >> >>> but it really is very, very ugly.
>>> >> >>>
>>> >> >>> In few words, why does it looks ugly? or with other words,
what can
>>> >> >>> we
>>> >> >>> do
>>> >> >>> to make it cleaner? remove it? or just provide another
SPI
>>> >> >>> interface
>>> >> >>> and put
>>> >> >>> that method there? In practice, getFacesConfigData() merges
all
>>> >> >>> FacesConfig
>>> >> >>> information, and "on the way" it does order applicationFacesConfig
>>> >> >>> files
>>> >> >>> (the ones obtained from getClassloaderFacesConfig() and
>>> >> >>> getContextSpecifiedFacesConfig() ) . To do that it requires
to call
>>> >> >>> all six
>>> >> >>> methods from FacesConfigurationProvider, there is no other
way, so
>>> >> >>> I
>>> >> >>> don't
>>> >> >>> see why do that is considered ugly.
>>> >> >>>
>>> >> >>> At this moment we have the following courses of action:
>>> >> >>>
>>> >> >>> 1. Remove FacesConfigurationResource interface partially,
because
>>> >> >>> it
>>> >> >>> is
>>> >> >>> still too inmature and let it for myfaces core 2.0.4.
>>> >> >>> 2. Create another SPI interface for getFacesConfigData()
(please
>>> >> >>> suggest a
>>> >> >>> name for it, maybe FacesConfigurationMergerProvider?) and
remove
>>> >> >>> this
>>> >> >>> method
>>> >> >>> form FacesConfigurationResource. Apply the patch on MYFACES-2998
>>> >> >>> seems
>>> >> >>> to be
>>> >> >>> in this direction, but forget the reason why it is wanted
to expose
>>> >> >>> getFacesConfigData() to the container.
>>> >> >>> 3. Apply something like MYFACES-2998 patch, and refactor
this one
>>> >> >>> later in
>>> >> >>> myfaces core 2.0.4
>>> >> >>>
>>> >> >>> Suggestions are welcome.
>>> >> >>>
>>> >> >>> regards,
>>> >> >>>
>>> >> >>> Leonardo Uribe
>>> >> >>>
>>> >> >>>
>>> >> >>>
>>> >> >>>
>>> >> >>
>>> >> >>
>>> >> >>
>>> >> >> --
>>> >> >> Ivan
>>> >> >>
>>> >> >
>>> >> >
>>> >> >
>>> >> > --
>>> >> > Jakob Korherr
>>> >> >
>>> >> > blog: http://www.jakobk.com
>>> >> > twitter: http://twitter.com/jakobkorherr
>>> >> > work: http://www.irian.at
>>> >> >
>>> >>
>>> >>
>>> >>
>>> >> --
>>> >> Jakob Korherr
>>> >>
>>> >> blog: http://www.jakobk.com
>>> >> twitter: http://twitter.com/jakobkorherr
>>> >> work: http://www.irian.at
>>> >
>>> >
>>>
>>>
>>>
>>> --
>>> Jakob Korherr
>>>
>>> blog: http://www.jakobk.com
>>> twitter: http://twitter.com/jakobkorherr
>>> work: http://www.irian.at
>>
>>
>
>
>
> --
> Jakob Korherr
>
> blog: http://www.jakobk.com
> twitter: http://twitter.com/jakobkorherr
> work: http://www.irian.at
>



-- 
Jakob Korherr

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

Mime
View raw message