Merge pull request #50489 from yrodiere/i50470

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

Author: yrodiere

docs/src/main/asciidoc/hibernate-orm.adoc

@@ -464,7 +464,10 @@ using the `@PersistenceUnit` annotation at the class level is not supported in t
 
 Note that, similarly to what we do with the configuration property, we take into account the annotated package but also all its subpackages.
 
-==== CDI integration
+=== CDI integration
+
+[[injecting-entry-points]]
+==== Injecting entry points
 
 If you are familiar with using Hibernate ORM in Quarkus, you probably already have injected the `EntityManager` using CDI:
 
@@ -540,6 +543,40 @@ These components can also be injected with a specific persistence unit qualifier
 CriteriaBuilder criteriaBuilder;
 ----
 
+[[plugging-in-converters-entity-listeners]]
+==== Plugging in converters and entity listeners
+
+The Quarkus extension for Hibernate ORM supports injecting link:{hibernate-orm-docs-url}#basic-jpa-convert[attribute converters] and link:{hibernate-orm-docs-url}#events-jpa-callbacks[entity listeners] into Hibernate ORM.
+
+Just use them as you would in vanilla Hibernate ORM (see documentation linked above), and rely on CDI features (`@Inject`, `@PostConstruct`, `@PreDestroy`, ...) in the converter/listener implementation as needed.
+
+When no CDI scope is specified, the converter/listener will behave as if it was `@Dependent` and will be instantiated once per persistence unit it's used in.
+
+You can force a CDI scope by annotating the converter/listener class with for example `@ApplicationScoped`.
+
+In very exotic scenarios, if you need to prevent usage of CDI in a converter/listener even though it uses CDI annotations (`@Inject`, ...), annotate it with `@Vetoed`. The class will then be instantiated by Hibernate ORM through its default constructor.
+
+[[plugging-in-other-custom-components]]
+==== Plugging in other custom components
+
+The Quarkus extension for Hibernate ORM will automatically
+inject components annotated with `@PersistenceUnitExtension` into Hibernate Search.
+
+The annotation can optionally target a specific persistence unit with `@PersistenceUnitExtension(name = "nameOfYourPU")`.
+
+This feature is available for the following component types:
+
+`org.hibernate.Interceptor`::
+See <<interceptors>>.
+`org.hibernate.resource.jdbc.spi.StatementInspector`::
+See <<statement_inspectors>>.
+`org.hibernate.type.format.FormatMapper`::
+See <<json_xml_serialization_deserialization>>.
+`io.quarkus.hibernate.orm.runtime.tenant.TenantResolver`::
+See <<multitenancy>>.
+`io.quarkus.hibernate.orm.runtime.tenant.TenantConnectionResolver`::
+See <<programmatically-resolving-tenants-connections>>.
+
 [[persistence-unit-active]]
 === Activate/deactivate persistence units
 
@@ -1473,6 +1510,7 @@ quarkus.datasource.db-kind=postgresql <2>
 <1> Enable discriminator multi-tenancy.
 <2> xref:datasource.adoc[Configure the datasource].
 
+[[programmatically-resolving-tenants-connections]]
 === Programmatically Resolving Tenants Connections
 
 If you need a more dynamic configuration for the different tenants you want to support and don't want to end up with multiple entries in your configuration file,

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/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/JpaJandexScavenger.java

@@ -142,6 +142,12 @@ public JpaModelBuildItem discoverModelAndRegisterForReflection() throws BuildExc
             }
         }
 
+        // Hibernate ORM will fall back to instantiating these types using reflection if they are not CDI beans,
+        // so we need to enable that.
+        for (DotName javaType : collector.potentialCdiBeanTypes) {
+            reflectiveClass.produce(ReflectiveClassBuildItem.builder(javaType.toString()).constructors().build());
+        }
+
         return new JpaModelBuildItem(collector.packages, collector.entityTypes, managedClassNames,
                 collector.potentialCdiBeanTypes, collector.xmlMappingsByPU);
     }

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,36 +19,66 @@ 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();
+        // This can only return `theData` if Hibernate ORM correctly instantiates the converter through CDI.
+        return em.find(EntityWithAttributeConverters.class, entity.getId()).getMyDataRequiringCDIExplicitScope().getContent();
     }
 
     @GET
-    @Path("/without-cdi")
+    @Path("/with-cdi-implicit-scope")
     @Produces(MediaType.TEXT_PLAIN)
     @Transactional
-    public String withoutCdi(@RestQuery String theData) {
+    public String withCdiImplicitScope(@RestQuery String theData) {
         EntityWithAttributeConverters entity = new EntityWithAttributeConverters();
-        entity.setMyDataNotRequiringCDI(new MyDataNotRequiringCDI(theData));
+        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()).getMyDataNotRequiringCDI().getContent();
+        // This can only return `theData` if Hibernate ORM correctly instantiates the converter through CDI.
+        return em.find(EntityWithAttributeConverters.class, entity.getId()).getMyDataRequiringCDIImplicitScope().getContent();
+    }
+
+    @GET
+    @Path("/without-cdi-no-injection")
+    @Produces(MediaType.TEXT_PLAIN)
+    @Transactional
+    public String withoutCdiNoInjection(@RestQuery String theData) {
+        EntityWithAttributeConverters entity = new EntityWithAttributeConverters();
+        entity.setMyDataNotRequiringCDINoInjection(new MyDataNotRequiringCDI(theData));
+        em.persist(entity);
+
+        em.flush();
+        em.clear();
+
+        // This can only return `theData` if Hibernate ORM correctly instantiates the converter through reflection.
+        return em.find(EntityWithAttributeConverters.class, entity.getId()).getMyDataNotRequiringCDINoInjection().getContent();
+    }
+
+    @GET
+    @Path("/without-cdi-vetoed")
+    @Produces(MediaType.TEXT_PLAIN)
+    @Transactional
+    public String withoutCdiVetoed(@RestQuery String theData) {
+        EntityWithAttributeConverters entity = new EntityWithAttributeConverters();
+        entity.setMyDataNotRequiringCDIVetoed(new MyDataNotRequiringCDI(theData));
+        em.persist(entity);
+
+        em.flush();
+        em.clear();
+
+        // This can only return `theData` if Hibernate ORM correctly instantiates the converter through reflection.
+        return em.find(EntityWithAttributeConverters.class, entity.getId()).getMyDataNotRequiringCDIVetoed().getContent();
     }
 }