Fix handling of URL in file field (#7347)

* Add workaround of test not working on Windows Co-authored-by: Dominik Voigt <[email protected]> * Change input of throws Co-authored-by: Dominik Voigt <[email protected]> * Add link as String as constructor parameter * Add CHANGELOG entry * Move tests for FieldFieldParser to FileFieldParserTest (and fix handling of //) * Fix checkstyle Co-authored-by: Dominik Voigt <[email protected]> Co-authored-by: Carl Christian Snethlage <[email protected]>
JabRef · Jan 24, 2021 · ae43548 · ae43548
1 parent 7672b2d
commit ae43548
Show file tree

Hide file tree

Showing 5 changed files with 205 additions and 145 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -34,7 +34,8 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 - We fixed an issue where the "Find unlinked files" dialog would freeze JabRef on importing. [#7205](https://github.com/JabRef/jabref/issues/7205)
 - We fixed an issue where the "Find unlinked files" would stop importing when importing a single file failed. [#7206](https://github.com/JabRef/jabref/issues/7206)
 - We fixed an issue where an exception would be displayed for previewing and preferences when a custom theme has been configured but is missing [#7177](https://github.com/JabRef/jabref/issues/7177)
-- We fixed an issue where the regex based file search miss-interpreted specific symbols [#4342](https://github.com/JabRef/jabref/issues/4342)
+- We fixed an issue where URLs in `file` fields could not be handled on Windows. [#7359](https://github.com/JabRef/jabref/issues/7359)
+- We fixed an issue where the regex based file search miss-interpreted specific symbols. [#4342](https://github.com/JabRef/jabref/issues/4342)
 - We fixed an issue where the Harvard RTF exporter used the wrong default file extension. [4508](https://github.com/JabRef/jabref/issues/4508)
 - We fixed an issue where the Harvard RTF exporter did not use the new authors formatter and therefore did not export "organization" authors correctly. [4508](https://github.com/JabRef/jabref/issues/4508)
 - We fixed an issue where the field `urldate` was not exported to the corresponding fields `YearAccessed`, `MonthAccessed`, `DayAccessed` in MS Office XML [#7354](https://github.com/JabRef/jabref/issues/7354)

diff --git a/src/main/java/org/jabref/logic/importer/util/FileFieldParser.java b/src/main/java/org/jabref/logic/importer/util/FileFieldParser.java
@@ -2,6 +2,7 @@
 
 import java.net.MalformedURLException;
 import java.net.URL;
+import java.nio.file.InvalidPathException;
 import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.List;
@@ -17,7 +18,7 @@ public static List<LinkedFile> parse(String value) {
             return files;
         }
 
-        List<String> entry = new ArrayList<>();
+        List<String> linkedFileData = new ArrayList<>();
         StringBuilder sb = new StringBuilder();
         boolean inXmlChar = false;
         boolean escaped = false;
@@ -39,30 +40,38 @@ public static List<LinkedFile> parse(String value) {
                 sb.append(c);
                 inXmlChar = false;
             } else if (!escaped && (c == ':')) {
-                entry.add(sb.toString());
+                // We are in the next LinkedFile data element
+                linkedFileData.add(sb.toString());
                 sb = new StringBuilder();
             } else if (!escaped && (c == ';') && !inXmlChar) {
-                entry.add(sb.toString());
-                sb = new StringBuilder();
+                linkedFileData.add(sb.toString());
+                files.add(convert(linkedFileData));
 
-                files.add(convert(entry));
+                // next iteration
+                sb = new StringBuilder();
             } else {
                 sb.append(c);
             }
             escaped = false;
         }
         if (sb.length() > 0) {
-            entry.add(sb.toString());
+            linkedFileData.add(sb.toString());
         }
-
-        if (!entry.isEmpty()) {
-            files.add(convert(entry));
+        if (!linkedFileData.isEmpty()) {
+            files.add(convert(linkedFileData));
         }
-
         return files;
     }
 
-    private static LinkedFile convert(List<String> entry) {
+    /**
+     * Converts the given textual representation of a LinkedFile object
+     *
+     * SIDE EFFECT: The given entry list is cleared upon completion
+     *
+     * @param entry the list of elements in the linked file textual representation
+     * @return a LinkedFile object
+     */
+    static LinkedFile convert(List<String> entry) {
         // ensure list has at least 3 fields
         while (entry.size() < 3) {
             entry.add("");
@@ -71,17 +80,31 @@ private static LinkedFile convert(List<String> entry) {
         LinkedFile field = null;
         if (LinkedFile.isOnlineLink(entry.get(1))) {
             try {
-                field = new LinkedFile(new URL(entry.get(1)), entry.get(2));
+                field = new LinkedFile(entry.get(0), new URL(entry.get(1)), entry.get(2));
             } catch (MalformedURLException ignored) {
-                // ignored
+                // in case the URL is malformed, store it nevertheless
+                field = new LinkedFile(entry.get(0), entry.get(1), entry.get(2));
             }
         }
 
         if (field == null) {
-            field = new LinkedFile(entry.get(0), Path.of(entry.get(1)), entry.get(2));
+            String pathStr = entry.get(1);
+            if (pathStr.contains("//")) {
+                // In case the path contains //, we assume it is a malformed URL, not a malformed path.
+                // On linux, the double slash would be converted to a single slash.
+                field = new LinkedFile(entry.get(0), pathStr, entry.get(2));
+            } else {
+                try {
+                    // there is no Path.isValidPath(String) method
+                    Path path = Path.of(pathStr);
+                    field = new LinkedFile(entry.get(0), path, entry.get(2));
+                } catch (InvalidPathException e) {
+                    field = new LinkedFile(entry.get(0), pathStr, entry.get(2));
+                }
+            }
         }
 
-        // link is only mandatory field
+        // link is the only mandatory field
         if (field.getDescription().isEmpty() && field.getLink().isEmpty() && !field.getFileType().isEmpty()) {
             field = new LinkedFile("", Path.of(field.getFileType()), "");
         } else if (!field.getDescription().isEmpty() && field.getLink().isEmpty() && field.getFileType().isEmpty()) {

diff --git a/src/main/java/org/jabref/model/entry/LinkedFile.java b/src/main/java/org/jabref/model/entry/LinkedFile.java
@@ -33,15 +33,24 @@ public class LinkedFile implements Serializable {
     private transient StringProperty fileType = new SimpleStringProperty();
 
     public LinkedFile(String description, Path link, String fileType) {
+        this(Objects.requireNonNull(description), Objects.requireNonNull(link).toString(), Objects.requireNonNull(fileType));
+    }
+
+    /**
+     * Constructor for non-valid paths. We need to parse them, because the GUI needs to render it.
+     */
+    public LinkedFile(String description, String link, String fileType) {
         this.description.setValue(Objects.requireNonNull(description));
-        setLink(Objects.requireNonNull(link).toString());
+        setLink(link);
         this.fileType.setValue(Objects.requireNonNull(fileType));
     }
 
     public LinkedFile(URL link, String fileType) {
-        this.description.setValue("");
-        setLink(Objects.requireNonNull(link).toString());
-        this.fileType.setValue(Objects.requireNonNull(fileType));
+        this("", Objects.requireNonNull(link).toString(), Objects.requireNonNull(fileType));
+    }
+
+    public LinkedFile(String description, URL link, String fileType) {
+        this(description, Objects.requireNonNull(link).toString(), Objects.requireNonNull(fileType));
     }
 
     public StringProperty descriptionProperty() {

diff --git a/src/test/java/org/jabref/logic/bibtex/FileFieldWriterTest.java b/src/test/java/org/jabref/logic/bibtex/FileFieldWriterTest.java
@@ -1,13 +1,7 @@
 package org.jabref.logic.bibtex;
 
-import java.net.MalformedURLException;
-import java.net.URL;
 import java.nio.file.Path;
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.List;
 
-import org.jabref.logic.importer.util.FileFieldParser;
 import org.jabref.model.entry.LinkedFile;
 
 import org.junit.jupiter.api.Test;
@@ -17,121 +11,6 @@
 
 public class FileFieldWriterTest {
 
-    @Test
-    public void emptyListForEmptyInput() {
-        String emptyInput = "";
-
-        assertEquals(Collections.emptyList(), FileFieldParser.parse(emptyInput));
-        assertEquals(Collections.emptyList(), FileFieldParser.parse(null));
-    }
-
-    @Test
-    public void parseCorrectInput() {
-        String input = "Desc:File.PDF:PDF";
-
-        assertEquals(
-                Collections.singletonList(new LinkedFile("Desc", Path.of("File.PDF"), "PDF")),
-                FileFieldParser.parse(input));
-    }
-
-    @Test
-    public void parseCorrectOnlineInput() throws MalformedURLException {
-        String input = ":http\\://arxiv.org/pdf/2010.08497v1:PDF";
-        String inputURL = "http://arxiv.org/pdf/2010.08497v1";
-        List<LinkedFile> expected = Collections.singletonList(new LinkedFile(new URL(inputURL), "PDF"));
-
-        assertEquals(expected, FileFieldParser.parse(input));
-    }
-
-    @Test
-    public void parseFaultyOnlineInput() {
-        String input = ":htt\\://arxiv.org/pdf/2010.08497v1:PDF";
-        String inputURL = "htt://arxiv.org/pdf/2010.08497v1";
-        List<LinkedFile> expected = Collections.singletonList(new LinkedFile("", Path.of(inputURL), "PDF"));
-
-        assertEquals(expected, FileFieldParser.parse(input));
-    }
-
-    @Test
-    public void ingoreMissingDescription() {
-        String input = ":wei2005ahp.pdf:PDF";
-
-        assertEquals(
-                Collections.singletonList(new LinkedFile("", Path.of("wei2005ahp.pdf"), "PDF")),
-                FileFieldParser.parse(input));
-    }
-
-    @Test
-    public void interpreteLinkAsOnlyMandatoryField() {
-        String single = "wei2005ahp.pdf";
-        String multiple = "wei2005ahp.pdf;other.pdf";
-
-        assertEquals(
-                Collections.singletonList(new LinkedFile("", Path.of("wei2005ahp.pdf"), "")),
-                FileFieldParser.parse(single));
-
-        assertEquals(
-                Arrays.asList(
-                        new LinkedFile("", Path.of("wei2005ahp.pdf"), ""),
-                        new LinkedFile("", Path.of("other.pdf"), "")),
-                FileFieldParser.parse(multiple));
-    }
-
-    @Test
-    public void escapedCharactersInDescription() {
-        String input = "test\\:\\;:wei2005ahp.pdf:PDF";
-
-        assertEquals(
-                Collections.singletonList(new LinkedFile("test:;", Path.of("wei2005ahp.pdf"), "PDF")),
-                FileFieldParser.parse(input));
-    }
-
-    @Test
-    public void handleXmlCharacters() {
-        String input = "test&#44\\;st\\:\\;:wei2005ahp.pdf:PDF";
-
-        assertEquals(
-                Collections.singletonList(new LinkedFile("test&#44;st:;", Path.of("wei2005ahp.pdf"), "PDF")),
-                FileFieldParser.parse(input));
-    }
-
-    @Test
-    public void handleEscapedFilePath() {
-        String input = "desc:C\\:\\\\test.pdf:PDF";
-
-        assertEquals(
-                Collections.singletonList(new LinkedFile("desc", Path.of("C:\\test.pdf"), "PDF")),
-                FileFieldParser.parse(input));
-    }
-
-    @Test
-    public void subsetOfFieldsResultsInFileLink() {
-        String descOnly = "file.pdf::";
-        String fileOnly = ":file.pdf";
-        String typeOnly = "::file.pdf";
-
-        assertEquals(
-                Collections.singletonList(new LinkedFile("", Path.of("file.pdf"), "")),
-                FileFieldParser.parse(descOnly));
-
-        assertEquals(
-                Collections.singletonList(new LinkedFile("", Path.of("file.pdf"), "")),
-                FileFieldParser.parse(fileOnly));
-
-        assertEquals(
-                Collections.singletonList(new LinkedFile("", Path.of("file.pdf"), "")),
-                FileFieldParser.parse(typeOnly));
-    }
-
-    @Test
-    public void tooManySeparators() {
-        String input = "desc:file.pdf:PDF:asdf";
-
-        assertEquals(
-                Collections.singletonList(new LinkedFile("desc", Path.of("file.pdf"), "PDF")),
-                FileFieldParser.parse(input));
-    }
-
     @Test
     public void testQuoteStandard() {
         assertEquals("a", FileFieldWriter.quote("a"));
@@ -154,10 +33,10 @@ public void testQuoteNull() {
 
     @Test
     public void testEncodeStringArray() {
-        assertEquals("a:b;c:d", FileFieldWriter.encodeStringArray(new String[][]{{"a", "b"}, {"c", "d"}}));
-        assertEquals("a:;c:d", FileFieldWriter.encodeStringArray(new String[][]{{"a", ""}, {"c", "d"}}));
-        assertEquals("a:" + null + ";c:d", FileFieldWriter.encodeStringArray(new String[][]{{"a", null}, {"c", "d"}}));
-        assertEquals("a:\\:b;c\\;:d", FileFieldWriter.encodeStringArray(new String[][]{{"a", ":b"}, {"c;", "d"}}));
+        assertEquals("a:b;c:d", FileFieldWriter.encodeStringArray(new String[][] {{"a", "b"}, {"c", "d"}}));
+        assertEquals("a:;c:d", FileFieldWriter.encodeStringArray(new String[][] {{"a", ""}, {"c", "d"}}));
+        assertEquals("a:" + null + ";c:d", FileFieldWriter.encodeStringArray(new String[][] {{"a", null}, {"c", "d"}}));
+        assertEquals("a:\\:b;c\\;:d", FileFieldWriter.encodeStringArray(new String[][] {{"a", ":b"}, {"c;", "d"}}));
     }
 
     @Test