More conservative write

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`.

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'

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, ...

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);
 

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,30 +25,29 @@
  * <p>
  * In detail, the strategy is to:
  * <ol>
- * <li>Write to a temporary file (with .tmp suffix) in the same directory as the destination file.</li>
- * <li>Create a backup (with .bak suffix) of the original file (if it exists) in the same directory.</li>
- * <li>Move the temporary file to the correct place, overwriting any file that already exists at that location.</li>
- * <li>Delete the backup file (if configured to do so).</li>
+ * <li>Create a backup (with .bak suffix) of the original file (if it exists) in the <a href="https://github.com/harawata/appdirs#supported-directories">UserDataDir</a>.</li>
+ * <li>Write to a temporary file (with .tmp suffix) in the system-wide temporary directory.</li>
+ * <li>Copy the content of the temporary file to the .bib file, overwriting any content that already exists in that file.</li>
+ * <li>Delete the temporary file.</li>
+ * <li>Rename the .bak to .old.</li>
  * </ol>
  * If all goes well, no temporary or backup files will remain on disk after closing the stream.
  * <p>
  * Errors are handled as follows:
  * <ol>
  * <li>If anything goes wrong while writing to the temporary file, the temporary file will be deleted (leaving the
  * original file untouched).</li>
- * <li>If anything goes wrong while copying the temporary file to the target file, the backup of the original file is
- * kept.</li>
+ * <li>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.</li>
+ * <li>If that rescue goes wrong, JabRef knows at the start that there is a .bak file and will prompt the user.</li>
  * </ol>
- * <p>
- * Implementation inspired by code from <a href="https://github.com/martylamb/atomicfileoutputstream/blob/master/src/main/java/com/martiansoftware/io/AtomicFileOutputStream.java">Marty
- * Lamb</a> and <a href="https://github.com/apache/zookeeper/blob/master/src/java/main/org/apache/zookeeper/common/AtomicFileOutputStream.java">Apache</a>.
  */
 public class AtomicFileOutputStream extends FilterOutputStream {
 
     private static final Logger LOGGER = LoggerFactory.getLogger(AtomicFileOutputStream.class);
 
     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,14 +125,15 @@ 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 {
         try {
             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<PosixFilePermission> 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;
         }
     }

src/main/java/org/jabref/logic/exporter/AtomicFileWriter.java

@@ -22,11 +22,7 @@ public class AtomicFileWriter extends OutputStreamWriter {
     private final Set<Character> 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();
     }
 

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));
+    }
+}