gnodet-bot commented on code in PR #26575:
URL: https://github.com/apache/camel/pull/26575#discussion_r4045148078
##########
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:
❌ **Prior finding not addressed — version is never verified**
`assertGav` checks only `groupId` and `artifactId`. A missing property in
`parent/pom.xml` (typo in `${…}`, property deleted, or a new entry that forgets
to add the property) will have `mavenGavForClass` return a `MavenGav` with the
literal placeholder as its version — and this test will still pass, because
version is never compared.
The build-time validator in `PrepareKameletMainMojo` would catch a missing
property at compile time, but this test loads the **pre-generated** file from
the classpath (with versions already resolved), so it doesn't exercise that
path. What it does exercise is that the resolver can find and parse the
generated file correctly — which is exactly when version should be non-null and
non-placeholder.
```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);
}
```
(Add `import static org.junit.jupiter.api.Assertions.assertFalse;` to the
imports.)
##########
tooling/maven/camel-package-maven-plugin/src/main/java/org/apache/camel/maven/packaging/PrepareKameletMainMojo.java:
##########
@@ -104,6 +149,175 @@ public void execute() throws MojoExecutionException,
MojoFailureException {
} catch (Exception e) {
throw new MojoFailureException("Error updating
camel-factoryfinder-known-dependencies.properties", e);
}
+ try {
+ updateKnownThirdPartyDependencies();
+ } catch (MojoFailureException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new MojoFailureException("Error updating
camel-thirdparty-known-dependencies.properties", e);
+ }
+ }
+
+ /**
+ * Generates camel-thirdparty-known-dependencies.properties from the
curated list of third-party libraries
+ * (CAMEL-24809). Each input line maps a package to
groupId:artifactId:version, where the version is a ${property}
+ * of the project (inherited from camel-parent) or
@bom(groupId:artifactId:${property}) for a library whose version
+ * a BOM manages. The version is resolved here, so the runtime needs no
lookup, and a property that does not exist
+ * or a literal version fails the build. With verifyThirdPartyJars every
jar is resolved and the mapped package must
+ * be found in it.
+ */
+ protected void updateKnownThirdPartyDependencies() throws Exception {
+ File input = thirdPartyLibraries.isAbsolute()
+ ? thirdPartyLibraries : new File(project.getBasedir(),
thirdPartyLibraries.getPath());
+ if (!input.exists()) {
+ getLog().info("No " + input + ":
camel-thirdparty-known-dependencies.properties not generated");
+ return;
+ }
+ Properties in = new Properties();
+ try (InputStream is = new FileInputStream(input)) {
+ in.load(is);
+ }
+ Map<String, String> boms = new LinkedHashMap<>();
+ List<String> problems = new ArrayList<>();
+ Map<String, String> resolved = new TreeMap<>();
+ for (String pkg : in.stringPropertyNames()) {
+ String gav = in.getProperty(pkg).trim();
+ int i = gav.indexOf(':');
+ int j = gav.indexOf(':', i + 1);
+ if (i < 0 || j < 0) {
+ problems.add(pkg + " = " + gav + ": expected
groupId:artifactId:version");
+ continue;
+ }
+ String groupId = gav.substring(0, i);
+ String artifactId = gav.substring(i + 1, j);
+ String version = gav.substring(j + 1);
+ Matcher pm = PROPERTY_VERSION.matcher(version);
+ Matcher bm = BOM_VERSION.matcher(version);
+ if (pm.matches()) {
+ String value =
project.getProperties().getProperty(pm.group(1));
+ if (value == null) {
+ problems.add(pkg + ": no property " + pm.group(1) + " in
parent/pom.xml");
+ continue;
+ }
+ version = value;
+ } else if (bm.matches()) {
+ String bomVersion =
project.getProperties().getProperty(bm.group(3));
+ if (bomVersion == null) {
+ problems.add(pkg + ": no property " + bm.group(3) + " in
parent/pom.xml");
+ continue;
+ }
+ String bomKey = bm.group(1) + ":" + bm.group(2) + ":" +
bomVersion;
+ version = managedVersion(boms, bomKey, groupId, artifactId);
+ if (version == null) {
+ problems.add(pkg + ": " + groupId + ":" + artifactId + "
is not managed by " + bomKey);
+ continue;
+ }
+ } else {
+ problems.add(pkg + " = " + gav + ": a literal version; add a
<" + artifactId.toLowerCase(Locale.ROOT)
+ + "-version> property to parent/pom.xml and use
${...}");
+ continue;
+ }
+ resolved.put(pkg, groupId + ":" + artifactId + ":" + version);
+ }
+ if (!problems.isEmpty()) {
+ throw new MojoFailureException("Problems in " + input + ":\n " +
String.join("\n ", problems));
+ }
+ if (verifyThirdPartyJars) {
+ verifyPackagesInJars(resolved);
+ }
+ List<String> lines = new ArrayList<>();
+ lines.add("# Generated by
camel-package-maven-plugin:prepare-kamelet-main from src/main/"
+ + input.getName() + " (CAMEL-24809). Do not edit.");
+ lines.add("# Third-party libraries camel run downloads on demand,
mapped by package; the resolver matches the class"
+ + " and then each enclosing package.");
+ for (Map.Entry<String, String> e : resolved.entrySet()) {
+ lines.add(e.getKey() + " = " + e.getValue());
+ }
+ try (InputStream is =
getClass().getClassLoader().getResourceAsStream("license-header.txt")) {
+ this.licenseHeader = loadText(is);
+ }
Review Comment:
⚠️ **Silent failure: `license-header.txt` load has no catch block**
The two sibling methods (`updateKnownDependencies`,
`updateKnownFactoryFinders`) both wrap this same `loadText` call in a `catch
(Exception e)` that throws a `MojoFailureException` with context. Here the
`try-with-resources` has no catch — if `loadText` throws (e.g. resource not
found on classpath, `IOError` from malformed bytes), it propagates as a raw
unchecked exception with no Maven-readable message.
Bring this in line with the siblings:
```suggestion
try (InputStream is =
getClass().getClassLoader().getResourceAsStream("license-header.txt")) {
this.licenseHeader = loadText(is);
} catch (Exception e) {
throw new MojoFailureException("Error loading license-header.txt
file", e);
}
```
--
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]
