gnodet-bot commented on code in PR #26574:
URL: https://github.com/apache/camel/pull/26574#discussion_r4045125685
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/BeanRefChecks.java:
##########
@@ -340,6 +345,48 @@ 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,
camel-component-known-dependencies.properties and the generated
+ * camel-thirdparty-known-dependencies.properties of CAMEL-24809), 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.
+ */
Review Comment:
⚠️ **Orphaned Javadoc — still not fixed**
The old `/** A class named with its package… */` (lines 344–347) was the
Javadoc for `classNotFound()`. The new field and its own Javadoc were inserted
between that comment and its method, so the old comment still floats detached
from everything — it now documents neither the field nor `classNotFound()`.
Fix: move the old comment down so it sits directly above `static String
classNotFound(…)`, and keep the field's Javadoc above the field.
```suggestion
* The classes camel run resolves to a Maven dependency and downloads on
demand (camel-kamelet-main's
* camel-main-known-dependencies.properties,
camel-component-known-dependencies.properties and the generated
* camel-thirdparty-known-dependencies.properties of CAMEL-24809), 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.
*/
```
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/BeanRefChecks.java:
##########
@@ -340,6 +345,48 @@ 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,
camel-component-known-dependencies.properties and the generated
+ * camel-thirdparty-known-dependencies.properties of CAMEL-24809), 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:
⚠️ **Data race — comment still missing**
The lock-free lazy initialiser (`volatile` field, no `synchronized`) is
correct but not obviously so: two threads can both read `null`, both build a
`HashMap` independently, and the last `volatile` write wins — the maps are
identical so correctness holds.
Without a comment, every static analyser and future reviewer will flag this
as an unsafe publication. Add:
```suggestion
// Intentional benign race: two threads may both initialise this; the
last volatile-write wins.
// Correctness holds because the maps are built from immutable classpath
resources and are identical.
private static volatile Map<String, String> knownDependencies;
```
##########
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();
Review Comment:
⚠️ **Missing fallback test — still not added**
The existing test asserts `.startsWith("org.postgresql:postgresql")` — it
only passes because the properties files are on the test classpath. The `catch
(Exception e) { /* optimisation */ }` path (files absent from classpath) is
never exercised.
Add a test that resets the cache and calls `knownDependency()` in an
isolated classloader (or just with a thread-context classloader that has no
properties files). Without it, a deployment that somehow excludes
`camel-kamelet-main` at runtime will produce a `NullPointerException` or wrong
behaviour and no test will catch the regression.
--
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]
