Skip to content

CAMEL-24649: Add unmatched request handler to REST OpenAPI component - #26203

Open
ullgren wants to merge 17 commits into
apache:mainfrom
ullgren:CAMEL-24649-add-unmatched-request-handler
Open

ullgren wants to merge 17 commits into
apache:mainfrom
ullgren:CAMEL-24649-add-unmatched-request-handler

Conversation

@ullgren

@ullgren ullgren commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

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

  • I checked that the commit is targeting the correct branch (Camel 4 uses the main branch)

Tracking

  • If this is a large change, bug fix, or code improvement, I checked there is a JIRA issue filed for the change (usually before you start working on it).

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 -DskipTests locally from root folder and I have committed all auto-generated changes.

AI-assisted contributions

  • If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., Co-authored-by trailers) and the PR description identifies the AI tool used.

@ullgren
ullgren marked this pull request as draft September 8, 2026 11:23

@davsclaus davsclaus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread components/camel-rest-openapi/src/main/docs/rest-openapi-component.adoc Outdated
@ullgren

ullgren commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

@davsclaus Thanks for the review, I've done the proposed changes.
However it turns out that in later version, we where running a older version, camel-platform-http-vertex now use fine grained vertex routes (implemented in CAMEL-22971.
So now the platform-http layer intercepts and returns a standard error and the logic in Camel (and by this handler) is never executed.

A solution to this would be to introduce a new boolean option to the rest-openapi coomponent. Something like fine-graind-binding which would be true by default (keeping the intential change introduced in CAMEL-22971 .

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 ?

@Croway

Croway commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

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 RestOpenApiUnmatchedRequestHandler, then sent requests that match no operation:

POST /api/v3/no-such-op PATCH /api/v3/pet/123 handler invocations
camel-platform-http-vertx (Camel Main) 404, Vert.x's HTML page 405, empty 0
camel-platform-http-starter (Spring Boot) 404, Spring's {"timestamp":…,"error":"Not Found"} 405, Spring error JSON 0

The three runtimes have only two consumer implementations: camel-quarkus reuses camel-platform-http-vertx verbatim (PlatformHttpRecorder instantiates VertxPlatformHttpEngine over a Quarkus-managed VertxPlatformHttpRouter), so it shares the vertx code path above. I have not measured Quarkus — that row is inferred from the shared code, not tested.

Both implementations register only the exact OAS surface, so the HTTP layer rejects anything else before Camel is involved:

  • Vert.x: VertxPlatformHttpConsumer.startRestServicesContractFirst() creates one route per (path, method) and returns early, so no base-path catch-all exists (since 4.18.0, CAMEL-22971).
  • Spring Boot: DefaultRestOpenapiProcessorStrategy.validateOpenApi() calls addHttpEndpoint(uri, verbs, …) per OAS path, which CamelRequestHandlerMapping turns into RequestMappingInfos restricted to those verbs — an unknown path/verb becomes Spring MVC's 404/405.

So RestOpenApiProcessor:137 unmatchedRequestHandler.handle(...) is currently dead code. The PR's tests don't catch this because they drive the processor directly against a mocked PlatformHttpComponent.

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:

  • Spring Boot — one extra registration in DefaultRestOpenapiProcessorStrategy.validateOpenApi(): phc.addHttpEndpoint(basePath, null, null, null, consumer). The endpoint already has matchOnUriPrefix=true, so CamelRequestHandlerMapping rewrites it to /api/v3/{*matchOnUriPrefix}, and Spring's pattern-specificity ordering keeps the per-operation mappings winning. (Registering basePath + "/**" instead does not work — the rewrite appends /{*matchOnUriPrefix} and produces an invalid PathPattern, silently.)
  • Vert.x (Camel Main, and Quarkus by the same code) — drop the early return in VertxPlatformHttpConsumer.doStart():163 and fall through to the existing registration at line 169, which already builds router.route(path) (path ends in *) and applies configureSecurityHandler. Vert.x matches in registration order, so the contract-first routes still win.

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 Allow is more accurate than Spring's: GET, POST, DELETE for /pet/{petId}, where Spring answered PUT, GET, DELETE, POST (PUT is only declared on /pet and leaked in via the prefix pattern).

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 platform preserves today's behaviour, so no upgrade-guide entry is needed. I'd avoid auto-enabling it just because a handler bean is present — that would silently change 404 bodies on upgrade.

Two smaller notes on the current diff:

  1. The docs' "empty body" claim (rest-openapi-component.adoc:205, repeated at 211). DefaultRestOpenApiUnmatchedRequestHandler.handle() sets the status code and Allow but never clears the body, and at that point exchange.getMessage() is still the inbound request. Once the branch becomes reachable this is worth re-checking with a request that has a payload — an exchange.getMessage().setBody(null) in the default handler is probably wanted. The existing test passes only because it never sets a request body.
  2. Multiple registered handlers fall back silently. CamelContextHelper.findSingleByType returns null unless exactly one bean matches, so two registered handlers are ignored with no WARN. Also, the doc at 217-218 ("if two or more beans of this type are found in the registry, the default handler is used") isn't accurate when a META-INF/services/.../rest-openapi-unmatched-request-handler-factory file is also on the classpath — the factory-finder handler wins there, not the default.

Happy to share the probe tests if useful.

Claude Code on behalf of @Croway

@ullgren

ullgren commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

@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 platform vs camel value instead of a true/false flag is more clear.

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.

@Croway

Croway commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

I'd keep the changes in this PR, they are related to https://issues.apache.org/jira/browse/CAMEL-24649

@ullgren
ullgren force-pushed the CAMEL-24649-add-unmatched-request-handler branch from bc6ed7a to 34001d0 Compare September 16, 2026 11:39
@ullgren

ullgren commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

Had to take help from AI tooling to find all places to update for the new endpoint option, updated commit and issue accordingly.
I used GLM-5.3-Flash (hosted at berget.ai) to help analyze and generate code. All changes has been reviewed by me before committing.

@ullgren
ullgren marked this pull request as ready for review September 21, 2026 12:02
@davsclaus
davsclaus force-pushed the CAMEL-24649-add-unmatched-request-handler branch from 5ab3ef6 to 76210fd Compare September 22, 2026 07:13
@github-actions

Copy link
Copy Markdown
Contributor

🌟 Thank you for your contribution to the Apache Camel project! 🌟
🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

  • First-time contributors require MANUAL approval for the GitHub Actions to run
  • You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot although they are normally detected and executed by CI.
  • You can label PRs using skip-tests and test-dependents to fine-tune the checks executed by this PR.
  • Build and test logs are available in the summary page. Only Apache Camel committers have access to the summary.

⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

@gnodet-bot gnodet-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 gnodet-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +322 to +323
if (phc != null) {
if (phc != null && platformHttpConsumer != null) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Compile error introduced: The old 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:

Suggested change
if (phc != null) {
if (phc != null && platformHttpConsumer != null) {
if (phc != null && platformHttpConsumer != null) {

@oscerd oscerd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 davsclaus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. It doesn't compile: there's a stray if in RestOpenApiProcessor (inline).
  2. There's a broken xref in the docs, which fails PR doc validation (inline). The catalog copy of the doc needs regenerating too.
  3. testCatchAllRegisteredOnPlatformHttpWhenCamelHandling will likely fail once it compiles (inline).

Worth a look (non-blocking):

  • With two APIs in camel mode and nested base paths (/api and /api/v3), every catch-all is .last(), so whichever starts first answers unmatched requests for both. For example, PUT /api/v3/pet/123 can get a 404 from the /api processor instead of a 405 from /api/v3. testCatchAllDoesNotShadowApiWithNestedBasePath can't catch this, because both processors share the handler.
  • Spring Boot: was the catch-all via phc.addHttpEndpoint tested 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 the camel catch-all may accept requests that should get a 415/406. Is that intended?
  • From my earlier review, still open: should RestOpenApiUnmatchedRequestHandler move 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 unmatchedRequestHandling option. Several commit messages lack the CAMEL-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.

Comment thread components/camel-rest-openapi/src/main/docs/rest-openapi-component.adoc Outdated

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed wording reflect that other consumer components will decide what to do with the parameter.

Comment thread components/camel-rest-openapi/src/main/docs/rest-openapi-component.adoc Outdated

@gnodet-bot gnodet-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread components/camel-rest-openapi/src/main/docs/rest-openapi-component.adoc Outdated
Comment thread components/camel-rest-openapi/src/main/docs/rest-openapi-component.adoc Outdated
ullgren and others added 9 commits September 24, 2026 08:45
- 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>
Co-authored-by: Guillaume Nodet - AI Bot <gnodet+bot@gmail.com>
@ullgren
ullgren force-pushed the CAMEL-24649-add-unmatched-request-handler branch from 6a77fa3 to a19e0c8 Compare September 24, 2026 07:45

@gnodet-bot gnodet-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Test will fail — 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:

Suggested change
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());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Same 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.

Suggested change
verify(phc).removeHttpEndpoint(eq(""), isNull());
verify(phc).removeHttpEndpoint(eq(""), any());

Comment on lines +274 to +275
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Nit: "instead" appears twice — still not fixed since it was raised by @davsclaus and re-raised by gnodet-bot on SHA 6a77fa3.

Suggested change
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 davsclaus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 RestOpenApiConsumerFactory simply ignores unmatchedRequestHandling is not quite right — RestOpenApiEndpoint forwards 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

@ullgren

ullgren commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, working on it will let you know when I think all issues are fixed :-)

@ullgren

ullgren commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

@davsclaus regarding your "worth a look (non-blocking)" list:

  • With two APIs in camel mode and nested base paths (/api and /api/v3), every catch-all is .last(), so whichever starts first answers unmatched requests for both. For example, PUT /api/v3/pet/123 can get a 404 from the /api processor instead of a 405 from /api/v3. testCatchAllDoesNotShadowApiWithNestedBasePath can't catch this, because both processors share the handler.

Added documentation for this limitation.

  • Spring Boot: was the catch-all via phc.addHttpEndpoint tested against camel-spring-boot? And with base path /, does the root catch-all (/{*matchOnUriPrefix}) take over static resources or other MVC mappings?

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.
I will look into if there is something we can do for this or if there is a workaround we can add to the documentation,

  • Vert.x with serverRequestValidation=true: when content types don't match, Vert.x keeps looking at later routes, so the camel catch-all may accept requests that should get a 415/406. Is that intended?

No it is not. I will look into this.

  • From my earlier review, still open: should RestOpenApiUnmatchedRequestHandler move to camel-api so camel-rest-postman can reuse it? Fine to leave it component-local if you prefer, but a reply would help.

Sorry missed this comment. Will move RestOpenApiUnmatchedRequestHandler to camel-api

  • The JIRA description should be updated for the new unmatchedRequestHandling option. Several commit messages lack the CAMEL-24649: prefix, but a squash merge fixes that.

Will update the JIRA description. And yes will fix the commit message by doing a squash before merge.

@gnodet-bot gnodet-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📌 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@ullgren
ullgren marked this pull request as draft September 24, 2026 17:09
@ullgren

ullgren commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

Updated https://issues.apache.org/jira/browse/CAMEL-24649 to also include the new unmatchedRequestHandling parameter

@ullgren

ullgren commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

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 platform mode works as before.

Also added support for handling content negotiation failures when serverRequestValidation=true in the unmatched handler.

@ullgren
ullgren marked this pull request as ready for review September 24, 2026 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants