[
https://issues.apache.org/jira/browse/TOMEE-4643?focusedWorklogId=1036154&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1036154
]
ASF GitHub Bot logged work on TOMEE-4643:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 17/Aug/26 18:50
Start Date: 17/Aug/26 18:50
Worklog Time Spent: 10m
Work Description: 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.
Issue Time Tracking
-------------------
Worklog Id: (was: 1036154)
Time Spent: 1h (was: 50m)
> REST 3.1: no META-INF/services discovery for Feature/DynamicFeature
> -------------------------------------------------------------------
>
> Key: TOMEE-4643
> URL: https://issues.apache.org/jira/browse/TOMEE-4643
> Project: TomEE
> Issue Type: Bug
> Reporter: Markus Jung
> Assignee: Markus Jung
> Priority: Major
> Time Spent: 1h
> Remaining Estimate: 0h
>
> TomEE does not scan {{META-INF/services}} for {{Feature}} and
> {{DynamicFeature}} implementations. The REST 3.1 spec asks the runtime to
> pick these up through the standard Java {{ServiceLoader}} mechanism at deploy
> time. Because TomEE (through CXF) skips this step, a {{Feature}} or
> {{DynamicFeature}} packaged this way never registers, and any behavior it
> should add to the request or response pipeline never runs.
> Related upstream tracking: CXF-9005.
> This gap sits in TomEE's JAX-RS/CXF integration, not in the TCK or the test
> harness.
> Related to TOMEE-4321, the open umbrella ticket for JAX-RS TCK failures.
> NOTE: this issue originally also covered a second, unrelated gap — a request
> to an unmatched {{@Path}} returning HTTP 405 instead of 404. That has a
> different root cause and has been split out to TOMEE-4659.
> h2. Steps to reproduce / TCK reference
> Suite: Jakarta RESTful Web Services 4.0 TCK
> ({{jakarta.ws.rs:jakarta-restful-ws-tck:4.0.1}}).
> Feature/DynamicFeature discovery:
> *
> {{ee/jakarta/tck/ws/rs/jaxrs31/spec/extensions/JAXRSClientIT.java#featureIsRegisteredTest}}
> *
> {{ee/jakarta/tck/ws/rs/jaxrs31/spec/extensions/JAXRSClientIT.java#dynamicFeatureIsRegisteredTest}}
> Both are excluded via the surefire {{<excludes>}} block in
> {{tck/jax-rs/jax-rs-tests/pom.xml}} (as a whole-class exclusion of
> {{**/jaxrs31/spec/extensions/JAXRSClientIT.java}}, since surefire cannot
> exclude individual methods). Remove that exclusion once fixed and the suite
> should pass those tests.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
