Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix overwritting downloaded files with same name #6174

Merged
merged 3 commits into from
Mar 29, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
- We fixed an issue where opening a library from the recent libraries menu was not possible. [#5939](https://github.com/JabRef/jabref/issues/5939)
- We fixed an issue with inconsistent capitalization of file extensions when downloading files [#6115](https://github.com/JabRef/jabref/issues/6115)
- We fixed the display of language and encoding in the preferences dialog. [#6130](https://github.com/JabRef/jabref/pull/6130)

- We fixed an issue where search full-text documents downloaded files with same name, overwriting existing files. [#6174](https://github.com/JabRef/jabref/pull/6174)
### Removed

- We removed the obsolete `External programs / Open PDF` section in the preferences, as the default application to open PDFs is now set in the `Manage external file types` dialog. [#6130](https://github.com/JabRef/jabref/pull/6130)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.jabref.logic.externalfiles.LinkedFileHandler;
import org.jabref.logic.l10n.Localization;
import org.jabref.logic.net.URLDownload;
import org.jabref.logic.util.io.FileNameUniqueness;
import org.jabref.logic.xmp.XmpPreferences;
import org.jabref.logic.xmp.XmpUtilWriter;
import org.jabref.model.database.BibDatabaseContext;
Expand Down Expand Up @@ -419,6 +420,7 @@ public BackgroundTask<Path> prepareDownloadTask(Path targetDirectory, URLDownloa
String suggestedTypeName = externalFileType.getName();
linkedFile.setFileType(suggestedTypeName);
String suggestedName = linkedFileHandler.getSuggestedFileName(externalFileType.getExtension());
suggestedName = FileNameUniqueness.getNonOverWritingFileName(targetDirectory, suggestedName);
return targetDirectory.resolve(suggestedName);
})
.then(destination -> new FileDownloadTask(urlDownload.getSource(), destination))
Expand Down
49 changes: 49 additions & 0 deletions src/main/java/org/jabref/logic/util/io/FileNameUniqueness.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package org.jabref.logic.util.io;

import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Optional;

public class FileNameUniqueness {

/**
* Returns an absolute path to a file which does not match with any existing file names
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description seems to be off as no absolute path is returned but only the file name, right?

*
* @param targetDirectory The directory in which file name should be unique
* @param fileName Suggested name for the file
* @return a file-name such that it does not match any existing files in targetDirectory.
* */
public static String getNonOverWritingFileName(Path targetDirectory, String fileName) {
// String absoluteName = targetDirectory.resolve(fileName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this comment.

// .toString();

Optional<String> extensionOptional = FileUtil.getFileExtension(fileName);

// the suffix include the '.' , if extension is present Eg: ".pdf"
String extensionSuffix;
String fileNameWithoutExtension;

if (extensionOptional.isPresent()) {
extensionSuffix = '.' + extensionOptional.get();
fileNameWithoutExtension = fileName.substring(0, fileName.lastIndexOf('.'));
}
else {
extensionSuffix = "";
fileNameWithoutExtension = fileName;
}

// Path absolutePath = Paths.get(absoluteName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one as well.

String newFileName = fileName;

int counter = 1;
while ( Files.exists(
targetDirectory.resolve(newFileName))
) {
newFileName = fileNameWithoutExtension +
" (" + counter + ")" +
extensionSuffix;
counter++;
}
return newFileName;
}
}
56 changes: 56 additions & 0 deletions src/test/java/org/jabref/logic/util/io/FileNameUniquenessTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package org.jabref.logic.util.io;

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;

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;

public class FileNameUniquenessTest {

@TempDir
protected Path tempDir;

@Test
public void testGetNonOverWritingFileNameReturnsSameName(@TempDir Path tempDirectory) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a need to have the temporary directory as a variable as well as an argument? I would prefer if all tests use the same format (either variable or either paramater)


assertFalse(
Files.exists(tempDirectory.resolve("sameFile.txt"))
);

String outputFileName = FileNameUniqueness.getNonOverWritingFileName(tempDirectory, "sameFile.txt");
assertEquals("sameFile.txt", outputFileName);

}

@Test
public void testGetNonOverWritingFileNameReturnsUniqueNameOver1Conflict() throws IOException {
Path dummyFilePath1 = tempDir.resolve("differentFile.txt");

Files.createFile(dummyFilePath1);

String outputFileName = FileNameUniqueness.getNonOverWritingFileName(tempDir, "differentFile.txt");
assertEquals("differentFile (1).txt", outputFileName);

Files.delete(dummyFilePath1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleting the file shouldn't be necessary as the TempDir should take care of this.

}

@Test
public void testGetNonOverWritingFileNameReturnsUniqueNameOverNConflicts() throws IOException {
Path dummyFilePath1 = tempDir.resolve("manyfiles.txt");
Path dummyFilePath2 = tempDir.resolve("manyfiles (1).txt");

Files.createFile(dummyFilePath1);
Files.createFile(dummyFilePath2);

String outputFileName = FileNameUniqueness.getNonOverWritingFileName(tempDir, "manyfiles.txt");
assertEquals("manyfiles (2).txt", outputFileName);

Files.delete(dummyFilePath1);
Files.delete(dummyFilePath2);
}
}