CAMEL-24360: camel-undertow - use UndertowHeaderFilterStrategy as the endpoint default - #25367
Conversation
… endpoint default
DefaultUndertowHttpBinding installs an UndertowHeaderFilterStrategy in its
constructor, but UndertowEndpoint defaulted its own headerFilterStrategy to the
base HttpHeaderFilterStrategy and pushed that into the binding when lazily
creating it, overwriting the undertow-specific strategy. Unless the user
supplied a custom binding or a custom headerFilterStrategy, the binding ran the
base strategy and the undertow-specific filtering never executed.
Two behaviours that are documented as shipped were therefore inert on
endpoint-configured routes:
* the legacy websocket.* Exchange-header prefix added to the in and out
filters by CAMEL-23588 (4.14.8 / 4.18.3 / 4.21.0);
* the io.undertow.util.HttpString.tryFromString header-name validation in
UndertowHeaderFilterStrategy.applyFilterToExternalHeaders, which skips
header names undertow does not accept.
The endpoint now defaults to UndertowHeaderFilterStrategy, so both take effect.
Rest DSL consumers are unchanged: UndertowComponent already assigns
UndertowRestHeaderFilterStrategy, which extends UndertowHeaderFilterStrategy.
Endpoints that configure headerFilterStrategy or undertowHttpBinding explicitly
keep their existing behaviour.
Adds three UndertowEndpointTest cases covering the endpoint default, the
strategy the lazily created binding ends up running together with the websocket
prefixes actually being filtered, and an explicitly configured strategy still
reaching the binding. Adds a 4.22 upgrade-guide entry describing the change and
how to restore the previous behaviour.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 39 tested, 28 compile-only — current: 38 all testedMaveniverse Scalpel detected 67 affected modules (current approach: 38).
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
LGTM — genuine bugfix, minimal change, well-tested.
The root cause is confirmed: UndertowEndpoint line 88 defaulted to HttpHeaderFilterStrategy, and getUndertowHttpBinding() pushed that into the lazily created binding, overwriting the UndertowHeaderFilterStrategy installed by the binding's own constructor. The websocket.* prefix filtering from CAMEL-23588 was therefore never active on endpoint-configured routes.
One minor convention note (non-blocking):
- The new test methods use JUnit assertions (
assertInstanceOf,assertSame,assertTrue). Per project conventions, new test code should prefer AssertJ (assertThat(...)) for consistency.
Everything else checks out:
- Fix is correct and minimal (one-line default change)
- Tests cover three scenarios: default strategy, lazy binding propagation, explicit custom strategy
- Rest DSL consumers are unaffected (
UndertowRestHeaderFilterStrategyalready extendsUndertowHeaderFilterStrategy) - Upgrade guide documents the change and how to restore previous behaviour
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of davsclaus
| import org.apache.camel.http.base.HttpHeaderFilterStrategy; | ||
| import org.apache.camel.spi.HeaderFilterStrategy; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; |
There was a problem hiding this comment.
Non-blocking: per project conventions, new test code should prefer AssertJ assertions (assertThat(...)) over JUnit assertions (assertInstanceOf, assertSame, assertTrue).
…ases Addresses review feedback on apache#25367. The project convention is to prefer AssertJ over JUnit assertions in new test code. assertj-core was not on the camel-undertow test classpath, so it is declared here as a test dependency; the version comes from the parent dependencyManagement, matching how other modules declare it. Only the three new test methods are converted. The two pre-existing methods keep their JUnit assertions, per the convention that touched code migrates without sweeping the whole file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
|
Thanks for the review. Addressed in ff954f9 — the three new test methods now use AssertJ ( Two notes: 1. AssertJ was not on this module's classpath. The suggestion assumed it was already usable here, but <dependency>
<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>
<scope>test</scope>
</dependency>Flagging it because the project standards ask that new dependencies be justified. Happy to revert to JUnit assertions if you would rather not add it to this component just for assertion style. 2. Only the three new methods are converted. Module build is green: 191 tests pass, 0 failures. Full reactor build from root is clean with no regenerated-artifact drift. Also worth noting for the record: the earlier CI red on this PR was not a regression. Claude Code on behalf of oscerd |
…derFilterStrategy as the endpoint default (#25379) CAMEL-24360: camel-undertow - use UndertowHeaderFilterStrategy as the endpoint default UndertowEndpoint defaulted its headerFilterStrategy to the base HttpHeaderFilterStrategy and pushed it into the lazily created binding, overwriting the UndertowHeaderFilterStrategy the binding's constructor had installed. This made the undertow-specific filtering (websocket.* prefix filtering from CAMEL-23588, HttpString header-name validation) inert on endpoint-configured routes. The endpoint now defaults to UndertowHeaderFilterStrategy. Rest DSL consumers are unchanged (UndertowComponent already assigns UndertowRestHeaderFilterStrategy). Endpoints that configure headerFilterStrategy or undertowHttpBinding explicitly keep their existing behaviour. Adds UndertowEndpointTest cases and a 4.22 upgrade-guide entry. Closes #25367 Co-authored-by: Andrea Cosentino <ancosen@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…derFilterStrategy as the endpoint default (#25381) CAMEL-24360: camel-undertow - use UndertowHeaderFilterStrategy as the endpoint default UndertowEndpoint defaulted its headerFilterStrategy to the base HttpHeaderFilterStrategy and pushed it into the lazily created binding, overwriting the UndertowHeaderFilterStrategy the binding's constructor had installed. This made the undertow-specific filtering (websocket.* prefix filtering from CAMEL-23588, HttpString header-name validation) inert on endpoint-configured routes. The endpoint now defaults to UndertowHeaderFilterStrategy. Rest DSL consumers are unchanged (UndertowComponent already assigns UndertowRestHeaderFilterStrategy). Endpoints that configure headerFilterStrategy or undertowHttpBinding explicitly keep their existing behaviour. Adds UndertowEndpointTest cases and a 4.22 upgrade-guide entry. Closes #25367 Co-authored-by: Andrea Cosentino <ancosen@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Description
DefaultUndertowHttpBindinginstalls anUndertowHeaderFilterStrategyin its constructor:but
UndertowEndpointdefaulted its own field to the baseHttpHeaderFilterStrategyand pushed that into the binding when creating it lazily, overwriting what the binding had just set:Unless the user supplied a custom binding or a custom
headerFilterStrategy, the binding ran the base strategy and the undertow-specific filtering never executed.Impact
Two behaviours documented as shipped were inert on endpoint-configured routes:
websocket.*Exchange-header prefix added to the in/out filters by CAMEL-23588 (released in 4.14.8 / 4.18.3 / 4.21.0 and described in the 4.18 upgrade guide);io.undertow.util.HttpString.tryFromStringheader-name validation inUndertowHeaderFilterStrategy.applyFilterToExternalHeaders, which skips header names undertow does not accept.Documentation and runtime behaviour had therefore diverged since CAMEL-23588.
Fix
UndertowEndpointnow defaults toUndertowHeaderFilterStrategy, so both take effect.UndertowComponentalready assignsUndertowRestHeaderFilterStrategy, which extendsUndertowHeaderFilterStrategy.headerFilterStrategyorundertowHttpBindingexplicitly keep their behaviour.The now-unused
HttpHeaderFilterStrategyimport is removed. No generated metadata changes, since the catalog records only theHeaderFilterStrategyinterface and not the default implementation class.Changes
UndertowEndpoint- default strategy, unused importUndertowEndpointTest- three cases: the endpoint default is undertow-specific; the lazily created binding keeps it and actually filtersUndertowConstants.CONNECTION_KEY/CONNECTION_KEY_LIST/SEND_TO_ALL; an explicitly configured strategy still reaches the bindingTesting
camel-undertowmodule build green: 191 tests pass, 0 failures, 1 skipped. The websocket suites (UndertowWsConsumerRouteTest,UndertowWsTwoRoutesToSameEndpointSendToAllHeaderTest, ...) all still pass, since the producer reads those headers viain.getHeader(...), which does not go through theHeaderFilterStrategy.mvn clean install -DskipTests) green, no regenerated-artifact drift.Backport
UndertowEndpointline 88 andDefaultUndertowHttpBindingline 86 are identical onmain,camel-4.18.xandcamel-4.14.x, so this should be backported to both LTS lines (with the matching 4.18 / 4.14 upgrade-guide entries doc-synced back tomainper the backport policy). Backporting matters here because CAMEL-23588 shipped on those branches and its documented effect is what this restores.Review feedback (ff954f9)
The three new
UndertowEndpointTestmethods now use AssertJ (assertThat(...)) instead of JUnit assertions, per project convention. The two pre-existing methods keep their JUnit assertions, per the convention that touched code migrates without sweeping the whole file.assertj-corewas not on thecamel-undertowtest classpath, so it is also declared as a test dependency (version-less, supplied by the parentdependencyManagement). Flagged in the PR thread since the project standards ask that new dependencies be justified.Note on the earlier CI failure
The earlier red on this PR was not a regression.
build (17)failed incamel-cxf-spring-soapwithBindException: Address already in use/Soap 1.1 endpoint already registered, andbuild (25)was cancelled by matrix fail-fast rather than failing. That module has noundertow:endpoint usage in its tests, soUndertowEndpointis not on its code path (it publishes through CXF's owncxf-rt-transports-http-undertow). A re-run passed both jobs.Claude Code on behalf of oscerd