jsr372-experts@javaserverfaces-spec-public.java.net

[jsr372-experts] Re: 1078-DataModelRegistrable

From: Bauke Scholtz <balusc_at_gmail.com>
Date: Fri, 10 Jul 2015 15:01:23 +0200

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> 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> 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> wrote:
> >
> > Hi,
> >
> > On Monday, June 15, 2015, Josh Juneau <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
> >> 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>
> >> wrote:
> >>>
> >>> Hi,
> >>>
> >>> On Wed, Jun 10, 2015 at 3:20 PM, Kito Mann <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>
> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> On Wed, Jun 10, 2015 at 2:42 PM, Kito Mann <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>
> >>>>>> 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>
> >>>>>>> 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>
> >>>>>>>> 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>
> >>>>>>>>> wrote:
> >>>>>>>>>>
> >>>>>>>>>> +1
> >>>>>>>>>>
> >>>>>>>>>> Hanspeter
> >>>>>>>>>>
> >>>>>>>>>> Am 09.03.2015 20:41 schrieb "Frank Caputo" <
> frank_at_frankcaputo.de>:
> >>>>>>>>>>>
> >>>>>>>>>>> +1
> >>>>>>>>>>>
> >>>>>>>>>>> Ciao Frank
> >>>>>>>>>>>
> >>>>>>>>>>> Am 09.03.2015 um 17:31 schrieb Bauke Scholtz <balusc_at_gmail.com
> >:
> >>>>>>>>>>>
> >>>>>>>>>>> +1
> >>>>>>>>>>>
> >>>>>>>>>>> Cheers, B
> >>>>>>>>>>>
> >>>>>>>>>>> On Sun, Mar 8, 2015 at 11:22 PM, arjan tijms
> >>>>>>>>>>> <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
> >>>>>>
> >>>>>> * 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
> >>>>
> >>>> * 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
> >>>>
> >>>
> >>
> >
>