jakartaee-platform icon indicating copy to clipboard operation
jakartaee-platform copied to clipboard

A prototype of adding persistence requirements for CDI in EE environments

Open starksm64 opened this issue 2 years ago • 15 comments

This is a prototype of including component spec integration requirements in a platform specification. This is based on @gavinking persistence PR https://github.com/jakartaee/persistence/pull/460

Notes:

  • The persistence_3_0.xsd file will not be here in the future. It is here now because the embedded complex type of the persistence-unit element needed to be pulled to the top level in order for it to be extended by the persistence-cdi-3.2.xsd schema
  • The jakarta.persistence.spi.PersistenceUnitInfo interface is no longer a complete representation of the extended persistence.xml file. Maybe a jakarta.persistence.spi.cdi.PersistenceUnitExtInfo subinterface could be created to add the changes missing from the persistence PR.

Co-authored-by: @gavinking

starksm64 avatar Sep 07 '23 11:09 starksm64

Thanks, @starksm64 🙏

gavinking avatar Sep 08 '23 12:09 gavinking

I think that a Jakarta EE implementation could use CDI jakarta.enterprise.inject.spi.AfterDeploymentValidation event to know when the BeanManager becomes available for (CDI enabled) application deployments. WildFly is already using AfterDeploymentValidation to help with our custom two-phase approach of Persistence Unit deployment bootstrapping.

One of the parts that I am missing is how WildFly will be notified to call the EntityManager.close() for an entity manager for each of the different CDI scopes (assuming we do this specifically to work with CDI scopes)?

scottmarlow avatar Nov 14 '23 18:11 scottmarlow

@scottmarlow Does https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0#declaring_disposer_method illustrate one way this could be done?

bstansberry avatar Nov 14 '23 19:11 bstansberry

@scottmarlow Does https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0#declaring_disposer_method illustrate one way this could be done?

Yes, that does help I think, still reading for more context! Thanks Brian!

scottmarlow avatar Nov 14 '23 19:11 scottmarlow

I mentioned on this week's EE platform call how if integration requirements between two specs are covered in the platform spec instead of in the individual specs, then we need to think about what that means in the context of a modern appserver where ideally the user is only provisioning a subset of the platform. I'd only glanced at this at the time, and I see now it looks like you were thinking about that, @starksm64, i.e. by putting this in a section scoped to Web Profile, and referring to a "Jakarta EE environment". That's good, particularly the latter.

I think we should try and flesh out a bit what we mean by a Jakarta EE environment in the current day and age. For example give a facelift to section 2 of the platform spec. And then the spec can discuss these kinds of requirements with reference to what we say there, instead with reference to profiles.

I don't think what I'm saying needs to block this; currently profiles are the finest level of granularity above the individual specs, so scoping requirements to them is what's possible now. I'm mentioning this because this is an example of the kind of disconnect there is between how the EE spec works and how modern implementations work.

bstansberry avatar Nov 14 '23 21:11 bstansberry

One (new?) term that I think came up during this week's platform call is CDI Managed Persistence Contexts to describe what applications could use. If anyone comes up with a more descriptive term, we should consider that. Unless we decide to make CDI Managed Persistence Contexts to follow the same rules as Container Managed Persistence Contexts.

Currently the Persistence Specification defines the rules for how [Container Managed Persistence Contexts](https://jakarta.ee/specifications/persistence/3.1/jakarta-persistence-spec-3.1#container-managed-persistence-contexts) (also known as container-managed entity managers) work in general. Which includes that applications calling the EntityManager.close() will see a IllegalStateException thrown if the entity manager is container-managed (no problem if the entity manager is application-managed.) Also, there are specific rules about the container-managed entity manager joining active (Jakarta Transactions) transactions and the different situations + expectations.

scottmarlow avatar Nov 14 '23 22:11 scottmarlow

I mentioned on this week's EE platform call how if integration requirements between two specs are covered in the platform spec instead of in the individual specs, then we need to think about what that means in the context of a modern appserver where ideally the user is only provisioning a subset of the platform. I'd only glanced at this at the time, and I see now it looks like you were thinking about that, @starksm64, i.e. by putting this in a section scoped to Web Profile, and referring to a "Jakarta EE environment". That's good, particularly the latter.

I think we should try and flesh out a bit what we mean by a Jakarta EE environment in the current day and age. For example give a facelift to section 2 of the platform spec. And then the spec can discuss these kinds of requirements with reference to what we say there, instead with reference to profiles.

I don't think what I'm saying needs to block this; currently profiles are the finest level of granularity above the individual specs, so scoping requirements to them is what's possible now. I'm mentioning this because this is an example of the kind of disconnect there is between how the EE spec works and how modern implementations work.

Thanks Brian, I think these are great thoughts and since we are introducing a new thing and we aren't restricted to previous rules.

If we do target Web Profile does that mean CDI Managed Persistence Contexts would only work at a minimal with required Web Profile technologies? For example, Jakarta Transactions is a required technology so that would be expected to be present in the EE server.

Perhaps some EE implementations could deploy CDI Managed Persistence Context applications without Jakarta Transactions being on the application classpath meaning that deployed applications would not use Jakarta Transactions at all. These applications would strictly not use XA transactions which means they are limited to one database schema (e.g. database user login account used) per application database transaction. So IMO it might also be interesting to target less than the Web Profile as well.

scottmarlow avatar Nov 15 '23 15:11 scottmarlow

Regardless of the target EE profile, I would like to point out the impact on compatibility between application use of CDI Managed Persistence Context and Container Managed Persistence Context so we can all understand what will work and what won't.

As mentioned before, I'm adding a bit more detail here about what I meant by compatibility so we can discuss the possible options. A good question to bring up is what can applications expect for compatibility between traditional Container Manager Persistence Context and CDI Managed Persistence Contexts that reference the same persistence unit definition within the same EE component that is enlisted in a Jakarta Transactions transaction? Will the CDI Managed entity manager instance be guaranteed to be the same object as the container managed entity manager? Or would they be different instances such that one entity manager will not see pending (data) changes in the other entity manager?

Are there any reasons why we would want the two entity manager instances to be different which would consume more memory and also create synchronization issues for EE application components if they had to EntityManager#merge() the same entities into multiple EntityManager(s) (not a good practice IMO to do that)?

A related question is whether the original intent is for CDI managed Persistence Contexts to use a different EntityManagerFactory than container managed entity managers would be obtained from? Or do we agree that the same EntityManagerFactory instance representing the named persistence unit would be used for both CDI managed Persistence Contexts and Container Manager Persistence Context ?

There could be several CDI managed Persistence Contexts modes introduced depending on how many we come up with. One motivitation is to allow applications to choose the behaviour that they will get instead of having to develop their application based on how their EE server seems to work since they may not understand all of the nuances of the Persistence Specification.

Modes:

  1. Compatibility mode, which will make CDI managed Persistence Contexts follow the same rules as Container Manager Persistence Context rules [1]. The CDI Managed Persistence Context will work as best as possible for CDI scopes (TBD the mapping) with Container Manager Persistence Context rules [1]. This will be for applications that wish to access the same persistence context instances from both CDI Managed Persistence Context and Container Managed Persistence Context access which is appropriate for existing EE applications that you wish to start using CDI Managed Persistence Context inside of in an incremental fashion without refactoring the entire application.

  2. ResourceLocal mode, For applications where you will use CDI Managed Persistence Context but do not need to share the underlying persistence contexts with Container Managed Persistence Context, we will introduce improvements and at the same time reduce the (EE server runtime) overhead such as allowing EntityManager instances used outside of a Jakarta Transactions transaction (JTT or JTA?) to maintain state and update the database and execute queries with less overhead. TBD what the behaviour is when used when there is an active JTT, likely behaviour is that the entity manager doesn't join the transaction (TBD whether calling EntityManager#joinTransaction should throw an exception). TBD whether we should call this mode something else to avoid overloading the existing idea of a Resource-Local persistence unit definition.

  3. EnhancedEnterpriseBeans mode, we need a shorter name but this is mostly the rules of [1] except for some changes to improve non-JTT use in Enterprise Beans to work better with Persistence . Such as defer entity detach until persistence context is closed [2]. Also improve query use by not closing queries immediately after creating them with non-JTT use [3][4].

Any other modes to add?

[1] https://jakarta.ee/specifications/persistence/3.1/jakarta-persistence-spec-3.1#container-managed-persistence-contexts [2] https://issues.redhat.com/browse/WFLY-3674 [3] https://issues.redhat.com/browse/WFLY-12674 [4] https://download.oracle.com/javaee-archive/jpa-spec.java.net/users/2011/12/0069.html

scottmarlow avatar Nov 15 '23 15:11 scottmarlow

I don't think what I'm saying needs to block this; currently profiles are the finest level of granularity above the individual specs, so scoping requirements to them is what's possible now. I'm mentioning this because this is an example of the kind of disconnect there is between how the EE spec works and how modern implementations work.

Thanks Brian, I think these are great thoughts and since we are introducing a new thing and we aren't restricted to previous rules.

If we do target Web Profile does that mean CDI Managed Persistence Contexts would only work at a minimal with required Web Profile technologies? For example, Jakarta Transactions is a required technology so that would be expected to be present in the EE server.

Perhaps some EE implementations could deploy CDI Managed Persistence Context applications without Jakarta Transactions being on the application classpath meaning that deployed applications would not use Jakarta Transactions at all. These applications would strictly not use XA transactions which means they are limited to one database schema (e.g. database user login account used) per application database transaction. So IMO it might also be interesting to target less than the Web Profile as well.

@scottmarlow This is exactly the kind of question I'm getting at. But, I think the thing to do is for me to file a separate issue re this broader topic and discussions can move there. Now that I've raised the general point I don't want to overly derail the specific discussion about this particular change.

bstansberry avatar Nov 15 '23 16:11 bstansberry

So to date the Jakarta TCKs and platform specs only deal with the existence of the specs that make up the profile or platform or a greater superset. Subsetting is strictly undefined. For that level of details there would need to be a decision tree on what integration requirements existed at each node where a new specification was added.

On Wed, Nov 15, 2023 at 10:35 AM Brian Stansberry @.***> wrote:

I don't think what I'm saying needs to block this; currently profiles are the finest level of granularity above the individual specs, so scoping requirements to them is what's possible now. I'm mentioning this because this is an example of the kind of disconnect there is between how the EE spec works and how modern implementations work.

Thanks Brian, I think these are great thoughts and since we are introducing a new thing and we aren't restricted to previous rules.

If we do target Web Profile does that mean CDI Managed Persistence Contexts would only work at a minimal with required Web Profile technologies? For example, Jakarta Transactions is a required technology so that would be expected to be present in the EE server.

Perhaps some EE implementations could deploy CDI Managed Persistence Context applications without Jakarta Transactions being on the application classpath meaning that deployed applications would not use Jakarta Transactions at all. These applications would strictly not use XA transactions which means they are limited to one database schema (e.g. database user login account used) per application database transaction. So IMO it might also be interesting to target less than the Web Profile as well.

@scottmarlow https://github.com/scottmarlow This is exactly the kind of question I'm getting at. But, I think the thing to do is for me to file a separate issue re this broader topic and discussions can move there. Now that I've raised the general point I don't want to overly derail the specific discussion about this particular change.

— Reply to this email directly, view it on GitHub https://github.com/jakartaee/platform/pull/746#issuecomment-1812875691, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACRDMUBTAECZI2RT6BHKATYETVOPAVCNFSM6AAAAAA4OY5XRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJSHA3TKNRZGE . You are receiving this because you were mentioned.Message ID: @.***>

starksm64 avatar Nov 15 '23 16:11 starksm64

The big issue with what I'm getting at is TCKs, and as a practical matter that problem is likely a showstopper for doing something different from what you've done with this kind of thing.

In terms of spec language it should be possible to do something to define a "Jakarta EE environment". Section 2.2 of the spec gives a nod at that. In theory even the Core Profile could be defined as the minimum base.

But testing subsetting in a TCK seems impractical. And if it's not tested, having it covered in spec language doesn't mean it's truly portable. Someone provisioning a subset is still just relying on their vendor's guarantee that it works.

I think I won't raise a separate issue after all as I don't see a clear path to do much that's practical.

bstansberry avatar Nov 15 '23 18:11 bstansberry

But testing subsetting in a TCK seems impractical. And if it's not tested, having it covered in spec language doesn't mean it's truly portable. Someone provisioning a subset is still just relying on their vendor's guarantee that it works.

The Platform TCK signature testing does validate that no sub-setting or super-setting is done by an implementation that is being tested. There are platform-tck mailing list discussions from EE-9 time frame that capture feedback from a number of people that some express (long term held) concerns about forking EE in the way that Unix was forked years ago (and the damage that could cause to EE). Others expressed feedback that it would be better to adjust TCK Signature testing to only validate (Java) binary compatibility to free implementations to do more. As you said Brian, there will be a new issue for the broader discussion. I just wanted to echo this little bit of info so we (or I can remember to) later link to that discussion about subsetting + supersetting on the other (broader) issue to be created.

scottmarlow avatar Nov 15 '23 19:11 scottmarlow

The copy/paste error has been corrected and the configuration of the CDI scope/qualifier is based on the extension point as proposed in https://github.com/jakartaee/persistence/pull/566 that @lukasj suggested.

starksm64 avatar Jan 08 '24 22:01 starksm64

Briefly reviewed this on 2024-02-27. We'll not include this in M2 at this point.

edburns avatar Feb 27 '24 16:02 edburns

Folks, since the merge of https://github.com/jakartaee/persistence/pull/460, Jakarta Persistence now has a well-defined way to assign a CDI @ScopeType and @QualifierTypes to a persistence unit, via the persistence.xml document. (Actually, since those annotations were moved from CDI to jakarta.inject many years ago, this actually works for any dependency injection container, not just a CDI bean container.)

However, it's going to be up to the platform to define what these annotations mean in the Jakarta EE platform.

Here's what I think it should ideally say:

Obtaining an EntityManager or EntityManagerFactory using CDI

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

For each persistence unit, the container must make available a bean with:

  • bean type EntityManager,
  • the qualifiers specified by qualifier XML elements in persistence.xml, or jakarta.enterprise.inject.Default, if no qualifiers are explicitly specified,
  • the scope specified by the scope XML element in persistence.xml, or jakarta.transaction.TransactionScoped if no scope is explicitly specified,
  • no interceptor bindings,
  • a bean implementation which satisfies the requirements for a container-managed entity manager, as defined by the Jakarta Persistence specification.

Furthermore, the container must make available a bean with:

  • bean type EntityManagerFactory,
  • the qualifiers specified by qualifier XML elements in persistence.xml, or jakarta.enterprise.inject.Default, if no qualifiers are explicitly specified,
  • scope jakarta.enterprise.context.ApplicationScoped,
  • bean name given by the name of the persistence unit,
  • no interceptor bindings,
  • a bean implementation which satisfies the requirements for a container-managed entity manager factory, as defined by the Jakarta Persistence specification.

Furthermore, the container must make available five beans with:

  • bean types CriteriaBuilder, PersistenceUnitUtil, Cache, SchemaManager, and Metamodel, respectively,
  • the qualifiers specified by qualifier XML elements in persistence.xml, or jakarta.enterprise.inject.Default, if no qualifiers are explicitly specified,
  • scope jakarta.enterprise.context.Dependent,
  • no interceptor bindings,
  • a bean implementation which simply obtains the instance of the bean type by calling the appropriate getter method of the EntityManagerFactory bean.

gavinking avatar Feb 28 '24 15:02 gavinking

Hello @starksm64 , this PR is still in Draft state. Is this ready for review?

edburns avatar Jul 08 '24 14:07 edburns

@starksm64 @gavinking @lukasj

Except for a small question about interceptor bindings this looks fine to me.

I'm just wondering, why couldn't we have this text in the Persistence spec in a chapter or section titled "Container / Platform responsibilities" and opening with something like "In a Jakarta EE environment the following additional requirements hold" (or similar words to that effect)?

arjantijms avatar Jul 09 '24 13:07 arjantijms

The PR has been updated with the outstanding suggestions. At this point I think it is worthwhile to merge the PR and then iterate on it as we develop the TCK tests. There is a new CDI Injection Examples that is a TODO that should be populated with examples from the TCK tests.

starksm64 avatar Jul 30 '24 12:07 starksm64

@lukasj stated:

then I have to ask - why this functionality has not been deprecated yet? Shouldn't that be the very first step a platform should have taken in order to re-build itself on top of CDI?

While this is not a blocker for proceeding with merging this PR, I judge it is very important to not lose track of this aspect. @starksm64, can you please help me understand what must be done to address Lukas's concern?

edburns avatar Jul 30 '24 15:07 edburns

@edburns from the https://jakarta.ee/committees/specification/versioning/ process, we need to make a statement on the deprecation of @Resource and @Resources on the common-annotations feature, ideally with @Deprecation on the API/javadoc which probably requires a minor release, and in the platform spec. JNDI is just a platform feature so it just needs to be deprecated there.

Deprecation

API methods that are marked for deprecation should be annotated with @Deprecated, and where possible the JavaDoc for the method should be updated to point users towards the replacement (if there is one). This mandates a minor version increment.

As noted above, the “deprecation” of behaviour by changing it to something else can happen in two ways:

  • Using a flag to switch from the old to the new behaviour in a minor release. The flag and support for the old behaviour can be dropped in a later major release
  • Without using a flag to switch between the old and new behaviour in a major release. This should be avoided where possible to avoid sudden breaking changes.

Component features must be “deprecated” via a process similar to the Pruning process that existed before. A mandatory Component Feature is transitioned to being an optional feature in release N of the Component specification, and the optional feature should be marked as “Deprecated” in some manner (for example in a “Proposed for Removal” section of the specification). The optional feature may then be removed from the Component specification in release N+1 (or beyond). As noted above, the transition of the feature to being optional would mandate a major release, and the removal of it would mandate a subsequent major release.

An existing optional feature which has not yet been marked for deprecation must be marked as deprecated in a minor or major release, to then be removed in a subsequent major release.

It should be stressed that optional features do not have to be removed – the Component specification can maintain them as optional for as long as desired.

See the Platform Feature section for more details on how Component Features which are pruned in this manner are dealt with there.

Deprecation from a Platform specification stance has two scopes:

  • Removal of Component specifications
  • Removal of Component features

Component Features

When a Component feature is transitioned from being mandatory to optional, the Platform specification and its profiles must include a version of the specification which maintains this change for at least one release. Before an optional feature can be removed from the Platform specification, it must be marked for at least one release under a “Proposed for Removal” section. When removing an optional feature from the Platform specification or its profiles a reference must be provided to the version of the specification that marked the feature as “Proposed for Removal”.

It should be noted that the Platform specification and its profiles are not allowed to “jump” a release to introduce breaking changes without applying for an exception This is to discourage a component specification quickly doing two or more back-to-back releases to quickly remove a feature.

The intent is that there must be a release of the Platform specification which contains the feature in a “Proposed for Removal” state before it can be removed, but this should not necessarily prevent a component specification from moving at a quicker pace than that of the Platform.

For an example scenario:

  • A Component specification defines a mandatory feature in release 2.0 and this is included in the Platform specification release X.
  • The Component specification transitions the feature to being an optional one in release 3.0 and marks it for removal.
  • The Component specification removes the feature entirely in release 4.0.
  • Platform specification release X+1 must contain Component specification 3.0, mark the optional Component feature as included, and can also mark the optional Component feature under the “Proposed for Removal” section – it is not allowed to immediately include Component specification 4.0 since this would bypass the “Proposed for Removal” step.
  • Platform specification release X+2 can proceed to include version 4.0 of the Component specification with the feature removed. Alternatively, the Platform specification release X+2 may choose to maintain Component specification 3.0, but would not have to include the optional feature if it did not want to.

starksm64 avatar Jul 31 '24 13:07 starksm64

See #962

edburns avatar Oct 08 '24 00:10 edburns