jakartaee / persistence

https://jakartaee.github.io/persistence/
Other
189 stars 55 forks source link

define persistence.xml elements for use in dependency injection #460

Closed gavinking closed 4 months ago

gavinking commented 11 months ago

Draft spec for direct CDI integration.

see #377, #456, #309 and even #3, #46, #72

arjantijms commented 11 months ago

Note that under Eclipse, we renamed JPA to "Jakarta Persistence" and updated all references in Jakarta EE 9.

gavinking commented 11 months ago

Ah yeah, good point, thanks.

arjantijms commented 10 months ago

But since this imposes requirement on the CDI, it would be good to get an opinion on this change from them as well

I may have missed something, but what are the requirements this imposes on CDI? Isn't this just using portable extensions of CDI and the CDI SPI and API in general?

lukasj commented 10 months ago

This change is adding:

the CDI container must have built-in support for Jakarta Persistence.

gavinking commented 10 months ago

I may have missed something, but what are the requirements this imposes on CDI? Isn't this just using portable extensions of CDI and the CDI SPI and API in general?

This is a really interesting question.

I mean yeah, absolutely this could be implemented as a portable extension. But .... we're being asked to impose a requirement on someone here, and if it's just a portable extension, exactly who is being required to provide that?

It can't be the persistence provider which supplies it, IMO, which is why I was skeptical at first that this even really belongs in the JPA spec.

So @arjantijms perhaps you think I should adjust that wording to say it's the Jakarta EE container which is required to provide this integration (possibly via a portable extension) instead of the bean container. I guess I'm OK with that. I was trying to make it a bit more general-purpose I guess.

arjantijms commented 10 months ago

It can't be the persistence provider which supplies it, IMO, which is why I was skeptical at first that this even really belongs in the JPA spec.

I'm curious, why can't the persistence provider (Jakarta Persistence implementation) not supply this?

For instance, in Jakarta Security the SecurityContext can be obtained by injecting it using @Inject. The specification says that is required, and an implementation (e.g. Soteria) ships with a portable extension that makes a Bean available for exactly this. There is no need for Jakarta Security to bother CDI core with this.

There is a similar story for Jakarta Faces and the FacesContext, Jakarta Transactions and the UserTransaction etc.

gavinking commented 10 months ago

I'm curious, why can't the persistence provider (Jakarta Persistence implementation) not supply this?

Well, first, because that would be a pretty major change to the division of responsibilities between persistence provider and container. The spec was written so that the persistence provider is a simple library that really knows very little about container integration, so that injection, etc, was always the responsibility of the container.

Second, because unlike the SecruityContext, FacesContext, or UserTransaction, creating an EntityManagerFactory requires a very complex archive scanning process that the persistence provider is simply physically incapable of performing without being passed a lot of context from the container.

So:

This means that the JPA spec would have to start micromanaging a multi-phase interaction between the container and the persistence provider. Now, that's not impossible, but in my opinion the JPA spec is a very poor place to start writing down this sort of stuff.

By contrast, if registering beans is the responsibility of the container, then the spec can remain silent about how that occurs, and the container is free to do it whatever way works.

gavinking commented 10 months ago

it has to register portable extensions which are specific to those persistence units in a call to the Extension.

Or, alternatively, we could invent our whole own protocol between container and provider e.g. by passing the BeanManager to createEntityManagerFactory(), and then JPA would pick up a hard dependency on CDI. Now, that's not the end of the world (I love CDI for quite obvious reasons), but I know for sure it's something @lukasj would prefer to avoid.

arjantijms commented 10 months ago

The spec was written so that the persistence provider is a simple library that really knows very little about container integration, so that injection, etc, was always the responsibility of the container.

Wasn't that another time / era? I guess that was once a good idea so that not every spec had to duplicate an injection mechanism. Now that there is a general "injection service" available, this concern (if that indeed ever was the concern) is not really applicable anymore.

Jakarta Transactions (JTA at the time) was also written where the container had to have different (mostly more) responsibilities, yet it did evolve in a way such that UserTransaction, but also interceptors such as Transactional were able to be delivered by a portable CDI extension (see e.g. https://github.com/OmniFish-EE/omni-transact/tree/main/jta/src/main/java/ee/omnifish/transact/jta/cdi)

This means that the JPA spec would have to start micromanaging a multi-phase interaction between the container and the persistence provider.

Is that really all necessary? Why not just let things proceed as they do today, and then provide a bean that essentially wraps the EntityManager and when called by user code resolved the underlying entity manager in a way it normally would do programmatically.

I dabbled a little into that approach in Piranha:

https://github.com/piranhacloud/piranha/blob/current/extension/eclipselink/src/main/java/cloud/piranha/extension/eclipselink/EntityManagerProducer.java

arjantijms commented 10 months ago

p.s.

creating an EntityManagerFactory requires a very complex archive scanning process that the persistence provider is simply physically incapable of performing without being passed a lot of context from the container.

Does that mostly concerns scanning of EARs? As in the WAR case it does seem the persistence provider is capable of doing this (as evidenced by e.g. adding Hibernate or EclipseLink to Tomcat, but also by the fact that in Piranha I use the scanning process as being done by EclipseLink)

gavinking commented 10 months ago

Is that really all necessary? Why not just let things proceed as they do today, and then provide a bean that essentially wraps the EntityManager and when called by user code resolved the underlying entity manager in a way it normally would do programmatically.

Yes, I believe it is, because of the requirement to scan archives.

There is information in the PersistenceUnitInfo (a class loader, a ClassTransformer, the root URL of the persistence unit, the URLs of contained jar files) which simply isn't available to a CDI Extension.

I guess @scottmarlow is the expert on this.

gavinking commented 10 months ago

Does that mostly concerns scanning of EARs?

I'm not sure. Possibly?

arjantijms commented 10 months ago

There is information in the PersistenceUnitInfo (a class loader, a ClassTransformer, the root URL of the persistence unit, the URLs of contained jar files) which simply isn't available to a CDI Extension.

I think those don't have to be available to the CDI extension. The CDI extension can return a delegating entity manager, that delegates to the actual entity manager. The actual entity manager can be obtained via "traditional" means.

Traditional can then be done via something like e.g.

private EntityManager getNonTxScopedEntityManager() {
        return
            CDI.current()
               .select(NonTxEntityManagerHolder.class)
               .get()
               .computeIfAbsent(() ->
                   getEntityManagerFactory().createEntityManager(synchronizationType, properties));
    }

and

public EntityManagerFactory getEntityManagerFactory() {
        if (entityManagerFactory == null) {
            entityManagerFactory = CDI.current()
                    .select(EntityManagerFactoryCreator.class)
                    .get()
                    .get(unitName);
        }

        return entityManagerFactory;
    }

Where eventually EntityManagerFactoryCreator is a class provided by the persistence provider, which knows how to create its own EntityManagerFactory implementation (doing that in the same way it would do without any CDI in the loop).

gavinking commented 10 months ago

The CDI extension can return a delegating entity manager, that delegates to the actual entity manager.

But how does it even know what persistence units exist in the deployment? We would have to specify that the portable extension is started after all the persistence units are already created.

So we would be imposing a lifecycle on the container.

I'm not saying it can't work. I'm just saying that I don't see why the JPA spec should be imposing such things on container implementors when it could in principle, just say nothing on the topic.

I mean, look, if I'm implementing a container I feel like I want to have control over this stuff. I don't think I want the persistence provider to be doing it's own crap that imposes a particular lifecycle on me.

gavinking commented 10 months ago

I think you think that making the persistence provider provide an Extension makes this easier for the container implementor.

That's not my intuition: I think it makes things harder for both the Jakarta EE container implementor, and for the persistence provider implementor.

gavinking commented 10 months ago

P.S. I'm not sure how much this discussion relates to issue #70, but it might be a similar problem.

gavinking commented 10 months ago

So I think the language I used here was bad. In saying "a CDI container which supports Jakarta Persistence" I made it sound as if I was saying that the implementation had to be literally part of the CDI container. But that's not actually what I really intended.

So I've changed the language to:

A container environment might feature built-in integration of Jakarta Persistence with the CDI bean manager, allowing injection of a container-managed entity manager factory using the annotation jakarta.inject.Inject. In the Jakarta EE environment, the container must feature built-integration of Jakarta Persistence with the CDI bean manager.

WDYT folks, is that better?

arjantijms commented 10 months ago

But how does it even know what persistence units exist in the deployment?

It doesn't have to know that. It just collects what appears at injecting sites within the application. E.g.

public class EntityManagerProducer {

    /**
     *
     * @param injectionPoint the injectionPoint
     * @return EntityManager
     */
    @Produces
    public EntityManager produce(InjectionPoint injectionPoint) {
        Annotated annotated = injectionPoint.getAnnotated();
        if (annotated instanceof AnnotatedParameter<?> annotatedParameter) {
            annotated = annotatedParameter.getDeclaringCallable();
        }

        PersistenceContext persistenceContext = annotated.getAnnotation(PersistenceContext.class);

        return new PiranhaEntityManager(
                    persistenceContext.unitName(),
                    persistenceContext.type(),
                    persistenceContext.synchronization(),
                    propertiesToMap(persistenceContext.properties()));

    }

    private Map<Object, Object> propertiesToMap(PersistenceProperty[] properties) {
        return Arrays.stream(properties)
                     .collect(toMap(
                        PersistenceProperty::name,
                        PersistenceProperty::value));
    }
}
arjantijms commented 10 months ago

I think it makes things harder for both the Jakarta EE container implementor, and for the persistence provider implementor.

As a Jakarta EE container implementor for both Piranha Cloud and GlassFish, I think it makes my life easier, and not just a little but to a very large degree.

Maybe I'm just not seeing something still, which may well be the case.

But I've integrated EclipseLink in Piranha, and even having to still write the extension myself, it was pretty easy. If that extension already existed it would be trivial.

That said, I must admit that on Piranha I haven't even tried to run the Jakarta Persistence TCK, so maybe I've overlooked something very important.

arjantijms commented 10 months ago

WDYT folks, is that better?

My own preference, which I hope would be possible, is more something along the lines of:

"In a Jakarta EE environment, the Jakarta Persistence implementation MUST provide a build-in unqualified CDI bean implementing the EntityManager interface that represents the default entity manager. Additional build-in qualified CDI beans MUST be made available for every defined entity manager"

gavinking commented 10 months ago

It doesn't have to know that. It just collects what appears at injecting sites within the application. > E.g.

public class EntityManagerProducer {

   @Produces
   public EntityManager produce(InjectionPoint injectionPoint) {

Hahahahaha OMG section 1.3.5 of the CDI spec—I wrote that but I had completely forgotten it existed.

(Anyway it's pretty nice and it's nice that some people know it exists.)

Yeah OK, so I admit that this almost works. But if you have multiple providers, each with their own Extension, won't they "fight" over the persistence units?

And, worse, I fear you won't get proper startup-time messages from the bean container about unsatisfied dependencies on EntityManagers. Because every dependency on an EM is satisfied by that producer. Or am I wrong? Clearly I don't know CDI as well as I thought I did 😂

sebersole commented 10 months ago

The situation as @scottmarlow and I played with between Hibernate and WildFly was that a well-defined extension would be beneficial.

I don't think security, JTA, etc are good corollaries for this situation because here we have a bidirectional dependency - JPA expects that the BeanManager be available and ready to use when the EntityManagerFactory is being booted and CDI expects the EntityManagerFactory to be available (at least for reference injection) when the BeanManager is being booted.

In Hibernate we ended up handling with through delayed access to the BeanManager. We specifically have 2 delayed access strategies -

  1. The container passes us the BeanaManager and we simply agree to not use it until after the EntityManagerFactory is fully booted. In effect, this is a "first use" strategy.
  2. Scott and I developed an ExtendedBeanManager contract that is a delayed reference to the BeanManager from the container, which is used to register a callback for when the BeanManager is really ready to use.

Personally, I think the second approach is better because its a known point where we expect all the beans to be available and we can perform the CDI lifecycle at one point. The "first use" strategy also delays bean wiring exceptions, which is not particularly user-friendly in my opnion.

sebersole commented 10 months ago

Yeah OK, so I admit that this almost works. But if you have multiple providers, each with their own Extension, won't they "fight" over the persistence units?

If I understand the point you are trying to make, it was THE reason why CDI's built-in listener stuff would not work and why we ended up creating our ExtendedBeanManager contract - it allows scoping

sebersole commented 10 months ago

The "first use" strategy also delays bean wiring exceptions, which is not particularly user-friendly in my opnion.

I wanted to clarify a point here.

With immediate access, when the JPA provider attempts to resolve the bean and there is a problem, that problem can become an exception which triggers a failure in the deployment. Failing the deployment is ideal because we have a state where that bean will never be usable.

However, by themselves, both delayed approaches mean that we lose the ability. The ExtendedBeanManager approach at least gives the container the option of making that happen so long as the ExtendedBeanManager callback is made during the deployment phase - a 2 phase process:

  1. Create a ExtendedBeanManager
  2. Create the EntityManagerFactory passing along the ExtendedBeanManager
  3. Ceate the BeanManager
  4. Trigger callback on the ExtendedBeanManager passing along the usable BeanManager

where each of those steps happen during deployment.

gavinking commented 10 months ago

OK, so I have run this issue past our CDI and ORM people here internally, and the feedback I got was:

  1. They are happy with the (updated) language on this PR, where it's clear that we're making this functionality a requirement on the Java EE container.
  2. There are several different, all imperfect, ways to implement integration between CDI and JPA, ranging from some sort of phased lifecycle for CDI initialization, to lazy resolution of JPA beans, similar to what is outlined by @arjantijms above. Our preference has not been to implement this via lazy resolution since that essentially circumvents CDI's dependency validation (detection of ambiguous/unsatisfiable dependencies).

The insight from @sebersole was that there's a real chicken/egg problem here: the EMF needs beans from CDI for stuff like injection into entity listeners, but CDI beans need to inject EMFs. And that there's a hierarchy of solutions to this problem which range from "cleaner" to "messier", with the cleaner solutions having the disadvantage that CDI is bypassed and JPA does its own ad hoc dependency validation.

It seems to me that the core problem here is that we have two different "containers", the bean manager, and the EMF, each with their own "components" and cross-linked dependencies. Actually the problem is slightly worse, because we can have multiple providers each with their own EMFs. This is very different from the situation faced by other specs such as security and JTA!

So I would analogize our situation to the problem faced by the Java compiler and annotation processors. There, the solution is to process in "rounds", where at each round, the processing of a single component is either a success or a failure, meaning it will be reprocessed by the next round. I imagine that a "complete" solution to this problem would involve CDI adopting a similar multiphase lifecycle for bean registration. But that's a problem best solved in CDI, not here.

I'm sensitive to the fact that @arjantijms likes his producer method based on InjectionPoint, and I quite like it too, but it seems to me that it works for him precisely because there's only one Extension registering JPA beans, and that if, as he suggests, we were to move this responsibility to the providers, then I predict that his solution immediately breaks. Now, I might be wrong, and I know there's ways to make it work, but they're not quite so clean.

Anyway, in my opinion, the very fact that this discussion has even gone this far indicates to me that the "how to implement" part of this is not something that's ripe for standardization. Indeed, I think if we were to impose this requirement on providers (instead of on the EE container), I would be very fearful of all the complaints we would get that such-and-such persistence provider P works in container C but not in D, and could we please clarify what C and D are supposed to do in tricky corner case X.

Let's not go there. At least not now. One advantage of saying nothing about how this requirement is implemented, is that we can always decide to say something later, when we understand the problem better. Who knows, perhaps CDI will eventually offer something like Steve's and Scott's ExtendedBeanManager, or like my javac-inspired round-based processing.

arjantijms commented 10 months ago

Actually the problem is slightly worse, because we can have multiple providers each with their own EMFs.

Just out of curiosity, by your understanding or knowledge, does anyone ever actually use that? The feature is intended I understand that e.g. JBoss EAP can ship with both Hibernate and EclipseLink, and subsequently define two persistence contexts; one for Hibernate, and one for EclipseLink.

Applications would then arbitrarily (based on their requirements) persists a given entity with either Hibernate or EclipseLink.

I have been using JPA in production since before it was officially released (yes, I know), and I don't think I've ever seen any customer use that.

arjantijms commented 10 months ago

Our preference has not been to implement this via lazy resolution since that essentially circumvents CDI's dependency validation (detection of ambiguous/unsatisfiable dependencies).

I've been thinking about this indeed, and you are of course right. Still, I wonder, can't we do validation as soon as the EntityManagerFactory / persistence context comes online?

E.g. in a condensed timeline:

t=1 -> CDI container boots
t=2 -> Extensions add a delegating entity manager Bean (e.g. via producer or custom Bean<t>)

t=3 -> Jakarta Persistence boots
t=4 -> EntityManagerFactory / persistence context comes online / becomes available 

t=5 -> Validation of previously added entity manager Bean(s) take place

t=6 -> Servicing of requests start
arjantijms commented 10 months ago

Extension registering JPA beans, and that if, as he suggests, we were to move this responsibility to the providers, then I predict that his solution immediately breaks. Now, I might be wrong, and I know there's ways to make it work, but they're not quite so clean.

Just thinking out loud, and I haven't investigated, but...

There can only ever be at most one default persistence context (entity manager factory) and persistence unit (entity manager). This should be true regardless of how many Jakarta Persistence implementations there are on the class path.

All the other usages have to be qualified. For the persistence unit there is only the name (unitName in @PersistenceUnit) to take into consideration. Names need to be unique already, so a Jakarta Persistence implementation can only ever add a qualified bean for a unique name. Should a duplicate name be added, it blows up. As intended.

gavinking commented 10 months ago

Just out of curiosity, by your understanding or knowledge, does anyone ever actually use that?

I mean, not explicitly, but I imagine it would be a pretty reasonable thing to do if you're in the process of migrating from one provider to another. And it's certainly a core feature of JPA.

Still, I wonder, can't we do validation as soon as the EntityManagerFactory / persistence context comes online?

I mean, I imagine there are ways to do it. But that's pretty different to saying that I know this can be done cleanly with no tradeoffs in every single EE container, and that it's an approach I want the spec to mandate.

Names need to be unique already

In CDI we don't inject on the basis of name. We inject by matching type + qualifiers.

gavinking commented 10 months ago

(To be clear, I think disambiguating injection points using unitName is bad and untypesafe.)

arjantijms commented 10 months ago

(To be clear, I think disambiguating injection points using unitName is bad and untypesafe.)

I don't disagree, but what I meant is that any qualifier eventually must reference a name (unitName), which must ultimately be unique. So wouldn't that make clashes between different implementations unlikely, or even impossible?

arjantijms commented 10 months ago

I mean, not explicitly, but I imagine it would be a pretty reasonable thing to do if you're in the process of migrating from one provider to another. And it's certainly a core feature of JPA.

I dunno, I would migrate in one go really. It feels quite odd to migrate and then persist say entity User via Hibernate, but still persist Invoice using OpenJPA. Is that even possible if Invoice referenced User?

I also wonder why only Jakarta Persistence has this. Never heard the wish for say Faces to have both Mojarra and MyFaces at the same time active, and then render one part of a page with Mojarra, and another part with MyFaces.

arjantijms commented 10 months ago

I mean, I imagine there are ways to do it. But that's pretty different to saying that I know this can be done cleanly with no tradeoffs in every single EE container, and that it's an approach I want the spec to mandate.

Perhaps in the simplest case, we can add a spec demand for the container / jakarta persistence implementation to also validate @Inject EntityManager at the same it also validates @PersistenceUnit EntityManager?

I mean, it now already has to validate @PersistenceUnit EntityManager. What exactly would prevent whatever does that validation now, to optionally, also validate @Inject EntityManager?

lukasj commented 10 months ago

I mean, not explicitly, but I imagine it would be a pretty reasonable thing to do if you're in the process of migrating from one provider to another. And it's certainly a core feature of JPA.

I also wonder why only Jakarta Persistence has this. Never heard the wish for say Faces to have both Mojarra and MyFaces at the same time active, and then render one part of a page with Mojarra, and another part with MyFaces.

It is NOT only Persistence who has this. Counter example is XML Binding (formerly JAXB) - even GF has jaxb-ri and MOXy where both do implement the same spec for quite a long time. Not saying there was another runtime in the JDK (up to 10). And everything has been available through one API, even though it may be tricky to get the right implementation (rest endpoint with moxy and soap endpoint with jaxb-ri in one war is valid and expected to be supported use-case).

The motivation and sort of a rule behind this built-in support - as I remember it (...but I can be wrong) - is - If a spec targets Java SE - and Persistence as well as XML Binding (and few others) do - then it must allow and support co-existence of multiple independent implementations in one JVM.

arjantijms commented 10 months ago

even GF has jaxb-ri and MOXy where both do implement the same spec for quite a long time.

Well...

        <!-- Provides conflicting JAXB provider.
             See https://github.com/eclipse-ee4j/eclipselink/issues/1380
        <dependency>
            <groupId>org.eclipse.persistence</groupId>
            <artifactId>org.eclipse.persistence.moxy</artifactId>
            <exclusions>
                <exclusion>
                    <groupId>*</groupId>
                    <artifactId>*</artifactId>
                </exclusion>
            </exclusions>
        </dependency>
        -->
arjantijms commented 10 months ago

then it must allow and support co-existence of multiple independent implementations in one JVM.

Just like with the machinery behind EARs, I wonder if we didn't jump the shark there with such features.

scottmarlow commented 10 months ago

then it must allow and support co-existence of multiple independent implementations in one JVM.

Just like with the machinery behind EARs, I wonder if we didn't jump the shark there with such features.

For those that didn't watch the TV show Happy Days and haven't heard the term of jumping the shark, defining reference is https://en.wikipedia.org/wiki/Jumping_the_shark which describes how the Happy Days TV show ended when character Arthur Fonzarelli jumped over a shark. :-)

arjantijms commented 10 months ago

ended when character Arthur Fonzarelli jumped over a shark. :-)

Slightly off topic, but I loved watching that show, must have seen every episode. The shark episode was...troublesome though :| ;)

starksm64 commented 10 months ago

So where integration requirements are defined and where the associated TCK tests are produced has been an ongoing discussion that is coming to head in EE 11. In discussions at the platform group level, the feeling is that this needs to done as part of a platform profile or the platform spec and associated TCK. I'll take this work as the basis for a new Persistence/CDI integration spec section in the Web profile with associated TCK tests. It seems possible that there need not be any changes to the Persistence specification, but that needs to be verified.

I'll also propose that we update the specification operation process to require that any integration specs and TCKs have a sign off from the component specification team(s) that are impacted.

arjantijms commented 10 months ago

I'll also propose that we update the specification operation process to require that any integration specs and TCKs have a sign off from the component specification team(s) that are impacted.

I still find it very troublesome that the container / runtime needs to have such intimate knowledge about Jakarta Persistence that only the container can create an EntityManager.

Jakarta Persistence implementations should really be able to do this themselves. If they can't, then I strongly suspect the problem is elsewhere, and handing this problem off to the container is just some kind of hack.

For comparison again, in Jakarta Faces, the implementation can create a FacesContext. It doesn't need the container for this. In Jakarta Security, the implementation can create an HttpAuthenticationMechanism it doesn't need the container for this, etc for quite a number of other specifications.

mnriem commented 10 months ago

I like the delegating approach simply because for most applications this would be sufficient and thus enables developer productivity. One could narrowly define this way of integration only scoped for applications with just one persistence unit. Thoughts?

mnriem commented 10 months ago

@gavinking What is missing in the public EE API surface so CDI integration can be done in a vendor agnostic way?

sebersole commented 10 months ago

the EMF needs beans from CDI for stuff like injection into entity listeners, but CDI beans need to inject EMFs

To be clear, the EMF needs the ability to get the entity listeners themselves from CDI. Minor distinction, but the cause of the problem.

I like the delegating approach

Which is the "delegating approach"?

What is missing in the public EE API surface so CDI integration can be done in a vendor agnostic way?

In the solution between Hibernate and Wild Fly this boiled down to the persistence provider being able to get a scoped callback that the BeanManager is ready to use. The scoped part is important to be able to restrict this between singular EMF and BeanManager instances.

gavinking commented 10 months ago

@gavinking What is missing in the public EE API surface so CDI integration can be done in a vendor agnostic way?

Hi, I'm not quite sure what you're asking.

mnriem commented 10 months ago

I like the delegating approach

Which is the "delegating approach"?

The approach where the injected EntityManager is merely a proxy to the real thing.

What is missing in the public EE API surface so CDI integration can be done in a vendor agnostic way?

In the solution between Hibernate and Wild Fly this boiled down to the persistence provider being able to get a scoped callback that the BeanManager is ready to use. The scoped part is important to be able to restrict this between singular EMF and BeanManager instances.

So is the ServletContainerInitializer API sufficient to accomplish this or is there a need for more API surface?

arjantijms commented 10 months ago

To be clear, the EMF needs the ability to get the entity listeners themselves from CDI. Minor distinction, but the cause of the problem.

Could you explain this in more detail? Why is this exactly a problem?

Consider the following pseudo extension code:

public class HibernateCdiExtension implements Extension {

    public void afterBean(final @Observes AfterBeanDiscovery afterBeanDiscovery, BeanManager beanManager) {

        afterBeanDiscovery.addBean()
                .scope(...)
                .types(EntityManager.class)
                .id("some id")
                .createWith(cc -> HibernateFactory.create(...).createEntityManager());
}

And suppose we also have:

@EntityListeners(UserListener.class)
@Entity
public class User {
    //...
}

and

public class UserListener {

    @Inject
    Foo foo;

    @PrePersist   
    private void beforeAnyUpdate(User user) {
        foo.check(user);
    }

In terms of these classes, where do things exactly fail?

In Jakarta Security for instance it also happens that a bean added in AfterBeanDiscovery executes code in some path that itself needs the CDI API, either programmatically using the beanManger lookup methods or otherwise.

An example from Soteria of the latter:

        if ("form".equalsIgnoreCase(loginConfig.getAuthMethod())) {
                authenticationMechanismBean = new CdiProducer<HttpAuthenticationMechanism>()
                        .scope(ApplicationScoped.class)
                        .types(Object.class, HttpAuthenticationMechanism.class)
                        .addToId(FormAuthenticationMechanismDefinition.class)
                        .create(e -> {
                            FormAuthenticationMechanism authMethod = CdiUtils.getBeanReference(FormAuthenticationMechanism.class);
                            // ...

                            return authMethod;
                        });
                httpAuthenticationMechanismFound = true;
        }

        if (authenticationMechanismBean != null) {
            afterBeanDiscovery.addBean(
                decorator.decorateBean(authenticationMechanismBean, HttpAuthenticationMechanism.class, beanManager));
        }

There, the Security CDI extension adds a Bean<T> for the HttpAuthenticationMechanism, but internally, the HttpAuthenticationMechanism create method uses the CDI API (beanManager, etc, like in https://jakarta.ee/specifications/persistence/3.1/jakarta-persistence-spec-3.1#entity-listeners) to create the FormAuthenticationMechanism in this specific example.

sebersole commented 10 months ago

Sorry for the late reply, I've been traveling.

Consider the following pseudo extension code:

So long as the createWith callback happens only once the BeanManager is ready to use, then that should work. I'm not familiar enough with CDI at that level to say.

sebersole commented 10 months ago

Oh wait.. I (may) take that back.

Is the expectation of your pseudo code to do that for each bean that wants EMF injection? If so, no that's not really feasible.

arjantijms commented 10 months ago

So long as the createWith callback happens only once the BeanManager is ready to use, then that should work.

Of course, that's how CDI works. CDI will not create beans with CDI before CDI itself is ready to use.

Is the expectation of your pseudo code to do that for each bean that wants EMF injection? If so, no that's not really feasible.

No, the create() method is only executed once per scope. Within that scope all beans will use the same instance of the entity manager (not EMF in this example) here.

It's only pseudo code. The EntityManagerFactory itself, I guess, will be either statically available, or reside within application scope.

Updated example:

public class HibernateCdiExtension implements Extension {

    public void afterBean(final @Observes AfterBeanDiscovery afterBeanDiscovery, BeanManager beanManager) {

        afterBeanDiscovery.addBean()
                .scope(ApplicationScoped.class)
                .types(EntityManagerFactory.class)
                .id("some id")
                .createWith(cc -> HibernateFactory.create(...));

         afterBeanDiscovery.addBean()
                .scope(RequestScoped.class)
                .types(EntityManager.class)
                .id("some id")
                .createWith(cc -> 
                        CdiUtils.getBeanReference(EntityManagerFactory.class)
                                     .createEntityManager()
                );
}
beikov commented 10 months ago

The problem is with enhancement of JPA entity classes. Hibernate/JPA has to register a ClassTransformer before any user code can run, which is the first phase of the Hibernate bootstrap. Then, after CDI booted completely i.e. when the AfterDeploymentValidation event is fired and the BeanManager can be used to retrieve beans, the second phase of the Hibernate bootstrap has to be initiated, which is to actually eagerly build an EntityManagerFactory.