jungm commented on PR #2848: URL: https://github.com/apache/tomee/pull/2848#issuecomment-5108799219
Thanks — you were right that the bug wasn't actually fixed. Verified every claim before acting; all confirmed. Pushed 3 commits. **`WsDeployer` (the real gap).** Confirmed: `ConfigurationFactory:260-262` adds it whenever `openejb.webservices.enabled` && `WSDL4J_AVAILABLE`, `libre-wsdl4j` is `<optional>` at pom `:612-616`, and `processPorts` does `loadClass` at `:150` *before* the `isWebService` check at `:151` — so it hits every servlet, not just WS endpoints. Now resolved in its own try/catch that skips webservice detection for that servlet and continues. Also caught `NoClassDefFoundError` explicitly, since as you noted `catch (Exception)` never covered it. The remaining `catch (Exception)` can now only fire on genuine WS configuration failures, so I reworded its message rather than leaving it claiming a load failure. **The test couldn't do its job.** Correct — and demonstrated: added `missingServletClassDoesNotFailFullConfiguration`, which drives `ConfigurationFactory.configureApplication` so the whole chain runs. Reverting *only* `WsDeployer.java` makes it fail with `OpenEJBException: Unable to load servlet class: ...TestServlet1`, while the `AnnotationDeployer` warning still logs first — exactly the gap you described. wsdl4j is on the test classpath, so `WsDeployer` genuinely runs. Kept the original test too, since it pins the `AnnotationDeployer` behavior directly. **Discarded cause.** Fixed — all sites now pass `e` and `webModule.getJarLocation()`. **Filters/listeners vs §2.3.1.** You're right that §2.3.1's deferral allowance covers servlets only. Both now log at `error` rather than `warning`. I read the spec source at tag `6.1.0-RELEASE` rather than trusting my earlier summary: §6.2.1 says the container instantiates a filter "before a request causes the container to access a web resource" — later than deploy, but not skippable if the filter is ever used — so the comment now says that instead of the "must be instantiated at context start" I'd first written. Worth noting the spec's `must fail to deploy` clauses (`:3880`, `:3991`, `:4274`, `:4364`, `:4407`) are all web-fragment ordering conflicts, not unresolvable component classes, so nothing there requires the old fatal behavior. **`LightweightWebAppBuilder:183-186`.** Confirmed and fixed — it now skips the unloadable listener and logs at error, instead of throwing a raw CNFE mid-`createApplication`. **Fail-open side effect.** Agreed; called it out explicitly in the servlet comment. **MessageFormat.** Did all of them — remaining 7 in a separate commit (`a6e030785e`), message text untouched, indices only. **Handler chain.** Left fatal, now with a comment saying why: a silently missing handler leaves the endpoint running under a weaker contract than declared (e.g. a dropped security handler), which is worse than disabling one component. One thing I did not do: removing the `runner-standalone/exclusions/servlet.txt` entries and confirming both TCK classes pass, since that's the separate apache/tomee-tck repo and needs a full run against a built server. Unrelated pre-existing flakiness worth flagging: `WebAppEnvEntryTest`, `AutoDeployerTest`, `BuiltInEnvironmentEntriesTest`, `VagueEnvEntriesMatchTest` and `ActivationConfigPropertyOverrideTest` fail non-deterministically from shared static state — two identical runs of the same code give different failure sets, and they fail on unmodified `main` too. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_ -- 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]
