gnodet-bot commented on code in PR #26575:
URL: https://github.com/apache/camel/pull/26575#discussion_r4045023601
##########
dsl/camel-kamelet-main/src/main/resources/camel-main-known-dependencies.properties:
##########
@@ -88,3 +78,71 @@ dev.langchain4j.model.embedding.onnx =
dev.langchain4j:langchain4j-embeddings:${
org.apache.camel.component.ai.observability.GenAiObservabilityImpl =
camel:ai-observability
# camel-main property prefix (same pattern as camel.opentelemetry) — resolves
ai-observability when GenAI observability config is accessed
camel.aiObservability = camel:ai-observability
+
+# Third-party libraries by package (CAMEL-24809). The resolver matches the
class name and then each enclosing
+# package, so one line per library covers every class in it: org.postgresql
covers the Driver, PGSimpleDataSource,
+# the pooling datasources and the rest of the jar. Map at the library's own
package, never a shared parent such
+# as org.apache.commons, or the walk up the package would pick the wrong jar.
Versions are the properties of
+# camel-dependencies (KnownDependenciesVersionResolver); a literal version is
used where no property exists.
+
+# JDBC drivers, datasources and connection pools
+org.postgresql = org.postgresql:postgresql:${pgjdbc-driver-version}
+com.mysql.cj = com.mysql:mysql-connector-j:${debezium-mysql-connector-version}
+org.mariadb.jdbc = org.mariadb.jdbc:mariadb-java-client:${mariadb-version}
+com.microsoft.sqlserver.jdbc = com.microsoft.sqlserver:mssql-jdbc:12.10.0.jre11
+oracle.jdbc = com.oracle.database.jdbc:ojdbc17:23.8.0.25.04
+org.h2 = com.h2database:h2:${h2-version}
+com.zaxxer.hikari = com.zaxxer:HikariCP:6.3.0
Review Comment:
📝 **Hardcoded versions won't track `camel-dependencies` upgrades**
`com.microsoft.sqlserver:mssql-jdbc:12.10.0.jre11`,
`com.oracle.database.jdbc:ojdbc17:23.8.0.25.04`, and
`com.zaxxer:HikariCP:6.3.0` have no version property in `camel-dependencies`
(verified — none of `mssql-jdbc-version`, `ojdbc-version`, or
`HikariCP-version` exist in the parent POM). The block comment correctly
documents this as intentional, but these three entries will silently fall
behind whenever the project bumps those driver versions elsewhere.
If Camel's BOM/dependencies POM doesn't manage these yet, consider adding
version properties to `camel-dependencies` for them (HikariCP in particular is
heavily used). If that's deferred, at minimum add a TODO comment so the next
person knows why the literal is there:
```suggestion
com.microsoft.sqlserver.jdbc =
com.microsoft.sqlserver:mssql-jdbc:12.10.0.jre11
# TODO: switch to ${mssql-jdbc-version} once managed in camel-dependencies
oracle.jdbc = com.oracle.database.jdbc:ojdbc17:23.8.0.25.04
# TODO: switch to ${ojdbc-version} once managed in camel-dependencies
org.h2 = com.h2database:h2:${h2-version}
com.zaxxer.hikari = com.zaxxer:HikariCP:6.3.0
# TODO: switch to ${HikariCP-version} once managed in camel-dependencies
```
##########
dsl/camel-kamelet-main/src/test/java/org/apache/camel/main/download/KnownDependenciesResolverTest.java:
##########
@@ -53,4 +53,38 @@ void mavenGavForClass_returnsPackageScopedDependency() {
public static class SomeClass {
}
+
+ @Test
+ void theShippedMappingResolvesThirdPartyClassesByPackage() {
+ // CAMEL-24809: one line per library, matched by walking up the
package; the Artemis package sits under the
+ // classic ActiveMQ one and must win for its own classes
+ KnownDependenciesResolver resolver = new KnownDependenciesResolver(new
SimpleCamelContext(), null, null);
+ resolver.loadKnownDependencies();
+
+ assertGav(resolver, "org.postgresql.ds.PGSimpleDataSource",
"org.postgresql", "postgresql");
+ assertGav(resolver, "org.postgresql.ds.PGConnectionPoolDataSource",
"org.postgresql", "postgresql");
+ assertGav(resolver, "org.h2.jdbcx.JdbcDataSource", "com.h2database",
"h2");
+ assertGav(resolver, "com.zaxxer.hikari.HikariConfig", "com.zaxxer",
"HikariCP");
+ assertGav(resolver,
"org.apache.activemq.artemis.jms.client.ActiveMQConnectionFactory",
"org.apache.activemq",
+ "artemis-jakarta-client-all");
+ assertGav(resolver, "org.apache.activemq.ActiveMQConnectionFactory",
"org.apache.activemq", "activemq-client");
+ assertGav(resolver, "org.apache.qpid.jms.JmsConnectionFactory",
"org.apache.qpid", "qpid-jms-client");
+ assertGav(resolver, "com.fasterxml.jackson.databind.ObjectMapper",
"com.fasterxml.jackson.core", "jackson-databind");
+ assertGav(resolver, "com.fasterxml.jackson.dataformat.xml.XmlMapper",
"com.fasterxml.jackson.dataformat",
+ "jackson-dataformat-xml");
+ assertGav(resolver, "org.apache.commons.csv.CSVFormat",
"org.apache.commons", "commons-csv");
+ assertGav(resolver, "software.amazon.awssdk.services.sqs.SqsClient",
"software.amazon.awssdk", "sqs");
+ assertGav(resolver, "org.infinispan.client.hotrod.RemoteCacheManager",
"org.infinispan", "infinispan-client-hotrod");
+ assertGav(resolver, "org.infinispan.manager.DefaultCacheManager",
"org.infinispan", "infinispan-core");
+ assertGav(resolver, "freemarker.template.Configuration",
"org.freemarker", "freemarker");
+ // a shared parent package is deliberately not mapped
+ assertEquals(null,
resolver.mavenGavForClass("org.apache.commons.Anything"));
+ }
+
+ private static void assertGav(KnownDependenciesResolver resolver, String
className, String groupId, String artifactId) {
+ MavenGav gav = resolver.mavenGavForClass(className);
+ assertNotNull(gav, className);
+ assertEquals(groupId, gav.getGroupId(), className);
+ assertEquals(artifactId, gav.getArtifactId(), className);
+ }
Review Comment:
🔍 **Test gap — version placeholder typos are invisible to `assertGav`**
`assertGav` checks only `groupId` and `artifactId`. If a version placeholder
in the `.properties` file has a typo (e.g. `${pgjdbc-driver-versio}` instead of
`${pgjdbc-driver-version}`) or references a property that doesn't exist in
`camel-dependencies`, `mavenGavForClass` will return a `MavenGav` with a
literal placeholder string as the version — and this test will still pass,
because version is never compared.
For the 3 hardcoded literal versions (HikariCP 6.3.0, mssql-jdbc
12.10.0.jre11, ojdbc17 23.8.0.25.04), this makes it easy for them to silently
drift. Add at least spot-checks for version non-emptiness and
non-placeholder-literal:
```suggestion
private static void assertGav(KnownDependenciesResolver resolver, String
className, String groupId, String artifactId) {
MavenGav gav = resolver.mavenGavForClass(className);
assertNotNull(gav, className);
assertEquals(groupId, gav.getGroupId(), className);
assertEquals(artifactId, gav.getArtifactId(), className);
String version = gav.getVersion();
assertNotNull(version, className + " version is null");
assertFalse(version.startsWith("${"), className + " version is an
unresolved placeholder: " + version);
}
```
(You'll need `import static org.junit.jupiter.api.Assertions.assertFalse;`)
##########
dsl/camel-kamelet-main/src/main/resources/camel-main-known-dependencies.properties:
##########
@@ -88,3 +78,71 @@ dev.langchain4j.model.embedding.onnx =
dev.langchain4j:langchain4j-embeddings:${
org.apache.camel.component.ai.observability.GenAiObservabilityImpl =
camel:ai-observability
# camel-main property prefix (same pattern as camel.opentelemetry) — resolves
ai-observability when GenAI observability config is accessed
camel.aiObservability = camel:ai-observability
+
+# Third-party libraries by package (CAMEL-24809). The resolver matches the
class name and then each enclosing
+# package, so one line per library covers every class in it: org.postgresql
covers the Driver, PGSimpleDataSource,
+# the pooling datasources and the rest of the jar. Map at the library's own
package, never a shared parent such
+# as org.apache.commons, or the walk up the package would pick the wrong jar.
Versions are the properties of
+# camel-dependencies (KnownDependenciesVersionResolver); a literal version is
used where no property exists.
+
+# JDBC drivers, datasources and connection pools
+org.postgresql = org.postgresql:postgresql:${pgjdbc-driver-version}
+com.mysql.cj = com.mysql:mysql-connector-j:${debezium-mysql-connector-version}
+org.mariadb.jdbc = org.mariadb.jdbc:mariadb-java-client:${mariadb-version}
+com.microsoft.sqlserver.jdbc = com.microsoft.sqlserver:mssql-jdbc:12.10.0.jre11
+oracle.jdbc = com.oracle.database.jdbc:ojdbc17:23.8.0.25.04
+org.h2 = com.h2database:h2:${h2-version}
+com.zaxxer.hikari = com.zaxxer:HikariCP:6.3.0
+com.mchange.v2.c3p0 = com.mchange:c3p0:${c3p0-version}
+org.apache.commons.dbcp2 =
org.apache.commons:commons-dbcp2:${commons-dbcp2-version}
+
+# Messaging clients (the Artemis package is deeper than the classic ActiveMQ
one and is matched first)
+org.apache.activemq.artemis =
org.apache.activemq:artemis-jakarta-client-all:${activemq-artemis-version}
+org.apache.activemq = org.apache.activemq:activemq-client:${activemq6-version}
Review Comment:
⚠️ **Correctness gap — classic ActiveMQ 5 users get ActiveMQ 6 downloaded**
`org.apache.activemq` is shared by both the classic ActiveMQ 5 client
(`org.apache.activemq.ActiveMQConnectionFactory`) and ActiveMQ 6. Mapping the
whole package to `activemq-client:${activemq6-version}` (6.3.2) means a user
who writes:
```java
org.apache.activemq.ActiveMQConnectionFactory cf = ...
```
in a `camel run` script targeting ActiveMQ 5 will silently download v6
instead. The comment above says "classic ActiveMQ 6" which confirms the intent,
but a user on ActiveMQ 5 has no way to override this: the resolver picks the
first match walking up from the class name.
If the mapping is intentionally restricted to ActiveMQ 6, the comment should
say so and the entry should ideally be `org.apache.activemq.activemq6` or a
more specific anchor. If both major versions should be supported, there needs
to be a way to disambiguate (there isn't one with the current resolver design,
so document the limitation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
