gnodet-bot commented on code in PR #27514:
URL: https://github.com/apache/camel/pull/27514#discussion_r4211411310
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/RunHelper.java:
##########
@@ -90,12 +96,35 @@ public static RuntimeType detectRuntimeFromPom(Path
pomPath) {
}
}
}
+ // the Maven plugin of the runtime builds the application
+ if (model.getBuild() != null) {
+ for (Plugin plugin : model.getBuild().getPlugins()) {
+ String a = resolveProperty(model, plugin.getArtifactId());
+ if ("quarkus-maven-plugin".equals(a)) {
+ return RuntimeType.quarkus;
+ }
+ if ("spring-boot-maven-plugin".equals(a)) {
+ return RuntimeType.springBoot;
+ }
+ }
+ }
} catch (Exception e) {
// ignore
}
return null;
}
+ /** The value with a ${name} property of the pom resolved, as Maven would;
as written when the pom has none. */
+ static String resolveProperty(Model model, String value) {
+ if (value != null && value.startsWith("${") && value.endsWith("}") &&
model.getProperties() != null) {
+ String resolved =
model.getProperties().getProperty(value.substring(2, value.length() - 1));
+ if (resolved != null) {
+ return resolveProperty(model, resolved);
Review Comment:
⚠️ **Bug: `resolveProperty` can cause `StackOverflowError` on circular
property references**
The method recurses without a depth limit or cycle guard:
```java
static String resolveProperty(Model model, String value) {
if (value != null && value.startsWith("${") && value.endsWith("}") &&
model.getProperties() != null) {
String resolved =
model.getProperties().getProperty(value.substring(2, value.length() - 1));
if (resolved != null) {
return resolveProperty(model, resolved); // unbounded recursion
}
}
return value;
}
```
If a POM has circular properties (e.g. `<foo>${bar}</foo>` and
`<bar>${foo}</bar>`), this blows the stack. `StackOverflowError` extends
`Error`, not `Exception`, so the `catch (Exception e)` at line 111 does **not**
catch it — the CLI crashes instead.
Fix with a visited-set or a depth limit:
```suggestion
static String resolveProperty(Model model, String value) {
return resolveProperty(model, value, new java.util.HashSet<>());
}
private static String resolveProperty(Model model, String value,
java.util.Set<String> seen) {
if (value != null && value.startsWith("${") && value.endsWith("}")
&& model.getProperties() != null) {
String key = value.substring(2, value.length() - 1);
if (seen.add(key)) {
String resolved = model.getProperties().getProperty(key);
if (resolved != null) {
return resolveProperty(model, resolved, seen);
}
}
}
return value;
}
```
##########
dsl/camel-jbang/camel-jbang-plugin-tui/src/main/java/org/apache/camel/dsl/jbang/core/commands/tui/LaunchManager.java:
##########
@@ -342,7 +354,7 @@ private void checkDeferredLaunch(long now) {
.filter(i -> i.alive)
.map(i -> i.alias)
.collect(Collectors.toSet());
- if (runningAliases.containsAll(deferredLaunch.requiredInfra)) {
+ if
(deferredLaunch.requiredInfra.stream().map(LaunchManager::serviceOf).allMatch(runningAliases::contains))
{
Review Comment:
💡 **Missing test for `checkDeferredLaunch`**
The same `serviceOf`-mapping fix was applied here (line 357), but
`LaunchManagerInfraTest` only covers `findMissingInfraServices` and `serviceOf`
directly. There is no test that exercises the deferred-launch path where `"aws
sqs"` is required and an `"aws"` service becomes live — meaning a regression
here wouldn't be caught.
Consider adding a test that sets up a deferred launch, runs the
infra-becomes-live scenario, and asserts the deferred action fires.
--
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]
