Repository navigation
CAMEL-26376: camel-mtconnect - new component to read machine tools through their MTConnect agent - #28631
Conversation
…rough their MTConnect agent MTConnect is the open standard machine tool vendors ship for reading their machines: a read-only HTTP agent that answers probe, current, sample and assets requests with XML documents. Camel had nothing for it. The consumer runs the protocol against one agent, probe and current and then the streaming sample request, or one sample request per poll with streaming=false, and delivers one message per observation with the value as the body and the MTConnect metadata as CamelMTConnect* headers. It keeps the sequence cursor across outages, resynchronises from current on OUT_OF_RANGE, re-reads the device model when the agent restarts, reports an unreachable agent once per data item with quality Bad, and with assets=true delivers the agent's assets and their changes. The producer sends probe, current, sample and assets requests on demand. TLS goes through sslContextParameters, document answers are read with gzip. The component uses the JDK HTTP client and StAX and has no third-party dependency. 104 tests run against an in-process stub agent that speaks the cppagent 2.8 and the Mazak 1.4 and 1.6 dialects. The component list, catalog copies, docs navigation and the component and endpoint DSL factories are regenerated. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
oscerd
left a comment
There was a problem hiding this comment.
Thanks for this — a well-structured new component. It's large (≈11.7k lines, 84 files), so to be upfront about scope: I focused this pass on the security-relevant surfaces and the component structure, not every line of the parsing/consumer logic. A new component of this size deserves a thorough committer review on top of this, and — see the end — it needs CI.
Security surfaces (these are what matter most for a consumer that reads XML from an external agent, since that agent's data is untrusted input):
- XXE is handled in production.
DocumentParserbuilds itsXMLInputFactorywithSUPPORT_DTD=falseandIS_SUPPORTING_EXTERNAL_ENTITIES=false(plus namespace-aware, coalescing), which is the correct StAX hardening for parsing agent documents. The onlyDocumentBuilderFactoryuses are in the tests, and even those setdisallow-doctype-decl— good instinct, though test-controlled input isn't a vector anyway. - TLS is done the right way.
httpsagents go throughSSLContextParameters(the JSSE utility), falling back to the component's global SSL context and then the runtime default — notrustAllCertificates/all-trustingTrustManageranywhere in production, and the JDKHttpClientverifies host names by default.followRedirects(NEVER)is a nice touch: it avoids an agent (or a MITM on a plain-httpagent) bouncing the client to an attacker URL or leaking anything on a redirect. - No credential options to mark. There's no
username/password/token@UriParam, which fits how MTConnect agents are usually deployed (unauthenticated on an OT network, or client-cert via the SSL context);sslContextParametersis correctly in thesecuritygroup. If agent auth is ever added, mark those optionssecret/security=per the component checklist.
Structure looks conventional: it's a direct child of components/ (so no MojoHelper registration needed), the class names follow the MTConnectComponent/Endpoint/Consumer/PollingConsumer/Producer pattern, and the generated catalog, endpoint/component DSL, configurer and URI-factory files plus src/main/docs + the catalog doc mirror are all present and committed.
The one thing that has to happen before this can seriously be considered for merge:
- CI hasn't run (0 check-runs — fork PR). For a brand-new component especially, the full reactor needs to be green: it's the check that the generated files and catalog actually match the sources, that it builds across the modules it touches (
parent,components, the DSLs,camel-main,kamelet-main), and that the 20 test classes pass. Please get a committer to trigger it.
Areas I did not review in depth and would flag for the committer review: the streaming/long-poll path (MultipartReader/AgentProtocol — multipart/x-mixed-replace from the agent, read bounds via ReadDeadline), the consumer's polling/reconnect/backoff behaviour, and the breadth of the observation/asset parsing. The security-sensitive parts above check out; these are correctness surfaces worth a careful human read.
Nice work overall — the hardened parser and the TLS/redirect handling show the right instincts.
Claude Code on behalf of oscerd
gnodet-bot
left a comment
There was a problem hiding this comment.
Well-engineered new IoT component with clean architecture, correct protocol state machine, and thorough test coverage (13 test classes, including live-agent fixture data from real cppagent 2.8 and Mazak dialects). No blocking issues; one minor memory-hygiene point and a low-severity hardening note.
Overall:
- XXE:
XMLInputFactorywithIS_SUPPORTING_EXTERNAL_ENTITIES=falseandSUPPORT_DTD=false— correct - SSRF:
validate()restricts scheme tohttp/https, requires a host, andHttpClient.Redirect.NEVER— correct - TLS: null
SSLContextcorrectly falls back to JVM default trust store — correct - Thread safety:
DocumentParser.parse()creates its ownXMLStreamReaderper call;AssetWriter.copy()creates its own writer per call — both re-entrant; concurrent producer calls are safe MTConnectErrorException.getErrorCodes()/getErrorTexts():Stream.toList()returns an unmodifiable list since Java 16 — the static analysis flag is a false positive- Resource lifecycle: abort → wait → close ordering in both consumers is correct;
AgentReader.open()properly cleans up on partial failure;ReadDeadline.Watchis always used in try-with-resources
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
There was a problem hiding this comment.
TL;DR
Verdict: A careful, dependency-free MTConnect component with strong tests. One generated file is missing, which CI's uncommitted-changes check will catch.
Blockers
core/camel-util/src/main/java/org/apache/camel/util/ArtifactUtils.javaneeds the regeneratedmtconnectentry.
Other findings: 5 non-blocking (test timing bounds, Awaitility atMost, test logging config, canonical YAML in the docs, two narrow stop races).
Thanks a lot for this contribution, @etzel-mes. The protocol handling is impressively thorough (cursor across outages, OUT_OF_RANGE with both HTTP 200 and 400 dialects, instance-id re-probe, refresh without duplicates), the security surface is handled well (StAX with DTDs and external entities off, http/https only, redirects never followed, TLS through SSLContextParameters), and the stub agent built from real dialect captures makes the tests convincing.
Must fix before merge
Generated ArtifactUtils not committed. The COMPONENTS map in ArtifactUtils.java is regenerated by the update-artifact-helper goal of catalog/camel-catalog, and CI fails on the uncommitted change. Rebuilding catalog/camel-catalog after adding the component should add, right after the mongodb-gridfs entry:
Map.<String, String> entry("mtconnect", "camel-mtconnect"),
The camel-hibernate PR (#27935) shows the same file set plus this one.
Non-blocking
- Wall-clock upper bounds in tests.
MTConnectOutageTestlines 49 and 83,MTConnectPollingTestline 63 andMTConnectResyncTestline 101 assert detection or recovery within 1.0 to 1.55 s. On busy CI agents (ppc64le, s390x) these can flake, and the functional assertions around them already prove the behaviour. A hint: widen the budgets to a few seconds. - Explicit Awaitility
atMost. The project guidelines ask for an explicitatMost; mostawait()calls rely on the 10 s default. A helper inMTConnectTestSupport, e.g.static ConditionFactory awaitAgent() { return await().atMost(10, TimeUnit.SECONDS); }, would cover them. - Test logging. Most components have a
src/test/resources/log4j2.propertieswritingtarget/camel-<name>-test.log(seecomponents/camel-clickhouse). Without it only ERROR is logged, so the outage and resync logs would be missing if a timing test flakes. - Docs: canonical YAML. The YAML example at
mtconnect-component.adoc:229-238uses the shorthand (from:,filter: simple:,log: "..."). The docs use the canonical form:- route: from: uri/parameters/steps,filter: expression: simple: expression:,log: message:. - Two narrow stop races (hint). In
AgentHttpClient.send(160-168) anInterruptedExceptionfromfuture.get()leaves the request running, so the body may never be closed:catch (InterruptedException e) { future.cancel(true); throw e; }. InAgentReader.send(163-165) a response completed just beforeabort()is added toopenafter abort iterated it; closing it when the client is already cancelled would cover that.
On the two bot threads: keeping sentAssets across a restart looks deliberate, since anAgentRestartReadsTheAssetsAgain asserts unchanged assets are not re-sent, so a short reply may be enough. On the part-size thread, readNBytes(int) grows in chunks rather than allocating the declared length up front, so a size cap is optional hardening.
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.
…til for the new component Output of the update-artifact-helper goal of catalog/camel-catalog: the mtconnect entry. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…unds, a test log, canonical YAML docs and two stop races closed Review follow-ups: - Every await() goes through awaitAgent(), await().atMost(10 s), as the project guidelines ask. - The four wall-clock bounds on outage detection and recovery allow a 3 s slack for slow CI agents. - src/test/resources/log4j2.properties writes target/camel-mtconnect-test.log, as other components do. - The YAML example uses the canonical route, filter expression and log message form; catalog copy refreshed. - AgentHttpClient.send cancels the request when the waiting thread is interrupted and closes the body of an answer that arrived meanwhile. - AgentReader.send closes and fails a response registered after abort() iterated the open set. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks for the thorough read, @davsclaus. Pushed in 7ff41f6 and 3e1090b:
The two bot threads are answered inline. Claude Code on behalf of etzel-mes |
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — all issues have been addressed.
Davsclaus's findings (commits 7ff41f6 + 3e1090b):
- ✅
ArtifactUtils.javaregenerated with themtconnectentry (alphabetically aftermongodb-gridfs) - ✅ Wall-clock timing bounds widened:
SLACK = 3000ms inMTConnectTestSupport, applied inMTConnectOutageTestandMTConnectPollingTest - ✅
awaitAgent()helper added toMTConnectTestSupport(await().atMost(10, SECONDS)) and applied across all test classes - ✅
src/test/resources/log4j2.propertiesadded, writingtarget/camel-mtconnect-test.log - ✅ YAML docs example converted to canonical form (
- route:,filter: expression: simple: expression:,log: message:) - ✅
AgentHttpClient.sendinterrupt race:InterruptedExceptionnow cancels the in-flight future and closes a body that arrived between the interrupt and the cancel — correct - ✅
AgentReader.sendabort race: post-registrationclient.cancelled()check closes and fails a response thatabort()could not see — correct
My earlier inline findings:
sentAssetsnot cleared on re-probe: the author's explanation is correct. After re-probe the protocol re-reads all assets viaassets(), andsentAssetsdeduplicates bytimestamp/removedversion — an unchanged asset across a restart correctly suppresses re-delivery. Not a bug.Content-Lengthlarge allocation: the author's explanation is correct.readNBytesreads in 16 KiB chunks regardless of the declared length, so a spoofedContent-Lengthcosts only what the agent actually sends, bounded by theReadDeadlinesilence timeout. A per-part cap would be inconsistent with how other paths (noContent-Length, document responses) are bounded.
LGTM on the updated commits.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 587 of 709 tested, 0 compile-only — current: 586 all testedMaveniverse Scalpel detected 587 affected modules (current approach: 586). Skip-tests mode would test 587 modules (11 direct + 30 downstream), skip tests for 0 (generated code, meta-modules)
|
| Module | Duration | Status |
|---|---|---|
| Camel :: Component DSL | 55.6s | SUCCESS |
| Camel :: MTConnect | 49.5s | SUCCESS |
| Camel :: Main | 36.4s | SUCCESS |
| Camel :: Catalog :: Camel Catalog | 23.5s | SUCCESS |
| Camel :: Docs | 15.1s | SUCCESS |
| Camel :: Kamelet Main | 11.2s | SUCCESS |
| Camel :: Util | 9.6s | SUCCESS |
| Camel :: All Components Sync point | 5.0s | SUCCESS |
| Camel :: Components | 3.9s | SUCCESS |
| Camel :: Parent | 1.2s | SUCCESS |
| Camel :: BOM | 0.1s | SUCCESS |
| Camel :: Endpoint DSL | n/a |
Top 20 slowest modules:
Camel :: Component DSL(55.6s)Camel :: MTConnect(49.5s)Camel :: Main(36.4s)Camel :: Catalog :: Camel Catalog(23.5s)Camel :: Docs(15.1s)Camel :: Kamelet Main(11.2s)Camel :: Util(9.6s)Camel :: All Components Sync point(5.0s)Camel :: Components(3.9s)Camel :: Parent(1.2s)Camel :: BOM(0.1s)
oscerd
left a comment
There was a problem hiding this comment.
TL;DR
Comment. The two new commits address the review points, and CI has now built and tested the PR (green on 3e1090b9, including build (21, false)). That was the open point of my earlier review.
Blockers: None
Other findings: none.
Since my last review (deefb7a → 3e1090b)
- Generated file:
ArtifactUtils.javanow has themtconnectentry, in order aftermongodb-gridfs. - Stop races: both fixes look right to me.
- In
AgentHttpClient.send, an interrupt now cancels the request. If the answer had already arrived,cancelhas no effect and thethenAcceptcloses its body. If it had not, the future completes as cancelled and nothing is left open. - In
AgentReader.send, the response is registered inopenbefore the volatilecancelledflag is read, andabort()sets that flag before it walksopen. Whichever of the two runs second therefore closes the response, and none can stay open.
- In
- Tests: the wall-clock bounds now carry a
SLACKof 3 s,awaitAgent()gives every wait an explicitatMost, andlog4j2.propertieswrites a test log. All of these match the suggestions. - Docs: the YAML example uses the canonical form, and the catalog copy of the doc is regenerated.
As I wrote before, a component of this size still deserves a committer's deep review of the streaming and multipart path and of the consumer's reconnect handling, beyond the security and structure checks I did.
This is a rules-and-conventions review. It does not replace static analysis tools.
Reviewed with Claude Code on behalf of oscerd. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
davsclaus
left a comment
There was a problem hiding this comment.
Reviewed the changes since my last review: the points I raised are addressed, and CI is green on the current head.
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.
Description
MTConnect (ANSI/MTC1.4-2018) is the open standard machine tool vendors ship for reading their machines: a read-only HTTP agent that answers
probe(device model),current(latest values),sample(a range or an open stream of observations) andassets(cutting tools, NC programs) with XML documents. Mazak, Okuma, DMG MORI, Haas, Makino and the Fanuc and Siemens adapters ship or support such an agent; it is the machine tool counterpart of OPC UA, and the two are what a CNC speaks to the outside. Camel hascamel-milofor OPC UA and nothing for MTConnect.Where it sits. A machine tool integration has the machine's agent on one side and the plant's systems on the other: MES or ERP (job and tool status), a broker or unified namespace (MQTT, Kafka), a historian, condition monitoring and OEE. Camel has every component for the second side; this is the missing first side. It turns the agent's protocol into one Camel message per observation with the device, component, data item, units, sequence and quality on the message, so the rest of the route is plain Camel.
Why a component and not a route. We first built it as a route:
http+timer+ a Groovy script of about 140 lines holding the state machine (probe, current,sample?from=,OUT_OF_RANGEresynchronisation,instanceIdre-probe, per-itemBadquality on timeouts). It worked against the same agents and showed why it does not belong in a route: the agent's streamingsampleanswer is a multipart stream that never ends, andcamel-httpreads a response as a whole, so a route can only poll, with a round trip per interval per machine; the protocol's correctness lives in details a script hides (the cursor across outages, overrun with HTTP 400 on 2.x agents but 200 on 1.x agents, agent restart, probe metadata on every observation, asset changes a lost event can hide,UNAVAILABLEversus unreachable), which every integration repeats and re-debugs; and a script is invisible to the catalog, the docs, Camel JBang and the MCP tools, while a component declares every option and header.This PR adds
camel-mtconnect(schememtconnect), consumer and producer, with no third-party dependency (JDK HTTP client, StAX).Consumer. Runs the protocol against one agent:
probe,current, then the streamingsamplerequest (or onesamplerequest per poll withstreaming=false, for agents behind proxies that cut long-lived responses). One message per observation: the value as body (BigDecimal, text,Mapfor data sets and tables,List<BigDecimal>for time series) and the MTConnect metadata asCamelMTConnect*headers (device, component, data item, category, type, units, sequence, timestamp, quality). It keeps the sequence cursor across outages, resynchronises fromcurrentonOUT_OF_RANGE, re-reads the device model when the agent'sinstanceIdchanges, reports an unreachable agent once per data item with qualityBad, and refreshes every item after recovery without delivering an observation twice. Withassets=trueit also delivers the agent's assets and their changes and removals, the body being the asset's own XML.Producer.
operation=probe|current|sample|assets, parameters as options or headers; the device model and asset documents come back as the agent's XML, observations as a list of maps with the consumer's fields;OUT_OF_RANGEand other agent errors fail the exchange withMTConnectErrorExceptioncarrying the agent's error codes.TLS and compression.
sslContextParametersand the component'suseGlobalSslContextParameters; gzip requested and decoded for document answers (never for the stream).Tests. 104 tests on
CamelTestSupportagainst an in-process stub agent that speaks the cppagent 2.8 (schema 2.5,multipart/mixed) and the Mazak 1.4/1.6 (schemas 1.3/1.5, vendor namespace,multipart/x-mixed-replace,OUT_OF_RANGEwith HTTP 200) dialects, with knobs for pause, refuse, restart, overrun, closed streams, error documents, gzip, TLS with a private authority and asset changes. NoThread.sleep; Awaitility and AssertJ. Fixtures are captures of real agents with identities replaced.Live runs (not part of the PR, reported here): a cppagent 2.8.0.4, Mazak 1.4.0.12 and Mazak 1.6.0.6 vendor agents and demo.mtconnect.org over https; see the table below.
heartbeat=2000)Current; 5 values written through the agent arrived asSamplein order, once each, with the probe metadata; write-to-message latency 14 to 361 msBad/UNREACHABLEonce, 3.9 s after the pause (two heartbeats); 0.25 s after the gate opened the 12 values arrived once and in order, then the 13 untouched items were refreshed once asCurrent; no duplicate sequenceCurrentwithCamelMTConnectResync=OUT_OF_RANGEand the latest value 0.22 s after the gate opened; no sequence below the agent'sfirstSequenceassets=true)AssetChanged/AssetRemovedobservations followed by exactly threeAssetmessages, the last withCamelMTConnectAssetRemoved=trueCurrent(every item once), 414Samplein strictly ascending order, 0 duplicates, 0UNREACHABLE/ERRORstreaming=false&delay=1000) 40 s beside a streaming routeCurrent, 243Sample, every polled sequence also in the stream's set, no duplicateassets=true25 sCurrent, no error, no asset message (the agent holds none)probe12,896 characters as the agent sends them;current86 maps with the probe metadata;assets156 CuttingTool and 72 File, equal to the agent's own answers;samplewithcount=3gives 3 maps andNextSequence=from+3;samplewithfrom=1fails withMTConnectErrorExceptionOUT_OF_RANGECurrent, 1,308Sample, 0 duplicates, no TLS error; producerassets66 CuttingToolDesign notes a reviewer may ask about.
HttpClientrather than camel-http or Vert.x: the streamingsampleanswer is a multipart stream read part by part while it arrives, the request in flight must be cancellable on stop, and the component needs no dependency;camel-telegram,camel-opaand six other components use the JDK client the same way.camel-miloandcamel-plc4xdo the same.Fixes the component lists, catalog copies, docs nav and the component/endpoint DSL factories as the build generates them.
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.The component was written with Claude Code (Claude Opus and Fable models) under the operator's direction and reviewed and verified by the operator against the agents above. JIRA user: mardonius. @davsclaus @oscerd would you have a look?
Claude Code on behalf of etzel-mes