OmniFaces breaks FacesConverter contract #25

Closed
michaelom opened this Issue May 8, 2014 · 15 comments

Projects

None yet

4 participants

@michaelom

Repost from googlecode:

Ever since FacesConverters have been made available as CDI InjectionTargets, one of our FacesConverters does not work as expected. The converter has a no-args Constructor (for partial state saving feature) plus a constructor with a Class<?> parameter.

public MyCoolConverter(Class<?> target) {
    this.clazz = target;
}

The FacesConverter's JavaDoc states:
"...If a converter has a single argument constructor that takes a Class instance and the Class of the data to be converted is known at converter instantiation time, this constructor must be used to instantiate the converter instead of the zero-argument version..."

Using plain Mojarra the Converter is instantiated as expected, using the aforementioned constructor. Since OmniFaces uses CDI to manage the instantiation (and lifecycle) of a FacesConverter-Bean I do not see how this can be achieved (in a clear manner).

Any advice on workarounds is appreciated.

@arjantijms
Member

Hmmm, the 2 constructors are invalid in CDI so unfortunately I'm afraid that will never work.

What might be possible is that we inject the target class into the converter bean. This may be implemented by us putting the target class used by "application.createConverter(converterForClass)" into some scope or location, and then having a qualified producer for Class that knows about that place, fetches it from there and returns it.

It may then become something like:

@FacesConverter("exampleEntityConverter")
public class ExampleEntityConverter implements Converter {

@Inject @TargetConverterClass
private Class clazz;

// ... rest of code

}

@michaelom

Hi Arjan,
thanks for your response.

I like the suggested approach, though strictly speaking that still breaks the contract and is not backwards compatible. You could add the qualifier plus an @Inject-annotation to the constructor during bean discovery by wrapping AnnotatedConstructor but this would introduce a bunch of other issues and lead to a DefinitionError for Class <?>. So I really think your solution is the best for this problem.

Having access to the converter class is fundamental for any Enum-based and Interface-based converter. I am surprised this problem has not been reported before.

@arjantijms
Member

Adding the injection annotation dynamically would itself be cool, but if I'm not mistaken the biggest issue is having the two constructors. I don't think it's possible to veto one constructor (dynamically or not) specifically for CDI, is it?

So indeed, it unfortunately does break the contract and backwards compatibility but as long as the converter is a CDI bean it seems to be an impossible situation. To preserve compatibility maybe it's an option to skip managing a converter as CDI bean as soon as we discover it has multiple constructors.

I am surprised this problem has not been reported before.

Vital as the functionality may be, I've not seen it being used too often. Maybe people don't know this variant exists and just find other workarounds, I'm not sure, but it may explain why no one has reported it before.

Just to be absolutely sure we're on the same page, can you post a minimal but full example of the type of converter you're talking about and which I can use for testing? Thanks!

@michaelom

Having multiple constructors is not an issue (afaik) as long as there is only one constructor with @Inject:
http://docs.jboss.org/cdi/spec/1.0/html/implementation.html#d0e2436

You can even mix field and constructor injection. Maybe you want to give it a try?

I will post an example.

@arjantijms
Member

Hi,

Having multiple constructors is not an issue (afaik) as long as there is
only one constructor with @Inject https://github.com/Inject:
http://docs.jboss.org/cdi/spec/1.0/html/implementation.html#d0e2436

You're right, I was confused with a different situation. We're also still
learning a lot about CDI, so all community help is welcome here ;)

You can even mix field and constructor injection. Maybe you want to give
it a try?

For sure! I think we now have 3 potential situations to look at:

  1. Converter with only no-args ctor and injection of target class into field
  2. Converter with one or more ctors, but only one with @Inject for target
    class
  3. Converter complying to the JSF Converter contract having a 1 argument
    ctor for target class, but no @Inject annotations

Case 1 and 2 could be handled by the aforementioned producer. Case 3 may be
problematic.

I haven't tried case 3 yet, but you mentioned there would be other problems
if dynamically adding the injection annotation. What are those problems
exactly?

I will post an example.

Thanks again, will be much appreciated.


Reply to this email directly or view it on GitHubhttps://github.com/omnifaces/omnifaces/issues/25#issuecomment-42724338
.

@BalusC BalusC closed this May 11, 2014
@BalusC BalusC reopened this May 11, 2014
@BalusC
Member
BalusC commented May 11, 2014

Sorry for the accidental close, I somehow automatically pressed the first button saying "Comment" instead of the colored button.

Just wanted to say, it "works" when you remove the default c'tor. You only get the JSF-managed instance back, not a CDI-managed one.

@michaelom

Hi BalusC,

as I mentioned in my initial post, I do have a no-args constructor.

The converter has a no-args Constructor (for partial state saving feature) plus a constructor with a Class<?> parameter.

I also need dependency injection. I could manually lookup the BeanManger from JNDI. However, that won't solve the problem as the converter target class would still be null. In fact that was how the converter used to work prior to OmniFaces 1.6. (JNDI-lookup for every conversion) Removing the (probably useless) no-args constructor will not solve my problem.

Also I do not think that a default constructor should advice the framework (OmniFaces) to use a CDI bean instead of a JSF-managed instance. CDI does not require a no-arg constructor for @dependend-scoped beans. I could very well use constructor injection instead of field injection. If you need to make such restrictions, a DefinitionError should be thrown during bean discovery. My application should not loose it's correctness by applying a default-constructor.

I post an example next week. I have to synthesize one because I cannot post my client's code. If you can't wait: The example can easily be reproduced with a FacesConverter that takes an Interface as the forClass-argument.

@michaelom

Removing the (probably useless) no-args constructor will not solve my problem.

Ha! Actually, it will. I just would have to do a JNDI-lookup.

Yet, I do not think that developers should have to do that. That requires framework-knowledge (implementation of ConverterManager).

@michaelom

The problem has apparently been posted on stackoverflow:
http://stackoverflow.com/questions/19304827/facesconverter-inject-workaround-constructor-alternative

The suggested workaround is also to manually lookup the BeanManager.

I am OK with removing the default constructor but I still consider this as a FacesConverter contract issue. If both a default and a single-args-constructor are present the behavior of the converter should not be altered by OmniFaces.
Maybe a simple veto() will do it? Or throwing a DefinitionError if the converter defines injection points?

@michaelom

@arjantijms
Thank you for summing up potential solutions.

Here's my input: :-)
Dynamically adding annotations is actually straightforward in CDI, if you put aside the amount of boilerplate code necessary to wrap an AnnotatedType/AnnotatedConstructor/AnnotatedParamter. It's problematic because of the magic it introduces (implicit injection and implizit qualifier annotations). Your more explizit approaches are better I think.

Probably the best solution is to just veto the converter if there is a class-constructor and do not offer DI at all. (Note that veto is only necessary if there is a class-constrcutor plus a no-args-constructor.)

Anyway, thank you both for pointing me to a solution/workaround.

@arjantijms
Member

Here's my input: :-)

Dynamically adding annotations is actually straightforward in CDI, if you
put aside the amount of boilerplate code necessary to wrap an
AnnotatedType/AnnotatedConstructor/AnnotatedParamter.

Yes, I'm familiar with the boilerplate code, but luckily I have written
some of that already when I wrote this one:
http://jdevelopment.nl/disabling-ejb-timers-java-ee-6 ;)

It's problematic because of the magic it introduces (implicit injection and

implizit qualifier annotations). Your more explizit approach are better I
think.

Probably the best solution is to just veto the converter if there is a
class-constructor and do not offer DI at all. (Note that veto is only
necessary if there is a class-constrcutor plus a no-args-constructor.)

Okay, let's meditate a little over this first and then we'll try to make a
decision on how to best proceed.

@VsevolodGolovanov

Is there a way to disable the OmniFaces' CDI in FacesConverter functionality for the time being? I guess I could reconfigure the Application Chain with my factory, but that's kinda hacky.

@BalusC
Member
BalusC commented Aug 30, 2015

Noted should be that new JSF 2.3 @FacesConverter(managed=true) in its current form also doesn't support it.

@arjantijms
Member

Additionally it should be noted that the 2.3 @FacesConverter(managed=true) does not do any attempt to look up super classes or super interfaces.

In other words, of the following list from the Application#createConverter javadoc:

  • Locate a Converter registered for the target class itself.
  • Locate a Converter registered for interfaces that are implemented by the target class (directly or indirectly).
  • Locate a Converter registered for the superclass (if any) of the target class, recursively working up the inheritance hierarchy.

Only the first item it supported.

@BalusC BalusC added a commit that closed this issue Jan 7, 2017
@BalusC BalusC Fix #25: Explicitly skip converter for CDI if it does not have a default
constructor, or has a constructor taking Class argument.
2107282
@BalusC BalusC closed this in 2107282 Jan 7, 2017
@BalusC
Member
BalusC commented Jan 7, 2017 edited

Best I could do for now is to simply skip the coverter for CDI if it has no default c'tor or has a c'tor taking Class argument. Fix is available as per today's 2.6-SNAPSHOT.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment