gnodet-bot commented on code in PR #27862:
URL: https://github.com/apache/camel/pull/27862#discussion_r4279969499
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/AuthoringTools.java:
##########
@@ -1070,13 +1070,117 @@ private static void putFileNotes(JsonObject result,
String file, String content)
}
}
+ private static final Pattern JAVA_PACKAGE =
Pattern.compile("^\\s*package\\s+([\\w.]+)\\s*;", Pattern.MULTILINE);
+
+ /** The package a Java source declares, or the empty string for none. */
+ static String javaPackage(String content) {
+ Matcher m = content != null ? JAVA_PACKAGE.matcher(content) : null;
+ return m != null && m.find() ? m.group(1) : "";
+ }
+
+ /**
+ * The other files of the project with the name of the file. Folders that
hold no sources of the app are left out:
+ * dot folders, build output and test sources.
+ */
+ private static List<Path> sameNamedFiles(Path dir, Path path) {
+ Path root = dir.toAbsolutePath().normalize();
+ Path self = path.toAbsolutePath().normalize();
+ String name = self.getFileName().toString();
+ List<Path> found = new ArrayList<>();
+ try {
+ Files.walkFileTree(root, EnumSet.noneOf(FileVisitOption.class),
12, new SimpleFileVisitor<>() {
+ @Override
+ public FileVisitResult preVisitDirectory(Path d,
BasicFileAttributes attrs) {
+ String n = d.getFileName() != null ?
d.getFileName().toString() : "";
+ String relative =
root.relativize(d).toString().replace('\\', '/');
+ if (!d.equals(root) && (n.startsWith(".") ||
"target".equals(n) || "build".equals(n)
+ || "node_modules".equals(n) ||
"src/test".equals(relative) || relative.endsWith("/src/test"))) {
+ return FileVisitResult.SKIP_SUBTREE;
+ }
+ return FileVisitResult.CONTINUE;
+ }
+
+ @Override
+ public FileVisitResult visitFile(Path f, BasicFileAttributes
attrs) {
+ if (attrs.isRegularFile() &&
name.equals(f.getFileName().toString())
+ && !self.equals(f.toAbsolutePath().normalize())) {
+ found.add(f);
+ }
+ return FileVisitResult.CONTINUE;
+ }
+ });
+ } catch (IOException e) {
+ // best effort: without the list the write goes ahead as before
+ }
+ return found;
+ }
+
+ /**
+ * The file of the project that already defines the class a new Java
source would define (the same package and
+ * name), relative to the project directory, or null. Asked to change a
class of a camel run project, whose sources
+ * sit next to the routes, a model wrote a second copy under src/main/java
instead, 7 times out of 11 in a benchmark
+ * (CAMEL-25673): two classes of the same name, of which the runtime uses
one, while the change is in the other.
+ */
+ static String fileOfSameClass(Path dir, Path path, String content) {
+ String pkg = javaPackage(content);
+ Path root = dir.toAbsolutePath().normalize();
+ for (Path other : sameNamedFiles(dir, path)) {
+ if (pkg.equals(javaPackage(read(other)))) {
Review Comment:
⚠️ **Bug: `read(other)` breaks best-effort semantics on IO failure**
`read(other)` (around line 1540) catches `IOException` and rethrows it as an
unchecked `ToolExecutionException`. If `other` is unreadable (e.g. permissions
issue, broken symlink, race with file deletion), that exception propagates
through `fileOfSameClass` → `writeFile`, aborting the write entirely.
Meanwhile `sameNamedFiles` wraps its own `IOException` in a best-effort
`catch` — so the two error-handling strategies are inconsistent within the same
feature.
Consider wrapping the `read(other)` call in a try/catch so an unreadable
candidate is silently skipped rather than crashing the write:
```suggestion
String otherContent;
try {
otherContent = read(other);
} catch (ToolExecutionException e) {
continue; // unreadable — skip this candidate
}
if (pkg.equals(javaPackage(otherContent))) {
```
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/AuthoringTools.java:
##########
@@ -1070,13 +1070,117 @@ private static void putFileNotes(JsonObject result,
String file, String content)
}
}
+ private static final Pattern JAVA_PACKAGE =
Pattern.compile("^\\s*package\\s+([\\w.]+)\\s*;", Pattern.MULTILINE);
+
+ /** The package a Java source declares, or the empty string for none. */
+ static String javaPackage(String content) {
+ Matcher m = content != null ? JAVA_PACKAGE.matcher(content) : null;
+ return m != null && m.find() ? m.group(1) : "";
+ }
+
+ /**
+ * The other files of the project with the name of the file. Folders that
hold no sources of the app are left out:
+ * dot folders, build output and test sources.
+ */
+ private static List<Path> sameNamedFiles(Path dir, Path path) {
+ Path root = dir.toAbsolutePath().normalize();
+ Path self = path.toAbsolutePath().normalize();
+ String name = self.getFileName().toString();
+ List<Path> found = new ArrayList<>();
+ try {
+ Files.walkFileTree(root, EnumSet.noneOf(FileVisitOption.class),
12, new SimpleFileVisitor<>() {
+ @Override
+ public FileVisitResult preVisitDirectory(Path d,
BasicFileAttributes attrs) {
+ String n = d.getFileName() != null ?
d.getFileName().toString() : "";
+ String relative =
root.relativize(d).toString().replace('\\', '/');
+ if (!d.equals(root) && (n.startsWith(".") ||
"target".equals(n) || "build".equals(n)
+ || "node_modules".equals(n) ||
"src/test".equals(relative) || relative.endsWith("/src/test"))) {
+ return FileVisitResult.SKIP_SUBTREE;
+ }
+ return FileVisitResult.CONTINUE;
+ }
+
+ @Override
+ public FileVisitResult visitFile(Path f, BasicFileAttributes
attrs) {
+ if (attrs.isRegularFile() &&
name.equals(f.getFileName().toString())
+ && !self.equals(f.toAbsolutePath().normalize())) {
+ found.add(f);
+ }
+ return FileVisitResult.CONTINUE;
+ }
+ });
+ } catch (IOException e) {
+ // best effort: without the list the write goes ahead as before
+ }
+ return found;
+ }
+
+ /**
+ * The file of the project that already defines the class a new Java
source would define (the same package and
+ * name), relative to the project directory, or null. Asked to change a
class of a camel run project, whose sources
+ * sit next to the routes, a model wrote a second copy under src/main/java
instead, 7 times out of 11 in a benchmark
+ * (CAMEL-25673): two classes of the same name, of which the runtime uses
one, while the change is in the other.
+ */
+ static String fileOfSameClass(Path dir, Path path, String content) {
+ String pkg = javaPackage(content);
+ Path root = dir.toAbsolutePath().normalize();
+ for (Path other : sameNamedFiles(dir, path)) {
+ if (pkg.equals(javaPackage(read(other)))) {
+ return
root.relativize(other.toAbsolutePath().normalize()).toString().replace('\\',
'/');
+ }
+ }
+ return null;
+ }
+
+ /**
+ * A note when a new beans.yaml or application.properties has the name of
a file the project already has in another
+ * folder: two files of that name are easy to mix up (CAMEL-25673). Null
for any other file, or when there is none.
+ */
+ static String sameNamedConfigNote(Path dir, Path path) {
+ String name = path.getFileName().toString();
+ boolean config = "application.properties".equals(name)
+ || name.startsWith("application-") &&
name.endsWith(".properties")
Review Comment:
💡 **Style: add parentheses for clarity around mixed `||` / `&&`**
The operator precedence is correct (`&&` binds tighter than `||` per JLS),
but mixing them without parentheses on a single line is easy to misread.
Explicit grouping makes the intent immediately obvious:
```suggestion
|| (name.startsWith("application-") &&
name.endsWith(".properties"))
```
--
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]
