TOMEE-4654 - Allow the component naming context to be made read-only

[jungm]

What

Adds working support for the read-only java:comp component naming context that the Enterprise Beans spec (10.4.4) and EE.5.3.4 require, behind openejb.forceReadOnlyAppNamingContext. The default stays writable, so this changes nothing for existing applications; the TCK turns the flag on for its runs.

Why

The IvmContext read-only enforcement has existed since d5b3b93 (2017) — checkReadOnly() throws OperationNotSupportedException and setReadOnly() cascades through the name tree — but nothing ever turned it on, and turning it on at the obvious place did not work. This makes the switch actually usable.

Per review discussion, enabling it by default is a behavioural change that would break applications writing into their own ENC after deployment, so it remains opt-in until that can be a release-wide change. TomEE therefore still deviates from the spec out of the box; this PR provides the mechanism and the TCK coverage rather than flipping the behaviour for everyone.

Changes

• Assembler: openejb.forceReadOnlyAppNamingContext is read from appInfo.properties first with the system property as fallback (as OPENEJB_TIMERS_ON and friends do), so it can be set per application or container-wide, and parsed with Boolean.parseBoolean so =FALSE is honoured. Default false.
• Lifecycle — where the marking happens is the substance of this PR. The intent is recorded on the AppContext when the application is configured and applied at the end of startEjbs:
  • isSkip defers the EJB modules of an EAR's web modules to the web app builders, which call initEjbs/startEjbs after createApplication has returned. Marking in createApplication makes JndiBuilder's app/<module>/<bean> bindings fail with OperationNotSupportedException on that later pass, while the java:comp of exactly those modules is never closed at all.
  • The containers bind comp/EJBContext, comp/WebServiceContext and comp/TimerService into each bean ENC from SingletonInstanceManager.deploy()/StatelessInstanceManager.deploy(), which run in startEjbs — so marking at the end of initEjbs breaks singleton and stateless deployment outright.
  • The shared application context is only closed once no further module can bind into it, tracked by a count of the late modules still to come.
• AppContext: carries the read-only intent and the pending-late-module count.
• Tests: new JavaCompReadOnlyTest opts into the flag, deploys a real application and asserts every write op is refused on java:comp and java:app, that nothing written becomes observable, and that pre-existing bindings survive; AppNamingReadOnlyTest covers the enabled and default paths plus the late-module deferral.

Verification

• openejb-core full suite: 4096 tests, the only 6 failures are pre-existing security-test failures that reproduce on a clean main. (ConnectionFactoryTxTest failed once with a null injected ConnectionFactory and passed on a repeat full run plus 5 isolated runs; InjectionProcessor performs no ENC writes, so read-only cannot affect injection — a pre-existing flake against the shared ActiveMQ broker.)
• Jakarta EE 11 Web Profile TCK enterprise-beans-30 with the flag enabled: the naming/context write assertions pass in both the EJB and web (servlet / filtered-servlet / JSF) vehicles.

Reviewer notes

• The matching tomee-tck harness change (enabling the flag and un-excluding the write tests) is a separate PR in apache/tomee-tck.
• There is deliberately no WebContext handling: WebContext.jndiEnc holds an InitialContext or a WebInitialContext proxy, never an IvmContext/ContextHandler, so marking it would be a no-op.
• The TCK run surfaced a separate, pre-existing read-side bug (TOMEE-4658): in web components TomEE hands out Tomcat's org.apache.naming.NamingContext instead of IvmContext, so java:comp/env lists extra comp/module entries and close() fails. Orthogonal to this change and not addressed here.

🤖 Generated with Claude Code

[rzo1]

The underlying bug is real and worth fixing — the read-only machinery has been there since
d5b3b93 (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.

[jungm]

Thanks — this was a genuinely useful review, and you were right on every point. Pushed in 5cf5d33.

Timing. Moved as you suggested, but the end of initEjbs turned out to still be too early: the containers bind comp/EJBContext, comp/WebServiceContext and comp/TimerService into each bean ENC from SingletonInstanceManager.deploy()/StatelessInstanceManager.deploy(), which run in startEjbs. Marking at the end of initEjbs made JavaCompReadOnlyTest fail to deploy at all with Failed to bind EJBContext/WebServiceContext/TimerService — so it would have broken every singleton and stateless deployment. The marking now happens at the end of startEjbs, with the intent recorded on the AppContext at configuration time, so the late modules from TomcatWebAppBuilder are covered and all container-internal binds have run first.

The shared app context. Since initEjbs/startEjbs run once per web module for an EAR, closing appContext.getAppJndiContext() on the first pass would have broken the later ones in exactly the way you described. AppContext now carries a count of the late modules still to come (appInfo.webAppAlone ? 0 : appInfo.webApps.size()) and only closes the app context on the final pass. AppNamingReadOnlyTest.testAppContextStaysWritableUntilTheLastModule and testAppContextWaitsForEveryLateModule pin that.

Dead WebContext loop. Confirmed and removed — setJndiEnc gets new InitialContext() from TomcatWebAppBuilder or a WebInitialContext proxy from LightweightWebAppBuilder, never an IvmContext/ContextHandler. You're also right that the PR body's claim about it fixing the web vehicles was unsupported; the web-tier TCK writes pass because the EJB contexts are marked. I've corrected the description. The remaining web-tier read-side gap is filed as TOMEE-4658.

Opt-out. Now reads appInfo.properties first with the system property as fallback, matching OPENEJB_TIMERS_ON, and parsed with Boolean.parseBoolean so =FALSE is honoured.

EmbeddedTomEEContainerTest. Inverted to testEjbCannotCreateSubContextByDefault, accepting either refusal mode.

Test nits. assertWriteRefused no longer requires OperationNotSupportedException (it tolerates both outcomes and the not-observable assertion carries the weight), deploy() moved inside the try/finally, tautological rename/destroySubcontext assertions replaced, and the if(/for( spacing fixed in the new code.

Full openejb-core suite: 4096 tests, 6 failures, all pre-existing security ones that reproduce on a clean main. One run also failed ConnectionFactoryTxTest with a null injected ConnectionFactory, which looked like a plausible consequence of a read-only ENC — but InjectionProcessor performs no ENC writes, it passed 5 isolated runs and a repeat full run, so it is a pre-existing flake against the shared broker rather than fallout from this change.

Agreed on the Arquillian point; the inverted test above is the real-container coverage, and I'm happy to add more if you'd like a specific EAR-with-WAR scenario.

🤖 Addressed by Claude Code

[rzo1]

Since this is a behavioural change: WDYT about leaving the default writeable and set the config to non-writable for the TCK only? @jungm ? Fullbuild: #54

[jungm]

Good call — done in 3c75c91. openejb.forceReadOnlyAppNamingContext is back to false by default, so nothing changes for existing applications, and the TCK turns it on for its runs. I've retitled the PR and rewritten the description to match, since it now adds the capability rather than flipping the behaviour.

The lifecycle work from the last round stays: when the flag is set, the contexts are marked at the end of startEjbs, so an EAR's late web modules are covered and the container's own binds (app/<module>/<bean>, comp/EJBContext, comp/WebServiceContext, comp/TimerService) all run first. Also made the constant public so the harness can set it, opted JavaCompReadOnlyTest into the flag explicitly, and restored EmbeddedTomEEContainerTest.testEjbCanCreateSubContextByDefault to asserting the write succeeds.

Full openejb-core suite after the flip: 4096 tests, 6 failures, all the pre-existing security ones that reproduce on a clean main.

One consequence worth stating plainly: with the default off, TomEE still deviates from EE.5.3.4 / Enterprise Beans 10.4.4 out of the box, so the behaviour TOMEE-4654 reported is unchanged for anyone who doesn't set the flag. Happy to leave it here and revisit the default for a major release, or file a follow-up to track it — whichever you prefer.

The matching tomee-tck change (setting the flag in the TomEE config and un-excluding the write tests) will be a separate PR there.

🤖 Addressed by Claude Code

[rzo1]

Currently short on time, so AI review only: I ran an adversarial review pass over this PR, followed by a second pass whose job was to refute the first one's findings against the actual code. Everything below survived that second pass — including one correction where the refutation pass ran the real classes and disproved the reviewer's stated failure mode, which turned out to be the most interesting result here. Take it as input, not as a verdict — I have not run the build.

Overall: the core idea is sound and the risky parts hold up. Moving the marking from the end of createApplication into the end of startEjbs is genuinely necessary — all the comp/EJBContext, comp/WebServiceContext and comp/TimerService bindings happen in Container.deploy(), which runs inside startEjbs. Nothing between startEjbs and the end of createApplication writes into a bean ENC or the app JNDI context (bindGlobals, postConstructResources, the app-client loop, and the AssemblerAfterApplicationCreated observers in WsService/RESTService/Hessian/BatchEE all target the container-system global context or only read). Each component context is a fresh IvmContext root, so setReadOnly cannot leak into a shared tree; the ContextHandler wrapper delegates writes to the underlying IvmContext so enforcement still applies; undeploy only unbinds from the global context; and the flag defaults off. The AtomicInteger races are benign.

What does not hold up is the mechanism for deciding when the shared application context can be closed.

1. The pendingLateModules counter never drains when a deployment pass has no EJBs (minor)

setAppNamingContextReadOnly() locates the AppContext via the new appContextOf(allDeployments) helper, which returns null for an empty list — and then returns false before calling appContext.lastModuleDeployed(). So the counter is only decremented by passes that happen to contain at least one BeanContext, while setPendingLateModules(appInfo.webAppAlone ? 0 : appInfo.webApps.size()) assumes exactly 1 + webApps.size() passes.

Both ends break on ordinary packagings:

• The createApplication pass is empty whenever the EAR has no EJB module in lib — isSkip (Assembler.java:1642) skips every ejbJar.webapp == true module when webappId == null. So an EAR whose beans live only in its WARs produces N passes for a count of N.
• A late pass is empty whenever an EAR web module has no EJBs (a plain JSF/servlet WAR). TomcatWebAppBuilder.java:1468 still calls assembler.startEjbs(true, beanContexts) unconditionally, but with an empty list — and a WAR with no EJBs and no beans.xml gets no synthetic .Comp bean either, since AnnotationDeployer only adds CompManagedBean when beans != null.

Counting passes rather than tracking which modules are outstanding is the root cause. The AppContext is available to startEjbs through the caller in every pass, so it does not need to be recovered from the bean list.

Important correction to the impact, because the first pass got this wrong and the refutation pass disproved it by running the real classes: java:app is not left writable to applications. EnterpriseBeanBuilder:153 binds the shared java:app subcontext into every bean ENC, and NameNode.setReadOnly recurses into Federation members (NameNode.java:300-311), so marking any single bean ENC already flags the shared java:app subtree read-only. An EJB doing new InitialContext().lookup("java:app").createSubcontext("x") gets OperationNotSupportedException even when the counter never drained. What actually leaks is only the root AppContext.getAppJndiContext() handle, reachable via javaURLContextFactory when there is no ThreadContext and the TCCL is the app classloader — a narrow, fail-open gap in an opt-in feature. Hence minor, not major.

Note the added unit test cannot catch this: testAppContextWaitsForEveryLateModule drives lastModuleDeployed() directly and always passes a non-empty bean list.

2. That transitive marking means the counter machinery is largely redundant (minor — design)

Following from the verification above: since the first startEjbs pass already makes java:app read-only for every application-visible path, the AtomicInteger pass counter, the new AppContext.lastModuleDeployed() API and the tests built around them buy nothing except deferring the flag on the root context object. The documented invariant "the application context still accepts the container's own bindings" happens to hold only because JndiBuilder.bindJava binds through the root instance, which never checks the node-level flag.

Verified directly: after comp.setReadOnly(true) on a comp context that has "app" bound, a write reached as comp → app throws OperationNotSupportedException, while binds issued on the shared app root IvmContext still succeed.

So rather than fixing the counter per finding 1, it may be worth deleting it and marking the app context on the first pass (or at least correcting the javadoc to describe what is actually enforcing the invariant). That would remove both this and finding 3.

3. The embedded path skips startEjbs when CDI is off (nit)

LightweightWebAppBuilder.deployWebApps() calls assembler.initEjbs(...) for every web module when !webAppAlone (line ~146) but only calls assembler.startEjbs(true, beanContexts) inside if (!appInfo.webAppAlone && hasCdi(appInfo)) (line ~180). Since the patch hangs the "last module in" decision exclusively off startEjbs, an EAR deployed embedded with CDI disabled never reaches the final pass. Same root cause as finding 1 on a different call site, and it makes the feature behave differently between the embedded and Tomcat runtimes. Nit rather than minor because in that configuration those bean contexts are never deployed into their containers at all (a pre-existing embedded quirk), and the federated java:app view is still read-only.

4. Two vacuous test assertions and a duplicated test (nit)

JavaCompReadOnlyTest.assertWriteRefused only fails when an exception other than OperationNotSupportedException escapes, so a write that silently succeeds is accepted and the invariant is delegated to the follow-up assertNotBound/lookup checks. That is fine for bind/rebind/createSubcontext, but IvmContext.rename (IvmContext.java:378) and IvmContext.destroySubcontext (line 412) throw OperationNotSupportedException unconditionally, with no readOnly check — so those two lines would pass with the feature completely removed. Either drop them or assert them against a writable context too.

Separately, testReadOnlyAppNamingContext and testAppNamingContextReadOnlyWhenEnabled in AppNamingReadOnlyTest are byte-for-byte identical.