rzo1 commented on PR #2846:
URL: https://github.com/apache/tomee/pull/2846#issuecomment-5088905273
The underlying bug is real and worth fixing — the read-only machinery has
been there since
d5b3b93d4f (2017) and nothing ever enabled it, so every ENC has been
writable in violation
of EE.5.3.4 / Enterprise Beans 10.4.4. I applied the patch and confirmed
`JavaCompReadOnlyTest`
fails on unpatched main with `bind should have been refused` and passes with
the fix, and that
the read-only cascade doesn't leak into the container root context or the
per-app global/module
contexts.
I can't approve as-is though — the marking happens at the wrong point in the
lifecycle, and it
cuts both ways:
**Container-internal binds can now fail.** `Assembler.isSkip` (:1634-1645)
skips every webapp
`EjbJarInfo` when the app is not `webAppAlone`. So for an EAR containing a
WAR, that WAR's
EJB/managed-bean module is deployed later, by
`TomcatWebAppBuilder.startInternal` via
`assembler.initEjbs` (:1453) and `startEjbs` (:1468) — i.e. after
`createApplication` has already
marked `appContext.getAppJndiContext()` read-only. `initEjbs` ->
`jndiBuilder.build` ->
`JndiBuilder.bindJava` does `appContext.bind("app/" + moduleName + beanName,
ref)`
(`JndiBuilder:694`, `:722`), outside the `NameAlreadyBoundException` catch,
and the caller wraps
`NamingException` into `OpenEJBRuntimeException` (:443).
I probed this directly in openejb-core: with the patch applied, after
`createApplication` returns,
`app1.getAppJndiContext().bind("app/mod3/SomeBean", ref)` fails with
`javax.naming.OperationNotSupportedException`. I couldn't run a full EAR
deployment here, so the
end-to-end failure is inferred from the call chain — but the refusal itself
is proven and the
call chain is unconditional. Reload of such a WAR re-enters the same path
(`TomcatWebAppBuilder:2087` only destroys the app when
`isUnDeployable`/`webAppAlone`).
**And the same modules never get marked.** The flip side:
`setAppNamingContextReadOnly` only
iterates `allDeployments`, the BeanContexts built during
`createApplication`. The webapp modules
excluded by `isSkip` get their BeanContexts created afterwards, so their
`java:comp` stays fully
writable — the spec violation you're targeting survives for exactly the EJBs
that live in an EAR's
web modules.
Both problems have one shape of fix: record the read-only intent on the
`AppContext` and apply it
where the BeanContexts are actually created (end of `initEjbs`/`startEjbs`),
so late modules inherit
it and the container's own binds all run before the flag takes effect.
Other things:
- The new `WebContext` loop is dead code. Neither `TomcatWebAppBuilder` nor
`LightweightWebAppBuilder` stores an `IvmContext` or `ContextHandler` in
`WebContext.jndiEnc`, so
`markReadOnly(webContext.getJndiEnc())` never matches either branch. That
means the PR body's
claim that this is what fixed the web vehicles isn't supported by the
code, and web components
still get a writable ENC.
- `arquillian-tomee-embedded`'s
`EmbeddedTomEEContainerTest.testEjbCanCreateSubContextByDefault`
(:72-88) still asserts the pre-PR semantic. It needs to be inverted in
this PR, and honestly a
container-wide default flip with this blast radius shouldn't land with
only openejb-core unit
coverage — an Arquillian test against a real TomEE would be worth it.
- The opt-out is read from `SystemInstance` only, never from
`appInfo.properties`. Most other
Assembler switches support both (e.g. `OPENEJB_TIMERS_ON` at :1501/:1560
reads
`appInfo.properties.getProperty(..., globalDefault)`). As written, one
legacy app that writes into
its ENC forces the whole container off the spec-required behaviour.
Reading `appInfo.properties`
first with the system property as fallback keeps it per-application.
- `assertWriteRefused` hard-requires `OperationNotSupportedException`, which
couples the new test to
`openejb.jndiExceptionOnFailedWrite` — the `createSubcontext` case right
below it correctly
tolerates both outcomes. Make them consistent.
- Minor test things: the `rename`/`destroySubcontext` assertions are
tautological as written;
`deploy()` runs outside the try/finally so a failure there skips
`SystemInstance.reset()` and
pollutes the next test.
-
`"true".equals(SystemInstance.get().getProperty(FORCE_READ_ONLY_APP_NAMING,
"true"))` is
case-sensitive, so `-Dopenejb.forceReadOnlyAppNamingContext=FALSE`
silently keeps read-only on.
Now that this is the opt-out for a spec-behaviour change, that matters
more than it did.
- `if(`/`for(` without the space doesn't match the file's convention.
Pre-existing in the lines you
moved, but the new `markReadOnly` helper is new code.
--
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]
