gnodet-bot commented on code in PR #26574:
URL: https://github.com/apache/camel/pull/26574#discussion_r4044970563
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/BeanRefChecks.java:
##########
@@ -340,6 +345,46 @@ static int indentOf(String line) {
* A class named with its package that is neither next to the route nor on
the CLI classpath: the wrong package
* (org.apache.camel.support.StringAggregationStrategy) or a missing
dependency. Null when the class is fine.
*/
+ /**
+ * The classes camel run resolves to a Maven dependency and downloads on
demand (camel-kamelet-main's
+ * camel-main-known-dependencies.properties and
camel-component-known-dependencies.properties), so a
+ * #class:org.postgresql.ds.PGSimpleDataSource bean is fine without a
dependency declared even though the class is
+ * not on the CLI classpath. Matched the way the runtime matches: the
class name, then each enclosing package.
+ */
+ private static volatile Map<String, String> knownDependencies;
Review Comment:
⚠️ **Orphaned Javadoc / misplaced comment**
The existing `/** A class named with its package... */` comment immediately
above (lines 345–347) was the Javadoc for `classNotFound()`. The new code
inserts the `knownDependencies` field (with its own Javadoc block) between that
comment and its method, so the old comment now floats as an unattached Javadoc
— it documents nothing and will confuse anyone reading the file.
Fix: move the old comment down so it sits directly above `static String
classNotFound(...)`, or fold the two Javadoc blocks into one (since they're
adjacent concepts). Currently `classNotFound` ends up with no Javadoc at all.
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/BeanRefChecks.java:
##########
@@ -340,6 +345,46 @@ static int indentOf(String line) {
* A class named with its package that is neither next to the route nor on
the CLI classpath: the wrong package
* (org.apache.camel.support.StringAggregationStrategy) or a missing
dependency. Null when the class is fine.
*/
+ /**
+ * The classes camel run resolves to a Maven dependency and downloads on
demand (camel-kamelet-main's
+ * camel-main-known-dependencies.properties and
camel-component-known-dependencies.properties), so a
+ * #class:org.postgresql.ds.PGSimpleDataSource bean is fine without a
dependency declared even though the class is
+ * not on the CLI classpath. Matched the way the runtime matches: the
class name, then each enclosing package.
+ */
+ private static volatile Map<String, String> knownDependencies;
+
+ static String knownDependency(String fqcn) {
+ Map<String, String> known = knownDependencies;
+ if (known == null) {
+ known = new HashMap<>();
+ for (String name : new String[] {
+ "camel-main-known-dependencies.properties",
"camel-component-known-dependencies.properties" }) {
+ try {
+ Enumeration<URL> resources =
BeanRefChecks.class.getClassLoader().getResources(name);
+ while (resources.hasMoreElements()) {
+ try (InputStream is =
resources.nextElement().openStream()) {
+ Properties prop = new Properties();
+ prop.load(is);
+ for (String key : prop.stringPropertyNames()) {
+ known.put(key, prop.getProperty(key));
+ }
+ }
+ }
+ } catch (Exception e) {
+ // the mapping is an optimisation of the message, not a
requirement
+ }
+ }
+ knownDependencies = known;
Review Comment:
⚠️ **Benign data race — needs a comment explaining the intent**
The lazy initialiser uses `volatile` on the field but no `synchronized`
block. This is intentional "safe publication without a lock": two threads can
both read `null` and both build a `HashMap` concurrently, but because the field
is `volatile`, each thread's final write is globally visible and the maps are
identical (same immutable source). The last writer wins, and correctness holds.
However, this pattern is not self-evident. Without a comment, reviewers (and
static analysers) will flag this as a data race. Add a short comment:
```suggestion
knownDependencies = known; // benign race: two threads may both
build the map; volatile guarantees safe publication
```
##########
dsl/camel-jbang/camel-jbang-core/src/test/java/org/apache/camel/dsl/jbang/core/commands/ai/SourceValidatorBeanRefsTest.java:
##########
@@ -350,6 +350,43 @@ void aClassInTheWrongPackageIsReported(@TempDir Path dir)
throws IOException {
.contains("did you mean
org.apache.camel.processor.aggregate.StringAggregationStrategy?");
}
+ @Test
+ void aClassCamelRunDownloadsIsNotReportedAsMissing(@TempDir Path dir)
throws IOException {
+ // the Postgres datasource and the Artemis connection factory are not
on the CLI classpath, but camel run
+ // resolves them to their Maven dependency
(camel-main-known-dependencies.properties) and downloads it, so a
+ // bean of that type runs; the validator must not contradict the
runtime
+ List<String> msgs = SourceValidator.validate("r.camel.yaml", """
+ - beans:
+ - name: postgresDS
+ type: "#class:org.postgresql.ds.PGSimpleDataSource"
+ properties:
+ url: "jdbc:postgresql://localhost:5432/postgres"
+ - name: artemisCF
+ type:
"#class:org.apache.activemq.artemis.jms.client.ActiveMQConnectionFactory"
+ - route:
+ from:
+ uri: "timer:tick?period=1000"
+ steps:
+ - to:
+ uri: "sql:select 1?dataSource=#postgresDS"
+ """, CATALOG, null, dir);
+ assertThat(msgs).isEmpty();
+
assertThat(BeanRefChecks.knownDependency("org.postgresql.ds.PGSimpleDataSource"))
+ .startsWith("org.postgresql:postgresql");
+
assertThat(BeanRefChecks.knownDependency("com.example.NoSuchThing")).isNull();
+ }
+
+ @Test
+ void aClassFromAnUnknownLibrarySaysHowToDeclareTheDependency(@TempDir Path
dir) throws IOException {
+ List<String> msgs = SourceValidator.validate("r.camel.yaml", """
+ - beans:
+ - name: pool
+ type: "#class:com.zaxxer.hikari.HikariDataSourceX"
+ """, CATALOG, null, dir);
+ assertThat(msgs).hasSize(1);
+ assertThat(msgs.get(0)).contains("was not
found").contains("camel.jbang.dependencies=<groupId>:<artifactId>:<version>");
+ }
+
@Test
void aSiblingClassImportedFromTheWrongPackageIsNamed(@TempDir Path dir)
throws IOException {
Review Comment:
💡 **Missing edge-case test: properties files absent from classpath**
`knownDependency()` silently returns a map built from whatever it finds — if
neither `camel-main-known-dependencies.properties` nor
`camel-component-known-dependencies.properties` is on the classpath the result
is an empty map, and the validator falls back to reporting the class as missing
(which is the pre-PR behaviour).
That fallback path is not exercised here. A quick test that calls
`BeanRefChecks.knownDependency("org.postgresql.ds.PGSimpleDataSource")` in an
isolated classloader (or simply checks that the method returns a non-null
result when the file is present, and `null` when it isn't) would guard against
a future packaging mistake that silently drops the properties file.
--
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]
