CAMEL-24589: Fix platform-http shared path consumer removal on route stop - #26054
Conversation
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks for the fix, @atiaomar1978-hub — the root-cause analysis is spot-on and the change is well-tested. I checked out the branch and ran the full camel-platform-http suite: 36 tests pass (1 skipped), including your 7 new tests. Mockito resolves transitively via camel-test-junit6, and using JUnit assertions is the right call here since the module is entirely JUnit (no AssertJ), so that follows convention.
The core fix is correct and nicely targeted: consumer-identity equals/hashCode + LinkedHashSet lets sibling consumers coexist on a shared path, and doStop() now removes only the stopping consumer's model (matching the platformHttpConsumer reference registered in doStart()). Registration/removal stay symmetric.
A couple of points to consider (non-blocking):
Visible behavior change — getHttpEndpoints() can now hold multiple entries with the same URI, so the startup log (MainHttpServerUtil) and dev console (PlatformHttpConsole) will show a shared path once per verb/consumer instead of a single merged line, and ordering changes from URI-sorted (TreeSet) to insertion order (LinkedHashSet). This is arguably more correct, but it's user-visible — worth confirming no downstream code assumes URI-uniqueness of that set, and possibly a short upgrade-guide note. (I verified nothing in apache/camel does toMap/dedup on it.)
See the two inline comments below. Overall this is close — the main thing I'd like clarified is the Comparable removal.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Croway
left a comment
Croway
left a comment
There was a problem hiding this comment.
Claude Code on behalf of Croway — findings from reviewing this branch, verified with local reproducers. Inline comments below cover what's touched by this diff; two more findings sit in files this PR doesn't modify, so noting them here instead:
DefaultRestOpenapiProcessorStrategy.doStop()(rest-openapi, line 456) andVertxMcpServerEngine.doStop()(mcp-server, line 139) still callPlatformHttpComponent.removeHttpEndpoint(String), which removes every consumer sharing that uri. That's the exact bug class this PR fixes forDefaultPlatformHttpConsumer— still reachable through these two callers. If a rest-openapi or MCP route shares a path with another platform-http route, stopping one still wipes the other's registration. Worth switching both to remove by identity (a capturedConsumerfor rest-openapi, or the returnedHttpEndpointModelfor mcp-server, since it registers with anullconsumer).MainHttpServerUtil's "log the endpoints summary only if changed" check (last.containsAll(endpoints)) now depends onHttpEndpointModel#equals(), which includes consumer identity. A route reload creates a fresh inner consumer, so the rebuilt model is never.equals()to the previous one even when uri/verbs are unchanged — the summary will re-log on everycamel run --devreload instead of only real changes.
…stop When multiple platform-http consumers share the same path with different HTTP methods, stopping one route must not unregister siblings. Track HttpEndpointModel identity by consumer reference and remove endpoints by consumer on doStop instead of by path alone. Co-authored-by: Cursor <cursoragent@cursor.com>
Use LinkedHashSet instead of TreeSet for endpoint registry, guard null consumer removal, and add restart/null-consumer lifecycle tests. Co-authored-by: Cursor <cursoragent@cursor.com>
- Restore HttpEndpointModel Comparable with consumer-aware compareTo - Refactor endpoint removal to a shared Predicate-based helper - Add removeHttpEndpoint(String, Consumer) for null-consumer registrations - Guard doStart registration when platformHttpConsumer is null - Fix rest-openapi and MCP server to remove by consumer identity - Stabilize MainHttpServerUtil startup summary across route reloads - Document registry behavior change in the 4.23 upgrade guide Co-authored-by: Cursor <cursoragent@cursor.com>
c0ec871 to
c9208fa
Compare
|
Rebased onto latest main ( Cursor Agent on behalf of atiaomar1978-hub |
|
Replying to @Croway's review notes on callers outside the original diff — all addressed in
Cursor Agent on behalf of atiaomar1978-hub |
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
✅ Generated files have been updatedA regen commit was automatically pushed to this branch. CI will re-run shortly. |
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 24 tested, 26 compile-only — current: 23 all testedMaveniverse Scalpel detected 50 affected modules (current approach: 23).
|
|
Thanks everyone ! 👍 |
5 participants
Summary
Fixes CAMEL-24589: stopping one
platform-httpconsumer on a shared path no longer unregisters sibling consumers registered for different HTTP methods.Problem
With two consumers on the same path (e.g. GET and POST on
/shared), stopping the route registered second caused the first consumer to be unregistered from the component registry. Spring Boot'sCamelRequestHandlerMappinglistens to these registrations, so the surviving GET route returned HTTP 405 after stopping the POST route.Root causes in
camel-platform-http:HttpEndpointModelused URI-onlyequals/hashCode, so only one model per path could be tracked reliably.DefaultPlatformHttpConsumer.doStop()calledremoveHttpEndpoint(path), removing all endpoints on that path.Solution
HttpEndpointModelidentity (equals,hashCode).TreeSettoLinkedHashSetso multiple consumers on the same path coexist.removeHttpEndpoint(Consumer)/removeHttpManagementEndpoint(Consumer)with null-safe removal.removeHttpEndpoint(platformHttpConsumer)fromDefaultPlatformHttpConsumer.doStop().removeHttpEndpoint(String uri)for callers that intentionally remove all endpoints on a path (e.g. rest-openapi).Tests
HttpEndpointModelTest— set retains multiple consumers on the same path; identity is consumer-based.PlatformHttpSharedPathRouteLifecycleTest— stopping GET or POST preserves the other consumer, route restart re-registers, URI-based bulk removal, null-consumer guard.Notes
The Spring Boot starter integration test lives in
apache/camel-spring-boot; this PR fixes the core component behavior that the starter depends on.AI-generated PR description on behalf of atiaomar1978-hub