rzo1 commented on PR #2847:
URL: https://github.com/apache/tomee/pull/2847#issuecomment-5088905089
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 `@Ignore`d, 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
