gnodet-bot commented on code in PR #29805:
URL: https://github.com/apache/camel/pull/29805#discussion_r4565520157
##########
dsl/camel-jbang/camel-jbang-core/src/test/java/org/apache/camel/dsl/jbang/core/commands/UserConfigDefaultsTest.java:
##########
@@ -78,6 +78,68 @@ void localConfigurationTakesPrecedence() throws Exception {
assertThat(run("echo")).isEqualTo("sort=age repos=null");
}
+ @Test
+ void malformedUserConfigDoesNotCrashCli() throws Exception {
+ // A Windows-style path stored without escaping produces a malformed
\\uXXXX sequence,
+ // which throws IllegalArgumentException from Properties.load — not
IOException.
+ // The CLI must warn and continue instead of propagating the exception.
+ UserConfigHelper.createUserConfig("path=C:\\users\\name");
+
+ // The command must succeed (not throw); the option falls back to its
own default.
+ assertThat(runCapturingWarning("echo")).isEqualTo("sort=pid
repos=null");
+ }
+
+ @Test
+ void malformedConfigLoadPropertiesReturnsFalseAndWarns() throws Exception {
+ // loadProperties must not throw and must return false for a malformed
config file
+ UserConfigHelper.createUserConfig("path=C:\\users\\name");
+
+ StringPrinter out = new StringPrinter();
+ boolean[] loaded = { true };
+ loaded[0] = CommandLineHelper.loadProperties(props -> {
+ // should NOT be called since the 3-arg overload does not call
consumer on failure
Review Comment:
⚠️ **Test logic bug**: This test doesn't actually verify that the consumer
is not called.
In Java, `loaded[0] = CommandLineHelper.loadProperties(lambda, false, out)`
evaluates the full method call (including any lambda invocations) first, then
assigns the return value. So:
1. `loadProperties` runs — if it calls the lambda, `loaded[0]` is set to
`true`
2. `loadProperties` returns `false`
3. `false` is assigned to `loaded[0]`, **overwriting** whatever the lambda
set
The final `assertThat(loaded[0]).isFalse()` only verifies the return value
of `loadProperties`, not whether the consumer was called. The test comment says
`// should NOT be called` but the assertion doesn't enforce that.
A correct approach would use a separate flag:
```suggestion
boolean[] consumerCalled = { false };
loaded[0] = CommandLineHelper.loadProperties(props -> {
// should NOT be called since the 3-arg overload does not call
consumer on failure
consumerCalled[0] = true;
}, false, out);
```
Then assert `assertThat(consumerCalled[0]).as("consumer must not be called
on failure").isFalse();`
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/common/PluginHelper.java:
##########
@@ -813,28 +853,65 @@ public static JsonObject createPluginConfig(Path
configPath) {
if (parent != null) {
Files.createDirectories(parent);
}
- Files.writeString(f, config.toJson(),
- StandardOpenOption.CREATE,
- StandardOpenOption.WRITE,
- StandardOpenOption.TRUNCATE_EXISTING);
+ atomicWriteJson(f, config.toJson());
} catch (IOException e) {
throw new RuntimeCamelException("Failed to create plugin
configuration", e);
}
return config;
}
+ /**
+ * Writes {@code content} to {@code target} atomically: writes to a
sibling temp file first, then moves it into
+ * place with {@link StandardCopyOption#REPLACE_EXISTING}. When atomic
move is not supported by the underlying
+ * filesystem (e.g. across mounts), falls back to a plain overwriting
write, which is still safer than
+ * truncate-then-write because the write is a single syscall.
+ */
Review Comment:
💬 **Nitpick** (Javadoc accuracy): The fallback path description has two
minor inaccuracies:
1. `AtomicMoveNotSupportedException` is NOT thrown for cross-mount moves —
the "e.g. across mounts" example is misleading. It fires on exotic filesystems
that don't support the atomic rename flag (some NFS/FUSE configurations).
2. Since the temp file is always created in the **same directory** as the
target (`Files.createTempFile(dir, ...)`), the fallback
`Files.move(REPLACE_EXISTING)` will always use `rename(2)` — so the safety
claim is correct, but the explanation attributes it to the wrong reason.
Suggested reword: `"falls back to a plain overwriting move (rename(2)),
which is still safe — a reader never observes a half-written file."` (The "e.g.
across mounts" example should be dropped or corrected.)
--
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]
