Conversation
davsclaus
left a comment
There was a problem hiding this comment.
Review
Thanks for this Pontus — nice piece of work. The extension point follows the exact idiom the JIRA asked for (lookupUnmatchedRequestHandler() mirrors RestBindingAdviceFactory.lookupRestClientRequestValidator()), the default handler reproduces the previous behaviour verbatim so there is no regression for existing users, both doc copies are in sync, and the tests cover default and custom handlers for both status codes.
Verification performed
| Check | Result |
|---|---|
mvn verify in components/camel-rest-openapi (build cache disabled) |
pass |
| Test totals | 148 run, 0 failures / errors / skipped |
New RestOpenApiUnmatchedRequestHandlerTest |
5 tests pass |
mvn formatter:format impsort:sort |
no diff produced |
src/main/docs vs catalog/.../docs mirror |
byte-identical |
xref:manual::registry.adoc target |
resolves |
Prior-art check: the 404/405 block dates back to 447dcd3c4571 (CAMEL-20557, the original contract-first implementation), so this change does not revert any later intentional decision.
No blocking issues. Three inline notes below, plus these three that are not tied to a single line:
Should the interface live in camel-api rather than the component?
components/camel-rest-postman/src/main/java/org/apache/camel/component/rest/postman/RestPostmanProcessor.java#L117-L125 contains a byte-identical 404/405 block. The precedent the JIRA cites, RestClientRequestValidator, lives in core/camel-api under org.apache.camel.spi rather than in a component. Putting this interface there instead would let camel-rest-postman reuse it, rather than growing a second, incompatible handler SPI later. Component-local is defensible for a first cut — mainly flagging it so the choice is deliberate.
No @UriParam endpoint option
The sibling extension point on this same component, restOpenapiProcessorStrategy, is exposed as @UriParam(label = "consumer,advanced") on RestOpenApiEndpoint, which gives per-endpoint configuration plus catalog and tooling discoverability. The new handler is resolved once per CamelContext, so two rest-openapi endpoints in one context cannot use different handlers. RestClientRequestValidator is registry-only too, so this matches the model you were asked to follow — but the in-component precedent points the other way. Your call.
Commit body
Commit 91dd59f has a meaningful subject but an empty body, while the PR checklist ticks "each commit has a meaningful subject line and body". Minor, but easy to fill in on the next push.
Note: the PR is still a draft and CI has not run yet, so I have not identified reviewers — that step applies once it is marked ready for review.
Scope: this is a rules-and-conventions review against the project's .oss-ai-helper-rules/ and CLAUDE.md, backed by a real local build and test run. It is not a substitute for CodeRabbit, Sourcery, or SonarCloud static analysis.
Claude Code on behalf of davsclaus
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
@davsclaus Thanks for the review, I've done the proposed changes. A solution to this would be to introduce a new boolean option to the If/when set to false it would instead register a catch-all route. What do you think about this ? Could this be inscope of this PR or would it be the subject of a separate issue ? |
|
The SPI here looks good, but I think it can't take effect yet: the branch it plugs into is never reached on the supported runtimes. I built this branch and registered a custom
The three runtimes have only two consumer implementations: camel-quarkus reuses Both implementations register only the exact OAS surface, so the HTTP layer rejects anything else before Camel is involved:
So The good news is the missing half is small, and it doesn't need any change to what you've written here. I prototyped it on top of this branch:
With that, on both tested runtimes: matched operations unchanged (200), and unmatched requests reach the handler — 404/405 with the custom body, invocations 2. As a bonus Camel's To keep this opt-in I'd gate it on a new endpoint option, which also gives users the choice explicitly: @UriParam(label = "consumer,advanced", defaultValue = "platform", enums = "platform,camel",
description = "Who answers requests matching no operation in the OAS: the HTTP layer "
+ "(Spring Boot's error controller, the Vert.x router) or Camel via the unmatchedRequestHandler.")
private String unmatchedRequestHandling = "platform";Default Two smaller notes on the current diff:
Happy to share the probe tests if useful. Claude Code on behalf of @Croway |
|
@Croway Thanks for the review. Yes this is the "problem" I also found that the changes in CAMEL-22971 makes this dead code. Agree that a However my question remains, would this change within this PR or should I open a separate issue/PR for those changes. Will fix the docs issues you pointed out. |
|
I'd keep the changes in this PR, they are related to https://issues.apache.org/jira/browse/CAMEL-24649 |
bc6ed7a to
34001d0
Compare
|
Had to take help from AI tooling to find all places to update for the new endpoint option, updated commit and issue accordingly. |
5ab3ef6 to
76210fd
Compare
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review of the updated commits (SHA 76210fd).
Previous findings
Checked all findings from the prior rounds (davsclaus, Croway):
| Finding | Status |
|---|---|
unmatchedRequestHandler field uninitialised (NPE on inactive path) |
✅ Fixed — now initialised at declaration with DefaultRestOpenApiUnmatchedRequestHandler |
| Factory-finder route has no test | ✅ Fixed — testCustomHandlerFromFactoryFinderIsCalled added with a mocked ClassResolver |
| Docs missing factory-finder route and single-bean constraint | ✅ Fixed — both are documented with accurate precedence order |
| Dead code (handler unreachable on supported runtimes) | ✅ Fixed — unmatchedRequestHandling=camel now registers a catch-all on VertxPlatformHttpConsumer and via PlatformHttpComponent.addHttpEndpoint for Spring Boot |
| "empty body" claim in docs (Croway) | ✅ Addressed — DefaultRestOpenApiUnmatchedRequestHandler.handle() now calls exchange.getMessage().setBody(null) |
| Multiple registered handlers fall back silently with no WARN (Croway) | ✅ Addressed — docs now state the exact precedence; code behaviour matches |
New commits — two nits
One test typo, one missing defensive null-guard (invariant-safe, but not obvious to the reader).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review of the updated commits (SHA 171f755e).
Previous findings
| Finding | Status |
|---|---|
Typo nit: ablet → able, regitered → registered (test comment) |
✅ Fixed |
Defensive null guard in doStop() — add && platformHttpConsumer != null |
❌ Not fixed — introduced a compile error |
New issue: duplicate if guard breaks compilation
The attempt to add the null guard inserted a new if line but left the old one in place, producing two consecutive if guards with only one closing brace before finally. The outer if (phc != null) block is never closed, which is a syntax error.
See inline comment.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| if (phc != null) { | ||
| if (phc != null && platformHttpConsumer != null) { |
There was a problem hiding this comment.
if (phc != null) guard was left in place when the new guard was added, creating two consecutive if blocks with an unmatched open brace. The outer if (phc != null) is never closed before the finally — this does not compile.
Remove the old guard and keep only the combined condition:
| if (phc != null) { | |
| if (phc != null && platformHttpConsumer != null) { | |
| if (phc != null && platformHttpConsumer != null) { |
oscerd
left a comment
There was a problem hiding this comment.
Nice feature — an unmatched-request handler SPI for rest-openapi is a useful addition. I held off on the design review because CI is red; both failures are quick mechanical fixes, so flagging them so you can get it green:
1. Compile error — RestOpenApiProcessor.java (blocks build (17, false))
The maven build step fails at impsort with the cryptic "the Java file contained parse errors". The real cause is a duplicated line in doStop():
PlatformHttpComponent phc = camelContext.getComponent("platform-http", PlatformHttpComponent.class);
if (phc != null) { // <-- stray, unclosed
if (phc != null && platformHttpConsumer != null) {
phc.removeHttpEndpoint(unmatchedRequestCatchAllPath,
platformHttpConsumer.getPlatformHttpConsumer());
}
} finally { // 'finally' without 'try'The extra if (phc != null) { (line 322) opens a block that is never closed, so the finally no longer pairs with its try and the class doesn't parse. Deleting that one stray line fixes it — the remaining if (phc != null && platformHttpConsumer != null) already does the right null-guard (I confirmed brace balance goes from +1 to 0 with just that line removed, so there's no second syntax issue behind it).
2. Doc validation — rest-openapi-component.adoc (blocks PR doc validation)
ERROR (asciidoctor): target of xref not found: platform-http-vertx.adoc
Line ~258 references xref:platform-http-vertx.adoc[camel-platform-http-vertx], but there is no platform-http-vertx.adoc page — camel-platform-http-vertx doesn't ship its own component doc, and the only platform-http page is platform-http-component.adoc. Point the xref at xref:platform-http-component.adoc[...] (or drop the xref and keep camel-platform-http-vertx as plain text). Remember to regenerate so the mirrored copy under catalog/camel-catalog/src/generated/.../docs/ matches, otherwise the uncommitted-changes check will trip.
Once CI is green I'll come back for a proper review of the SPI shape (RestOpenApiUnmatchedRequestHandler factory + default impl) and the OpenApiDefinition/RestDefinition model additions. Thanks!
This review was generated with AI assistance and reviewed/issued by the human operator. Claude Code on behalf of oscerd
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @ullgren, the rework in response to Croway's finding is a good direction, and most of my earlier points are addressed: the handler is initialised, there's a test for the factory-finder route, and the docs explain how the handler is resolved.
Blocking:
- It doesn't compile: there's a stray
ifinRestOpenApiProcessor(inline). - There's a broken xref in the docs, which fails PR doc validation (inline). The catalog copy of the doc needs regenerating too.
testCatchAllRegisteredOnPlatformHttpWhenCamelHandlingwill likely fail once it compiles (inline).
Worth a look (non-blocking):
- With two APIs in
camelmode and nested base paths (/apiand/api/v3), every catch-all is.last(), so whichever starts first answers unmatched requests for both. For example,PUT /api/v3/pet/123can get a 404 from the/apiprocessor instead of a 405 from/api/v3.testCatchAllDoesNotShadowApiWithNestedBasePathcan't catch this, because both processors share the handler. - Spring Boot: was the catch-all via
phc.addHttpEndpointtested against camel-spring-boot? And with base path/, does the root catch-all (/{*matchOnUriPrefix}) take over static resources or other MVC mappings? - Vert.x with
serverRequestValidation=true: when content types don't match, Vert.x keeps looking at later routes, so thecamelcatch-all may accept requests that should get a 415/406. Is that intended? - From my earlier review, still open: should
RestOpenApiUnmatchedRequestHandlermove to camel-api so camel-rest-postman can reuse it? Fine to leave it component-local if you prefer, but a reply would help. - The JIRA description should be updated for the new
unmatchedRequestHandlingoption. Several commit messages lack theCAMEL-24649:prefix, but a squash merge fixes that.
Claude Code on behalf of davsclaus
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
|
||
| The option is supported by the built-in `platform-http` consumer component: Camel Main when using | ||
| xref:platform-http-vertx.adoc[camel-platform-http-vertx], and Spring Boot when using the platform-http starter | ||
| (`camel-platform-http-starter`). Other consumer components ignore the option. |
There was a problem hiding this comment.
Not quite: RestOpenApiEndpoint forwards unmatchedRequestHandling into the delegate consumer's parameters, so a third-party RestOpenApiConsumerFactory that validates its parameters could reject it rather than ignore it. Low risk, since platform-http is the only in-tree factory, but the wording should reflect that (or the option shouldn't be forwarded).
There was a problem hiding this comment.
Changed wording reflect that other consumer components will decide what to do with the parameter.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review of SHA 6a77fa3.
Previous findings status
| Finding | Status |
|---|---|
doStop() stray if (phc != null) { (compile error) — gnodet-bot + oscerd + davsclaus on SHA 171f755e |
❌ Still present |
Broken xref:platform-http-vertx.adoc — oscerd + davsclaus on SHA 171f755e |
❌ Still present |
testCatchAllRegisteredOnPlatformHttpWhenCamelHandling will fail — davsclaus |
❌ Still present |
| "instead ... instead" doubled word in docs — davsclaus | ❌ Still present |
All four findings from the prior review round remain unaddressed in this push. See inline comments for exact fixes.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
- Fixes CAMEL-24649 - Adds an unmatchedRequestHandling option (platform|camel, default platform) to the rest-openapi component/consumer endpoint, the platform-http consumer endpoint, and the rest DSL openApi section - With unmatchedRequestHandling=camel the rest-openapi component registers a catch-all for the API base path on platform-http so unmatched requests (404/405) are routed to Camel and can be answered by the RestOpenApiUnmatchedRequestHandler for custom response bodies; the catch-all is removed again when the processor stops, and a warning is logged when the specification is not bound to platform-http - The Vert.x catch-all is marked last on the shared router so it cannot shadow operation routes of other APIs with nested base paths such as /api vs /api/v3 - Adds regression tests covering catch-all registration/removal on platform-http and a Vert.x scenario with two rest-openapi endpoints and nested base paths - Documents the option and behavior in the rest-openapi and platform-http component docs; regenerates configurers, URI factories, DSL builders, model writers/parsers, schemas and catalog mirrors Co-authored-by: opencode (GLM-5.3-Flash) <noreply@berget.ai>
Co-authored-by: Guillaume Nodet - AI Bot <gnodet+bot@gmail.com>
6a77fa3 to
a19e0c8
Compare
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review of SHA a19e0c8.
Previous findings status
| Finding | Status |
|---|---|
Compile error — stray if (phc != null) { in doStop() (gnodet-bot + oscerd + davsclaus on SHA 6a77fa3) |
✅ Fixed |
Broken xref platform-http-vertx.adoc in docs (gnodet-bot + davsclaus) |
✅ Fixed — now points to platform-http-component.adoc |
Test assertions wrong: isNull() for consumer arg in addHttpEndpoint / removeHttpEndpoint (gnodet-bot on SHA 6a77fa3) |
❌ Still not fixed |
Nit: "instead … instead" doubled word in docs (davsclaus + gnodet-bot on SHA 6a77fa3) |
❌ Still not fixed |
Two issues remain before this is mergeable.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| PlatformHttpComponent phc = camelContext.getComponent("platform-http", PlatformHttpComponent.class); | ||
|
|
||
| // a catch-all for the api base path (without verbs) must be registered so unmatched requests are routed to Camel | ||
| verify(phc).addHttpEndpoint(eq(""), isNull(), isNull(), isNull(), isNull()); |
There was a problem hiding this comment.
isNull() on a non-null consumer. (Not fixed since SHA 6a77fa3.)
The production code in afterPropertiesConfigured calls:
phc.addHttpEndpoint(path, null, null, null, platformHttpConsumer.getPlatformHttpConsumer());createMockPlatformHttpConsumerAware() sets up the mock so that getPlatformHttpConsumer() returns a non-null PlatformHttpConsumer mock. The 5th argument is therefore not null — the isNull() matcher fails at runtime.
Fix: use any(PlatformHttpConsumer.class) (or a specific eq(mockPlatformHttpConsumer)) for the 5th argument:
| verify(phc).addHttpEndpoint(eq(""), isNull(), isNull(), isNull(), isNull()); | |
| verify(phc).addHttpEndpoint(eq(""), isNull(), isNull(), isNull(), any()); |
| openApiProcessor = null; | ||
|
|
||
| // and removed again when the processor stops | ||
| verify(phc).removeHttpEndpoint(eq(""), isNull()); |
There was a problem hiding this comment.
isNull() bug for removeHttpEndpoint. (Not fixed since SHA 6a77fa3.)
doStop() calls:
phc.removeHttpEndpoint(unmatchedRequestCatchAllPath, platformHttpConsumer.getPlatformHttpConsumer());platformHttpConsumer.getPlatformHttpConsumer() is the non-null mock — isNull() fails.
| verify(phc).removeHttpEndpoint(eq(""), isNull()); | |
| verify(phc).removeHttpEndpoint(eq(""), any()); |
| is used: when two or more beans of this type are found in the registry, none of them is used, instead the | ||
| handler from the factory finder is used instead (if present), otherwise the default handler is used. |
There was a problem hiding this comment.
📝 Nit: "instead" appears twice — still not fixed since it was raised by @davsclaus and re-raised by gnodet-bot on SHA 6a77fa3.
| is used: when two or more beans of this type are found in the registry, none of them is used, instead the | |
| handler from the factory finder is used instead (if present), otherwise the default handler is used. | |
| is used: when two or more beans of this type are found in the registry, the handler from the factory finder | |
| handler from the factory finder is used (if present), otherwise the default handler is used. |
davsclaus
left a comment
There was a problem hiding this comment.
Re-reviewed at a19e0c853654. Two of the three blockers are fixed and I verified both locally: the module compiles (the stray if is gone), the xref resolves, and src/main/docs is byte-identical to the catalog copy.
The third blocker is real — I ran the test rather than predicting this time.
mvn test -Dtest=RestOpenApiUnmatchedRequestHandlerTest (components/camel-rest-openapi)
Tests run: 9, Failures: 1
testCatchAllRegisteredOnPlatformHttpWhenCamelHandling
Argument(s) are different! Wanted:
platformHttpComponent.addHttpEndpoint("", isNull(), isNull(), isNull(), isNull());
Actual invocation:
platformHttpComponent.addHttpEndpoint("", null, null, null, Mock for PlatformHttpConsumer);
The production side is correct — the catch-all is registered at "" with null verbs, exactly as intended. It is the assertion that is wrong: createProcessor(...) installs createMockPlatformHttpConsumerAware(), so RestOpenApiProcessor:254 passes a non-null PlatformHttpConsumer as the fifth argument, and isNull() cannot match it. Same for verify(phc).removeHttpEndpoint(eq(""), isNull()) on the stop path — it will fail for the same reason as soon as the first verification stops failing first.
Match the stubbed consumer instead of isNull(); any(PlatformHttpConsumer.class) or notNull() both work, and keeping the other four matchers as they are still pins the catch-all shape (empty path, no verbs).
Worth knowing before this merges: GitHub reports no checks at all on CAMEL-24649-add-unmatched-request-handler — gh pr checks 26203 returns "no checks reported". So nothing has run this test in CI, and the green-looking PR page is green only because nothing ran. Please make sure a full run happens on the fixed head rather than merging on the current state.
Two things from my last review are still unaddressed, both small and in the docs:
- Line 274-275: "none of them is used, instead the handler from the factory finder is used instead" — doubled "instead".
- Line ~259: the note that a third-party
RestOpenApiConsumerFactorysimply ignoresunmatchedRequestHandlingis not quite right —RestOpenApiEndpointforwards the option into the delegate consumer's parameters, so a factory that validates its parameters would reject it. Low risk (platform-http is the only in-tree factory), but the wording should say so, or the option should not be forwarded.
And still open from earlier rounds, neither blocking, but a reply on each would let this close out: whether RestOpenApiUnmatchedRequestHandler should live in camel-api so camel-rest-postman can drop its byte-identical 404/405 block, and the nested-base-path / Spring Boot / Vert.x-validation questions from my last review.
Claude Code on behalf of davsclaus
|
Thanks, working on it will let you know when I think all issues are fixed :-) |
|
@davsclaus regarding your "worth a look (non-blocking)" list:
Added documentation for this limitation.
Tested this in a stand alone application. You are correct the catch-all, as implemented now, takes over static resources and other MVC mappings. Actuators survive since they are registered on a lower order.
No it is not. I will look into this.
Sorry missed this comment. Will move RestOpenApiUnmatchedRequestHandler to camel-api
Will update the JIRA description. And yes will fix the commit message by doing a squash before merge. |
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review of SHA fcbe419.
Previous findings status
| Finding | Status |
|---|---|
Compile error — stray if (phc != null) { in doStop() |
✅ Fixed |
Broken xref platform-http-vertx.adoc |
✅ Fixed |
isNull() on non-null consumer in testCatchAllRegisteredOnPlatformHttpWhenCamelHandling — addHttpEndpoint 5th arg |
✅ Fixed — now any(PlatformHttpConsumer.class) |
isNull() on non-null consumer in testCatchAllRegisteredOnPlatformHttpWhenCamelHandling — removeHttpEndpoint 2nd arg |
✅ Fixed — now any(PlatformHttpConsumer.class) |
Doubled "instead" in docs |
✅ Fixed — rephrased |
Open items
Two open items from earlier rounds, both raised by @davsclaus and acknowledged by @ullgren in comment thread:
1. Interface placement — in the comment thread @ullgren said "Will move RestOpenApiUnmatchedRequestHandler to camel-api", but this push still has it in org.apache.camel.component.rest.openapi. The interface lives in a component module, so third-party embedders who want to implement a custom handler must depend on camel-rest-openapi rather than the stable SPI jar. RestClientRequestValidator, the precedent cited by the JIRA, lives in core/camel-api under org.apache.camel.spi. Keeping the interface component-local also blocks camel-rest-postman reuse.
If you've decided to leave it here for now, that's fine — but please reply so we can close this thread out.
2. Spring Boot catch-all ordering — @ullgren confirmed in comments that the catch-all via phc.addHttpEndpoint does shadow Spring MVC static resources and other MVC mappings (actuators survive due to different order). There's no doc warning for this on the Spring Boot path. The nested-base-path [NOTE] block added in the latest commit is good for Vert.x, but Spring Boot users with static resources or MVC controllers under the API base path will see silent breakage with no warning to help them diagnose it. A short note in the == section covering the Spring Boot behavior would address this.
No blocking issues on the current commit — the compile error is gone, the test assertions are now correct, and the integration test suite (including testCatchAllDoesNotShadowApiWithNestedBasePath) gives reasonable coverage of the Vert.x path.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| * limitations under the License. | ||
| */ | ||
| package org.apache.camel.component.rest.openapi; | ||
|
|
There was a problem hiding this comment.
📌 Interface placement — you said in comments you'd move this to camel-api (mirroring RestClientRequestValidator under org.apache.camel.spi), but it's still here. This is the only remaining open item from @davsclaus's design feedback. If the decision is to keep it component-local for now, please say so and we'll close this thread.
There was a problem hiding this comment.
Yeah I will move it, future tense, as an LLM you should understand language ;-P
(OK will mark this PR as draft so I do not spend your tokens)
|
Updated https://issues.apache.org/jira/browse/CAMEL-24649 to also include the new unmatchedRequestHandling parameter |
|
Could not find a good and easy way to support static resources and other MVC mappings with Spring Boot. Documented as a limitation and suggestion of a work around. The default Also added support for handling content negotiation failures when |
Description
Adding a unmatched request handler allowing for library users to customize the response that REST OpenAPI component sends back when the request does not match any operation in the OAS.
Target
mainbranch)Tracking
Apache Camel coding standards and style
I checked that each commit in the pull request has a meaningful subject line and body.
I have run
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.AI-assisted contributions
Co-authored-bytrailers) and the PR description identifies the AI tool used.