TOMEE-4643 - discover Feature/DynamicFeature from META-INF/services

[jungm]

Jakarta REST requires Feature and DynamicFeature implementations declared in META-INF/services to be registered through the JDK ServiceLoader at deploy time ("Services", Providers / Lifecycle and Environment).

CXF does not implement this lookup itself — it relies on the container to hand it already resolved providers, which is why the CXF TCK run stays green on GlassFish. TomEE assembles its provider list on its own, so features packaged this way never registered.

Changes

• CxfRsHttpListener scans both SPIs while assembling the provider list.
• Service loading is skipped when the Application maps jakarta.ws.rs.loadServices to Boolean.FALSE, as the spec mandates, and the existing cxf.jaxrs.skip-provider-scanning property is still honoured.
• Entries are loaded one at a time so a single broken descriptor entry is logged and skipped instead of discarding the remaining providers of that type.
• Discovered instances flow through the existing providers(...) path, so the @ConstrainedTo and deactivation checks still apply.
• Drops the now-passing TCK exclusion for jaxrs31/spec/extensions/JAXRSClientIT.

Testing

• New ServiceLoaderProviderDiscoveryTest (8 tests) exercises discovery through a throwaway URLClassLoader, so it does not register these features into other tests in the module. Each assertion was confirmed to fail when the corresponding production change is reverted.
• ee.jakarta.tck.ws.rs.jaxrs31.spec.extensions.JAXRSClientIT passes 2/2 unexcluded against a rebuilt TomEE plus distribution; the wider jaxrs31.** slice passes 4/4.
• Full openejb-cxf-rs suite: 123 tests, the only failure being EJBExceptionMapperTest.security, which fails identically on a clean checkout of main.

🤖 Generated with Claude Code

[jungm]

https://ci-builds.apache.org/job/Tomee/view/tomee-11.x/job/pull-request-manual-with-tck/57/

[rzo1]

@jungm Can you check on ee.jakarta.tck.ws.rs.jaxrs21.ee.sse.ssebroadcaster.JAXRSClientIT.sseBroadcastTest ? Is this a regression?

[jungm]

@rzo1 think its flaky but will check

[jungm]

its flaky, sseBroadcastTest starts 5 SSE clients on threads, waits a fixed 2.5s for them to connect, broadcasts 7 messages, then waits only on the first client before asserting all 5 received exactly 8 events. Clients could not be connecting in time or still be processing evens on a crowded machine

Also seems to have failed in other runs I did here and there

[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. Take it as input, not as a verdict — I have not run the build.

Overall: the mechanism looks sound. addServiceLoaderProviders is wired into configureFactory after the application.getClasses() provider loop, so the alreadyRegistered check de-dupes against both the Class entries and the singleton instances RESTService puts into additionalProviders. The local ArrayList copy means the caller's collection is not mutated. The classloader story holds: configureFactory runs with the TCCL set to CxfUtil.initBusLoader() and CxfContainerClassLoader.findResources delegates to the per-thread webapp loader, so webapp descriptors are visible. InternalApplication.getProperties() delegates to the original application, so the jakarta.ws.rs.loadServices opt-out is not lost by the wrapper. The hand-rolled hasNext()/next() error loop was reproduced standalone against JDK 17 ServiceLoader — it recovers from missing classes, throwing constructors and missing no-arg constructors, and terminates in every case.

1. @ConstrainedTo(CLIENT) features are rejected with an exception instead of being skipped (minor)

CxfRsHttpListener.java — the discovered instances are appended to additionalProviders and therefore flow into providers(...), whose instance branch calls isNotServerProvider(o.getClass()). That method does not merely filter: with openejb.jaxrs.fail-on-constrainedto defaulting to true it throws IllegalArgumentException.

That check was previously only ever applied to providers the application itself declared, i.e. things the developer opted into. Service loading changes the population being validated: it now includes every Feature declared by any jar visible to the webapp classloader. @ConstrainedTo(RuntimeType.CLIENT) is precisely the mechanism the spec gives a library to say "do not register me on the server", so the container should skip it, not fail.

Scope check, so this is not overstated: the exception escapes configureFactory → deployApplication → RESTService.afterApplicationCreated, but ObserverManager.MethodInvocation.invoke catches InvocationTargetException, fires ObserverFailed and logs SEVERE without rethrowing — so Assembler.createApplication continues and the application still deploys. What is lost is the REST publication of that app (and of any webapp later in the loop), with a SEVERE stack trace. Also, no artifact in a full local ~/.m2 scan ships a META-INF/services/jakarta.ws.rs.core.Feature descriptor at all today, so this is a forward-looking robustness gap rather than an observable regression.

Suggested fix: filter @ConstrainedTo inside addServiceLoaderProviders and drop the entry with a warning, or pass a flag so isNotServerProvider never throws for service-loaded entries. FAIL_ON_CONSTRAINED_TO exists to catch mistakes in the app's own provider set; it should not judge ambient classpath entries.

2. jakarta.ws.rs.loadServices is only honoured as a Boolean, not as "false" (minor)

isServiceLoadingEnabled() uses !Boolean.FALSE.equals(properties.get(LOAD_SERVICES_PROPERTY)). Application.getProperties() is user-supplied and commonly carries values that originated as strings (config files, init-params, property files merged into the map). "false" is not Boolean.FALSE, so the opt-out is silently ignored and the scan still runs. Accepting Boolean.FALSE or "false" (case-insensitive) would make the spec switch behave as users expect.

3. The comment and test name overstate the per-entry recovery (nit)

The comment ("a single broken entry must not discard the remaining providers of this type") and brokenEntryDoesNotDiscardTheOthers promise more than the JDK allows. Per-entry recovery works for unresolvable class names, missing no-arg constructors and throwing constructors — all three verified. But a malformed line makes ServiceLoader.parse() abort the whole file (it reads the descriptor eagerly and fails it entirely, dropping even entries listed before the bad line), and since configs has already consumed that URL the loop resumes at the next descriptor. The loop is still safe and terminates — this is purely a wording issue. Either soften the comment or add the malformed-line case to the test so the real limit is documented.

Test coverage

All eight new tests call addServiceLoaderProviders/isServiceLoadingEnabled in isolation. Nothing exercises the new if (!ignoreAutoProviders && isServiceLoadingEnabled(application)) wiring in configureFactory, nor the providers(...) path the discovered instances actually take — which is where finding 1 lives. A test declaring a @ConstrainedTo(RuntimeType.CLIENT) feature in a descriptor and asserting the deployment still publishes would pin that invariant.