Repository navigation
CAMEL-26179: camel-milo - a milo-client write of a null body fails the exchange - #28514
Conversation
…e exchange A milo-client producer wrote a null body as a null value, which the server refused with Bad_TypeMismatch; an awaited write completed without an error and with that status as the body. The producer now fails the exchange with an InvalidPayloadException before writing, awaited or not. A DataValue or Variant body still writes a null value explicitly. A method call with a null body is unchanged: it calls the method without input arguments. A write of a body that the client cannot encode, such as a Map, no longer disposes the shared connection since CAMEL-25912; a regression test now covers it: the exchange fails, and the consumer of another node on the same connection reports no loss and keeps its session. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Good fix — failing fast with a clear InvalidPayloadException beats the old behaviour where a null-body write went through, the server refused it with Bad_TypeMismatch, and an awaited write "succeeded" with that status as the body (a confusing silent-ish failure).
The guard is correctly scoped: value == null && !read && this.methodId == null fails only a genuine write — a read (via CamelMiloNodeIds) needs no body, and a method call with a null body still calls the method with no input args (nicely called out in the upgrade guide). Hoisting the containsKey(HEADER_NODE_IDS) check into the read local also removes the duplicate test below it. InvalidPayloadException is the right type — users can still catch it via onException.
Tests cover it well: testWriteNullBodyFails is parameterized over awaited/non-awaited, and writeOfABodyThatCannotBeEncodedKeepsTheConnection additionally pins down that an un-encodable body (a Map → UaSerializationException) fails the exchange without tearing down the shared connection, and a later valid write on the same session still succeeds. JUnit assertions + MockEndpoint.assertIsSatisfied + @Timeout, consistent with the module. Docs and the upgrade-guide entry (with the DataValue/Variant escape hatch) are in place.
No blockers. CI hasn't run here (fork PR) — worth triggering before merge.
Claude Code on behalf of oscerd
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks for this. I checked that the new guard in MiloClientProducer.java:77 fails only a real write: reads via CamelMiloNodeIds and method calls are unchanged. A DataValue or Variant body still goes through MiloClientConnection.mapWriteValue untouched, so the documented way to write a null value works. The existing MiloClientProducerWriteFailureTest uses a non-null body and is unaffected. The catalog copy of the doc matches, and the upgrade-guide entry covers the change for both awaited and fire-and-forget writes. The two tests pin down the null-body failure and that a body the client cannot encode no longer rebuilds the shared connection.
Two small, non-blocking notes:
- The description says "Fixes CAMEL-26179", but item 3 of that issue (milo-server) is not in this PR. We should keep the JIRA open for it, or move item 3 to its own issue, so merging this does not close it by accident.
- Nit: in
milo-client-component.adoc:174(and the upgrade guide), it could help to nameVariant.NULL_VALUEas the concrete way to write a null value.
LGTM once CI has run green.
Reviewed with Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔁 1 test passed only after a retry on JDK 21 (1 retried attempt) Recovered flaky tests on JDK 21 (1)
🔬 Scalpel shadow comparison — Scalpel: 8 of 708 tested, 25 compile-only — current: 8 all testedMaveniverse Scalpel detected 8 affected modules (current approach: 8). Skip-tests mode would test 8 modules (3 direct + 7 downstream), skip tests for 25 (generated code, meta-modules) Modules Scalpel would test (8)
Modules with tests skipped (25)
All tested modules (35 modules, 5m 59s total)Total reactor time: 5m 59s
Top 20 slowest modules:
|
|
@etzel-mes CI is green now, thanks. Could you look at the two notes from my review before we merge?
Claude Code on behalf of davsclaus |
… a null value Review note from davsclaus: the component doc, the 4.24 upgrade guide and the InvalidPayloadException hint now name Variant.NULL_VALUE instead of a generic DataValue or Variant body. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks @davsclaus, both notes are addressed:
The new commit only changes these wording lines. I ran the module with Claude Code on behalf of etzel-mes |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
LGTM. Guard logic is correctly scoped to the write path only (value == null && !read && this.methodId == null) — reads via CamelMiloNodeIds don't use the body, method calls with null still invoke the method with no arguments, and DataValue/Variant bodies carrying a null value pass through unchanged (the value variable is not null in those cases). The 4-arg InvalidPayloadException constructor produces a clear, actionable message.
Pre-fix test validation (static trace): testWriteNullBodyFails asserts InvalidPayloadException — without the guard, the write reaches the server which rejects it with Bad_TypeMismatch, a different exception type, so the test is correctly red before the fix. writeOfABodyThatCannotBeEncodedKeepsTheConnection pins the CAMEL-25912 regression — without that fix the shared connection is rebuilt, the consumer receives an error event, and the one-message assertion fails.
Docs and upgrade guide are complete: Variant.NULL_VALUE / DataValue escape hatch documented in both the component page (and its catalog copy) and the 4.24 upgrade guide.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
🔄 Backport BotThis bugfix targets
Add a Backport PRs will be created automatically on merge (once a port label is added). Comment ℹ️ If you push additional commits after |
Description
Items 1 and 2 of CAMEL-26179; item 3 (milo-server) is left to the reporter.
ProducerOnSharedConnectionTest.writeOfABodyThatCannotBeEncodedKeepsTheConnection: an awaited write of aMapbody fails withUaSerializationException, the consumer of another node on the same connection receives only its next value (noBad_NoCommunication, no resent value over 2 s), and a following write on the same client and session answersGOOD. It fails whenoutcomeOfcallshandleConnectionFailureagain as before CAMEL-25912.milo-clientwrite of anullbody now fails the exchange with anInvalidPayloadExceptionbefore anything is sent, awaited or not. Before, it wrote a null value, which the server refused withBad_TypeMismatch, and an awaited write completed without an error and with that status as the body. ADataValueorVariantbody still writes a null value explicitly (the exception hint says so). A method call with anullbody is unchanged: it calls the method without input arguments, as documented. Test:WriteClientTest.testWriteNullBodyFails(awaited and not awaited), which fails on the unmodified code.Docs: one paragraph in the milo-client component doc (and its catalog copy) and a section in the 4.24 upgrade guide.
Module
-Psourcecheck verifywith-Dsurefire.rerunFailingTestsCount=0: 128 tests, 0 failures, 1 skipped (as upstream); the two changed classes green in 3 further runs.Part of CAMEL-26179: the issue stays open for item 3 (milo-server).
@davsclaus @oscerd
Target
mainbranch)Tracking
Apache Camel coding standards and style
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.Claude Code on behalf of etzel-mes