gnodet-bot commented on code in PR #29805:
URL: https://github.com/apache/camel/pull/29805#discussion_r4568757384
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/plugin/PluginBaseCommand.java:
##########
@@ -56,11 +57,16 @@ private Path pluginConfigPath() {
}
JsonObject loadConfig() {
- Path path = pluginConfigPath();
- if (path != null) {
- return PluginHelper.getOrCreatePluginConfig(path);
+ try {
+ Path path = pluginConfigPath();
+ if (path != null) {
+ return PluginHelper.getOrCreatePluginConfig(path);
+ }
+ return PluginHelper.getOrCreatePluginConfig();
+ } catch (RuntimeCamelException e) {
+ printer().printErr(e.getMessage() + ". Fix the JSON by hand or
delete the file.");
+ throw e;
Review Comment:
⚠️ **Still open (davsclaus item 2):** `loadConfig()` catches
`RuntimeCamelException`, prints the user-friendly message, then re-throws it.
The `CliExecutionExceptionHandler` maps `RuntimeCamelException` to `INTERNAL`
(exit code 70) and dumps a stack trace — the user sees the friendly message
*and* a stack trace. Using `CliException.failure(...)` instead gives exit code
1 with no stack trace (unless `--verbose`), which is the right UX for a known,
user-actionable failure.
```suggestion
} catch (RuntimeCamelException e) {
throw CliException.failure(e.getMessage() + ". Fix the JSON by
hand or delete the file.");
}
```
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/common/PluginHelper.java:
##########
@@ -813,33 +852,81 @@ 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, falls back to a plain overwriting move (rename(2)), which
is still safe — a reader never observes a
+ * half-written file.
+ */
+ private static void atomicWriteJson(Path target, String content) throws
IOException {
+ // Resolve symlinks so the move replaces the target file, not the
symlink itself.
+ Path realTarget;
+ try {
+ realTarget = target.toRealPath();
+ } catch (IOException e) {
+ // file does not exist yet — no symlink to resolve
+ realTarget = target;
+ }
+ Path dir = realTarget.getParent();
+ if (dir != null) {
+ Files.createDirectories(dir);
+ }
+ Path tmp = dir != null
+ ? Files.createTempFile(dir, "." + realTarget.getFileName(),
".tmp")
+ : Files.createTempFile("." + realTarget.getFileName(), ".tmp");
+ try {
+ Files.writeString(tmp, content, StandardOpenOption.WRITE,
StandardOpenOption.TRUNCATE_EXISTING);
+ try {
+ Files.move(tmp, realTarget,
StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE);
+ tmp = null; // moved successfully, nothing to delete
+ } catch (AtomicMoveNotSupportedException e) {
+ // fall back: non-atomic replace — still better than
truncate-then-write
+ Files.move(tmp, realTarget,
StandardCopyOption.REPLACE_EXISTING);
+ tmp = null;
+ }
+ } finally {
+ if (tmp != null) {
+ try {
+ Files.deleteIfExists(tmp);
+ } catch (IOException ignored) {
+ // best effort
+ }
+ }
+ }
+ }
+
public static void savePluginConfig(JsonObject plugins) {
savePluginConfig(plugins, null);
}
public static void savePluginConfig(JsonObject plugins, Path configPath) {
Path f = getCustomPluginConfigPath(configPath);
try {
- Files.writeString(f, plugins.toJson(),
- StandardOpenOption.CREATE,
- StandardOpenOption.WRITE,
- StandardOpenOption.TRUNCATE_EXISTING);
+ atomicWriteJson(f, plugins.toJson());
} catch (IOException e) {
throw new RuntimeCamelException("Failed to save plugin
configuration", e);
}
}
+ /**
+ * Warns about a corrupt or unreadable plugin configuration file, once per
path per JVM run, so multiple callers
+ * (e.g. {@code getActivePlugins} and {@code printAvailablePlugins}) do
not repeat the message.
+ */
+ public static void warnCorruptPluginConfig(Printer printer, Exception e) {
+ String key = e.getMessage();
+ if (WARNED_PLUGIN_PATHS.add(key)) {
Review Comment:
💡 **Low:** `warnCorruptPluginConfig` uses `e.getMessage()` as the dedup key
for `WARNED_PLUGIN_PATHS`. This works because `getPluginConfig`'s
`RuntimeCamelException` always embeds the path, but an unexpected exception
with a `null` message would throw NPE on
`ConcurrentHashMap.newKeySet().add(null)`. A null guard makes the intent
explicit and matches the Javadoc's "once per path per JVM run" description.
```suggestion
public static void warnCorruptPluginConfig(Printer printer, Exception e)
{
String key = e.getMessage() != null ? e.getMessage() :
"unknown-corrupt-plugin-config";
if (WARNED_PLUGIN_PATHS.add(key)) {
printer.printErr("Warning: " + e.getMessage() + ". Use 'camel
plugin' commands to fix or delete the file.");
}
}
```
##########
dsl/camel-jbang/camel-launcher/src/main/resources/bin/camel.bat:
##########
@@ -83,7 +83,14 @@ if "!_CDS_REST!"=="." set "_CDS_CMD=!_CDS_ARG!"
:cdsCommandDone
for %%j in ("!LAUNCHER_JAR!") do set "_CDS_VERSION=%%~nj"
set "_CDS_VERSION=!_CDS_VERSION:camel-launcher-=!"
-set "_CDS_DIR=!LOCALAPPDATA!\camel-cli\cds\!_CDS_VERSION!"
+@REM Include a hash of the install directory so two installs of the same
version get separate archives.
+@REM Different install methods (SDKMAN, zip) place the launcher in different
directories;
+@REM a shared archive from another install points at the wrong JAR or returns
wrong install metadata.
+@REM Pass BASEDIR via environment to avoid trailing-backslash breaking
PowerShell string quoting.
+set "_CDS_PS_INPUT=!BASEDIR!"
+for /f "delims=" %%h in ('powershell -NoProfile -Command
"$b=[System.Text.Encoding]::UTF8.GetBytes($env:_CDS_PS_INPUT);$h=[System.Security.Cryptography.SHA256]::Create().ComputeHash($b);($h[0..3]|ForEach-Object{$_.ToString(\"x2\")})
-join \"\""') do set "_CDS_INSTALL_HASH=%%h"
Review Comment:
💡 **Low (davsclaus item 5, partial):** PowerShell stderr is not suppressed.
If `WindowsPowerShell\v1.0` is not on PATH (e.g., PowerShell 7+ only
installations), cmd will print `'powershell' is not recognized as an internal
or external command...` on every `camel` invocation. The `if not defined
_CDS_INSTALL_HASH` fallback handles the case silently for the hash itself, but
the error still appears on the console. Add `2>nul` after the PowerShell
command to suppress it. Consider also using the full path
`%SystemRoot%\System32\WindowsPowerShell\v1.0\powershell.exe` with an `if
exist` guard as davsclaus suggested, to make the invocation predictable.
--
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]
