Align CDI instantiation of AttributeConverters/EntityListeners without an explicit scope on the Jakarta Persistence spec

.. while preserving a sane/unsuprising (and pre-existing) behavior when
explicit annotations are added.

See the javadoc of `QuarkusArcBeanContainer` for details.

Author: yrodiere

extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmCdiProcessor.java

@@ -39,6 +39,7 @@
 import io.quarkus.arc.ActiveResult;
 import io.quarkus.arc.deployment.AdditionalBeanBuildItem;
 import io.quarkus.arc.deployment.AnnotationsTransformerBuildItem;
+import io.quarkus.arc.deployment.AutoAddScopeBuildItem;
 import io.quarkus.arc.deployment.BeanDefiningAnnotationBuildItem;
 import io.quarkus.arc.deployment.BeanDiscoveryFinishedBuildItem;
 import io.quarkus.arc.deployment.SyntheticBeanBuildItem;
@@ -226,6 +227,7 @@ void generateHibernateBeans(HibernateOrmRecorder recorder,
 
     @BuildStep
     void registerBeans(BuildProducer<AdditionalBeanBuildItem> additionalBeans,
+            BuildProducer<AutoAddScopeBuildItem> autoAddScope,
             BuildProducer<UnremovableBeanBuildItem> unremovableBeans,
             Capabilities capabilities,
             List<PersistenceUnitDescriptorBuildItem> descriptors,
@@ -250,9 +252,26 @@ void registerBeans(BuildProducer<AdditionalBeanBuildItem> additionalBeans,
                 .addBeanClasses(unremovableClasses.toArray(new Class<?>[unremovableClasses.size()]))
                 .build());
 
-        // Some user-injectable beans are retrieved programmatically and shouldn't be removed
-        unremovableBeans.produce(UnremovableBeanBuildItem.beanTypes(AttributeConverter.class));
+        // For AttributeConverters and EntityListeners (which are all listed in getPotentialCdiBeanClassNames),
+        // we want to achieve the behavior described in the javadoc of QuarkusArcBeanContainer.
+        // In particular:
+        // 1. They may be retrieved dynamically by Hibernate ORM, so if they are CDI beans, they should not be removed.
+        // NOTE: We don't use .unremovable on AutoAddScopeBuildItem, because that would only make beans unremovable
+        //       when we automatically add a scope.
         unremovableBeans.produce(UnremovableBeanBuildItem.beanTypes(jpaModel.getPotentialCdiBeanClassNames()));
+        // TODO: Remove, this is there for backwards compatibility.
+        //  It should only have an effect in edge cases where an attribute converter was imported from
+        //  a library not indexed in Jandex, but it's doubtful the attribute converter would work in that case.
+        unremovableBeans.produce(UnremovableBeanBuildItem.beanTypes(AttributeConverter.class));
+        // 2. Per spec, they may be handled as CDI beans even if they don't have a user-declared scope,
+        //    so we need them to default to @Dependent-scoped beans.
+        //    See https://github.com/quarkusio/quarkus/issues/50470
+        autoAddScope.produce(AutoAddScopeBuildItem.builder()
+                .match((clazz, annotations, index) -> jpaModel.getPotentialCdiBeanClassNames().contains(clazz.name()))
+                .defaultScope(BuiltinScope.DEPENDENT)
+                // ... but if they don't use CDI, we can safely default to instantiating in Hibernate ORM through reflection.
+                .requiresContainerServices()
+                .build());
     }
 
     @BuildStep

extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/cdi/QuarkusArcBeanContainer.java

@@ -11,6 +11,28 @@
 import org.hibernate.resource.beans.container.spi.ContainedBeanImplementor;
 import org.hibernate.resource.beans.spi.BeanInstanceProducer;
 
+/**
+ * A {@link org.hibernate.resource.beans.container.spi.BeanContainer} that works with other build-time elements in Quarkus to
+ * achieve a behavior that is as unsurprising as possible, while complying with the Jakarta Persistence spec where possible.
+ * The effective behavior is as follows:
+ * <ol>
+ * <li>When a class is annotated with a scope, such as <code>@ApplicationScoped</code> or <code>@Dependent</code>, we will
+ * comply with that (with some gotchas, see last item).
+ * This is not in line with the behavior from the Jakarta Persitence spec, which would ignore explicit scopes.
+ * <li>When a class is NOT annotated with any scope, it will behave as a <code>@Dependent</code> CDI bean,
+ * at least for attribute converters and entity listeners (with some gotchas, see last item).
+ * This is in line with what the Jakarta Persistence spec requires.
+ * <li>Regardless of the above, when a class is annotated with @Vetoed, it will never be instantiated through CDI, but rather
+ * through Hibernate ORM's reflection instantiation.
+ * This may or may not be in line with the behavior described in the Jakarta Persistence spec -- I did not check -- but
+ * seems sensible and intuitive enough.
+ * <li>Hibernate ORM's internal behavior means components may be cached at the persistence unit level,
+ * so even for <code>@Dependent</code> components, there would be at most one instance per persistence unit.
+ * TODO: this is what we've always done and what we assume in tests, but is this what we want?
+ * <p>
+ * Note this behavior is only possible because we give attribute converters and entity listeners the dependent scope by default:
+ * see {@code io.quarkus.hibernate.orm.deployment.HibernateOrmCdiProcessor#registerBeans}.
+ */
 @Singleton
 public class QuarkusArcBeanContainer extends AbstractCdiBeanContainer {
 
@@ -25,18 +47,15 @@ public BeanManager getUsableBeanManager() {
     @Override
     public <B> ContainedBean<B> getBean(Class<B> beanType, LifecycleOptions lifecycleOptions,
             BeanInstanceProducer fallbackProducer) {
-        // We can only support these lifecycle options. See QuarkusBeanContainerLifecycleOptions.
-        // Usually that's what we get passed (when using QuarkusManagedBeanRegistry),
-        // but in some cases Hibernate ORM calls the bean cotnainer directly and bypasses the registry,
-        // so we need to override options in that case.
-        return super.getBean(beanType, QuarkusBeanContainerLifecycleOptions.INSTANCE, fallbackProducer);
+        // Overriding lifecycle options; see QuarkusBeanContainerLifecycleOptions.
+        return super.getBean(beanType, QuarkusBeanContainerLifecycleOptions.of(lifecycleOptions), fallbackProducer);
     }
 
     @Override
     public <B> ContainedBean<B> getBean(String beanName, Class<B> beanType, LifecycleOptions lifecycleOptions,
             BeanInstanceProducer fallbackProducer) {
-        // Overriding lifecycle options; see comments in the other getBean() method.
-        return super.getBean(beanName, beanType, QuarkusBeanContainerLifecycleOptions.INSTANCE, fallbackProducer);
+        // Overriding lifecycle options; see QuarkusBeanContainerLifecycleOptions.
+        return super.getBean(beanName, beanType, QuarkusBeanContainerLifecycleOptions.of(lifecycleOptions), fallbackProducer);
     }
 
     @Override

extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/cdi/QuarkusBeanContainerLifecycleOptions.java

@@ -2,24 +2,50 @@
 
 import org.hibernate.resource.beans.container.spi.BeanContainer;
 
+/**
+ * An override of lifecycle options covering what can actually be supported in Quarkus.
+ * <p>
+ * Usually the {@link #DEFAULT} is used (when using {@link QuarkusManagedBeanRegistry}),
+ * but in some cases Hibernate ORM calls the bean container directly and bypasses the registry,
+ * so we need to override options in that case -- see {@link #of(BeanContainer.LifecycleOptions)}.
+ */
 final class QuarkusBeanContainerLifecycleOptions implements BeanContainer.LifecycleOptions {
-    public static final QuarkusBeanContainerLifecycleOptions INSTANCE = new QuarkusBeanContainerLifecycleOptions();
+    private static final QuarkusBeanContainerLifecycleOptions WITH_CACHE = new QuarkusBeanContainerLifecycleOptions(true);
+    private static final QuarkusBeanContainerLifecycleOptions WITHOUT_CACHE = new QuarkusBeanContainerLifecycleOptions(false);
 
-    private QuarkusBeanContainerLifecycleOptions() {
+    // Caching is the default in Hibernate ORM, see ManagedBeanRegistryImpl#canUseCachedReferences.
+    public static final QuarkusBeanContainerLifecycleOptions DEFAULT = WITH_CACHE;
+
+    public static QuarkusBeanContainerLifecycleOptions of(BeanContainer.LifecycleOptions options) {
+        if (options.canUseCachedReferences()) {
+            return WITH_CACHE;
+        } else {
+            return WITHOUT_CACHE;
+        }
+    }
+
+    private final boolean cache;
+
+    private QuarkusBeanContainerLifecycleOptions(boolean cache) {
+        this.cache = cache;
     }
 
     @Override
     public boolean useJpaCompliantCreation() {
         // Arc doesn't support all the BeanManager methods required to implement JPA-compliant bean creation.
-        // Anyway, JPA-compliant bean creation means we completely disregard the scope of beans
+
+        // In any case, JPA-compliant bean creation means we completely disregard the scope of beans
         // (e.g. @Dependent, @ApplicationScoped), which doesn't seem wise.
-        // So we're probably better off this way.
+        // What we do instead in Quarkus is:
+        // 1. Disable JPA-compliant creation, so we look up CDI beans and fall back to reflection if there is none.
+        // 2. Add a default scope to relevant bean types -- see io.quarkus.hibernate.orm.deployment.HibernateOrmCdiProcessor.registerBeans
+        // In effect, this gives us scope-compliant creation when classes are annotated with CDI scopes,
+        // and spec-compliant creation when they are not.
         return false;
     }
 
     @Override
     public boolean canUseCachedReferences() {
-        // Let Arc do the caching based on scopes
-        return false;
+        return cache;
     }
 }

extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/cdi/QuarkusManagedBeanRegistry.java

@@ -12,7 +12,7 @@
 /**
  * A replacement for ManagedBeanRegistryImpl that:
  * <ul>
- * <li>forces the use of QuarkusManagedBeanRegistry,
+ * <li>forces the use of {@link QuarkusArcBeanContainer},
  * which works with Arc and respects configured scopes when instantiating CDI beans.</li>
  * <li>is not stoppable and leaves the release of beans to {@link QuarkusArcBeanContainer},
  * so that the bean container and its beans can be reused between static init and runtime init,
@@ -47,15 +47,15 @@ public BeanContainer getBeanContainer() {
     @Override
     public <T> ManagedBean<T> getBean(Class<T> beanClass, BeanInstanceProducer fallbackBeanInstanceProducer) {
         return new ContainedBeanManagedBeanAdapter<>(beanClass,
-                beanContainer.getBean(beanClass, QuarkusBeanContainerLifecycleOptions.INSTANCE,
+                beanContainer.getBean(beanClass, QuarkusBeanContainerLifecycleOptions.DEFAULT,
                         fallbackBeanInstanceProducer));
     }
 
     @Override
     public <T> ManagedBean<T> getBean(String beanName, Class<T> beanContract,
             BeanInstanceProducer fallbackBeanInstanceProducer) {
         return new ContainedBeanManagedBeanAdapter<>(beanContract,
-                beanContainer.getBean(beanName, beanContract, QuarkusBeanContainerLifecycleOptions.INSTANCE,
+                beanContainer.getBean(beanName, beanContract, QuarkusBeanContainerLifecycleOptions.DEFAULT,
                         fallbackBeanInstanceProducer));
     }
 

integration-tests/jpa/src/main/java/io/quarkus/it/jpa/attributeconverter/AttributeConverterResource.java

@@ -19,20 +19,37 @@ public class AttributeConverterResource {
     EntityManager em;
 
     @GET
-    @Path("/with-cdi")
+    @Path("/with-cdi-explicit-scope")
     @Produces(MediaType.TEXT_PLAIN)
     @Transactional
-    public String withCdi(@RestQuery String theData) {
+    public String withCdiExplicitScope(@RestQuery String theData) {
         EntityWithAttributeConverters entity = new EntityWithAttributeConverters();
-        entity.setMyDataRequiringCDI(new MyDataRequiringCDI(theData));
+        entity.setMyDataRequiringCDIExplicitScope(new MyDataRequiringCDI(theData));
         em.persist(entity);
 
         em.flush();
         em.clear();
 
         // This can only return `theData` if Hibernate ORM correctly instantiates MyDataConverter through CDI
         // so that MyDataConversionService is injected.
-        return em.find(EntityWithAttributeConverters.class, entity.getId()).getMyDataRequiringCDI().getContent();
+        return em.find(EntityWithAttributeConverters.class, entity.getId()).getMyDataRequiringCDIExplicitScope().getContent();
+    }
+
+    @GET
+    @Path("/with-cdi-implicit-scope")
+    @Produces(MediaType.TEXT_PLAIN)
+    @Transactional
+    public String withCdiImplicitScope(@RestQuery String theData) {
+        EntityWithAttributeConverters entity = new EntityWithAttributeConverters();
+        entity.setMyDataRequiringCDIImplicitScope(new MyDataRequiringCDI(theData));
+        em.persist(entity);
+
+        em.flush();
+        em.clear();
+
+        // This can only return `theData` if Hibernate ORM correctly instantiates MyDataConverter through CDI
+        // so that MyDataConversionService is injected.
+        return em.find(EntityWithAttributeConverters.class, entity.getId()).getMyDataRequiringCDIImplicitScope().getContent();
     }
 
     @GET

integration-tests/jpa/src/main/java/io/quarkus/it/jpa/attributeconverter/EntityWithAttributeConverters.java

@@ -11,8 +11,11 @@ public class EntityWithAttributeConverters {
     @GeneratedValue
     private Long id;
 
-    @Convert(converter = MyDataRequiringCDIConverter.class)
-    private MyDataRequiringCDI myDataRequiringCDI;
+    @Convert(converter = MyDataRequiringCDIExplicitScopeConverter.class)
+    private MyDataRequiringCDI myDataRequiringCDIExplicitScope;
+
+    @Convert(converter = MyDataRequiringCDIImplicitScopeConverter.class)
+    private MyDataRequiringCDI myDataRequiringCDIImplicitScope;
 
     @Convert(converter = MyDataNotRequiringCDIConverter.class)
     private MyDataNotRequiringCDI myDataNotRequiringCDI;
@@ -25,12 +28,20 @@ public void setId(Long id) {
         this.id = id;
     }
 
-    public MyDataRequiringCDI getMyDataRequiringCDI() {
-        return myDataRequiringCDI;
+    public MyDataRequiringCDI getMyDataRequiringCDIExplicitScope() {
+        return myDataRequiringCDIExplicitScope;
+    }
+
+    public void setMyDataRequiringCDIExplicitScope(MyDataRequiringCDI myDataRequiringCDI) {
+        this.myDataRequiringCDIExplicitScope = myDataRequiringCDI;
+    }
+
+    public MyDataRequiringCDI getMyDataRequiringCDIImplicitScope() {
+        return myDataRequiringCDIImplicitScope;
     }
 
-    public void setMyDataRequiringCDI(MyDataRequiringCDI myDataRequiringCDI) {
-        this.myDataRequiringCDI = myDataRequiringCDI;
+    public void setMyDataRequiringCDIImplicitScope(MyDataRequiringCDI myDataRequiringCDI) {
+        this.myDataRequiringCDIImplicitScope = myDataRequiringCDI;
     }
 
     public MyDataNotRequiringCDI getMyDataNotRequiringCDI() {

integration-tests/jpa/src/main/java/io/quarkus/it/jpa/attributeconverter/MyDataNotRequiringCDIConverter.java

@@ -1,10 +1,12 @@
 package io.quarkus.it.jpa.attributeconverter;
 
+import jakarta.enterprise.inject.Vetoed;
 import jakarta.inject.Inject;
 import jakarta.persistence.AttributeConverter;
 import jakarta.persistence.Converter;
 
 @Converter
+@Vetoed // We really don't want CDI for some reason.
 public class MyDataNotRequiringCDIConverter implements AttributeConverter<MyDataNotRequiringCDI, String> {
     // This will always be null.
     // It's only here to check that this class is instantiated without relying on CDI

integration-tests/jpa/src/main/java/io/quarkus/it/jpa/attributeconverter/MyDataRequiringCDIConverter.java renamed to integration-tests/jpa/src/main/java/io/quarkus/it/jpa/attributeconverter/MyDataRequiringCDIExplicitScopeConverter.java

@@ -7,19 +7,25 @@
 
 @Converter
 @ApplicationScoped
-public class MyDataRequiringCDIConverter implements AttributeConverter<MyDataRequiringCDI, String> {
+public class MyDataRequiringCDIExplicitScopeConverter implements AttributeConverter<MyDataRequiringCDI, String> {
     @Inject
     MyCdiContext cdiContext;
 
     @Override
     public String convertToDatabaseColumn(MyDataRequiringCDI attribute) {
+        if (attribute == null) {
+            return null;
+        }
         MyCdiContext.checkAvailable(cdiContext);
-        return attribute == null ? null : attribute.getContent();
+        return attribute.getContent();
     }
 
     @Override
     public MyDataRequiringCDI convertToEntityAttribute(String dbData) {
+        if (dbData == null) {
+            return null;
+        }
         MyCdiContext.checkAvailable(cdiContext);
-        return dbData == null ? null : new MyDataRequiringCDI(dbData);
+        return new MyDataRequiringCDI(dbData);
     }
 }

integration-tests/jpa/src/main/java/io/quarkus/it/jpa/attributeconverter/MyDataRequiringCDIImplicitScopeConverter.java

@@ -0,0 +1,30 @@
+package io.quarkus.it.jpa.attributeconverter;
+
+import jakarta.inject.Inject;
+import jakarta.persistence.AttributeConverter;
+import jakarta.persistence.Converter;
+
+@Converter
+// No CDI scope here: it's implicit, which is allowed by the JPA spec
+public class MyDataRequiringCDIImplicitScopeConverter implements AttributeConverter<MyDataRequiringCDI, String> {
+    @Inject
+    MyCdiContext cdiContext;
+
+    @Override
+    public String convertToDatabaseColumn(MyDataRequiringCDI attribute) {
+        if (attribute == null) {
+            return null;
+        }
+        MyCdiContext.checkAvailable(cdiContext);
+        return attribute.getContent();
+    }
+
+    @Override
+    public MyDataRequiringCDI convertToEntityAttribute(String dbData) {
+        if (dbData == null) {
+            return null;
+        }
+        MyCdiContext.checkAvailable(cdiContext);
+        return new MyDataRequiringCDI(dbData);
+    }
+}