Fix for issue 6197: Do not store downloaded file if it is already att…

…ached (#7702)

CHANGELOG.md

@@ -95,6 +95,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 - We fixed an issue where the article title with colon fails to download the arXiv link (pdf file). [#7660](https://github.com/JabRef/issues/7660)
 - We fixed an issue where the keybinding for delete entry did not work on the main table [7580](https://github.com/JabRef/jabref/pull/7580)
 - We fixed an issue where the RFC fetcher is not compatible with the draft [7305](https://github.com/JabRef/jabref/issues/7305)
+- We fixed an issue where duplicate files (both file names and contents are the same) is downloaded and add to linked files [#6197](https://github.com/JabRef/jabref/issues/6197)
 - We fixed an issue where changing the appearance of the preview tab did not trigger a restart warning. [#5464](https://github.com/JabRef/jabref/issues/5464)
 
 ### Removed

src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java

@@ -434,29 +434,25 @@ public void download() {
 
             BackgroundTask<Path> downloadTask = prepareDownloadTask(targetDirectory.get(), urlDownload);
             downloadTask.onSuccess(destination -> {
-                LinkedFile newLinkedFile = LinkedFilesEditorViewModel.fromFile(destination, databaseContext.getFileDirectories(filePreferences), externalFileTypes);
-
-                List<LinkedFile> linkedFiles = entry.getFiles();
-                int oldFileIndex = -1;
-                int i = 0;
-                while ((i < linkedFiles.size()) && (oldFileIndex == -1)) {
-                    LinkedFile file = linkedFiles.get(i);
-                    // The file type changes as part of download process (see prepareDownloadTask), thus we only compare by link
-                    if (file.getLink().equalsIgnoreCase(linkedFile.getLink())) {
-                        oldFileIndex = i;
-                    }
-                    i++;
-                }
-                if (oldFileIndex == -1) {
-                    linkedFiles.add(0, newLinkedFile);
-                } else {
-                    linkedFiles.set(oldFileIndex, newLinkedFile);
+                boolean isDuplicate = false;
+                try {
+                    isDuplicate = FileNameUniqueness.isDuplicatedFile(targetDirectory.get(), destination.getFileName(), dialogService);
+                } catch (IOException e) {
+                    LOGGER.error("FileNameUniqueness.isDuplicatedFile failed", e);
+                    return;
                 }
-                entry.setFiles(linkedFiles);
-                // Notify in bar when the file type is HTML.
-                if (newLinkedFile.getFileType().equals(StandardExternalFileType.URL.getName())) {
-                    dialogService.notify(Localization.lang("Downloaded website as an HTML file."));
-                    LOGGER.debug("Downloaded website {} as an HTML file at {}", linkedFile.getLink(), destination);
+
+                if (!isDuplicate) {
+                    LinkedFile newLinkedFile = LinkedFilesEditorViewModel.fromFile(destination, databaseContext.getFileDirectories(filePreferences), externalFileTypes);
+                    List<LinkedFile> linkedFiles = entry.getFiles();
+
+                    entry.addLinkedFile(entry, linkedFile, newLinkedFile, linkedFiles);
+
+                    // Notify in bar when the file type is HTML.
+                    if (newLinkedFile.getFileType().equals(StandardExternalFileType.URL.getName())) {
+                        dialogService.notify(Localization.lang("Downloaded website as an HTML file."));
+                        LOGGER.debug("Downloaded website {} as an HTML file at {}", linkedFile.getLink(), destination);
+                    }
                 }
             });
             downloadProgress.bind(downloadTask.workDonePercentageProperty());

src/main/java/org/jabref/logic/util/io/FileNameUniqueness.java

@@ -1,10 +1,18 @@
 package org.jabref.logic.util.io;
 
+import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.Objects;
 import java.util.Optional;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import org.jabref.gui.DialogService;
+import org.jabref.logic.l10n.Localization;
 
 public class FileNameUniqueness {
+    private static final Pattern DUPLICATE_MARK_PATTERN = Pattern.compile("(.*) \\(\\d+\\)");
 
     /**
      * Returns a file name such that it does not match any existing files in targetDirectory
@@ -41,4 +49,73 @@ public static String getNonOverWritingFileName(Path targetDirectory, String file
 
         return newFileName;
     }
+
+    /**
+     * This function decide whether the newly downloaded file has the same content with other files
+     * It returns ture when the content is duplicate, while returns false if it is not
+     *
+     * @param directory The directory which saves the files (.pdf, for example)
+     * @param fileName Suggest name for the newly downloaded file
+     * @param dialogService To display the error and success message
+     * @return true when the content of the newly downloaded file is same as the file with "similar" name,
+     *         false when there is no "similar" file name or the content is different from that of files with "similar" name
+     * @throws IOException Fail when the file is not exist or something wrong when reading the file
+     */
+    public static boolean isDuplicatedFile(Path directory, Path fileName, DialogService dialogService) throws IOException {
+
+        Objects.requireNonNull(directory);
+        Objects.requireNonNull(fileName);
+        Objects.requireNonNull(dialogService);
+
+        String extensionSuffix = FileUtil.getFileExtension(fileName).orElse("");
+        extensionSuffix = extensionSuffix.equals("") ? extensionSuffix : "." + extensionSuffix;
+        String newFilename = FileUtil.getBaseName(fileName) + extensionSuffix;
+
+        String fileNameWithoutDuplicated = eraseDuplicateMarks(FileUtil.getBaseName(fileName));
+        String originalFileName = fileNameWithoutDuplicated + extensionSuffix;
+
+        if (newFilename.equals(originalFileName)) {
+            return false;
+        }
+
+        Path originalFile = directory.resolve(originalFileName);
+        Path duplicateFile = directory.resolve(fileName);
+        int counter = 1;
+
+        while (Files.exists(originalFile)) {
+            if (com.google.common.io.Files.equal(originalFile.toFile(), duplicateFile.toFile())) {
+                if (duplicateFile.toFile().delete()) {
+                    dialogService.notify(Localization.lang("File '%1' is a duplicate of '%0'. Keeping '%0'", originalFileName, fileName));
+                } else {
+                    dialogService.notify(Localization.lang("File '%1' is a duplicate of '%0'. Keeping both due to deletion error", originalFileName, fileName));
+                }
+                return true;
+            }
+
+            originalFileName = fileNameWithoutDuplicated +
+                    " (" + counter + ")"
+                    + extensionSuffix;
+            counter++;
+
+            if (newFilename.equals(originalFileName)) {
+                return false;
+            }
+            originalFile = directory.resolve(originalFileName);
+        }
+        return false;
+    }
+
+    /**
+     * This is the opposite function of getNonOverWritingFileName
+     * It will recover the file name to origin if it has duplicate mark such as " (1)"
+     * change the String whose format is "xxxxxx (number)" into "xxxxxx", while return the same String when it does not match the format
+     * This is the opposite function of getNonOverWritingFileName
+     *
+     * @param fileName Suggested name for the file without extensionSuffix, if it has duplicate file name with other file, it will end with something like " (1)"
+     * @return Suggested name for the file without extensionSuffix and duplicate marks such as " (1)"
+     */
+    public static String eraseDuplicateMarks(String fileName) {
+        Matcher m = DUPLICATE_MARK_PATTERN.matcher(fileName);
+        return m.find() ? fileName.substring(0, fileName.lastIndexOf('(') - 1) : fileName;
+    }
 }

src/main/java/org/jabref/logic/util/io/FileUtil.java

@@ -69,6 +69,13 @@ public static String getBaseName(String fileNameWithExtension) {
         return com.google.common.io.Files.getNameWithoutExtension(fileNameWithExtension);
     }
 
+    /**
+     * Returns the name part of a file name (i.e., everything in front of last ".").
+     */
+    public static String getBaseName(Path fileNameWithExtension) {
+        return getBaseName(fileNameWithExtension.getFileName().toString());
+    }
+
     /**
      * Returns a valid filename for most operating systems.
      * <p>

src/main/java/org/jabref/model/entry/BibEntry.java

@@ -991,4 +991,24 @@ public ObservableMap<Field, String> getFieldsObservable() {
     public Observable[] getObservables() {
         return new Observable[] {fields, type};
     }
+
+    public void addLinkedFile(BibEntry entry, LinkedFile linkedFile, LinkedFile newLinkedFile, List<LinkedFile> linkedFiles) {
+        int oldFileIndex = -1;
+        int i = 0;
+        while ((i < linkedFiles.size()) && (oldFileIndex == -1)) {
+            LinkedFile file = linkedFiles.get(i);
+            // The file type changes as part of download process (see prepareDownloadTask), thus we only compare by link
+            if (file.getLink().equalsIgnoreCase(linkedFile.getLink())) {
+                oldFileIndex = i;
+            }
+            i++;
+        }
+        if (oldFileIndex == -1) {
+            linkedFiles.add(0, newLinkedFile);
+        } else {
+            linkedFiles.set(oldFileIndex, newLinkedFile);
+        }
+        entry.setFiles(linkedFiles);
+    }
+
 }

src/main/resources/l10n/JabRef_en.properties

@@ -2303,3 +2303,6 @@ Convert\ timestamp\ field\ to\ field\ 'creationdate'=Convert timestamp field to
 Convert\ timestamp\ field\ to\ field\ 'modificationdate'=Convert timestamp field to field 'modificationdate'
 
 New\ entry\ by\ type=New entry by type
+
+File\ '%1'\ is\ a\ duplicate\ of\ '%0'.\ Keeping\ '%0'=File '%1' is a duplicate of '%0'. Keeping '%0'
+File\ '%1'\ is\ a\ duplicate\ of\ '%0'.\ Keeping\ both\ due\ to\ deletion\ error=File '%1' is a duplicate of '%0'. Keeping both due to deletion error

src/test/java/org/jabref/logic/util/io/FileNameUniquenessTest.java

@@ -4,11 +4,15 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 
+import org.jabref.gui.DialogService;
+
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
 
 public class FileNameUniquenessTest {
 
@@ -44,4 +48,58 @@ public void testGetNonOverWritingFileNameReturnsUniqueNameOverNConflicts() throw
         String outputFileName = FileNameUniqueness.getNonOverWritingFileName(tempDir, "manyfiles.txt");
         assertEquals("manyfiles (2).txt", outputFileName);
     }
+
+    @Test
+    public void testIsDuplicatedFileWithNoSimilarNames() throws IOException {
+        DialogService dialogService = mock(DialogService.class);
+        String filename1 = "file1.txt";
+        Path filePath1 = tempDir.resolve(filename1);
+        Files.createFile(filePath1);
+
+        boolean isDuplicate = FileNameUniqueness.isDuplicatedFile(tempDir, filePath1, dialogService);
+        assertFalse(isDuplicate);
+    }
+
+    @Test
+    public void testIsDuplicatedFileWithOneSimilarNames() throws IOException {
+        DialogService dialogService = mock(DialogService.class);
+        String filename1 = "file.txt";
+        String filename2 = "file (1).txt";
+        Path filePath1 = tempDir.resolve(filename1);
+        Path filePath2 = tempDir.resolve(filename2);
+        Files.createFile(filePath1);
+        Files.createFile(filePath2);
+
+        boolean isDuplicate = FileNameUniqueness.isDuplicatedFile(tempDir, filePath2, dialogService);
+        assertTrue(isDuplicate);
+    }
+
+    @Test
+    public void testTaseDuplicateMarksReturnsOrignalFileName1() throws IOException {
+        String fileName1 = "abc def (1)";
+        String fileName2 = FileNameUniqueness.eraseDuplicateMarks(fileName1);
+        assertEquals("abc def", fileName2);
+    }
+
+    @Test
+    public void testTaseDuplicateMarksReturnsOrignalFileName2() throws IOException {
+        String fileName1 = "abc (def) gh (1)";
+        String fileName2 = FileNameUniqueness.eraseDuplicateMarks(fileName1);
+        assertEquals("abc (def) gh", fileName2);
+    }
+
+    @Test
+    public void testTaseDuplicateMarksReturnsSameName1() throws IOException {
+        String fileName1 = "abc def (g)";
+        String fileName2 = FileNameUniqueness.eraseDuplicateMarks(fileName1);
+        assertEquals("abc def (g)", fileName2);
+    }
+
+    @Test
+    public void testTaseDuplicateMarksReturnsSameName2() throws IOException {
+        String fileName1 = "abc def";
+        String fileName2 = FileNameUniqueness.eraseDuplicateMarks(fileName1);
+        assertEquals("abc def", fileName2);
+    }
+
 }