From de537f0b1457192d07eec2348dbcc378d60b7758 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Mon, 24 Feb 2020 10:36:40 +0100 Subject: [PATCH 1/3] Throw UncheckedIOException on directory creation errors Previously, Config#mkdir and #mkAppDir threw ConfigException if any underlying IOException was thrown from Files#createDirectory or Files#createDirectories respectively. This resulted in only a simple error message being reported at the command line and the details of the underlying exception's stack trace were lost. This change wraps and rethrows the underlying IOException as an UncheckedIOException such that it gets caught by the catch(Throwable) clause in BisqExecutable, which does print a stacktrace to stderr. This is the way it always should have worked; throwing ConfigException in these cases was an oversight in the original implementation. This change also removes the ConfigException constructor that allows for passing a nested exception to be wrapped as there is actually no use case for this now that these two anomalies have been corrected. This change was made in the process of attempting to reproduce #3998. --- common/src/main/java/bisq/common/config/Config.java | 4 ++-- common/src/main/java/bisq/common/config/ConfigException.java | 4 ---- common/src/test/java/bisq/common/config/ConfigTests.java | 5 +++-- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/common/src/main/java/bisq/common/config/Config.java b/common/src/main/java/bisq/common/config/Config.java index a9fc517a215..dac6afe545c 100644 --- a/common/src/main/java/bisq/common/config/Config.java +++ b/common/src/main/java/bisq/common/config/Config.java @@ -802,7 +802,7 @@ private static File mkAppDataDir(File appDataDir) { try { Files.createDirectories(path); } catch (IOException ex) { - throw new ConfigException(ex, "Application data directory '%s' could not be created", path); + throw new UncheckedIOException(format("Application data directory '%s' could not be created", path), ex); } return appDataDir; } @@ -819,7 +819,7 @@ private static File mkdir(File parent, String child) { try { Files.createDirectory(path); } catch (IOException ex) { - throw new ConfigException(ex, "Directory '%s' could not be created", path); + throw new UncheckedIOException(format("Directory '%s' could not be created", path), ex); } } return dir; diff --git a/common/src/main/java/bisq/common/config/ConfigException.java b/common/src/main/java/bisq/common/config/ConfigException.java index 4a4e955456d..0d709dcf53e 100644 --- a/common/src/main/java/bisq/common/config/ConfigException.java +++ b/common/src/main/java/bisq/common/config/ConfigException.java @@ -7,8 +7,4 @@ public class ConfigException extends BisqException { public ConfigException(String format, Object... args) { super(format, args); } - - public ConfigException(Throwable cause, String format, Object... args) { - super(cause, format, args); - } } diff --git a/common/src/test/java/bisq/common/config/ConfigTests.java b/common/src/test/java/bisq/common/config/ConfigTests.java index de9593917be..6109342b339 100644 --- a/common/src/test/java/bisq/common/config/ConfigTests.java +++ b/common/src/test/java/bisq/common/config/ConfigTests.java @@ -7,6 +7,7 @@ import java.io.IOException; import java.io.PrintStream; import java.io.PrintWriter; +import java.io.UncheckedIOException; import org.junit.Rule; import org.junit.Test; @@ -228,10 +229,10 @@ public void whenConfigIsConstructed_thenAppDataDirAndSubdirsAreCreated() { } @Test - public void whenAppDataDirCannotBeCreated_thenConfigExceptionIsThrown() throws IOException { + public void whenAppDataDirCannotBeCreated_thenUncheckedIoExceptionIsThrown() throws IOException { // set a userDataDir that is actually a file so appDataDir cannot be created File aFile = Files.createTempFile("A", "File").toFile(); - exceptionRule.expect(ConfigException.class); + exceptionRule.expect(UncheckedIOException.class); exceptionRule.expectMessage(containsString("Application data directory")); exceptionRule.expectMessage(containsString("could not be created")); configWithOpts(opt(USER_DATA_DIR, aFile)); From 303eadec39c30b83bbd3c8e49fdec4eba7063a75 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Tue, 25 Feb 2020 11:22:11 +0100 Subject: [PATCH 2/3] Refactor Config#mkdir and #mkAppDataDir This is a pure refactoring that renames and inlines variables to tighten up and make more consistent the implementation of these two methods. It is done in preparation for a subsequent substantive change that fixes a bug. --- .../src/main/java/bisq/common/config/Config.java | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/common/src/main/java/bisq/common/config/Config.java b/common/src/main/java/bisq/common/config/Config.java index dac6afe545c..2222ce03313 100644 --- a/common/src/main/java/bisq/common/config/Config.java +++ b/common/src/main/java/bisq/common/config/Config.java @@ -797,14 +797,13 @@ private static File tempUserDataDir() { * nothing if the directory already exists. * @return the given directory, now guaranteed to exist */ - private static File mkAppDataDir(File appDataDir) { - Path path = appDataDir.toPath(); + private static File mkAppDataDir(File dir) { try { - Files.createDirectories(path); + Files.createDirectories(dir.toPath()); } catch (IOException ex) { - throw new UncheckedIOException(format("Application data directory '%s' could not be created", path), ex); + throw new UncheckedIOException(format("Application data directory '%s' could not be created", dir), ex); } - return appDataDir; + return dir; } /** @@ -815,11 +814,10 @@ private static File mkAppDataDir(File appDataDir) { private static File mkdir(File parent, String child) { File dir = new File(parent, child); if (!dir.exists()) { - Path path = dir.toPath(); try { - Files.createDirectory(path); + Files.createDirectory(dir.toPath()); } catch (IOException ex) { - throw new UncheckedIOException(format("Directory '%s' could not be created", path), ex); + throw new UncheckedIOException(format("Directory '%s' could not be created", dir), ex); } } return dir; From cbe6f192e00d56a1469c0570d585a15d48779552 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Tue, 25 Feb 2020 11:32:38 +0100 Subject: [PATCH 3/3] Do not attempt to create already existing appDataDir This change fixes #3998 by avoiding the attempt to create a user's appDataDir if it already exists. Normally this attempt is not a problem, as Files#createDirectories does nothing if the target is a directory that already exists, but in the case that the target is a symbolic link, Files#createDirectories throws a FileAlreadyExistsException. This change prevents the call to Files#createDirectories if the target already exists, and if in fact the target is a symbolic link pointing to an existing directory, this is not a problem and everything proceeds as per usual. This mimics the behavior in Bisq v1.2.5 and prior, where File#mkdirs was used instead of Files#createDirectories. File#mkdirs does not throw an exception when the target directory is a symlink, it just returns false. --- .../main/java/bisq/common/config/Config.java | 10 ++++++---- .../java/bisq/common/config/ConfigTests.java | 17 +++++++++++++++++ 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/common/src/main/java/bisq/common/config/Config.java b/common/src/main/java/bisq/common/config/Config.java index 2222ce03313..fc34b80b98f 100644 --- a/common/src/main/java/bisq/common/config/Config.java +++ b/common/src/main/java/bisq/common/config/Config.java @@ -798,10 +798,12 @@ private static File tempUserDataDir() { * @return the given directory, now guaranteed to exist */ private static File mkAppDataDir(File dir) { - try { - Files.createDirectories(dir.toPath()); - } catch (IOException ex) { - throw new UncheckedIOException(format("Application data directory '%s' could not be created", dir), ex); + if (!dir.exists()) { + try { + Files.createDirectories(dir.toPath()); + } catch (IOException ex) { + throw new UncheckedIOException(format("Application data directory '%s' could not be created", dir), ex); + } } return dir; } diff --git a/common/src/test/java/bisq/common/config/ConfigTests.java b/common/src/test/java/bisq/common/config/ConfigTests.java index 6109342b339..fbc40151939 100644 --- a/common/src/test/java/bisq/common/config/ConfigTests.java +++ b/common/src/test/java/bisq/common/config/ConfigTests.java @@ -1,6 +1,7 @@ package bisq.common.config; import java.nio.file.Files; +import java.nio.file.Path; import java.io.ByteArrayOutputStream; import java.io.File; @@ -238,6 +239,22 @@ public void whenAppDataDirCannotBeCreated_thenUncheckedIoExceptionIsThrown() thr configWithOpts(opt(USER_DATA_DIR, aFile)); } + @Test + public void whenAppDataDirIsSymbolicLink_thenAppDataDirCreationIsNoOp() throws IOException { + Path parentDir = Files.createTempDirectory("parent"); + Path targetDir = parentDir.resolve("target"); + Path symlink = parentDir.resolve("symlink"); + Files.createDirectory(targetDir); + try { + Files.createSymbolicLink(symlink, targetDir); + } catch (Throwable ex) { + // An error occurred trying to create a symbolic link, likely because the + // operating system (e.g. Windows) does not support it, so we abort the test. + return; + } + configWithOpts(opt(APP_DATA_DIR, symlink)); + } + // == TEST SUPPORT FACILITIES ========================================================