rzo1 commented on PR #2877:
URL: https://github.com/apache/tomee/pull/2877#issuecomment-5318833333
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.
--
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]
