Revert #5157

[struberg]

Describe the bug

The issue #5157 changed a behaviour of Mojarra which was there since 10 years++ to a new version which breaks Apache OpenWebBeans and thus TomEE and other containers. Thus I'd kindly ask to revert to the old implementation.

See https://github.com/eclipse-ee4j/mojarra/issues/5157

To Reproduce

See https://issues.apache.org/jira/browse/TOMEE-4355 on how to reproduce

Expected behavior

The CDI spec is not very outspoken about what a custom Bean should return for the getBeanClass() method. JBoss Weld and Apache OpenWebBeans use this information in subtly different ways. It also seems that something did change in behaviour in late 2022 in either Weld or Wildfly resulting in a visibility issue in Wildfly.

In section "3.5. Bean constructors" the bean class is defined as the class the constructor is called for. If you e.g. have a custom Bean<UIViewRoot> then you want the constructor of UIViewRoot to be invoked, not the one of the Extension class registering that bean. In OWB this information is used to determine which proxy to be created for the contextual references of that bean. There are also other points in the spec which underline the OWB interpretation. Further note that it would be rather easy to store away the information about the CDI Extension which registers a bean with the various addBean() methods as the CDI container always exactly knows which CDI Extension.

3rd note: visibility in CDI was broken from the beginning for more complex cases like EAR deployments. This started with the famous CDI-18 and CDI-129 tickets and went on to this day. To fix this the spec would need to introduce a HierarchicBeanManager to correspond with the hierarchic ClassLoaders. I had a long discussion with Bill, Linda, Pete et al back then. Some findings made it to a blog post https://struberg.wordpress.com/2015/02/18/cdi-in-ears/ All this might not get enough traction as the importance of EARs mostly faded over the last decade.

Note that this is a well known topic in the CDI spec and has been discussed since the CDI-1.0 days with no decision in either direction.

[BalusC]

cc: @arjantijms @jasondlee @manovotn

[manovotn]

@BalusC I do not think we should revert this.

In his comment, Mark has a similar notion to what I arrived to in my original issue - the spec isn't explicit on what exactly are all the valid values for the bean class in case of synth. beans.

The specification section on Bean constructors that was mentioned above talks about class-based beans which isn't helping here as the topic of the previous issue/PR were synthetic beans.

From the little that you can find, I'd point out the following javadocs. Looking at jakarta.enterprise.inject.spi.Bean#getBeanClass:

The bean {@linkplain Class class} of the managed bean or session bean or of the bean that declares the producer method or field.

So even producers do not return the class of the actual bean, but that of its declaring bean. Therefore, the specification section quoted above wouldn't even apply here so implying that it should be the case for synth. beans is in my opinion quite a stretch.

Furthermore, looking at jakarta.enterprise.inject.spi.configurator.BeanConfigurator#beanClass:

Set the class of the configured Bean. If not set, the extension class is used.

This explicitly says that not setting the class results in extension class being used as a default and that such setting is valid.

My PR changes weren't based solely on Weld behavior but on what I could find in the CDI specification to be a workable default for any implementation. Yes, both Weld and OWB use the beanClass information in some nuanced way but the PR doesn't break anything per specification - it actually uses the only specified default value.

As a side note, you could also draw quite a few similarities between declaring a producer and declaring a synth bean - in both cases you control/declare the creation login, bean types, qualifiers and so on. Neither producers nor synth. beans necessarily have a backing class behind them and it therefore seems logical to me that the beanClass should behave similarly meaning you get the "declaring class" for such a bean - an extension in case of synth beans.

[rmannibucau]

I agree with Mark, most of the spec, including the Bean javadoc states that bean class must back the bean class except for producers which have a special meaning.

Indeed it is only relevant for proxied classes - ignored in all other cases - where it must be used as a base proxy class to avoid a random algorithm based on getTypes() - note that a "clever" algorithm will end random as soon as you customize types. I agree there are cases it can work but as much as trivial it is to break that. So long story short the only way to get something robust is to respect beanClass and set the beanclass to use - and it is fine it is a type from getTypes().

So current API usage is likely the promise of issues and shouldn't be nor promoted but be considered as a bug and API misusage IMHO.

[struberg]

@manovotn the definition of the producer method shows exactly what I claimed: It is the class of the bean type, except for producer methods where it is the bean class of the owner bean. The term "bean class" is mentioned 135 times in the CDI 2.0 spec. Almost all of those rather point to the interpretation that it represents the 'thing to be injected'. Note that the other quote from the API (default method) stems from a much later version of the spec and still does not say anything about what the beanClass actually should be.

Just for understanding the Weld position: what does Weld do if Bean#getTypes() return a Set of 7 unrelated interfaces but no concrete class? Do you create different interface proxies for each injection point?

And what is the benefit of having the Extension class returned in getBeanClass()? Where in the spec can I find that getBeanClass() is used for any visibility rules like it seems is - also not covered by the spec - assumed and even requested by Weld. Note that the term 'visible' in the CDI spec is solely used for the visibility of Contextual Instances in relation to others. E.g. which @RequestScoped class User Contextual Instance can be seen in a very thread from another say @ApplicationScoped class MailService;.

[rmannibucau]

@struberg weld does a subclass of the runtime instance and uses the parent class of getTypes() (TypeInfo) which means if you tune a bit the types you end up with Object as root class...but since it does not work weld also has some code path where it uses getBeanClass. So the point is there, there is no location in the CDI spec except beanClass to provide the proper class to create the proxy based on (the "parent") and there is no way with CDI API to deduce the right class in all cases. So the best an impl can do is not to randomly pick a class (think getRef(Object) case where reflection must work) but rather use what the impl provided, ie beanClass as soon as a proxy is needed - for all other cases it is fine to put whatever you want in beanClass. This is the best CDI users and vendors can do until CDI provide a getDefaultRuntimeClass in its API - but once again this is totally useless since beanClass is really the method for that. Side note: returning an extension class in beanClass is a clear abuse of the CDI original intent since you then couple the bean with their source violating IoC/CDI concept.

[manovotn]

Note that the other quote from the API (default method) stems from a much later version of the spec and still does not say anything about what the beanClass actually should be.

Much older being literally the first commit adding that API which can be seen here. There is no history past that point, this was part of the initial version of the BeanConfigurator API. And yes, it does not say what the value should be but it says what it can be - and that's enough for the sake of this discussion.

the definition of the producer method shows exactly what I claimed: It is the class of the bean type, except for producer methods where it is the bean class of the owner bean

I guess we won't agree here because you can impose an equally valid assumption that synth. beans should be viewed as produced bean and the bean class should be that of declaring extension :shrug: Neither is confirmed or refuted by the specification and both are sensible yet differing interpretations.

And what is the benefit of having the Extension class returned in getBeanClass()? Where in the spec can I find that getBeanClass() is used for any visibility rules like it seems is - also not covered by the spec - assumed and even requested by Weld.

My previous comment didn't mention anything about visibility rules; I intentionally used arguments and a default value defined by the specification. You could delve deeper into the WFLY related issues and PR to see what Weld does but that's not really relevant to the discussion here.

But, speaking of visibility in the specification, I can add another slightly related quote. This one is from enabling alternatives per bean archive:

For a custom implementation of the Bean interface defined in The Bean interface, the container calls isAlternative() to determine whether the bean is an alternative, and getBeanClass() and getStereotypes() to determine whether an alternative is selected in a certain bean archive.

Based on this, the getBeanClass() method is used to derive whether a bean is enabled in a given bean archive. So if your bean class is java.lang.Object or java.util.Map (which were previously present in Mojarra IIRC), how do you tell in which archive you enable such bean? Whereas having an extension as beanClass value allows you to identify that because extensions are typically part of a bean archive.

Nonetheless, the essence of this issue seems to be that the CDI specification is not really clear on valid values for beanClass but has always had a well defined default value that Mojarra now uses. Back when I sent the PR, I took care to follow what I could find in the spec, not what Weld uses it for, and I had no idea that OWB just chose to not adhere to it.

I am sorry but with the above in mind, I don't think that claims such as the default value being a clear abuse of the specification hold any water.

[rmannibucau]

And yes, it does not say what the value should be but it says what it can be - and that's enough for the sake of this discussion.

This is true and we all agreed it can be the extension class...when there is no runtime constraint after and there there is the issue since it needs a proxy so the doc is right and the bug report as well.

Neither is confirmed or refuted by the specification and both are sensible yet differing interpretations.

This is true but also means mojarra does not support CDI, it does support some Weld version so it is worth fixing.

So if your bean class is java.lang.Object or java.util.Map (which were previously present in Mojarra IIRC), how do you tell in which archive you enable such bean?

As an implementation you know it and it would be insane if the extension can change its module definition so think it is a counter example.

I am sorry but with the above in mind, I don't think that claims such as the default value being a clear abuse of the specification hold any water.

What is obvious is that:

1. it is a regression
2. the current behavior is not CDI friendly (in the sense it is not portable per spec - spec not being clear enough)

So whatever the details the revert makes sense if mojarra intend to not run only on weld.

[rzo1]

From my point of view, this is a CDI (spec) discussion atm between OWB and Weld both dealing with unclarity of the spec.

From a user perspective, I would love to see a working Mojarra 4 given it just breaks OWB-based containers in a minor version update atm, i.e. it works with 4.0.0 and suddendly breaks.

The original Wildfly issue was merly about a warning, right? So why breaking (non-Weld) users for the sake of resolving a warning based upon spec unclarity.

Could we add a workaround in Mojarra like a (system) property to conditionally add the bean class, so it doesn't break users until the ambiquity ist actually resolved in the spec itself?

[struberg]

@manovotn

There is no history past that point, this was part of the initial version of the BeanConfigurator API.

That got introduced for 2.0, that was in 2016. This is literally 8 years after all that other work we've done and the original definition of the Bean class - including the discussed method! I really consider this LATE - and it kind of contradicts the previous work, which is against EE rules.

My previous comment didn't mention anything about visibility rules;

The linked ticket does!

For a custom implementation of the Bean interface defined in The Bean interface, the container calls isAlternative() to determine whether the bean is an alternative, and getBeanClass() and getStereotypes() to determine whether an alternative is selected in a certain bean archive.

Not quite sure what I should think about it, because your interpretation directly contradicts the 12.1 Bean archives definition:

You claim

because extensions are typically part of a bean archive.

But the spec actually says:

An archive which: • contains a beans.xml file with the bean-discovery-mode of none, or, • contains an extension and no beans.xml file is not a bean archive.

Well, there IS an Extension in this JAR and there is NO beans.xml file. That means that the whole Mojarra jar is actually not a bean archive. So there is nothing to activate in. You'd need to add a META-INF/beans.xml for this to work. But that would likely break other things. This whole interpretation is unlucky I fear.

I do agree though, that the spec is not really clear. But otoh this is not a new discussion. We had this topic a few times in the EG, mostly in the context of visibility in the (now mostly obsolete) EAR deployment scenario: https://issues.redhat.com/browse/CDI-456

[manovotn]

Could we add a workaround in Mojarra like a (system) property to conditionally add the bean class, so it doesn't break users until the ambiquity ist actually resolved in the spec itself?

I am not sure this will be easily solvable on spec level given existing different approaches, interpretations and the above disagreements. That being said, +1 to some Mojarra config option which would optionally switch it to previous values so that OWB can use it. I am not sure what integrator config options Mojarra has these days but @BalusC might be able to help us out there?

[struberg]

Yes, the config option could e.g. also look up for some defaults depending on a Class.forName on key classes for OWB and Weld.

[BalusC]

Yes, the config option could e.g. also look up for some defaults depending on a Class.forName on key classes for OWB and Weld.

That's doable. There's already something in place in Mojarra which detects if Weld is being used. We could reuse that.

On the other hand, I'm wondering how this all works out when MyFaces is used. Do the Faces artifact producers work irrespective of the CDI impl being used? If so then we should probably take a look at its approach (cc: @arjantijms @tandraschko).

[BalusC]

Okay, for the record, this is what happens when I try to use @Inject FacesContext on a bean managed by OpenWebBeans 4.0.2 running on Tomcat 10.1.23:

SEVERE: Error Rendering View[/test.xhtml]
java.lang.IllegalArgumentException: Can not set jakarta.faces.context.FacesContext field com.example.Bean.facesContext to com.sun.faces.cdi.CdiExtension$$OwbNormalScopeProxy0
    at java.base/jdk.internal.reflect.UnsafeFieldAccessorImpl.throwSetIllegalArgumentException(UnsafeFieldAccessorImpl.java:167)
    at java.base/jdk.internal.reflect.UnsafeFieldAccessorImpl.throwSetIllegalArgumentException(UnsafeFieldAccessorImpl.java:171)
    at java.base/jdk.internal.reflect.UnsafeObjectFieldAccessorImpl.set(UnsafeObjectFieldAccessorImpl.java:81)
    at java.base/java.lang.reflect.Field.set(Field.java:799)
    at org.apache.webbeans.inject.InjectableField.doInjection(InjectableField.java:68)
    at org.apache.webbeans.portable.InjectionTargetImpl.injectFields(InjectionTargetImpl.java:231)
    at org.apache.webbeans.portable.InjectionTargetImpl.inject(InjectionTargetImpl.java:217)
    at org.apache.webbeans.portable.InjectionTargetImpl.inject(InjectionTargetImpl.java:207)
    at org.apache.webbeans.component.AbstractOwbBean.create(AbstractOwbBean.java:128)
    at org.apache.webbeans.component.ManagedBean.create(ManagedBean.java:66)
    at org.apache.webbeans.container.SerializableBean.create(SerializableBean.java:123)
    at com.sun.faces.application.view.ViewScopeContextManager.createBean(ViewScopeContextManager.java:133)
    at com.sun.faces.application.view.ViewScopeContext.get(ViewScopeContext.java:111)
    at org.apache.webbeans.context.CustomPassivatingContextImpl.get(CustomPassivatingContextImpl.java:46)
    at org.apache.webbeans.intercept.NormalScopedBeanInterceptorHandler.getContextualInstance(NormalScopedBeanInterceptorHandler.java:101)
    at org.apache.webbeans.intercept.NormalScopedBeanInterceptorHandler.get(NormalScopedBeanInterceptorHandler.java:71)
    at com.example.Bean$$OwbNormalScopeProxy0.toString(com/example/Bean.java)
    at org.apache.el.lang.ELSupport.coerceToString(ELSupport.java:492)
    at org.apache.el.lang.ELSupport.coerceToType(ELSupport.java:527)
    at org.apache.el.ExpressionFactoryImpl.coerceToType(ExpressionFactoryImpl.java:46)
    at jakarta.el.ELContext.convertToType(ELContext.java:319)
    at org.apache.el.ValueExpressionImpl.getValue(ValueExpressionImpl.java:192)
    at org.apache.webbeans.el22.WrappedValueExpression.getValue(WrappedValueExpression.java:67)
    at com.sun.faces.facelets.el.ELText$ELTextVariable.writeText(ELText.java:208)
    at com.sun.faces.facelets.el.ELText$ELTextComposite.writeText(ELText.java:119)
    at com.sun.faces.facelets.compiler.TextInstruction.write(TextInstruction.java:44)
    at com.sun.faces.facelets.compiler.UIInstructions.encodeBegin(UIInstructions.java:42)
    at com.sun.faces.facelets.compiler.UILeaf.encodeAll(UILeaf.java:158)
    at jakarta.faces.render.Renderer.encodeChildren(Renderer.java:146)
    at jakarta.faces.component.UIComponentBase.encodeChildren(UIComponentBase.java:556)
    at jakarta.faces.component.UIComponent.encodeAll(UIComponent.java:1435)
    at jakarta.faces.component.UIComponent.encodeAll(UIComponent.java:1438)
    at jakarta.faces.component.UIComponent.encodeAll(UIComponent.java:1438)
    at com.sun.faces.application.view.FaceletViewHandlingStrategy.renderView(FaceletViewHandlingStrategy.java:449)
    at com.sun.faces.application.view.MultiViewHandler.renderView(MultiViewHandler.java:160)
    at jakarta.faces.application.ViewHandlerWrapper.renderView(ViewHandlerWrapper.java:125)
    at com.sun.faces.lifecycle.RenderResponsePhase.execute(RenderResponsePhase.java:93)
    at com.sun.faces.lifecycle.Phase.doPhase(Phase.java:72)
    at com.sun.faces.lifecycle.LifecycleImpl.render(LifecycleImpl.java:150)
    at jakarta.faces.webapp.FacesServlet.executeLifecyle(FacesServlet.java:692)
    at jakarta.faces.webapp.FacesServlet.service(FacesServlet.java:449)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:196)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:140)
    at org.apache.tomcat.websocket.server.WsFilter.doFilter(WsFilter.java:51)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:165)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:140)
    at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:167)
    at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:90)
    at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:482)
    at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:115)
    at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:93)
    at org.apache.catalina.valves.AbstractAccessLogValve.invoke(AbstractAccessLogValve.java:673)
    at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:74)
    at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:344)
    at org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:391)
    at org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:63)
    at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:896)
    at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1736)
    at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:52)
    at org.apache.tomcat.util.threads.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1191)
    at org.apache.tomcat.util.threads.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:659)
    at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:63)
    at java.base/java.lang.Thread.run(Thread.java:840)

PR #5458 fixes that.