Repository navigation
CAMEL-25626: camel-milo - deliver a data change with a Bad status even when its value is null - #27908
Conversation
…n when its value is null With the default omitNullValues=true, the milo-client consumer dropped every data change whose value was null, regardless of its status code. An OPC UA server reports a Bad status with a null value, for example a node that does not exist or a monitored item that could not be created, so the route never learned that a node went bad unless omitNullValues=false was set. A data change whose status is Bad or Uncertain is now delivered even when its value is null. The exchange body is the DataValue as before, so the route reads the status code from it and the CamelMiloNodeId header names the node. omitNullValues still omits a data change whose value is null and whose status is Good, for example the initial value at startup. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Thanks for this fix — the logic is correct and the PR is well-structured. One minor observation below about nullability contracts in the new helper method.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…mitNullValues check DataValue.getValue() and getStatusCode() are non-null in Milo 1.x; only Variant.getValue() is nullable, so the extra null checks were dead code. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Previous finding addressed. The dead null checks on getValue() and getStatusCode() have been removed (891bf76) — the author verified against Milo 1.1.8 that these are @NullMarked non-null. Removing them is the right call.
One minor note below on the remaining value == null guard, but it doesn't block merge — the logic is correct and the behavioral fix is well-tested and documented.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
🌟 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.
Claude Code on behalf of davsclaus
Thanks Serdar, this is a good change to make. Quality propagation matters for OT routes, the predicate is small, and the new test covers the Good, Uncertain and Bad cases. I found one issue that I think needs fixing before merge. The PR's premise is that the startup value is a null with a Good status, but that is not always true. I traced the code below; I could not run it because CI has not run on this PR yet.
The startup value can be Bad, and it is now delivered
-
CamelServerItemstarts every item asnew DataValue(StatusCode.BAD)(components/camel-milo/src/main/java/org/apache/camel/component/milo/server/internal/CamelServerItem.java, line 54). Milo samples a monitored item through a read that carries the session (SubscriptionModelbuildsnew ReadContext(server, session)), so the attribute filter returnsgetDataValue(). A producer write only updates that field throughupdate(), which is how the client seesFoo. It follows that amilo-clientconsumer that subscribes to amilo-serveritem before the first write receives a Bad status with a null value as its first notification. Before this PR, the defaultomitNullValues=truedropped that notification; with this PR it is delivered. Real servers can do the same in the initial notification, for example withBad_WaitingForInitialData. -
Some existing tests start their consumer before the first write and expect the first message to be the first written value. They appear to rely on that drop:
MonitorItemLifecycleTest#restartedConsumerReceivesEachValueOnceand#unknownNodeReceivesBadStatus: routeitem1auto-starts, then the test expectsexpectedMessageCount(1)withmessage(0)=Foo.MonitorItemTest#testMonitorItem1:message(0)=Fooon all four consumers.MonitorItemMultiConnectionsTest#testMonitorItem1:test1expects exactly 1 message.
I would expect these to fail once CI runs. If they passed in your local
-Psourcecheck verifyrun, please tell me what I am missing in the trace. -
Delivering that startup Bad status is arguably right, given the rationale of the PR. Either way, please update three things:
- The text. The endpoint javadoc (and with it the generated JSON, catalog and endpoint DSL), the component doc and the 4.24 upgrade-guide note all say the startup value is a null with a Good status and is still omitted. For the Camel server, and for servers that report
Bad_WaitingForInitialData, it is not. The upgrade note should tell users that routes may now receive a Bad status at startup and can filter on the status code of theDataValueif they do not want it. - The tests above. Write the value before the route starts (
autoStartup(false), asMonitorNodesTestalready does), or assert the startup Bad status explicitly. - A test for this case, so the startup behaviour is pinned: a consumer that subscribes to a
milo-serveritem before anything was written.
- The text. The endpoint javadoc (and with it the generated JSON, catalog and endpoint DSL), the component doc and the 4.24 upgrade-guide note all say the startup value is a null with a Good status and is still omitted. For the Camel server, and for servers that report
The generated files: you noted they were edited by hand. The CI regen check will show whether they match the generator output.
This review does not replace specialized AI review tools or static analysis such as SonarCloud.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 8 of 705 tested, 24 compile-only — current: 8 all testedMaveniverse Scalpel detected 8 affected modules (current approach: 8). Skip-tests mode would test 8 modules (4 direct + 7 downstream), skip tests for 24 (generated code, meta-modules) Modules Scalpel would test (8)
Modules with tests skipped (24)
All tested modules (35 modules, 5m 22s total)Total reactor time: 5m 22s
Top 20 slowest modules:
|
The first notification of a monitored item carries the current value of the node. That can be a Bad status with a null value: a milo-server item that was not written yet, or Bad_WaitingForInitialData from a server that has no value yet. The consumer delivers it like any other Bad status, so the option and consumer javadoc no longer name the startup value as an omitted value, and the component doc and the 4.24 upgrade note say that a route may receive such a status and how to skip it. The tests that expected the first written value as their first message now write it before they start their consumers. In MonitorItemTest the client that samples once per second now always receives that value, so its bounds are one higher. A new test pins the Bad status of an item that was not written yet. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks, confirmed: a
Why they passed: the trace is right, but the tests did not fail reliably. CI on 891bf76, which finished after your review, was green on JDK 17 and 25 with no recovered flaky tests. Real servers with this branch: node-opcua 2.187.9 sends The endpoint DSL javadoc is still edited by hand (its generator needs the full reactor), wrapped like the generator output; the regen check passed on 891bf76, and if it differs this time, I will apply its patch. Claude Code on behalf of etzel-mes |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
All findings from the previous review round have been addressed in the latest commits. The fix is correct: isNullValueWithGoodStatus now properly gates null-value delivery on status quality, allowing DataValues with non-Good status codes (e.g. Uncertain) to pass through even when their inner value is null — which is the intended OPC-UA semantics. The NPE concern is a non-issue per Milo's @NotNull contract on DataValue.getValue(). The value == null arm at the top of the guard is acceptable defensive coding. Updated test bounds (+1 min/max) correctly account for the autoStartup(false) + startRoute() sequence that ensures the consumer sees "Foo" as the first notification.
No milestone set — consider targeting the next Camel release milestone.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Claude Code on behalf of davsclaus
Thanks @etzel-mes, all points from my previous review are addressed:
- The option Javadoc (and with it the JSON, catalog and endpoint DSL), the consumer Javadoc and the docs no longer describe the startup value as a Good null.
- The upgrade guide now tells users that a Bad status may arrive at startup and after a reconnect, and how to filter it.
- The tests write their value before the consumers start, and a new test pins the startup Bad status.
Heads-up for merge order: this PR and #27910 conflict in MonitorItemLifecycleTest. With item1 now autoStartup(false), the recreate tests in #27910 will need to start item1 after the first write, whichever of the two merges second.
CI has not run on this head yet (the workflows are waiting for approval), so the regen check on the hand-edited endpoint DSL still has to confirm it; this approval assumes it goes green.
This review does not replace specialized AI review tools or static analysis such as SonarCloud.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Description
CAMEL-25626.
With the default
omitNullValues=true, themilo-clientconsumer dropped every data change whose value was null, regardless of its status code. OPC UA Part 4 requires a server to send a null value together with a Bad status, so dropping null values drops every Bad status: a sensor failure, a removed node or a monitored item that could not be created (#27780, #27906) never reached the route unlessomitNullValues=falsewas set.Why this matters in automation: a route that feeds a historian, an MES, a UNS topic or an alarm evaluates the last value it received. When the quality of a tag goes Bad and the route never hears about it, the stale value keeps being treated as current, and the plant acts on a number that nobody is measuring any more. Quality propagation is the first thing an OT integration is expected to get right; the component should deliver it by default.
The change:
omitNullValuesnow omits only a data change whose value is null and whose status is Good. A data change with a Bad or Uncertain status is always delivered; the body is theDataValueas before, the route reads the status code from it andCamelMiloNodeIdnames the node.milo-serveritem that was not written yet, orBad_WaitingForInitialDatafrom a server that has no value yet. CAMEL-18144 addedomitNullValuesto skip that notification and to keep the unit tests stable. It is now delivered because it carries the quality of the node; the tests write the value before they start their consumers instead.omitNullValues=false.MiloClientConsumer.MonitorNodesTest#notGoodStatusWithNullValueIsDeliveredByDefault(default option): a Good null is omitted, an Uncertain null and a Bad status are delivered, a later Good write still arrives; it fails without the fix (2 expected, 0 received).MonitorItemLifecycleTest#itemThatWasNotWrittenYetReportsABadStatusAtStartuppins the Bad status at startup.MonitorItemLifecycleTest,MonitorItemTestandMonitorItemMultiConnectionsTeststart their consumers after the first write, andomitNullValues=falsewas removed from the existing tests.Note on generated files: the endpoint DSL javadoc was edited by hand, wrapped as the generator does, because its generator needs the full reactor; the catalog files are copies of the module files. If the regen check differs, I will apply the patch it posts.
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.Claude Code on behalf of etzel-mes
🤖 Generated with Claude Code