Skip to content

TOMEE-4650 - Guard undeploy against a closed EMF and add JPA 3.2 CDI qualifier beans#2847

Open
jungm wants to merge 1 commit into
mainfrom
claude/tomee-4650-fix-26d0cd
Open

TOMEE-4650 - Guard undeploy against a closed EMF and add JPA 3.2 CDI qualifier beans#2847
jungm wants to merge 1 commit into
mainfrom
claude/tomee-4650-fix-26d0cd

Conversation

@jungm

@jungm jungm commented Jul 23, 2026

Copy link
Copy Markdown
Member

Fixes TOMEE-4650. Two independent problems, both reproducing on Plume (EclipseLink) and the webprofile distribution (OpenJPA) — so neither is a persistence-provider defect.

1. Undeploy closes an already-closed EntityManagerFactory

When a test (or application) closes a container-managed EntityManagerFactory itself, TomEE's undeploy path called close() on it again, and Assembler.destroyApplication then failed with "Attempting to execute an operation on a closed EntityManagerFactory". The test method passed; only the undeploy step errored.

ReloadableEntityManagerFactory.close() now checks isOpen() before delegating to the underlying EMF.

2. Missing Jakarta Persistence 3.2 CDI qualifier beans

TomEE did not register the CDI beans that the Jakarta EE 11 / Persistence 3.2 CDI integration requires for persistence.xml-declared units. Injecting a qualified EntityManagerFactory, EntityManager, or PersistenceUnitUtil (e.g. @Inject @CtsEm2Qualifier) failed at deploy time with UnsatisfiedResolutionException.

A new JpaCDIExtension registers, for each persistence unit:

  • EntityManagerFactory@ApplicationScoped, bean name = persistence-unit name
  • EntityManager — in the scope given by <scope>, defaulting to jakarta.transaction.TransactionScoped
  • CriteriaBuilder, PersistenceUnitUtil, Cache, SchemaManager, Metamodel@Dependent, each obtained from the matching getter of the EMF

All of them carry the qualifiers declared by the <qualifier> elements, or @Default when none is declared. The bean-registration contract (scopes, qualifiers, bean name) was checked against the Jakarta EE 11 Platform spec (CDI-JPA) and the Persistence 3.2 schema rather than from memory.

Supporting plumbing:

  • <qualifier>/<scope> added to the persistence.xml JAXB model (PersistenceUnit) in the XSD-mandated position. This model is hand-maintained, not generated (the xjc profile does not cover persistence schemas), and TomEE does not validate persistence.xml against a bundled XSD, so no schema files needed updating.
  • Fields carried through PersistenceUnitInfo and AppInfoBuilder, which also honours the jakarta.persistence.qualifiers / jakarta.persistence.scope override properties per the spec.
  • Extension registered in OptimizedLoaderService, alongside the analogous ConcurrencyCDIExtension it is modeled on.

Tests

  • ReloadableEntityManagerFactoryCloseTest — verifies a second close() is a no-op. Confirmed it fails against the unpatched code (2 close calls instead of 1).
  • JpaCDIExtensionTest — verifies qualified and @Default EMF/PersistenceUnitUtil injection across two persistence units. Confirmed it fails against the unpatched code with the exact UnsatisfiedResolutionException from the issue.

Notes for reviewers

  • The excluded TCK tests named in the issue (entityManagerFactoryCloseExceptions/ClientP{m,u}servletTest, ee.cdi.ServletEMLookupTest) live in the separate apache/tomee-tck harness repo; their exclusion lines still need removing there once this merges. That change is not in this PR.
  • SchemaManager/Metamodel/CriteriaBuilder/Cache beans are registered per spec but only EntityManagerFactory and PersistenceUnitUtil are exercised in the unit tests — the others eagerly trigger OpenJPA entity enhancement, which the unenhanced test setup in this module cannot satisfy. The full validation is a TCK run.

🤖 Generated with Claude Code

…qualifier beans

Two independent problems, both reproducing on Plume (EclipseLink) and the
webprofile distribution (OpenJPA), so neither is a provider defect.

1. When an application closes a container-managed EntityManagerFactory itself,
   TomEE's undeploy path closed it again, and Assembler.destroyApplication then
   failed with "Attempting to execute an operation on a closed
   EntityManagerFactory". ReloadableEntityManagerFactory.close() now checks
   isOpen() before delegating.

2. TomEE did not register the CDI beans required by the Jakarta Persistence 3.2 /
   Jakarta EE 11 CDI integration for persistence.xml-declared units, so injecting
   a qualified EntityManagerFactory / EntityManager / PersistenceUnitUtil failed
   with UnsatisfiedResolutionException. A new JpaCDIExtension registers, per
   persistence unit, an @ApplicationScoped EntityManagerFactory (bean name = PU
   name), an EntityManager in the <scope> element's scope (default
   TransactionScoped), and @dependent CriteriaBuilder, PersistenceUnitUtil, Cache,
   SchemaManager and Metamodel beans, all carrying the <qualifier> elements or
   @default when none is declared. Supporting this, <qualifier>/<scope> are added
   to the persistence.xml JAXB model (PersistenceUnit), to PersistenceUnitInfo,
   and copied through AppInfoBuilder, which also honours the
   jakarta.persistence.qualifiers / jakarta.persistence.scope override properties.

Bean-registration contract verified against the Jakarta EE 11 Platform spec
(CDI-JPA) and the Persistence 3.2 schema rather than from memory.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
@rzo1

rzo1 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Could you split this? There are two unrelated changes here and they're in very different states.

Part 1 — the ReloadableEntityManagerFactory.close() guard — I'd merge today. It's small,
correct, uses the non-lazy delegate field rather than delegate() so it doesn't force
initialisation, and the test genuinely fails without it. That's the part that fixes the reported
undeploy noise.

Part 2 — JpaCDIExtension — needs rework. It's modelled on ConcurrencyCDIExtension, which is
the right reference, but several of the guards that make that class safe were dropped in the copy.

Blocking:

  • For every PU without <qualifier>, validateAndCreateQualifiers returns {@Any, @Default} and
    registerBeans adds EntityManagerFactory and EntityManager beans with those qualifiers, with
    no getBeans() check. Beans added via AfterBeanDiscovery.addBean() are ordinary enabled beans,
    not built-ins, so nothing prefers an application producer over them — that's an
    AmbiguousResolutionException against the @Produces EntityManager pattern, which is about as
    common as CDI/JPA patterns get.

    Two tests in this very module should now fail deployment: ProducedExtendedEmTest
    (EntityManagerProducer.produceEm() + @Inject EntityManager in A, PU cdi-em-extended) and
    ResourceLocalCdiEmTest (EMFProducer.em() + @Inject EntityManager in PersistManager, PU
    rl-unit). Neither is @Ignored, and the extension does run in them —
    OptimizedLoaderService.loadExtensions adds it unconditionally at :130 and OpenEJBLifecycle
    sets CURRENT_APP_INFO at :190 before deployer.deploy().

    ConcurrencyCDIExtension.registerDefaultBeanIfMissing (:359) is exactly the guard that's missing —
    it takes the BeanManager and skips when beanManager.getBeans(type, Default.Literal.INSTANCE)
    is non-empty. OpenEJBLifecycle.addInternalBeans (:244-258) uses the same idiom.

  • The loop for (final PersistenceUnitInfo unitInfo : appInfo.persistenceUnits) has no dedup of
    qualifier sets and no filter on unitInfo.webappName. So two unqualified PUs in one app register
    duplicate @Default beans, and in an EAR a webapp gets beans for its siblings' PUs —
    TomcatWebAppBuilder:1454 sets CURRENT_APP_INFO to the whole EAR's AppInfo in the per-webapp
    !webAppAlone branch, while the same method correctly filters on unitInfo.webappName at :1425
    for EMF creation. ConcurrencyCDIExtension scopes this with isVisibleInCurrentApp(resource, currentAppIds)
    (:102); there's no equivalent here.

  • The annotation proxy breaks the Annotation equals/hashCode contract: equals is
    annotationType.isInstance(args[0]) and hashCode is annotationType.hashCode(). The spec
    mandates 0 for a marker annotation, and equals ignoring member values makes it asymmetric with a
    real annotation instance — so a qualifier with members can select the wrong PU. The correct
    implementation is ~150 lines away in ConcurrencyCDIExtension (annotationEquals :235,
    annotationHashCode :254, annotationToString) and was replaced here by two one-liners. Please
    reuse it rather than reimplementing.

    Related omission: ConcurrencyCDIExtension.validateAndCreateQualifiers (:184-195) rejects
    qualifiers with members lacking defaults and members lacking @Nonbinding, via two
    addDefinitionError calls. JpaCDIExtension.validateAndCreateQualifiers stops at the @Qualifier
    check. So @Qualifier @interface Unit { String value(); } gives getDefaultValue() == null, the
    proxy returns null from value(), and you get either an NPE inside OWB or a bean that can never
    be matched — with no diagnostic. The javadoc on createAnnotation asserts "the CDI qualifier
    rules guarantee [a default] to exist"; that guarantee doesn't exist.

Also:

  • jakarta.persistence.qualifiers and jakarta.persistence.scope aren't spec properties. I unpacked
    jakarta.persistence-api-3.2.0.jar — neither string appears anywhere in the jar, and
    PersistenceConfiguration declares constants for every standard override property (JDBC_,
    LOCK_TIMEOUT, QUERY_TIMEOUT, SCHEMAGEN_
    , VALIDATION_*, CACHE_MODE) with nothing for these two.
    persistence_3_2.xsd defines only the <qualifier>/<scope> elements. Please don't mint new
    property names under the jakarta.* namespace — openejb.* is the right prefix for a
    TomEE-specific carrier.

  • The SchemaManager bean can only ever inject null: addUtilityBean(..., SchemaManager.class, EntityManagerFactory::getSchemaManager)
    on the reloadable EMF. Either wire it properly or drop it.

  • The jakarta.transaction-absent fallback is unreachable — openejb-core has a hard dependency on it
    (cdi/transactional/TransactionContext imports TransactionManager/TransactionScoped directly
    and extends AbstractContext(TransactionScoped.class)); the module can't load without it. Its only
    possible effect is a silent scope change, and the javadoc describes reflective member access that
    isn't happening.

  • <scope> isn't validated as an actual CDI scope, unlike the <qualifier> right above it. A typo
    there fails much later and much less clearly.

  • The JNDI prefix is hardcoded rather than using JndiConstants.PERSISTENCE_UNIT_NAMING_CONTEXT.

  • qualifierSelectsTheMatchingPersistenceUnit is tautological — it would pass against a stub.

Question rather than a finding: produceWith(instance -> lookupEntityManagerFactory(unitInfo.id).createEntityManager())
hands out the provider EM unwrapped by JtaEntityManager, so it bypasses TomEE's JTA integration —
and resolveEntityManagerScope never consults unitInfo.transactionType (populated at
AppInfoBuilder:685), so a RESOURCE_LOCAL unit gets TransactionScoped. TransactionContext.isActive()
(:50-60) returns false with no JTA transaction, so that EM is unusable outside one. Is
TransactionScoped mandated unconditionally by the platform spec here, with apps expected to declare
<scope> for RESOURCE_LOCAL? The 3.2 XSD (:117) documents <scope> with no stated default, so I
couldn't settle it from the schema alone.

Low-priority, only reachable via the JPA JMX operations: the @Dependent
CriteriaBuilder/Metamodel/Cache/PersistenceUnitUtil beans call the accessor once at
instantiation, so they capture the current delegate. ReloadableEntityManagerFactory.reload()
(:383-389) replaces the delegate without closing the old one, so after a reload an
@ApplicationScoped bean holds a CriteriaBuilder bound to the superseded EMF while
@Inject EntityManagerFactory follows the new one. Resolving through the EMF per call, or a
documented limitation, would avoid the inconsistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants