From bbc74b384d733a65083ed7fc782f9c558d94f8ac Mon Sep 17 00:00:00 2001 From: Alexander Milster Date: Thu, 18 Apr 2024 09:56:16 +0200 Subject: [PATCH 1/8] Avoided overwriting of files --- cli/src/main/java/de/jplag/cli/CLI.java | 35 ++++++++++++++++++- .../java/de/jplag/cli/options/CliOptions.java | 3 ++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/cli/src/main/java/de/jplag/cli/CLI.java b/cli/src/main/java/de/jplag/cli/CLI.java index e64b81118..b04b727f1 100644 --- a/cli/src/main/java/de/jplag/cli/CLI.java +++ b/cli/src/main/java/de/jplag/cli/CLI.java @@ -25,6 +25,10 @@ public final class CLI { private static final Logger logger = LoggerFactory.getLogger(CLI.class); private static final String DEFAULT_FILE_ENDING = ".zip"; + private static final int NAME_COLLISION_TRIES = 4; + + private static final String OUTPUT_FILE_EXISTS = "The output file (also with suffixes e.g. results(1).zip) already exists. You can use --overwrite to overwrite the file."; + private static final String OUTPUT_FILE_NOT_WRITABLE = "The output file (%s) cannot be written to."; private final CliInputHandler inputHandler; @@ -85,11 +89,12 @@ public boolean executeCliAndHandleErrors() { * @throws FileNotFoundException If the file could not be written */ public File runJPlag() throws ExitException, FileNotFoundException { + File target = new File(getWritableFileName()); + JPlagOptionsBuilder optionsBuilder = new JPlagOptionsBuilder(this.inputHandler); JPlagOptions options = optionsBuilder.buildOptions(); JPlagResult result = JPlagRunner.runJPlag(options); - File target = new File(getResultFilePath()); OutputFileGenerator.generateJPlagResultZip(result, target); OutputFileGenerator.generateCsvOutput(result, new File(getResultFileBaseName()), this.inputHandler.getCliOptions()); @@ -126,6 +131,34 @@ private String getResultFileBaseName() { return defaultOutputFile.substring(0, defaultOutputFile.length() - DEFAULT_FILE_ENDING.length()); } + private String getOffsetFileName(int offset) { + if (offset <= 0) { + return getResultFilePath(); + } else { + return getResultFileBaseName() + "(" + offset + ")" + DEFAULT_FILE_ENDING; + } + } + + private String getWritableFileName() throws CliException { + int retryAttempt = 0; + while (!this.inputHandler.getCliOptions().advanced.overwrite && new File(getOffsetFileName(retryAttempt)).exists() + && retryAttempt < NAME_COLLISION_TRIES) { + retryAttempt++; + } + + String targetFileName = this.getOffsetFileName(retryAttempt); + File targetFile = new File(targetFileName); + if (!this.inputHandler.getCliOptions().advanced.overwrite && targetFile.exists()) { + throw new CliException(OUTPUT_FILE_EXISTS); + } + + if (!(targetFile.canWrite() || (!targetFile.exists() && targetFile.getAbsoluteFile().getParentFile().canWrite()))) { + throw new CliException(String.format(OUTPUT_FILE_NOT_WRITABLE, targetFileName)); + } + + return targetFileName; + } + public static void main(String[] args) { CLI cli = new CLI(args); if (cli.executeCliAndHandleErrors()) { diff --git a/cli/src/main/java/de/jplag/cli/options/CliOptions.java b/cli/src/main/java/de/jplag/cli/options/CliOptions.java index 999cd5635..19fa47a68 100644 --- a/cli/src/main/java/de/jplag/cli/options/CliOptions.java +++ b/cli/src/main/java/de/jplag/cli/options/CliOptions.java @@ -99,6 +99,9 @@ public static class Advanced { @Option(names = "--csv-export", description = "Export pairwise similarity values as a CSV file.") public boolean csvExport = false; + + @Option(names = "--overwrite", description = "If this option is specified and a result file already exists it will be overwritten.") + public boolean overwrite = false; } public static class Clustering { From 5edbad1ae00c5c5daa4004bc4caa49f4b2656065 Mon Sep 17 00:00:00 2001 From: Alexander Milster Date: Thu, 2 May 2024 09:40:00 +0200 Subject: [PATCH 2/8] Improved readability --- cli/src/main/java/de/jplag/cli/CLI.java | 3 ++- .../java/de/jplag/cli/options/CliOptions.java | 2 +- .../main/java/de/jplag/util/FileUtils.java | 22 +++++++++++++++++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/cli/src/main/java/de/jplag/cli/CLI.java b/cli/src/main/java/de/jplag/cli/CLI.java index b04b727f1..5f5f8d9a7 100644 --- a/cli/src/main/java/de/jplag/cli/CLI.java +++ b/cli/src/main/java/de/jplag/cli/CLI.java @@ -16,6 +16,7 @@ import de.jplag.exceptions.ExitException; import de.jplag.logging.ProgressBarLogger; import de.jplag.options.JPlagOptions; +import de.jplag.util.FileUtils; /** * Command line interface class, allows using via command line. @@ -152,7 +153,7 @@ private String getWritableFileName() throws CliException { throw new CliException(OUTPUT_FILE_EXISTS); } - if (!(targetFile.canWrite() || (!targetFile.exists() && targetFile.getAbsoluteFile().getParentFile().canWrite()))) { + if (FileUtils.checkWritable(targetFile)) { throw new CliException(String.format(OUTPUT_FILE_NOT_WRITABLE, targetFileName)); } diff --git a/cli/src/main/java/de/jplag/cli/options/CliOptions.java b/cli/src/main/java/de/jplag/cli/options/CliOptions.java index 19fa47a68..4ef2200f8 100644 --- a/cli/src/main/java/de/jplag/cli/options/CliOptions.java +++ b/cli/src/main/java/de/jplag/cli/options/CliOptions.java @@ -100,7 +100,7 @@ public static class Advanced { @Option(names = "--csv-export", description = "Export pairwise similarity values as a CSV file.") public boolean csvExport = false; - @Option(names = "--overwrite", description = "If this option is specified and a result file already exists it will be overwritten.") + @Option(names = "--overwrite", description = "Existing result files will be overwritten.") public boolean overwrite = false; } diff --git a/language-api/src/main/java/de/jplag/util/FileUtils.java b/language-api/src/main/java/de/jplag/util/FileUtils.java index 5ce3c62b6..1f645df75 100644 --- a/language-api/src/main/java/de/jplag/util/FileUtils.java +++ b/language-api/src/main/java/de/jplag/util/FileUtils.java @@ -178,4 +178,26 @@ public static void write(File file, String content) throws IOException { writer.write(content); writer.close(); } + + /** + * Checks if the given file can be written to. If the file does not exist checks if it can be created. + * @param file The file to check + * @return true, if the file can be written to + */ + public static boolean checkWritable(File file) { + if (file.exists()) { + return file.canWrite(); + } else { + return checkParentWritable(file); + } + } + + /** + * Checks if the parent file can be written to. + * @param file The file to check + * @return true, if the parent can be written to + */ + public static boolean checkParentWritable(File file) { + return file.getAbsoluteFile().getParentFile().canWrite(); + } } From 9dd7971c71a3d642c0888171da75108d6dd2fb25 Mon Sep 17 00:00:00 2001 From: Alexander Milster Date: Thu, 2 May 2024 10:36:16 +0200 Subject: [PATCH 3/8] Fixed writable file condition --- cli/src/main/java/de/jplag/cli/CLI.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/src/main/java/de/jplag/cli/CLI.java b/cli/src/main/java/de/jplag/cli/CLI.java index 5f5f8d9a7..d4504b796 100644 --- a/cli/src/main/java/de/jplag/cli/CLI.java +++ b/cli/src/main/java/de/jplag/cli/CLI.java @@ -153,7 +153,7 @@ private String getWritableFileName() throws CliException { throw new CliException(OUTPUT_FILE_EXISTS); } - if (FileUtils.checkWritable(targetFile)) { + if (!FileUtils.checkWritable(targetFile)) { throw new CliException(String.format(OUTPUT_FILE_NOT_WRITABLE, targetFileName)); } From 66b2abcc289a539a0d9eb868932c5d23c3f28bef Mon Sep 17 00:00:00 2001 From: Alexander Milster Date: Thu, 2 May 2024 10:43:22 +0200 Subject: [PATCH 4/8] Added simple test for checking if the result file is writable. --- .../java/de/jplag/cli/ArgumentBuilder.java | 20 ++++ .../cli/CheckResultFileWritableTest.java | 96 +++++++++++++++++++ 2 files changed, 116 insertions(+) create mode 100644 cli/src/test/java/de/jplag/cli/CheckResultFileWritableTest.java diff --git a/cli/src/test/java/de/jplag/cli/ArgumentBuilder.java b/cli/src/test/java/de/jplag/cli/ArgumentBuilder.java index 49848a96b..305331b1c 100644 --- a/cli/src/test/java/de/jplag/cli/ArgumentBuilder.java +++ b/cli/src/test/java/de/jplag/cli/ArgumentBuilder.java @@ -168,6 +168,26 @@ public ArgumentBuilder shownComparisons(String value) { return this; } + /** + * Sets the result file + * @param path The path to the result file + * @return self reference + */ + public ArgumentBuilder resultFile(String path) { + this.arguments.add("-r"); + this.arguments.add(path); + return this; + } + + /** + * Adds the overwrite argument + * @return self reference + */ + public ArgumentBuilder overwrite() { + this.arguments.add("--overwrite"); + return this; + } + /** * Sets the shown comparisons option * @param value The option value diff --git a/cli/src/test/java/de/jplag/cli/CheckResultFileWritableTest.java b/cli/src/test/java/de/jplag/cli/CheckResultFileWritableTest.java new file mode 100644 index 000000000..f5908397a --- /dev/null +++ b/cli/src/test/java/de/jplag/cli/CheckResultFileWritableTest.java @@ -0,0 +1,96 @@ +package de.jplag.cli; + +import java.io.File; +import java.io.IOException; +import java.lang.reflect.Field; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.nio.file.Files; + +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Assumptions; + +import de.jplag.cli.picocli.CliInputHandler; + +public class CheckResultFileWritableTest extends CommandLineInterfaceTest { + private static Field inputHandlerField; + private static Method getWritableFileMethod; + + @BeforeClass + public static void setup() throws NoSuchFieldException, NoSuchMethodException { + Class cliClass = CLI.class; + inputHandlerField = cliClass.getDeclaredField("inputHandler"); + getWritableFileMethod = cliClass.getDeclaredMethod("getWritableFileName"); + + inputHandlerField.setAccessible(true); + getWritableFileMethod.setAccessible(true); + } + + @Test + public void testNonExistingWritableFile() throws Throwable { + File directory = Files.createTempDirectory("JPlagTest").toFile(); + File targetFile = new File(directory, "results.zip"); + + String path = runResultFileCheck(defaultArguments().resultFile(targetFile.getAbsolutePath())); + Assertions.assertEquals(targetFile.getAbsolutePath(), path); + } + + @Test + public void testNonExistingNotWritableFile() throws IOException { + File directory = Files.createTempDirectory("JPlagTest").toFile(); + Assumptions.assumeTrue(directory.setWritable(false)); + File targetFile = new File(directory, "results.zip"); + + Assertions.assertThrows(CliException.class, () -> { + runResultFileCheck(defaultArguments().resultFile(targetFile.getAbsolutePath())); + }); + } + + @Test + public void testExistingFile() throws Throwable { + File directory = Files.createTempDirectory("JPlagTest").toFile(); + File targetFile = new File(directory, "results.zip"); + Assumptions.assumeTrue(targetFile.createNewFile()); + + String path = runResultFileCheck(defaultArguments().resultFile(targetFile.getAbsolutePath())); + Assertions.assertEquals(new File(directory, "results(1).zip").getAbsolutePath(), path); + } + + @Test + public void testExistingFileOverwrite() throws Throwable { + File directory = Files.createTempDirectory("JPlagTest").toFile(); + File targetFile = new File(directory, "results.zip"); + Assumptions.assumeTrue(targetFile.createNewFile()); + + String path = runResultFileCheck(defaultArguments().resultFile(targetFile.getAbsolutePath()).overwrite()); + Assertions.assertEquals(targetFile.getAbsolutePath(), path); + } + + @Test + public void testExistingNotWritableFile() throws IOException { + File directory = Files.createTempDirectory("JPlagTest").toFile(); + File targetFile = new File(directory, "results.zip"); + Assumptions.assumeTrue(targetFile.createNewFile()); + Assumptions.assumeTrue(targetFile.setWritable(false)); + + Assertions.assertThrows(CliException.class, () -> { + runResultFileCheck(defaultArguments().resultFile(targetFile.getAbsolutePath()).overwrite()); + }); + } + + private String runResultFileCheck(ArgumentBuilder builder) throws Throwable { + String[] args = builder.getArgumentsAsArray(); + CLI cli = new CLI(args); + + CliInputHandler inputHandler = (CliInputHandler) inputHandlerField.get(cli); + inputHandler.parse(); + + try { + return (String) getWritableFileMethod.invoke(cli); + } catch (InvocationTargetException e) { + throw e.getCause(); + } + } +} From 2be252d2ad5d44b6e789bf0c3f39fc1da2564e13 Mon Sep 17 00:00:00 2001 From: Alexander Milster Date: Fri, 3 May 2024 09:58:06 +0200 Subject: [PATCH 5/8] Wrong JUnit imports fixed --- .../test/java/de/jplag/cli/CheckResultFileWritableTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cli/src/test/java/de/jplag/cli/CheckResultFileWritableTest.java b/cli/src/test/java/de/jplag/cli/CheckResultFileWritableTest.java index f5908397a..5965220bc 100644 --- a/cli/src/test/java/de/jplag/cli/CheckResultFileWritableTest.java +++ b/cli/src/test/java/de/jplag/cli/CheckResultFileWritableTest.java @@ -7,10 +7,10 @@ import java.lang.reflect.Method; import java.nio.file.Files; -import org.junit.BeforeClass; -import org.junit.Test; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Assumptions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; import de.jplag.cli.picocli.CliInputHandler; @@ -18,7 +18,7 @@ public class CheckResultFileWritableTest extends CommandLineInterfaceTest { private static Field inputHandlerField; private static Method getWritableFileMethod; - @BeforeClass + @BeforeAll public static void setup() throws NoSuchFieldException, NoSuchMethodException { Class cliClass = CLI.class; inputHandlerField = cliClass.getDeclaredField("inputHandler"); From ed910f1c3201ef155dfe1d1388f5125093bbe21b Mon Sep 17 00:00:00 2001 From: TwoOfTwelve Date: Sat, 4 May 2024 14:47:08 +0200 Subject: [PATCH 6/8] Improved constant name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Timur Sağlam --- cli/src/main/java/de/jplag/cli/CLI.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/src/main/java/de/jplag/cli/CLI.java b/cli/src/main/java/de/jplag/cli/CLI.java index d4504b796..5e5359033 100644 --- a/cli/src/main/java/de/jplag/cli/CLI.java +++ b/cli/src/main/java/de/jplag/cli/CLI.java @@ -26,7 +26,7 @@ public final class CLI { private static final Logger logger = LoggerFactory.getLogger(CLI.class); private static final String DEFAULT_FILE_ENDING = ".zip"; - private static final int NAME_COLLISION_TRIES = 4; + private static final int NAME_COLLISION_ATTEMPTS = 4; private static final String OUTPUT_FILE_EXISTS = "The output file (also with suffixes e.g. results(1).zip) already exists. You can use --overwrite to overwrite the file."; private static final String OUTPUT_FILE_NOT_WRITABLE = "The output file (%s) cannot be written to."; From e0ed4cc0c2688229c5755a5c42cb44514f852c44 Mon Sep 17 00:00:00 2001 From: Alexander Milster Date: Wed, 8 May 2024 11:53:52 +0200 Subject: [PATCH 7/8] Fixed wrong constant name. --- cli/src/main/java/de/jplag/cli/CLI.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/src/main/java/de/jplag/cli/CLI.java b/cli/src/main/java/de/jplag/cli/CLI.java index 5e5359033..fc5b9cd33 100644 --- a/cli/src/main/java/de/jplag/cli/CLI.java +++ b/cli/src/main/java/de/jplag/cli/CLI.java @@ -143,7 +143,7 @@ private String getOffsetFileName(int offset) { private String getWritableFileName() throws CliException { int retryAttempt = 0; while (!this.inputHandler.getCliOptions().advanced.overwrite && new File(getOffsetFileName(retryAttempt)).exists() - && retryAttempt < NAME_COLLISION_TRIES) { + && retryAttempt < NAME_COLLISION_ATTEMPTS) { retryAttempt++; } From 68c526175f8dc6545ef5140316404a5a3dc6e149 Mon Sep 17 00:00:00 2001 From: Alexander Milster Date: Thu, 16 May 2024 12:55:11 +0200 Subject: [PATCH 8/8] Removed redundant "public" for tests. --- .../java/de/jplag/cli/CheckResultFileWritableTest.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cli/src/test/java/de/jplag/cli/CheckResultFileWritableTest.java b/cli/src/test/java/de/jplag/cli/CheckResultFileWritableTest.java index 5965220bc..4baab95e4 100644 --- a/cli/src/test/java/de/jplag/cli/CheckResultFileWritableTest.java +++ b/cli/src/test/java/de/jplag/cli/CheckResultFileWritableTest.java @@ -29,7 +29,7 @@ public static void setup() throws NoSuchFieldException, NoSuchMethodException { } @Test - public void testNonExistingWritableFile() throws Throwable { + void testNonExistingWritableFile() throws Throwable { File directory = Files.createTempDirectory("JPlagTest").toFile(); File targetFile = new File(directory, "results.zip"); @@ -38,7 +38,7 @@ public void testNonExistingWritableFile() throws Throwable { } @Test - public void testNonExistingNotWritableFile() throws IOException { + void testNonExistingNotWritableFile() throws IOException { File directory = Files.createTempDirectory("JPlagTest").toFile(); Assumptions.assumeTrue(directory.setWritable(false)); File targetFile = new File(directory, "results.zip"); @@ -49,7 +49,7 @@ public void testNonExistingNotWritableFile() throws IOException { } @Test - public void testExistingFile() throws Throwable { + void testExistingFile() throws Throwable { File directory = Files.createTempDirectory("JPlagTest").toFile(); File targetFile = new File(directory, "results.zip"); Assumptions.assumeTrue(targetFile.createNewFile()); @@ -59,7 +59,7 @@ public void testExistingFile() throws Throwable { } @Test - public void testExistingFileOverwrite() throws Throwable { + void testExistingFileOverwrite() throws Throwable { File directory = Files.createTempDirectory("JPlagTest").toFile(); File targetFile = new File(directory, "results.zip"); Assumptions.assumeTrue(targetFile.createNewFile()); @@ -69,7 +69,7 @@ public void testExistingFileOverwrite() throws Throwable { } @Test - public void testExistingNotWritableFile() throws IOException { + void testExistingNotWritableFile() throws IOException { File directory = Files.createTempDirectory("JPlagTest").toFile(); File targetFile = new File(directory, "results.zip"); Assumptions.assumeTrue(targetFile.createNewFile());