fix(CDI) Auto-detect beans on Rest services Refs: #30187 #30417
Conversation
…om/dotCMS/core into issue-30187-fixing-cdi-auto-detect
…om/dotCMS/core into issue-30187-fixing-cdi-auto-detect
|
@fabrizzio-dotCMS Some extra info and context. The Producer methods are only useful to create a bean if there is not already another annotated bean class, having both will create the conflicts. The default behaviour for a bean is to use the local class name as the name of the bean. As found, if there is more than one class that is being initialised as a bean with the same local name it will cause conflicts. It is possible to overide the default bean name using the @nAmed annotation e.g. @nAmed("cart"). To match this up at the point of injection you would have to use the same annotation on the injection point. Changing the class name is a cleaner fix to prevent confusion if possible, and usually using a custom Qualifier annotation is better than using @nAmed as there is no chance of typos. I am thinking we are getting conflicts here with classes not annotated with a bean defining annotation e.g. @ApplicationScoped. This may be due to a change in the default behavior of the bean discovery mode between the versions. With the downgrade we have changed to version="1.1" bean-discovery-mode="all" rather than bean-discovery-mode="annotated". It is better to use annotated as it will not load classes as beans that are not intended to be injected which can produce unintended conflicts and decrease performance due to having to scan all classes. We may be restricted here due to the behavior of version 1.1 of cdi we currently need but need to be aware of the difference and should be working towards using "annotated" always. |
dotCMS/src/main/java/com/dotcms/jobs/business/queue/PostgresJobQueue.java
Outdated
Show resolved
Hide resolved
Indeed, @spbolton , using the @nAmed annotation is a much cleaner approach. However, @jgambarios explained that the requirement involves making the producer dynamic. Based on a parameter read from the config class, it should decide whether the queue implementation is Postgres, Redis, or another option. In this case, I had to veto the Bean to exclude it accordingly. Additionally, I couldn’t get the discovery strategy for annotated Beans to work; apparently, CDI-1 only allows for full discovery of all Beans. So, that’s essentially the status of this pull request. |
|
@fabrizzio-dotCMS Yes it seems that annotation option is only for cdi 1.2 and above. I see the reason for the Producer and the need to exclude the base class in this case as it requires the dynamic behavior. Still bugging me that we should be able to get CDI 2 working even with Tomcat 9, but we have spent too many cycles on this and can cover this better when we can upgrade tomcat. We will have to pay the possible perf penalty in the short term by not having the "annotated" option though. |
…om/dotCMS/core into issue-30187-fixing-cdi-auto-detect
There was a problem hiding this comment.
Just a sec there may be an issue here with the way the beans are loaded
Looking at what happens when the beans.xml references cdi 2.0 and using annotation scanning I saw that it is trying to inject CubeJSClientFactory and failing because it is not annotated as a bean. In cdi 1.0 with "all". option it will automatically create this object as a bean. This may make things appear to be working, but in reality what is going to happen is that CDI is going to create an instance of the bean, and may in fact create a different instance of CubeJSClientFactoryImpl for each bean it is used. This will not be the same instance that is created and used from the FactoryLocator. The CubeJSClientFactory is injected in ContentAnalyticsFactoryImpl, and Application Scoped bean which is annotated.
This is the case where to properly handle this behavior both in the cdi context and non cdi context we either need to annotate the CubeJSClientFactoryImpl as @ApplicationScoped, and then modify the FactoryLocater to return the instance from the CDI context instead of creating its own, or we would annotate the getCubeJSClientFactory() method in FactoryLocator with @produces and @ApplicationScoped, to make CDI use the same instance as the Factory class. The first option would make an easier transition later.
In CDI 1.0 it is implicitly creating the bean which although seems to work on the surface may cause issues and confusion due to the multiple instances of the same class being used in the system.
It looks as if we have already done the former approach for the CONTENT_ANALYTICS_FACTORY and would just need to do the same for CUBEJS_CLIENT_FACTORY as well as add the @ApplicationScoped annotation to the implementation class.
case CONTENT_ANALYTICS_FACTORY: CDIUtils.getBean(ContentAnalyticsFactory.class).orElseThrow(() -> new DotRuntimeException("ContentAnalyticsFactory not found"));
Eventually analyticsHelper and analyticsAPI in CubeJSClientFactoryImpl would themselves be injected beans but in the short term this is not required.
|
### Proposed Changes * Jersey 2.28 works with CDI1.x * beans.xml were downgraded to use CDI 1.x * A Few "Producer" classes were removed as they were generating conflicts trying to decide what implementation had to be chosen * Updated the Resources where there were previous attempts to introduce injection * Some classes were located in different projects but had identical names like GenericBundleActivatorTest Weld didn't like that so I had to rename one of them. * CDI Utils has a new method that throws an exception as some people complained about the one returning an Optional * We have support for Integration tests through these two runners (JUnit4WeldRunner,DataProviderWeldRunner) * If we want to use CDI2.x we will have to wait until we move to tomcat 10+
Proposed Changes
This PR fixes: #30187