Hi all,
I realize that people would like this, but I would like to see actual
testing instead of reasoning about the change. So unless someone does
actual testing I like to keep the change at the end to limit any
potential regressions.
The rationale behind this is:
"Adding it at the end is adding new behavior"
"Putting it up higher is changing existing behavior".
Thanks!
Kind regards,
Manfred Riem.
On 7/10/15, 8:01 AM, Bauke Scholtz wrote:
> Hi,
>
> Indeed, in corner case as described in #3 the user is itself the
> responsible for consequences, not JSF.
>
> Go ahead with checking it early.
>
> Only those ArrayDataModel checks can be simplified as if(current
> instanceof Object[]) or if(current.getClass().isArray()).
>
> Cheers, B
>
>
> On Thu, Jul 9, 2015 at 9:35 PM, arjan tijms <arjan.tijms_at_gmail.com
> <mailto:arjan.tijms_at_gmail.com>> wrote:
>
> Hi,
>
> It's now over a month since I posted the change bundle, and I wonder
> how to continue.
>
> With the code as posted the feature works for UIData. It still has to
> be repeated for UIRepeat. There were some remarks about changing the
> ordering. Currently the code checks for a user provided DataModel
> wrapper as the LAST thing in the chain, so after the existing code
> checks if the data is a List, Collection, etc.
>
> The requested change was to have it as one of the first checks, right
> after the null check and the check to see if the data is not already
> of type DataModel.
>
> The pro of doing the check early is that users or libraries can
> provide alternative wrappers for List, Collection, etc (in essence,
> overriding the JSF default behaviour).
>
> The con is that there is a (likely) rare, but possible case where
> behaviour might change.
>
> See these 3 cases I identified:
>
> 1. When an existing JSF 2.2 or earlier application that normally runs
> on a JSF 2.2/EE7 implementation now runs on the new JSF 2.3/EE8
> implementation NOTHING changes. This application nor its libraries can
> provide an @FacesDataModel annotated wrapper, since this was new in
> 2.3.
>
> 2. When an existing JSF 2.2 or earlier application that now runs on
> the new JSF 2.3/EE8 implementation upgrades one of its libraries, and
> this library happens to provide an @FacesDataModel annotated wrapper
> that happens to wrap List, and the application happens to rely on an
> obscure behaviour of the build-in List wrapper, something may
> potentially break. This is however not a silent breakage, since the
> upgrade of the library was an explicit user action, and the library
> explicitly overrides the build-in List. This is not different from any
> other library that automatically overrides some JSF default behaviour.
>
> 3. When an existing JSF 2.2 or earlier application that still runs on
> the old JSF 2.2/EE7 implementation upgrades one of its libraries, and
> this library happens to provide an @FacesDataModel annotated wrapper
> that happens to wrap List, and the application happens to rely on an
> obscure behaviour of the build-in List wrapper, then initially nothing
> happens (JSF 2.2 will not do anything with the provided
> @FacesDataModel). However if the application after having upgraded one
> of its libraries upgrades to JSF 2.3, it will now "suddenly" face
> different behaviour. Namely, the behaviour that was dormant before
> will now "suddenly" be activated.
>
> It's a question how likely 3) is to occur in practice, and how much of
> a problem this really is. After all, the user did 2 explicit upgrade
> actions and by using a library that overrides standard behaviour the
> user or library could be seen as responsible, not JSF.
>
> As for the code, checking for the DataModel wrapper EARLY looks
> like this:
>
> protected DataModel getDataModel() {
>
> // Return any previously cached DataModel instance
> if (this.model != null) {
> return (model);
> }
>
> // Synthesize a DataModel around our current value if possible
> Object current = getValue();
> if (current == null) {
> setDataModel(new ListDataModel(Collections.EMPTY_LIST));
> } else if (current instanceof DataModel) {
> setDataModel((DataModel) current);
> } else {
>
> // A type other than DataModel was provided. Check if
> there's
> // a DataModel wrapper registered for this type.
> DataModel<?> dataModel =
> createDataModel(current.getClass());
> if (dataModel != null) {
> dataModel.setWrappedData(current);
> setDataModel(dataModel);
> } else {
>
> // No registered DataModel wrapper available. Try
> our list
> // of build in wrappers
> if (current instanceof List) {
> setDataModel(new ListDataModel((List) current));
> } else if
> (Object[].class.isAssignableFrom(current.getClass())) {
> setDataModel(new ArrayDataModel((Object[])
> current));
> } else if (current instanceof ResultSet) {
> setDataModel(new
> ResultSetDataModel((ResultSet) current));
> } else if (current instanceof Result) {
> setDataModel(new ResultDataModel((Result)
> current));
> } else if (current instanceof Collection) {
> setDataModel(new
> CollectionDataModel((Collection) current));
> } else if (current instanceof Iterable) {
> setDataModel(new IterableDataModel<>((Iterable<?>)
> current));
> } else if (current instanceof Map) {
> setDataModel(new IterableDataModel<>(((Map<?, ?>)
> current).entrySet()));
> } else {
> setDataModel(new ScalarDataModel(current));
> }
> }
> }
>
> return (model);
>
> }
>
> Checking for the model LAST looks like this:
>
> protected DataModel getDataModel() {
>
> // Return any previously cached DataModel instance
> if (this.model != null) {
> return (model);
> }
>
> // Synthesize a DataModel around our current value if possible
> Object current = getValue();
> if (current == null) {
> setDataModel(new ListDataModel(Collections.EMPTY_LIST));
> } else if (current instanceof DataModel) {
> setDataModel((DataModel) current);
> } else if (current instanceof List) {
> setDataModel(new ListDataModel((List) current));
> } else if
> (Object[].class.isAssignableFrom(current.getClass())) {
> setDataModel(new ArrayDataModel((Object[]) current));
> } else if (current instanceof ResultSet) {
> setDataModel(new ResultSetDataModel((ResultSet) current));
> } else if (current instanceof Result) {
> setDataModel(new ResultDataModel((Result) current));
> } else if (current instanceof Collection) {
> setDataModel(new CollectionDataModel((Collection)
> current));
> } else if (current instanceof Iterable) {
> setDataModel(new IterableDataModel<>((Iterable<?>)
> current));
> } else if (current instanceof Map) {
> setDataModel(new IterableDataModel<>(((Map<?, ?>)
> current).entrySet()));
> } else {
> DataModel<?> dataModel =
> createDataModel(current.getClass());
> if (dataModel != null) {
> dataModel.setWrappedData(current);
> setDataModel(dataModel);
> } else {
> setDataModel(new ScalarDataModel(current));
> }
> }
> return (model);
>
> }
>
> Kind regards,
> Arjan Tijms
>
> On Mon, Jun 15, 2015 at 8:04 PM, Josh Juneau <juneau001_at_gmail.com
> <mailto:juneau001_at_gmail.com>> wrote:
> > I think it will be fine for beginners to set the parameter. I
> am going to
> > guess that most IDEs can automatically apply the parameter to
> new (or
> > upgraded) JSF apps anyways.
> >
> > Thanks for all if your excellent work on making JSF even better!
> >
> > Josh Juneau
> > http://jj-blogger.blogspot.com
> > https://www.apress.com/index.php/author/author/view/id/1866
> >
> > On Jun 15, 2015, at 12:16 PM, arjan tijms <arjan.tijms_at_gmail.com
> <mailto:arjan.tijms_at_gmail.com>> wrote:
> >
> > Hi,
> >
> > On Monday, June 15, 2015, Josh Juneau <juneau001_at_gmail.com
> <mailto:juneau001_at_gmail.com>> wrote:
> >>
> >> +1 to checking custom first and making it the default
> behavior....less
> >> configuration is better.
> >
> >
> > Definitely agree with that, although every JSF 2.3 application
> will need to
> > set the javax.faces.ENABLE_CDI_RESOLVER_CHAIN parameter already.
> >
> > So hooking into that particular parameter should not be a big
> issue, since
> > for 2.3 applications it's already there.
> >
> > For people starting with JSF (and therefore have no 2.2 or earlier
> > compatibility concern) the parameter is a bit unfortunate
> perhaps. It's
> > likely hard to explain why they need to put something that will
> probably
> > look like gibberish to them in their web.xml to fully "activate"
> JSF.
> >
> > But I don't quite see how to do it differently.
> >
> > Kind regards,
> > Arjan Tijms
> >
> >
> >
> >
> >>
> >>
> >> Josh Juneau
> >> juneau001_at_gmail.com <mailto:juneau001_at_gmail.com>
> >> http://jj-blogger.blogspot.com
> >> https://www.apress.com/index.php/author/author/view/id/1866
> >>
> >>
> >> On Wed, Jun 10, 2015 at 10:15 AM, arjan tijms
> <arjan.tijms_at_gmail.com <mailto:arjan.tijms_at_gmail.com>>
> >> wrote:
> >>>
> >>> Hi,
> >>>
> >>> On Wed, Jun 10, 2015 at 3:20 PM, Kito Mann
> <kito.mann_at_virtua.com <mailto:kito.mann_at_virtua.com>> wrote:
> >>>>
> >>>> Only people who are using JSF 2.3 will be using this
> annotation, right?
> >>>
> >>>
> >>> Basically yes. But I was half thinking about people running
> their JSF 2.2
> >>> application on a Java EE 8 server and using a library that may
> contain this
> >>> new annotation as a default for some type.
> >>>
> >>>
> >>>>
> >>>> So I say we check custom ones first and make that the default
> behavior.
> >>>
> >>>
> >>> If this was an OmniFaces feature there would be no doubt in my
> mind to
> >>> check custom ones first, so functionality wise I agree with
> this ;)
> >>>
> >>> I was just being extra careful with backwards compatibility,
> as from
> >>> previous discussions here I more or less picked up the signal
> that backwards
> >>> compatibility is the single most important consideration. But
> maybe there
> >>> isn't really an issue to begin with. So if we all, and
> specifically the spec
> >>> leads, agree to do custom checks first I'd be more than happy
> to change
> >>> this.
> >>>
> >>>
> >>>>
> >>>> Maybe provide a switch, but don't tie it to the version number.
> >>>
> >>>
> >>> Well, I was contemplating if it's wise or not to tie in to the
> existing
> >>> (in JSF 2.3) javax.faces.ENABLE_CDI_RESOLVER_CHAIN switch.
> >>>
> >>> See
> >>>
> https://github.com/omnifaces/mojarra/commit/6dd5dd1b4095cc8dbec62021ba948379aa4b51bd
> >>>
> >>> I specifically wonder what Manfred thinks about this, as he
> introduced
> >>> this switch.
> >>>
> >>> Kind regards,
> >>> Arjan Tijms
> >>>
> >>>
> >>>>
> >>>>
> >>>>
> >>>> On Wednesday, June 10, 2015, arjan tijms
> <arjan.tijms_at_gmail.com <mailto:arjan.tijms_at_gmail.com>> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> On Wed, Jun 10, 2015 at 2:42 PM, Kito Mann
> <kito.mann_at_virtua.com <mailto:kito.mann_at_virtua.com>>
> >>>>> wrote:
> >>>>>>
> >>>>>> I didn't review the code, but one thing I wonder about is the
> >>>>>> inability to register a custom DataModel for types like
> List. That is a
> >>>>>> pretty common use case.
> >>>>>
> >>>>>
> >>>>> Yes, this is a design decision we as an EG have to make.
> Maybe there's
> >>>>> even some compromise possible. E.g. using the same 2.3
> switch that's used
> >>>>> for CDI resolving to activate this feature, and then when
> activated in "2.3
> >>>>> mode" do give it the ability to override List and such.
> Implementation of
> >>>>> that is trivial (just move the check up in the list)
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On Wednesday, June 10, 2015, arjan tijms
> <arjan.tijms_at_gmail.com <mailto:arjan.tijms_at_gmail.com>>
> >>>>>> wrote:
> >>>>>>>
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> Just wondering, anyone has any feedback on the changebundle?
> >>>>>>>
> >>>>>>> Kind regards,
> >>>>>>> Arjan
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> On Sun, Jun 7, 2015 at 11:45 PM, arjan tijms
> <arjan.tijms_at_gmail.com <mailto:arjan.tijms_at_gmail.com>>
> >>>>>>> wrote:
> >>>>>>>>
> >>>>>>>> Hi there,
> >>>>>>>>
> >>>>>>>> I just attached the changebundle for this at:
> >>>>>>>>
> https://java.net/jira/secure/attachment/54975/changebundle.txt
> >>>>>>>>
> >>>>>>>> The main and only addition to the public API is the
> annotation
> >>>>>>>> @FacesDataModel that when applied to a user provided
> DataModel registers it
> >>>>>>>> with the runtime. Via an attribute it declares the class
> it's able to wrap.
> >>>>>>>>
> >>>>>>>> As for the Implementation, those classes are collected in
> >>>>>>>> CdiExtension#processBean
> >>>>>>>>
> >>>>>>>> After the collecting phase the collected classes are
> sorted in
> >>>>>>>> CdiExtension#processBean such that if an instance of any
> class from the
> >>>>>>>> collection would be a subtype of any other class in that
> collection, it has
> >>>>>>>> to appear before it. Apart from this constraint the order
> does not matter.
> >>>>>>>>
> >>>>>>>> This sorting allows a sequential scan through the
> collection to find
> >>>>>>>> the best matching type. E.g. suppose the collection would
> contain List.class
> >>>>>>>> and Collection.class, then List.class would appear before
> Collection.class.
> >>>>>>>> Given an instance of ArrayList, List would match before
> Collection.
> >>>>>>>>
> >>>>>>>> The resulting collection is then made available via CDI in
> >>>>>>>> application scope, albeit via an implementation specific
> name. For Mojarra I
> >>>>>>>> choose "comSunFacesDataModelClassesMap" for this name.
> >>>>>>>>
> >>>>>>>> Finally UIData obtains said collection via CDI and does a
> >>>>>>>> filter/findFirst on it checking if the object for which a
> DataModel wrapper
> >>>>>>>> needs to be found is assignable to the type that each
> DataModel declared to
> >>>>>>>> be able to handle. As explained above, the first match is
> the best batch.
> >>>>>>>>
> >>>>>>>> One design decision was to have the custom DataModel
> check to be
> >>>>>>>> done *after* all existing checks for wrappers are done
> (e.g. the hardcoded
> >>>>>>>> checks for List, Collection, Iterable, etc). This means
> that a custom
> >>>>>>>> DataModel cannot override those types. This was done to
> ensure maximum
> >>>>>>>> backwards compatibility, although one could argue it's
> more powerful to
> >>>>>>>> check for the custom DataModels first.
> >>>>>>>>
> >>>>>>>> Thoughts?
> >>>>>>>>
> >>>>>>>> Kind regards,
> >>>>>>>> Arjan Tijms
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On Wed, Jun 3, 2015 at 1:04 AM, arjan tijms
> <arjan.tijms_at_gmail.com <mailto:arjan.tijms_at_gmail.com>>
> >>>>>>>> wrote:
> >>>>>>>>>
> >>>>>>>>> Hi y'all,
> >>>>>>>>>
> >>>>>>>>> Just some quick update on this. After a small hiatus I
> returned to
> >>>>>>>>> this issue and started implementing it. There were a few
> small challenges
> >>>>>>>>> that made this issue a bit more difficult than initially
> anticipated.
> >>>>>>>>>
> >>>>>>>>> Specifically the fact that registering a DataModel for say
> >>>>>>>>> java.util.List requires some clever matching of class
> types, as the runtime
> >>>>>>>>> object encountered can be e.g. an ArrayList or whatever
> sub type, and the
> >>>>>>>>> "whatever sub type" can have a chain of super classes
> and a tree of
> >>>>>>>>> interfaces and their super interfaces, where each super
> class can also have
> >>>>>>>>> its own tree of interfaces and their super interfaces.
> >>>>>>>>>
> >>>>>>>>> A small hindrance is also that UIData (of course) contains
> >>>>>>>>> implementation code, but it's in the API project of
> Mojarra which
> >>>>>>>>> can't/doesn't have utility classes. So UIData and
> UIRepeat can't share
> >>>>>>>>> between each other directly, nor can either of them use
> a utility class from
> >>>>>>>>> the impl. project.
> >>>>>>>>>
> >>>>>>>>> Nevertheless I'm making good progress and hope to present a
> >>>>>>>>> changebundle for evaluation soon.
> >>>>>>>>>
> >>>>>>>>> Kind regards,
> >>>>>>>>> Arjan Tijms
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On Tue, Mar 10, 2015 at 9:17 AM, Hanspeter
> <hampidu_at_gmail.com <mailto:hampidu_at_gmail.com>>
> >>>>>>>>> wrote:
> >>>>>>>>>>
> >>>>>>>>>> +1
> >>>>>>>>>>
> >>>>>>>>>> Hanspeter
> >>>>>>>>>>
> >>>>>>>>>> Am 09.03.2015 20:41 schrieb "Frank Caputo"
> <frank_at_frankcaputo.de <mailto:frank_at_frankcaputo.de>>:
> >>>>>>>>>>>
> >>>>>>>>>>> +1
> >>>>>>>>>>>
> >>>>>>>>>>> Ciao Frank
> >>>>>>>>>>>
> >>>>>>>>>>> Am 09.03.2015 um 17:31 schrieb Bauke Scholtz
> <balusc_at_gmail.com <mailto:balusc_at_gmail.com>>:
> >>>>>>>>>>>
> >>>>>>>>>>> +1
> >>>>>>>>>>>
> >>>>>>>>>>> Cheers, B
> >>>>>>>>>>>
> >>>>>>>>>>> On Sun, Mar 8, 2015 at 11:22 PM, arjan tijms
> >>>>>>>>>>> <arjan.tijms_at_gmail.com <mailto:arjan.tijms_at_gmail.com>>
> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>> Hi,
> >>>>>>>>>>>>
> >>>>>>>>>>>> As the third part of extending the data model
> wrappers for
> >>>>>>>>>>>> UIData and
> >>>>>>>>>>>> UIRepeat, it was suggested to make data model wrappers
> >>>>>>>>>>>> registrable by
> >>>>>>>>>>>> the user. Issue JAVASERVERFACES_SPEC_PUBLIC-1078 was
> created for
> >>>>>>>>>>>> this,
> >>>>>>>>>>>> but it was also mentioned as part of
> >>>>>>>>>>>> JAVASERVERFACES_SPEC_PUBLIC-1103.
> >>>>>>>>>>>>
> >>>>>>>>>>>> I took a quick look, and now with CDI available it
> seems almost
> >>>>>>>>>>>> trivial to implement this.
> >>>>>>>>>>>>
> >>>>>>>>>>>> JSF would provide a CDI qualifier annotation like:
> >>>>>>>>>>>>
> >>>>>>>>>>>> @Retention(RUNTIME)
> >>>>>>>>>>>> @Target(TYPE)
> >>>>>>>>>>>> @Inherited
> >>>>>>>>>>>> @Qualifier
> >>>>>>>>>>>> public @interface FacesDataModel {
> >>>>>>>>>>>> Class<?> forClass();
> >>>>>>>>>>>> }
> >>>>>>>>>>>>
> >>>>>>>>>>>> The user then provides something like the following:
> >>>>>>>>>>>>
> >>>>>>>>>>>> @FacesDataModel(forClass=MyObject.class)
> >>>>>>>>>>>> public class MyDataModel<E> extends DataModel<E> {
> >>>>>>>>>>>> // datamodel methods
> >>>>>>>>>>>> }
> >>>>>>>>>>>>
> >>>>>>>>>>>> The runtime then looks up an instance of that via CDI
> (just like
> >>>>>>>>>>>> happens now for CD Converters and Validators, which
> is almost a
> >>>>>>>>>>>> one-liner in Manfred's CDIUtils).
> >>>>>>>>>>>>
> >>>>>>>>>>>> Thoughts?
> >>>>>>>>>>>>
> >>>>>>>>>>>> Kind regards,
> >>>>>>>>>>>> Arjan Tijms
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>> --
> >>>>>> ___
> >>>>>>
> >>>>>> Kito D. Mann | @kito99 | Author, JSF in Action
> >>>>>> Virtua, Inc. | http://www.virtua.com | JSF/Java EE training and
> >>>>>> consulting
> >>>>>> http://www.JSFCentral.com | @jsfcentral
> >>>>>> +1 203-998-0403 <tel:%2B1%20203-998-0403>
> >>>>>>
> >>>>>> * Listen to the Enterprise Java Newscast:
> >>>>>> http://enterprisejavanews.com
> >>>>>> * JSFCentral Interviews Podcast:
> >>>>>> http://www.jsfcentral.com/resources/jsfcentralpodcasts/
> >>>>>> * Sign up for the JSFCentral Newsletter:
> >>>>>> http://oi.vresp.com/?fid=ac048d0e17
> >>>>>>
> >>>>>
> >>>>
> >>>>
> >>>> --
> >>>> ___
> >>>>
> >>>> Kito D. Mann | @kito99 | Author, JSF in Action
> >>>> Virtua, Inc. | http://www.virtua.com | JSF/Java EE training and
> >>>> consulting
> >>>> http://www.JSFCentral.com | @jsfcentral
> >>>> +1 203-998-0403 <tel:%2B1%20203-998-0403>
> >>>>
> >>>> * Listen to the Enterprise Java Newscast:
> http://enterprisejavanews.com
> >>>> * JSFCentral Interviews Podcast:
> >>>> http://www.jsfcentral.com/resources/jsfcentralpodcasts/
> >>>> * Sign up for the JSFCentral Newsletter:
> >>>> http://oi.vresp.com/?fid=ac048d0e17
> >>>>
> >>>
> >>
> >
>
>