gnodet-bot commented on code in PR #26575:
URL: https://github.com/apache/camel/pull/26575#discussion_r4045259475
##########
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 typo in a property
placeholder (e.g. `${pgjdbc-driver-versio}`) or a missing property in
`parent/pom.xml` would have `mavenGavForClass` return a `MavenGav` with the
literal placeholder as its version — and this test still passes, because
version is never compared. The test loads the **pre-generated** file (with
versions already resolved), so it's exactly the right place to assert that no
placeholder leaked through.
```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:
❌ **Prior finding not addressed — `license-header.txt` load has no catch
block**
The two sibling methods (`updateKnownDependencies` at line 359,
`updateKnownFactoryFinders` at line 380) both wrap this same
`getResourceAsStream` + `loadText` call in a `catch (Exception e)` that throws
a `MojoFailureException` with a readable message. Here the `try-with-resources`
has no catch — if `loadText` throws (resource not found on classpath,
`IOError`), it propagates as a raw unchecked exception with no Maven-readable
context.
```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]
