diff --git a/CHANGELOG.md b/CHANGELOG.md index 560c53118de..7b3e012d768 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,9 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve ### Changed +- We changed the writing method of a `.bib` file: Existing files should have been modified instead of being replaced. +- When JabRef writes a `.bib` file, it makes first a backup (`.bak`) of the `.bib`. +- When JabRef writes a `.bib` file, it first attempts to write into a separate local directory. - We improved the Citavi Importer to also import so called Knowledge-items into the field `comment` of the corresponding entry [#9025](https://github.com/JabRef/jabref/issues/9025) - We removed wrapping of string constants when writing to a `.bib` file. - We call backup files `.bak` and temporary writing files now `.sav`. diff --git a/build.gradle b/build.gradle index 830991e02c4..2946f3515a4 100644 --- a/build.gradle +++ b/build.gradle @@ -216,6 +216,12 @@ dependencies { testImplementation "org.testfx:testfx-junit5:4.0.16-alpha" testImplementation "org.hamcrest:hamcrest-library:2.2" + testImplementation ('com.google.jimfs:jimfs:1.2') { + exclude group: "com.google.auto.service" + exclude group: "com.google.code.findbugs" + exclude group: "org.checkerframework" + } + checkstyle 'com.puppycrawl.tools:checkstyle:10.3.2' // xjc needs the runtime as well for the ant task, otherwise it fails xjc group: 'org.glassfish.jaxb', name: 'jaxb-xjc', version: '3.0.2' diff --git a/docs/decisions/0026-safe-file-writing-by-writing-to-a-temporary-file-in-another-filder.md b/docs/decisions/0026-safe-file-writing-by-writing-to-a-temporary-file-in-another-filder.md new file mode 100644 index 00000000000..0f637d19943 --- /dev/null +++ b/docs/decisions/0026-safe-file-writing-by-writing-to-a-temporary-file-in-another-filder.md @@ -0,0 +1,34 @@ +--- +parent: Decision Records +nav_order: 26 +--- +# Safe File Writing by Writing to a Temporary File in Another Folder + +## Context and Problem Statement + +JabRef needs to write to the .bib file. A .bib file of a user should never be damaged. JabRef needs to provide a way to do "safe" writing of a bib file. + +## Considered Options + +* Create a temporary file in a local folder and copy after successful write +* Create temporary file next to bib file and atomically move it to the original file + +## Decision Outcome + +Chosen option: "Create a temporary file in a local folder and copy after successful write", because good usage of Dropbox outweighs potential recovery scenarios + +## Pros and Cons of the Options + +### Create a temporary file in a local folder and copy after successful write + +* Good, because Keeps directory of .bib file clean +* Good, because Keeping file access rights is simple as the file content is replaced, not the file itself +* Bad, because Error recovery is hard + +### Create temporary file next to bib file and atomically move it to the original file + +This implementation is available at [Marty Lamb's AtomicFileOutputStream](https://github.com/martylamb/atomicfileoutputstream/blob/master/src/main/java/com/martiansoftware/io/AtomicFileOutputStream.java) +and [Apache Zookeepr's AtomicFileOutputStream](https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/common/AtomicFileOutputStream.java). + +* Good, because Atomic move is an all-or-nothing move +* Bad, because Makes issues with Dropbox, OneDrive, ... diff --git a/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java b/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java index e22f5790ceb..1257161f113 100644 --- a/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java +++ b/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java @@ -227,7 +227,7 @@ private boolean saveDatabase(Path file, boolean selectedOnly, Charset encoding, SavePreferences savePreferences = this.preferences.getSavePreferences() .withSaveType(saveType); BibDatabaseContext bibDatabaseContext = libraryTab.getBibDatabaseContext(); - try (AtomicFileWriter fileWriter = new AtomicFileWriter(file, encoding, savePreferences.shouldMakeBackup())) { + try (AtomicFileWriter fileWriter = new AtomicFileWriter(file, encoding)) { BibWriter bibWriter = new BibWriter(fileWriter, bibDatabaseContext.getDatabase().getNewLineSeparator()); BibtexDatabaseWriter databaseWriter = new BibtexDatabaseWriter(bibWriter, generalPreferences, savePreferences, entryTypesManager); diff --git a/src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java b/src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java index 09b0e25f460..a78b8b5db7c 100644 --- a/src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java +++ b/src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java @@ -3,14 +3,14 @@ import java.io.FileOutputStream; import java.io.FilterOutputStream; import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; import java.nio.channels.FileLock; import java.nio.channels.OverlappingFileLockException; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardCopyOption; -import java.nio.file.attribute.PosixFilePermission; -import java.util.EnumSet; -import java.util.Set; +import java.nio.file.StandardOpenOption; import org.jabref.logic.util.BackupFileType; import org.jabref.logic.util.io.FileUtil; @@ -25,10 +25,11 @@ *

* In detail, the strategy is to: *

    - *
  1. Write to a temporary file (with .tmp suffix) in the same directory as the destination file.
    2. - *
  2. Create a backup (with .bak suffix) of the original file (if it exists) in the same directory.
    3. - *
  3. Move the temporary file to the correct place, overwriting any file that already exists at that location.
    4. - *
  4. Delete the backup file (if configured to do so).
    5. + *
  5. Create a backup (with .bak suffix) of the original file (if it exists) in the UserDataDir.
    6. + *
  6. Write to a temporary file (with .tmp suffix) in the system-wide temporary directory.
    7. + *
  7. Copy the content of the temporary file to the .bib file, overwriting any content that already exists in that file.
    8. + *
  8. Delete the temporary file.
    9. + *
  9. Rename the .bak to .old.
    10. *
* If all goes well, no temporary or backup files will remain on disk after closing the stream. *

@@ -36,12 +37,9 @@ *

    *
  1. If anything goes wrong while writing to the temporary file, the temporary file will be deleted (leaving the * original file untouched).
    2. - *
  2. If anything goes wrong while copying the temporary file to the target file, the backup of the original file is - * kept.
    3. + *
  3. If anything goes wrong while copying the temporary file to the target file, the backup of the original file is written into the original file.
    4. + *
  4. If that rescue goes wrong, JabRef knows at the start that there is a .bak file and will prompt the user.
    5. *
- *

- * Implementation inspired by code from Marty - * Lamb and Apache. */ public class AtomicFileOutputStream extends FilterOutputStream { @@ -49,6 +47,7 @@ public class AtomicFileOutputStream extends FilterOutputStream { private static final String TEMPORARY_EXTENSION = ".tmp"; private static final String SAVE_EXTENSION = BackupFileType.SAVE.getExtensions().get(0); + private static final String OLD_EXTENSION = ".old"; /** * The file we want to create/replace. @@ -59,26 +58,33 @@ public class AtomicFileOutputStream extends FilterOutputStream { * The file to which writes are redirected to. */ private final Path temporaryFile; + + // The lock can be a local variable, because the locking is done by the operating system (and not a Java-based lock) private final FileLock temporaryFileLock; + /** * A backup of the target file (if it exists), created when the stream is closed */ private final Path backupFile; - private final boolean keepBackup; + + private boolean errorDuringWrite = false; /** * Creates a new output stream to write to or replace the file at the specified path. * * @param path the path of the file to write to or replace - * @param keepBackup whether to keep the backup file after a successful write process */ - public AtomicFileOutputStream(Path path, boolean keepBackup) throws IOException { + public AtomicFileOutputStream(Path path) throws IOException { super(Files.newOutputStream(getPathOfTemporaryFile(path))); this.targetFile = path; this.temporaryFile = getPathOfTemporaryFile(path); - this.backupFile = getPathOfSaveBackupFile(path); - this.keepBackup = keepBackup; + this.backupFile = FileUtil.getPathOfBackupFileAndCreateDirectory(path, BackupFileType.SAVE); + + if (Files.exists(targetFile)) { + // Make a backup of the original file + Files.copy(targetFile, backupFile, StandardCopyOption.REPLACE_EXISTING); + } try { // Lock files (so that at least not another JabRef instance writes at the same time to the same tmp file) @@ -93,16 +99,18 @@ public AtomicFileOutputStream(Path path, boolean keepBackup) throws IOException } /** - * Creates a new output stream to write to or replace the file at the specified path. The backup file is deleted when the write was successful. - * - * @param path the path of the file to write to or replace + * Determines the path of the temporary file. */ - public AtomicFileOutputStream(Path path) throws IOException { - this(path, false); - } - - private static Path getPathOfTemporaryFile(Path targetFile) { - return FileUtil.addExtension(targetFile, TEMPORARY_EXTENSION); + static Path getPathOfTemporaryFile(Path targetFile) { + Path tempFile; + try { + tempFile = Files.createTempFile(targetFile.getFileName().toString(), TEMPORARY_EXTENSION); + } catch (IOException e) { + Path result = FileUtil.addExtension(targetFile, TEMPORARY_EXTENSION); + LOGGER.warn("Could not create bib writing temporary file, using {} as file", result, e); + return result; + } + return tempFile; } private static Path getPathOfSaveBackupFile(Path targetFile) { @@ -117,7 +125,7 @@ public Path getBackup() { } /** - * Override for performance reasons. + * Overridden because of cleanup actions in case of an error */ @Override public void write(byte b[], int off, int len) throws IOException { @@ -125,6 +133,7 @@ public void write(byte b[], int off, int len) throws IOException { out.write(b, off, len); } catch (IOException exception) { cleanup(); + errorDuringWrite = true; throw exception; } } @@ -138,23 +147,17 @@ public void abort() { Files.deleteIfExists(temporaryFile); Files.deleteIfExists(backupFile); } catch (IOException exception) { - LOGGER.debug("Unable to abort writing to file " + temporaryFile, exception); + LOGGER.debug("Unable to abort writing to file {}", temporaryFile, exception); } } private void cleanup() { - try { - Files.deleteIfExists(temporaryFile); - } catch (IOException exception) { - LOGGER.debug("Unable to delete file " + temporaryFile, exception); - } - try { if (temporaryFileLock != null) { temporaryFileLock.release(); } } catch (IOException exception) { - LOGGER.warn("Unable to release lock on file " + temporaryFile, exception); + LOGGER.warn("Unable to release lock on file {}", temporaryFile, exception); } } @@ -175,43 +178,43 @@ public void close() throws IOException { } super.close(); - // We successfully wrote everything to the temporary file, lets copy it to the correct place - // First, make backup of original file and try to save file permissions to restore them later (by default: 664) - Set oldFilePermissions = EnumSet.of(PosixFilePermission.OWNER_READ, - PosixFilePermission.OWNER_WRITE, - PosixFilePermission.GROUP_READ, - PosixFilePermission.GROUP_WRITE, - PosixFilePermission.OTHERS_READ); - if (Files.exists(targetFile)) { - Files.copy(targetFile, backupFile, StandardCopyOption.REPLACE_EXISTING); - if (FileUtil.IS_POSIX_COMPLIANT) { - try { - oldFilePermissions = Files.getPosixFilePermissions(targetFile); - } catch (IOException exception) { - LOGGER.warn("Error getting file permissions for file {}.", targetFile, exception); - } - } + if (!errorDuringWrite) { + // We successfully wrote everything to the temporary file, let's copy it to the correct place + replaceOriginalFileByWrittenFile(); } + } finally { + cleanup(); + } + } - // Move temporary file (replace original if it exists) - Files.move(temporaryFile, targetFile, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING); - - // Restore file permissions - if (FileUtil.IS_POSIX_COMPLIANT) { - try { - Files.setPosixFilePermissions(targetFile, oldFilePermissions); - } catch (IOException exception) { - LOGGER.warn("Error writing file permissions to file {}.", targetFile, exception); + private void replaceOriginalFileByWrittenFile() throws IOException { + // Move temporary file (replace original if it exists) + // We implement the move as write into the original and delete the temporary one to keep file permissions etc. + try (InputStream inputStream = Files.newInputStream(temporaryFile, StandardOpenOption.DELETE_ON_CLOSE); + OutputStream outputStream = Files.newOutputStream(targetFile)) { + inputStream.transferTo(outputStream); + } catch (IOException e) { + LOGGER.error("Could not write into final .bib file {}", targetFile, e); + if (Files.size(targetFile) != Files.size(backupFile)) { + LOGGER.debug("Size of target file and backup file differ. Trying to restore target file from backup."); + LOGGER.info("Trying to restore backup from {} to {}", backupFile, targetFile); + try (InputStream inputStream = Files.newInputStream(backupFile); + OutputStream outputStream = Files.newOutputStream(targetFile)) { + inputStream.transferTo(outputStream); + } catch (IOException ex) { + LOGGER.error("Could not restore backup from {} to {}", backupFile, targetFile, ex); + throw ex; } + LOGGER.info("Backup restored"); } - - if (!keepBackup) { - // Remove backup file - Files.deleteIfExists(backupFile); - } - } finally { - // Remove temporary file (but not the backup!) - cleanup(); + // we rethrow the original error to indicate that writing (somehow) went wrong + throw e; + } + String filenameOld = FileUtil.getBaseName(backupFile) + OLD_EXTENSION; + try { + Files.move(backupFile, backupFile.resolveSibling(filenameOld), StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.COPY_ATTRIBUTES); + } catch (IOException e) { + LOGGER.warn("Could not rename {} to {}", backupFile, filenameOld, e); } } @@ -231,6 +234,7 @@ public void write(int b) throws IOException { super.write(b); } catch (IOException exception) { cleanup(); + errorDuringWrite = true; throw exception; } } diff --git a/src/main/java/org/jabref/logic/exporter/AtomicFileWriter.java b/src/main/java/org/jabref/logic/exporter/AtomicFileWriter.java index 19274cbeafa..febbd4e7d45 100644 --- a/src/main/java/org/jabref/logic/exporter/AtomicFileWriter.java +++ b/src/main/java/org/jabref/logic/exporter/AtomicFileWriter.java @@ -22,11 +22,7 @@ public class AtomicFileWriter extends OutputStreamWriter { private final Set problemCharacters = new TreeSet<>(); public AtomicFileWriter(Path file, Charset encoding) throws IOException { - this(file, encoding, false); - } - - public AtomicFileWriter(Path file, Charset encoding, boolean keepBackup) throws IOException { - super(new AtomicFileOutputStream(file, keepBackup), encoding); + super(new AtomicFileOutputStream(file), encoding); encoder = encoding.newEncoder(); } diff --git a/src/test/java/org/jabref/logic/exporter/AtomicFileOutputStreamTest.java b/src/test/java/org/jabref/logic/exporter/AtomicFileOutputStreamTest.java new file mode 100644 index 00000000000..4d66b9acdbc --- /dev/null +++ b/src/test/java/org/jabref/logic/exporter/AtomicFileOutputStreamTest.java @@ -0,0 +1,58 @@ +package org.jabref.logic.exporter; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.FileSystem; +import java.nio.file.Files; +import java.nio.file.Path; + +import org.jabref.logic.util.BackupFileType; +import org.jabref.logic.util.io.BackupFileUtil; +import org.jabref.logic.util.io.FileUtil; + +import com.google.common.base.Strings; +import com.google.common.jimfs.Configuration; +import com.google.common.jimfs.Jimfs; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +class AtomicFileOutputStreamTest { + + /** + * Tests whether a failing write to a file keeps the respective .sav file + */ + @Test + public void savFileExistsOnDiskFull() throws Exception { + String fiftyChars = Strings.repeat("1234567890", 5); + String fiveThousandChars = Strings.repeat("A", 5_000); + + FileSystem fileSystem = Jimfs.newFileSystem( + Configuration.unix().toBuilder() + .setMaxSize(1_000) + .setBlockSize(1_000).build()); + + Path out = fileSystem.getPath("out.bib"); + Files.writeString(out, fiftyChars); + + // Running out of disk space should occur + assertThrows(IOException.class, () -> { + AtomicFileOutputStream atomicFileOutputStream = new AtomicFileOutputStream(out); + InputStream inputStream = new ByteArrayInputStream(fiveThousandChars.getBytes()); + inputStream.transferTo(atomicFileOutputStream); + atomicFileOutputStream.close(); + }); + + // Written file still has the contents as before the error + assertEquals(fiftyChars, Files.readString(out)); + } + + @Test + void tempAndBackupDifferentPaths() { + Path testFile = Path.of("test.bib"); + assertNotEquals(AtomicFileOutputStream.getPathOfTemporaryFile(testFile), BackupFileUtil.getPathForNewBackupFileAndCreateDirectory(testFile, BackupFileType.SAVE)); + } +} diff --git a/src/test/java/org/jabref/logic/exporter/AtomicFileWriterTest.java b/src/test/java/org/jabref/logic/exporter/AtomicFileWriterTest.java new file mode 100644 index 00000000000..4c28ad82643 --- /dev/null +++ b/src/test/java/org/jabref/logic/exporter/AtomicFileWriterTest.java @@ -0,0 +1,66 @@ +package org.jabref.logic.exporter; + +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Set; + +import org.jabref.model.database.BibDatabase; +import org.jabref.model.database.BibDatabaseContext; +import org.jabref.model.entry.BibEntry; +import org.jabref.model.entry.BibEntryTypesManager; +import org.jabref.model.entry.field.StandardField; +import org.jabref.model.metadata.MetaData; +import org.jabref.model.metadata.SaveOrderConfig; +import org.jabref.preferences.GeneralPreferences; + +import com.google.common.base.Strings; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.mockito.Answers; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +class AtomicFileWriterTest { + + @Test + void encodingIssueDoesNotLeadToCrash(@TempDir Path tempDir) throws Exception { + Path target = tempDir.resolve("test.txt"); + AtomicFileWriter atomicFileWriter = new AtomicFileWriter(target, StandardCharsets.US_ASCII); + atomicFileWriter.write("ñ"); + atomicFileWriter.close(); + assertTrue(atomicFileWriter.hasEncodingProblems()); + assertEquals(Set.of('ñ'), atomicFileWriter.getEncodingProblems()); + } + + @Test + void bibFileIsKeptAtError(@TempDir Path tempDir) throws Exception { + Path target = tempDir.resolve("test.bib"); + + String fiveThousandChars = Strings.repeat("A", 5_000); + Files.writeString(target, fiveThousandChars); + + AtomicFileWriter fileWriter = new AtomicFileWriter(target, StandardCharsets.UTF_8); + BibDatabase database = new BibDatabase(); + MetaData metaData = new MetaData(); + BibDatabaseContext bibDatabaseContext = new BibDatabaseContext(database, metaData); + BibWriter bibWriter = new BibWriter(fileWriter, "\n"); + + GeneralPreferences generalPreferences = mock(GeneralPreferences.class); + SavePreferences savePreferences = mock(SavePreferences.class, Answers.RETURNS_DEEP_STUBS); + when(savePreferences.getSaveOrder()).thenReturn(new SaveOrderConfig()); + when(savePreferences.takeMetadataSaveOrderInAccount()).thenReturn(true); + BibEntryTypesManager entryTypesManager = new BibEntryTypesManager(); + BibtexDatabaseWriter databaseWriter = new BibtexDatabaseWriter(bibWriter, generalPreferences, savePreferences, entryTypesManager); + + BibEntry bibEntry = new BibEntry().withField(StandardField.NOTE, "{"); + database.insertEntry(bibEntry); + + assertThrows(Exception.class, () -> databaseWriter.saveDatabase(bibDatabaseContext)); + fileWriter.close(); + } +} diff --git a/src/test/java/org/jabref/logic/util/io/FileUtilTest.java b/src/test/java/org/jabref/logic/util/io/FileUtilTest.java index 8cb061fc99e..faa6e9da285 100644 --- a/src/test/java/org/jabref/logic/util/io/FileUtilTest.java +++ b/src/test/java/org/jabref/logic/util/io/FileUtilTest.java @@ -12,14 +12,19 @@ import java.util.stream.Stream; import org.jabref.logic.layout.LayoutFormatterPreferences; +import org.jabref.logic.util.BackupFileType; import org.jabref.model.entry.BibEntry; import org.jabref.model.entry.field.StandardField; import org.jabref.model.util.FileHelper; +import net.harawata.appdirs.AppDirs; +import net.harawata.appdirs.AppDirsFactory; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import org.mockito.Answers; +import org.mockito.MockedStatic; +import org.mockito.Mockito; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -401,4 +406,30 @@ void testFindInListOfPath() { List result = FileUtil.find("existingTestFile.txt", paths); assertEquals(resultPaths, result); } + + @Test + void uniqueFilePrefix() { + // The number "7001d6e0" is "random" + assertEquals("7001d6e0", FileUtil.getUniqueFilePrefix(Path.of("/tmp/test.bib"))); + } + + @Test + void getPathOfBackupFileAndCreateDirectoryReturnsAppDirectoryInCaseOfNoError() { + String start = FileUtil.getAppDataBackupDir().toString(); + String result = FileUtil.getPathOfBackupFileAndCreateDirectory(Path.of("/tmp/test.bib"), BackupFileType.BACKUP).toString(); + // We just check the prefix + assertEquals(start, result.substring(0, start.length())); + } + + @Test + void getPathOfBackupFileAndCreateDirectoryReturnsSameDirectoryInCaseOfException() { + try (MockedStatic files = Mockito.mockStatic(Files.class, Answers.RETURNS_DEEP_STUBS)) { + files.when(() -> Files.createDirectories(FileUtil.getAppDataBackupDir())) + .thenThrow(new IOException()); + Path testPath = Path.of("tmp", "test.bib"); + Path result = FileUtil.getPathOfBackupFileAndCreateDirectory(testPath, BackupFileType.BACKUP); + // We just check the prefix + assertEquals(Path.of("tmp", "test.bib.bak"), result); + } + } }