Repository navigation
Conversation
…ce in two notations A runtime exporter adds its dependencies after the duplicates of the collected ones were removed, so an artifact it adds is named a second time when the recorded run holds the same artifact in another notation: camel:micrometer-prometheus and mvn:org.apache.camel:camel-micrometer-prometheus:4.23.0 are two strings for one artifact. With camel.metrics.enabled the exported pom.xml named camel-micrometer-prometheus twice, and Maven 3.10 fails such a build. CAMEL-24247 changed the notation of the camel-main metrics and health dependencies so that the Set collapses them. That covers the options the base exporter handles as well, and not the console dependencies, which all three runtimes name twice for the same reason. resolveDependencies is now the step that removes the duplicates, after the dependencies a runtime exporter adds: the collection moved to the overridable doResolveDependencies. A runtime exporter therefore sees the dependencies before the duplicates are removed. The camel-main exporter decided with a string check whether the main HTTP server is included, and removed camel-platform-http and -vertx by the same kind of check, so a dependency given as camel:platform-http would now survive the removal. Those three checks compare artifacts instead of dependency strings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e mapping of a runtime The duplicates of the dependencies are removed by the groupId:artifactId of each dependency as written, before a runtime exporter maps them to its own artifacts. Two dependencies with other coordinates become the same artifact through that mapping, and the exported pom.xml names it twice: with --runtime=quarkus --console, the camel-console that --console adds becomes org.apache.camel.quarkus:camel-quarkus-console, which a --dep=org.apache.camel.quarkus:camel-quarkus-console names as well. The Spring Boot export does the same through its -starter mapping, and Maven 3.10 fails such a build. The four places that turn the dependency strings into the MavenGavs of the pom (the camel-main list, the Quarkus list after its replacements, the Spring Boot list and the legacy catalog template of Spring Boot) now remove the duplicates after their mapping, with one rule in ExportHelper next to the comparator they share: a single dependency per groupId:artifactId, the first one in its position, except that one with a version wins over one without, as the mapping leaves the version of a runtime's own artifacts to the BOM. The pom entries keep their sort order. The camel-main list maps nothing of its own, where only two local JARs of one artifact converge; the rule is called there as well, so it holds for every pom the export writes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@davsclaus @gnodet would you have a look? I cannot request reviewers as an external contributor. This builds on your CAMEL-24247 fix rather than replacing it: the JIRA: CAMEL-26697, assigned to mardonius. Claude Code on behalf of etzel-mes |
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR
Approve: the export now names each artifact once in the pom.xml, both when the same artifact comes in two notations and when the runtime mapping folds two coordinates into one artifact.
Blockers: None
Other findings: none, all review questions resolved.
Thanks @etzel-mes, this is a careful fix with a very clear write-up. What I checked:
- Making
resolveDependenciesfinal. The only subclasses are the three package-private runtime exporters, and all of them now overridedoResolveDependenciesand callsuperfirst. Nothing outside the package can be broken byfinal. Running the string dedup after the runtime additions is the actual fix for the notation case. It also coversSpringBootExporteraddingmvn:org.apache.camel:camel-platform-httpnext to a user'scamel:platform-http, which would have produced a duplicate before. - The artifact checks in
CamelMainExporter.MavenGav.parseGavmapscamel:x,camel-x,org.apache.camel:camel-x[:v]andmvn:org.apache.camel:camel-x:vto the same groupId/artifactId, and it never throws on other strings (file:, plain GAVs). The exact artifactId comparison also fixes a gap in the oldcontains("org.apache.camel:camel-platform-http:")check, which missed a versionlessmvn:entry. ExportHelper.removeDuplicateArtifactsafter the mapping. First entry wins and keeps its position, and a versioned entry wins over an unversioned one. That matches the existing--depprecedence rule, and the sort still runs last. The CAMEL-24247camel:notation is kept, so this doesn't revert that change.
Tests: assertEachArtifactOnce over the three runtimes and the three metrics options, plus the --dep given as a runtime artifact, cover the cases in the table. shouldReplacePlatformHttpInAnyNotation pins the camel-main replacement. The ExportHelperTest additions follow that class's existing JUnit style.
The remaining SpringBootExporter contains("mvn:org.apache.camel:camel-platform-http") check is fine to leave as you describe. It is a prefix match that also covers -vertx/-main, so an exact-artifact swap would change its meaning and belongs to its own issue.
CI on this head is still waiting for workflow approval. This review is static and does not replace CI, SonarCloud or other specialized review tools.
Claude Code on behalf of Claus Ibsen
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.
TL;DR
Request changes: one small addition before merge. The SpringBootExporter HTTP-server check should use the new hasCamelArtifact helper as well.
Blockers
SpringBootExporter.java:141and:448: the HTTP-server check still compares dependency strings
Other findings: none. Everything else from my earlier review stands.
Sorry for the back and forth @etzel-mes. In my approval I agreed this could be left out. On a second look it's a two-line change with the helper you already added, so it's better to do it in this PR than in a separate issue.
The issue. Both places in SpringBootExporter decide whether the export has an HTTP server with:
deps.stream().anyMatch(s -> s.contains("mvn:org.apache.camel:camel-platform-http"))That misses the short notation (--dep=camel-platform-http, --dep=camel:platform-http, or camel.jbang.dependencies=camel:platform-http) when the recorded run doesn't also hold the mvn: form. At line 141 the exported application.properties then gets camel.main.run-controller=true although the project has a server. This is the same pattern the first commit replaced in CamelMainExporter.
Suggested change. hasCamelArtifact is protected static in Exporter, so SpringBootExporter can call it at both places. Keep camel-platform-http-vertx, because the current prefix match counts it as a server:
// line 141
boolean http = hasCamelArtifact(deps, "camel-platform-http")
|| hasCamelArtifact(deps, "camel-platform-http-vertx");
// line 448
boolean http = hasCamelArtifact(answer, "camel-platform-http")
|| hasCamelArtifact(answer, "camel-platform-http-vertx");Test. A Spring Boot sibling of shouldReplacePlatformHttpInAnyNotation: export with --runtime=spring-boot --dep=camel-platform-http, then assert that the exported application.properties has no camel.main.run-controller and that assertEachArtifactOnce holds. Please keep the JDK 21 guard that the other Spring Boot tests use.
This review is static and does not replace CI, SonarCloud or other specialized review tools.
Claude Code on behalf of Claus Ibsen
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
gnodet-bot
left a comment
There was a problem hiding this comment.
Clean, well-structured fix for the Maven 3.10 duplicate-GA rejection in exported pom.xml. The two-layer dedup strategy (string-level in resolveDependencies → MavenGav-level in buildDependencyList) is correct and non-interfering. The template method pattern (final resolveDependencies + overridable doResolveDependencies) is a solid design improvement. The notation-agnostic hasCamelArtifact/removeCamelArtifact helpers are strictly more correct than the old contains()-based string matching. Tests are thorough.
One minor readability note inline — otherwise this is good to go.
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:
|
|
🧪 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: 3 of 717 tested, 3 compile-only — current: 6 all testedMaveniverse Scalpel detected 3 affected modules (current approach: 6). Skip-tests mode would test 3 modules (2 direct + 2 downstream), skip tests for 3 (generated code, meta-modules) Modules only in current approach (3)
Modules Scalpel would test (3)
Modules with tests skipped (3)
💡 Manual integration tests recommended:
All tested modules (10 modules, 1m 44s total)Total reactor time: 1m 44s
Top 20 slowest modules:
|
…r in any notation The Spring Boot exporter decided with a string check whether the export has an HTTP server, so a dependency given as camel:platform-http was invisible to it. The exported application.properties would then get camel.main.run-controller=true although the project has a server, and an export with openapi would add camel-platform-http a second time. Both places compare artifacts now, as the camel-main exporter does since the first commit. camel-platform-http-vertx counts as a server as well, as the prefix of the string check did. camel-platform-http-main and -jolokia, which the recorded run can hold, no longer count: they are the server of camel-main, not of Spring Boot. No export changes its outcome by that, as a run with --console records camel-platform-http and -vertx next to them. The test pins that a Spring Boot export with platform-http gets no run controller. It does not distinguish the notations: with --dep the silent run records the mvn: notation as well, so the string check found the artifact too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The version preference of the duplicate removal mixes || and && without parentheses. The condition is the intended one, as && binds tighter, but a reader has to apply the precedence rules and static analysis reports the mix. The parentheses group the pair; the rule is unchanged: the first dependency of an artifact is kept, unless it has no version and a later one does. Suggested in the review of this pull request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — both issues are addressed.
- Parentheses in
ExportHelper— ✅ addressed.(!hasVersion(existing) && hasVersion(gav))is explicit now. SpringBootExporterHTTP check at both locations — ✅ addressed. Bothexport()(line ~141) anddoResolveDependencies(line ~448) now usehasCamelArtifact(deps, "camel-platform-http") || hasCamelArtifact(deps, "camel-platform-http-vertx"), matching whatdavsclausrequested. The newshouldKeepTheRunControllerOutWhenPlatformHttpIsGivenInAnyNotationtest covers it with the JDK 21 guard and assertscamel.main.run-controlleris absent.
Previous findings are all addressed. LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
There was a problem hiding this comment.
Re-review of cbc5389 and c3ace16. Thanks @etzel-mes, both points are addressed:
SpringBootExporter: both places (therun-controllercheck inexport()and the openapi check indoResolveDependencies) now usehasCamelArtifactforcamel-platform-httpandcamel-platform-http-vertx. Your note on the narrower match (-mainand-jolokiano longer count as a server) checks out: a--consolerun records the component next to them, so no export changes.ExportHelper: the explicit parentheses keep the same rule.- The new
shouldKeepTheRunControllerOutWhenPlatformHttpIsGivenInAnyNotationtest pins the behaviour, and I agree it can't separate the notations while the silent run always records themvn:form.
On the two checks you left on purpose (the contains("camel-platform-http") in CamelMainExporter and Exporter.hasOpenapi) and the ExportTest.createCommand problem (a later parse resets --dir and the test exports into camel-jbang-core itself): yes, please file those as a JIRA. The createCommand one in particular can overwrite the module's pom.xml in a local build, so it's worth fixing.
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.
|
@etzel-mes CI fails in Could you regenerate it and push the updated golden file? Only Claude Code on behalf of davsclaus. This was generated by an AI agent and may contain inaccuracies; please verify before applying. |
…h artifact once The options case of RunExportGoldenTest exports a camel-main run with --observe and --console. Its expected pom.xml listed camel-platform-http-main and camel-platform-http-jolokia twice: the duplicate in two notations that this change removes. Regenerated with -Dcamel.golden.update=true; only options.export.txt changes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier reviews — previous findings remain fully addressed and no new issues found.
The commit since last review (0a2642d) fixes the CI failure davsclaus flagged: options.export.txt removes the two duplicate lines (camel-platform-http-jolokia and camel-platform-http-main) that the new dedup logic now prevents from appearing in the exported pom.xml. The production fix, deduplication helpers, and new tests are sound — full review in my earlier comment.
Previous findings all addressed. LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
There was a problem hiding this comment.
Re-review of 0a2642d: the golden file options.export.txt now lists camel-platform-http-jolokia and camel-platform-http-main once each, which is the dedup fix working as intended. CI is green, the earlier threads are resolved. LGTM, thanks @etzel-mes.
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
camel exportwrote apom.xmlthat names the same artifact twice. Maven 3.9 warns about that, and Maven 3.10.0 (2026-09-27) fails the build; the exported project carries the 3.9.12 wrapper, so the failure appears when the project is built with a Maven of its own, as a build pipeline does.Two mechanisms produce such a pom, one commit each, and two more commits from the review.
The notation.
Exporter.resolveDependenciescollects the dependencies and removes the duplicates per groupId:artifactId as its last step. The three runtime exporters override it, callsuperfirst and then add their own dependencies, so what they add is not deduplicated — and an artifact in thecamel:notation is a different string from themvn:org.apache.camel:camel-...:<version>that the recorded run holds. CAMEL-24247 changed the notation of the camel-main metrics and health dependencies for this reason; that holds only where the base exporter adds the identical string before the duplicates are removed.The mapping. The duplicates are removed by the coordinates as written, while each runtime maps the dependencies to its own artifacts afterwards (
camel-x→camel-quarkus-x,camel-x→camel-x-starter, and thequarkus-dependencies.propertiesreplacements). Two dependencies with different written coordinates become the same artifact there.--runtime=camel-main,camel.metrics.enabled=true--runtime=camel-main --console, console options in application.properties--runtime=quarkus --console, the same--runtime=spring-boot --console, the same--runtime=quarkus --console --dep=org.apache.camel.quarkus:camel-quarkus-console--runtime=spring-boot --console --dep=org.apache.camel.springboot:camel-console-starterThe change:
resolveDependenciesis the step that removes the duplicates of the dependency strings and isfinal: it calls the overridabledoResolveDependenciesand deduplicates what comes back. The dependencies a runtime exporter adds are deduplicated as well, whatever notation they use.CamelMainExporterdecided withs.contains("mvn:org.apache.camel:camel-platform-http-main")whether the main HTTP server is included, and removedcamel-platform-httpand-vertxby the same kind of string check, so a dependency given ascamel:platform-httpwould have survived the removal. Those three checks compare artifacts (hasCamelArtifact/removeCamelArtifact).Exporter.buildDependencyList,QuarkusExporter.buildQuarkusDependencyListafter its replacements,SpringBootExporter's list and its legacy catalog template) remove the duplicates of the mapped artifacts, with one rule inExportHelper.removeDuplicateArtifacts: a single dependency per groupId:artifactId, the first, but one with a version wins over one without — the rule the string step already applies to an explicit--depversion. The pom entries keep their order, as the sort still runs last.SpringBootExporterdecided the same way whether the export has an HTTP server, for the run controller of the exportedapplication.propertiesand for the openapi dependency. Both places compare artifacts now (third commit, on review).camel-platform-http-vertxcounts as a server as well, as the prefix of the string check did.camel-platform-http-mainand-jolokiano longer count, as they are the server of camel-main and not of Spring Boot; no export changes its outcome by that, as a run with--consolerecordscamel-platform-httpand-vertxnext to them.removeDuplicateArtifactsreads in parentheses (fourth commit, on review). The condition is unchanged.camel:notation of CAMEL-24247 is kept; the fix does not revert it.Tests:
ExportTest.shouldNotDuplicateMetricsDependency(the test of CAMEL-24247) now runs over the three metrics options;shouldNotDuplicateConsoleDependenciesexports with--consoleand a profile that has the console options, for the three runtimes;shouldNotDuplicateConsoleDependencyGivenAsRuntimeArtifactadds a--depwith the runtime's own coordinates for Quarkus and Spring Boot. Each asserts that the exportedpom.xmlnames every artifact once. Six of those cases fail without the first two changes, andExportHelperTesthas the rule of the second one on its own.shouldReplacePlatformHttpInAnyNotationandshouldKeepTheRunControllerOutWhenPlatformHttpIsGivenInAnyNotationpin what camel-main and Spring Boot do with acamel:platform-httpdependency; both pass before their change as well, because the silent run records themvn:notation of a--deptoo, so the string checks found the artifact — they guard the behaviour rather than prove the fix.camel-jbang-run(257 tests) and the wholeExportTest(158) pass.Please check the whole change
camel-platform-http-mainand-jolokia, which the prefix of the old string check did. I looked for an export that changes its outcome by that and found none: a run with--consolerecordscamel-platform-httpand-vertxnext to those two, so the component is in the set whenever they are. The check is narrower in principle, with no set the recorded run produces to show it.--depthe silent run records themvn:notation as well, and no option skips the silent run, so the short notation never reaches the exporter alone in an export test. Both lines are provable by reading; the tests pin the behaviour and fail when the detection breaks.CamelMainExporter's "if platform-http is included then switch to camel-platform-http-main" (its substring also matches-vertxand-jolokia, which for camel-main should switch to the main server, so an exact comparison would change behaviour) andExporter.hasOpenapi. Both miss thecamel:notation, and both would carry the test limitation above. Say the word and they go in here or into an issue of their own.pom.xmlloses a line it should not have had.camel-4.18.x. Whether this one is backported is your call.mvn clean install -DskipTestsfrom the root folder was not run on this machine;camel-jbang-runandcamel-jbang-corewere built and their tests run, and the change feeds no generator.Target
mainbranch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes. (Only the two changed modules were built; see above.)AI-assisted contributions
Co-authored-bytrailers) and the PR description identifies the AI tool used.Written with Claude Code (Claude Opus 5 and Claude Fable 5.1), each commit reviewed by a separate Claude Fable 5.1 run.
Claude Code on behalf of etzel-mes
🤖 Generated with Claude Code