diff --git a/CHANGELOG.md b/CHANGELOG.md index 150c9f72d13..01046b9cb86 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -91,7 +91,8 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve ### Fixed -- We fixed an issue where linked fails containing parts of the main file directory could not be opened [#8991](https://github.com/JabRef/jabref/issues/8991) +- We fixed an issue where linked fails containing parts of the main file directory could not be opened. [#8991](https://github.com/JabRef/jabref/issues/8991) +- Linked files with an absolute path can be opened again. [#8991](https://github.com/JabRef/jabref/issues/8991) - We fixed an issue where the user could not rate an entry in the main table when an entry was not yet ranked. [#5842](https://github.com/JabRef/jabref/issues/5842) - We fixed an issue that caused JabRef to sometimes open multiple instances when "Remote Operation" is enabled. [#8653](https://github.com/JabRef/jabref/issues/8653) - We fixed an issue where linked files with the filetype "application/pdf" in an entry were not shown with the correct PDF-Icon in the main table [8930](https://github.com/JabRef/jabref/issues/8930) diff --git a/src/main/java/org/jabref/logic/importer/util/FileFieldParser.java b/src/main/java/org/jabref/logic/importer/util/FileFieldParser.java index 24a5c136fd5..3220beb7b63 100644 --- a/src/main/java/org/jabref/logic/importer/util/FileFieldParser.java +++ b/src/main/java/org/jabref/logic/importer/util/FileFieldParser.java @@ -15,7 +15,36 @@ public class FileFieldParser { private static final Logger LOGGER = LoggerFactory.getLogger(FileFieldParser.class); + private final String value; + + private StringBuilder charactersOfCurrentElement; + private boolean windowsPath; + + public FileFieldParser(String value) { + this.value = value; + } + + /** + * Converts the string representation of LinkedFileData to a List of LinkedFile + * + * The syntax of one element is description:path:type + * Multiple elements are concatenated with ; + * + * The main challenges of the implementation are: + * + * + */ public static List parse(String value) { + // We need state to have a more clean code. Thus, we instantiate the class and then return the result + FileFieldParser fileFieldParser = new FileFieldParser(value); + return fileFieldParser.parse(); + } + + public List parse() { List files = new ArrayList<>(); if ((value == null) || value.trim().isEmpty()) { @@ -32,44 +61,60 @@ public static List parse(String value) { } } + // data of each LinkedFile as split string List linkedFileData = new ArrayList<>(); - StringBuilder sb = new StringBuilder(); + + resetDataStructuresForNextElement(); boolean inXmlChar = false; boolean escaped = false; for (int i = 0; i < value.length(); i++) { char c = value.charAt(i); if (!escaped && (c == '\\')) { - escaped = true; - continue; + if (windowsPath) { + charactersOfCurrentElement.append(c); + continue; + } else { + escaped = true; + continue; + } } else if (!escaped && (c == '&') && !inXmlChar) { // Check if we are entering an XML special character construct such // as ",", because we need to know in order to ignore the semicolon. - sb.append(c); + charactersOfCurrentElement.append(c); if ((value.length() > (i + 1)) && (value.charAt(i + 1) == '#')) { inXmlChar = true; } } else if (!escaped && inXmlChar && (c == ';')) { // Check if we are exiting an XML special character construct: - sb.append(c); + charactersOfCurrentElement.append(c); inXmlChar = false; } else if (!escaped && (c == ':')) { - // We are in the next LinkedFile data element - linkedFileData.add(sb.toString()); - sb = new StringBuilder(); + if ((linkedFileData.size() == 1) && // we already parsed the description + (charactersOfCurrentElement.length() == 1)) { // we parsed one character + // special case of Windows paths + // Example: ":c:\test.pdf:PDF" + // We are at the second : (position 3 in the example) and "just" add it to the current element + charactersOfCurrentElement.append(c); + windowsPath = true; + } else { + // We are in the next LinkedFile data element + linkedFileData.add(charactersOfCurrentElement.toString()); + resetDataStructuresForNextElement(); + } } else if (!escaped && (c == ';') && !inXmlChar) { - linkedFileData.add(sb.toString()); + linkedFileData.add(charactersOfCurrentElement.toString()); files.add(convert(linkedFileData)); // next iteration - sb = new StringBuilder(); + resetDataStructuresForNextElement(); } else { - sb.append(c); + charactersOfCurrentElement.append(c); } escaped = false; } - if (sb.length() > 0) { - linkedFileData.add(sb.toString()); + if (charactersOfCurrentElement.length() > 0) { + linkedFileData.add(charactersOfCurrentElement.toString()); } if (!linkedFileData.isEmpty()) { files.add(convert(linkedFileData)); @@ -77,6 +122,11 @@ public static List parse(String value) { return files; } + private void resetDataStructuresForNextElement() { + charactersOfCurrentElement = new StringBuilder(); + windowsPath = false; + } + /** * Converts the given textual representation of a LinkedFile object * diff --git a/src/main/java/org/jabref/logic/util/io/FileNameCleaner.java b/src/main/java/org/jabref/logic/util/io/FileNameCleaner.java index 83d764509e7..4218759eb3f 100644 --- a/src/main/java/org/jabref/logic/util/io/FileNameCleaner.java +++ b/src/main/java/org/jabref/logic/util/io/FileNameCleaner.java @@ -6,6 +6,8 @@ * This class is based on http://stackoverflow.com/a/5626340/873282 * extended with LEFT CURLY BRACE and RIGHT CURLY BRACE * Replaces illegal characters in given file paths. + * + * Regarding the maximum length, see {@link FileUtil#getValidFileName(String)} */ public class FileNameCleaner { diff --git a/src/main/java/org/jabref/logic/util/io/FileUtil.java b/src/main/java/org/jabref/logic/util/io/FileUtil.java index 379d2318c80..d92e242afea 100644 --- a/src/main/java/org/jabref/logic/util/io/FileUtil.java +++ b/src/main/java/org/jabref/logic/util/io/FileUtil.java @@ -91,6 +91,8 @@ public static String getBaseName(Path fileNameWithExtension) { * Returns a valid filename for most operating systems. *

* Currently, only the length is restricted to 255 chars, see MAXIMUM_FILE_NAME_LENGTH. + * + * See also {@link FileHelper#detectBadFileName(String)} and {@link FileNameCleaner#cleanFileName(String)} */ public static String getValidFileName(String fileName) { String nameWithoutExtension = getBaseName(fileName); diff --git a/src/main/java/org/jabref/model/entry/LinkedFile.java b/src/main/java/org/jabref/model/entry/LinkedFile.java index 158336c7a53..7a6d8010bf8 100644 --- a/src/main/java/org/jabref/model/entry/LinkedFile.java +++ b/src/main/java/org/jabref/model/entry/LinkedFile.java @@ -118,9 +118,6 @@ public boolean equals(Object o) { /** * Writes serialized object to ObjectOutputStream, automatically called - * - * @param out {@link ObjectOutputStream} - * @throws IOException */ private void writeObject(ObjectOutputStream out) throws IOException { out.writeUTF(getFileType()); @@ -131,9 +128,6 @@ private void writeObject(ObjectOutputStream out) throws IOException { /** * Reads serialized object from ObjectInputStreamm, automatically called - * - * @param in {@link ObjectInputStream} - * @throws IOException */ private void readObject(ObjectInputStream in) throws IOException { fileType = new SimpleStringProperty(in.readUTF()); diff --git a/src/main/java/org/jabref/model/util/FileHelper.java b/src/main/java/org/jabref/model/util/FileHelper.java index 19df95ad417..f2ab92419b7 100644 --- a/src/main/java/org/jabref/model/util/FileHelper.java +++ b/src/main/java/org/jabref/model/util/FileHelper.java @@ -5,6 +5,7 @@ import java.io.InputStream; import java.net.URL; import java.nio.file.Files; +import java.nio.file.InvalidPathException; import java.nio.file.Path; import java.util.Arrays; import java.util.List; @@ -42,7 +43,7 @@ public class FileHelper { 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 34, 42, - 58, + 58, // ":" 60, 62, 63, 123, 124, 125 }; @@ -133,10 +134,22 @@ public static Optional find(String fileName, List directories) { /** * Detect illegal characters in given filename. * + * See also {@link org.jabref.logic.util.io.FileNameCleaner#cleanFileName} + * * @param fileName the fileName to detect - * @return Boolean whether there is a illegal name. + * @return Boolean whether there is an illegal name. */ public static boolean detectBadFileName(String fileName) { + // fileName could be a path, we want to check the fileName only (and don't care about the path) + // Reason: Handling of "c:\temp.pdf" is difficult, because ":" is an illegal character in the file name, + // but a perfectly legal one in the path at this position + try { + fileName = Path.of(fileName).getFileName().toString(); + } catch (InvalidPathException e) { + // in case the internal method cannot parse the path, it is surely illegal + return true; + } + for (int i = 0; i < fileName.length(); i++) { char c = fileName.charAt(i); if (!isCharLegal(c)) { @@ -161,12 +174,14 @@ public static boolean isCharLegal(char c) { public static Optional find(String fileName, Path directory) { Objects.requireNonNull(fileName); Objects.requireNonNull(directory); - // Explicitly check for an empty String, as File.exists returns true on that empty path, because it maps to the default jar location - // if we then call toAbsoluteDir, it would always return the jar-location folder. This is not what we want here + if (detectBadFileName(fileName)) { LOGGER.error("Invalid characters in path for file {} ", fileName); return Optional.empty(); } + + // Explicitly check for an empty string, as File.exists returns true on that empty path, because it maps to the default jar location. + // If we then call toAbsoluteDir, it would always return the jar-location folder. This is not what we want here. if (fileName.isEmpty()) { return Optional.of(directory); } diff --git a/src/test/java/org/jabref/architecture/MainArchitectureTests.java b/src/test/java/org/jabref/architecture/MainArchitectureTests.java index b3702e019dc..5f67761f064 100644 --- a/src/test/java/org/jabref/architecture/MainArchitectureTests.java +++ b/src/test/java/org/jabref/architecture/MainArchitectureTests.java @@ -31,7 +31,7 @@ public static void doNotUseApacheCommonsLang3(JavaClasses classes) { @ArchTest public static void doNotUseSwing(JavaClasses classes) { - // This checks for all all Swing packages, but not the UndoManager + // This checks for all Swing packages, but not the UndoManager noClasses().that().areNotAnnotatedWith(AllowedToUseSwing.class) .should().accessClassesThat() .resideInAnyPackage("javax.swing", diff --git a/src/test/java/org/jabref/logic/importer/util/FileFieldParserTest.java b/src/test/java/org/jabref/logic/importer/util/FileFieldParserTest.java index cfe53630acb..db6de11c190 100644 --- a/src/test/java/org/jabref/logic/importer/util/FileFieldParserTest.java +++ b/src/test/java/org/jabref/logic/importer/util/FileFieldParserTest.java @@ -37,7 +37,7 @@ public void check(LinkedFile expected, List input) { assertEquals(expected, FileFieldParser.convert(new ArrayList<>(input))); } - private static Stream stringsToParseTestData() throws Exception { + private static Stream stringsToParseTest() throws Exception { return Stream.of( // null string Arguments.of( @@ -114,6 +114,18 @@ private static Stream stringsToParseTestData() throws Exception { "desc:C\\:\\\\test.pdf:PDF" ), + // handleNonEscapedFilePath + Arguments.of( + Collections.singletonList(new LinkedFile("desc", Path.of("C:\\test.pdf"), "PDF")), + "desc:C:\\test.pdf:PDF" + ), + + // Source: https://github.com/JabRef/jabref/issues/8991#issuecomment-1214131042 + Arguments.of( + Collections.singletonList(new LinkedFile("Boyd2012.pdf", Path.of("C:\\Users\\Literature_database\\Boyd2012.pdf"), "PDF")), + "Boyd2012.pdf:C\\:\\\\Users\\\\Literature_database\\\\Boyd2012.pdf:PDF" + ), + // subsetOfFieldsResultsInFileLink: description only Arguments.of( Collections.singletonList(new LinkedFile("", Path.of("file.pdf"), "")), @@ -170,8 +182,8 @@ private static Stream stringsToParseTestData() throws Exception { } @ParameterizedTest - @MethodSource("stringsToParseTestData") - public void testParse(List expected, String input) { + @MethodSource + public void stringsToParseTest(List expected, String input) { assertEquals(expected, FileFieldParser.parse(input)); } } diff --git a/src/test/java/org/jabref/model/util/FileHelperTest.java b/src/test/java/org/jabref/model/util/FileHelperTest.java index 01bd79a00b4..12ee3e9df62 100644 --- a/src/test/java/org/jabref/model/util/FileHelperTest.java +++ b/src/test/java/org/jabref/model/util/FileHelperTest.java @@ -4,13 +4,19 @@ import java.nio.file.Path; import java.util.Optional; +import org.jabref.architecture.AllowedToUseLogic; +import org.jabref.logic.util.OS; + import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +@AllowedToUseLogic("Needs access to OS, because of OS-specific tests") class FileHelperTest { @Test @@ -65,4 +71,25 @@ public void testDoesNotFindsFileStartingWithTheSameDirectoryHasASubdirectory(@Te Files.createFile(testFile); assertEquals(Optional.of(testFile.toAbsolutePath()), FileHelper.find("files/test.pdf", firstFilesPath)); } + + public void testCTemp() { + String fileName = "c:\\temp.pdf"; + if (OS.WINDOWS) { + assertFalse(FileHelper.detectBadFileName(fileName)); + } else { + assertTrue(FileHelper.detectBadFileName(fileName)); + } + } + + @ParameterizedTest + @ValueSource(strings = {"/mnt/tmp/test.pdf"}) + public void legalPaths(String fileName) { + assertFalse(FileHelper.detectBadFileName(fileName)); + } + + @ParameterizedTest + @ValueSource(strings = {"te{}mp.pdf"}) + public void illegalPaths(String fileName) { + assertTrue(FileHelper.detectBadFileName(fileName)); + } }