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