Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[OWB-1442] derive returnType from bean types instead of beanClass by default #89

Closed
wants to merge 1 commit into from

Conversation

jungm
Copy link
Member

@jungm jungm commented Jul 7, 2024

Proposal to resolve OWB-1442

@@ -88,6 +92,8 @@ protected AbstractOwbBean(WebBeansContext webBeansContext,
this.webBeansType = webBeansType;
this.beanClass = beanClass;
this.webBeansContext = webBeansContext;

returnType = (Class<T>) WebBeansUtil.resolveReturnType(beanAttributes.getTypes());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks neat but is kind of against the spec which explicit states what beanClass is:

getBeanClass() returns the bean class of the managed bean or of the bean that declares the producer method or field.

So for me there is no ambiguity but I ack some extensions are buggy and don't comply to CDI (using extension class or Object, they should define it to the main bean class type - potentially without generics)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm not sure if synthetic beans are managed beans: https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0#implementation

Portable extensions and build compatible extensions may provide other kinds of beans.

This implies that beans added via a portable extension are not necessarily managed beans -> the restriction in the javadoc does not necessarily apply IMO which means OWB can't rely on beanClass to be something meaningful or even represent the type of the Bean (which is the case here)

Even if that is not the case and beans registered via a portable extension are indeed managed.. Is there a requirement for OWB to use beanClass to determine which type to proxy? (At least can't find something in the specs quickly searching through it)
Looking for the most specific type in getTypes should also be sufficient I think

But I could also be completely wrong here and this discussion really should happen on the Mojarra/Weld side 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure thre is much ambiguity (for ex 5.1.4 speaks of Bean whatever the source is).
So in CDI you have Beans, some specificities in the spec about producers (very old weld limitations) and be it.
Synthetic beans are just Bean at the end and behave the same.
You would note that the extension has a specific API (getSource) so beanClass is literally intended to behave as a managed bean.

Is there a requirement for OWB to use beanClass to determine which type to proxy.

Requirement as "in the spec" no, as "in the impl" yes since types can not overlap enough and you can pick the wrong one if you don't use bean class.

Looking for the most specific type in getTypes should also be sufficient I think

Is there any requirement? 👼
But this has some issues since you can not use a large enough type so will lead to broken cases as soon as types != union(typesOf(beanClass)) for a "standard"/simple bean case.

@jungm
Copy link
Member Author

jungm commented Jul 8, 2024

Had a private conversation with @struberg on this as well, this is very likely a Mojarra bug because they use CDI in a non-portable way that isn't actually defined anywhere (backing your opinion @rmannibucau)

The registered beans are just normal managed beans and thus beanClass must point to the bean class and not the extension that registers them:

A managed bean is a bean that is implemented by a Java class. This class is called the bean class of the managed bean.

@jungm jungm closed this Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants