More conservative write

CHANGELOG.md

@@ -13,6 +13,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 changed the button label from "Return to JabRef" to "Return to library" to better indicate the purpose of the action.

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/JabRefGUI.java

@@ -77,8 +77,8 @@ private void openWindow(Stage mainStage) {
         if (guiPreferences.isWindowMaximised()) {
             mainStage.setMaximized(true);
         } else if ((Screen.getScreens().size() == 1) && isWindowPositionOutOfBounds()) {
-            // corrects the Window, if its outside of the mainscreen
-            LOGGER.debug("The Jabref Window is outside the Main Monitor\n");
+            // corrects the Window, if it is outside the mainscreen
+            LOGGER.debug("The Jabref window is outside the main screen\n");
             mainStage.setX(0);
             mainStage.setY(0);
             mainStage.setWidth(1024);

src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java

@@ -228,11 +228,13 @@ private boolean save(Path targetPath, SaveDatabaseMode mode) {
     }
 
     private boolean saveDatabase(Path file, boolean selectedOnly, Charset encoding, SavePreferences.DatabaseSaveType saveType) throws SaveException {
+        // if this code is adapted, please also adapt org.jabref.logic.autosaveandbackup.BackupManager.performBackup
+
         GeneralPreferences generalPreferences = this.preferences.getGeneralPreferences();
         SavePreferences savePreferences = this.preferences.getSavePreferences()
                                                       .withSaveType(saveType);
-        try (AtomicFileWriter fileWriter = new AtomicFileWriter(file, encoding, savePreferences.shouldMakeBackup())) {
-            BibDatabaseContext bibDatabaseContext = libraryTab.getBibDatabaseContext();
+        BibDatabaseContext bibDatabaseContext = libraryTab.getBibDatabaseContext();
+        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/autosaveandbackup/BackupManager.java

@@ -57,7 +57,7 @@ private BackupManager(BibDatabaseContext bibDatabaseContext, BibEntryTypesManage
         this.bibDatabaseContext = bibDatabaseContext;
         this.entryTypesManager = entryTypesManager;
         this.preferences = preferences;
-        this.throttler = new DelayTaskThrottler(15000);
+        this.throttler = new DelayTaskThrottler(15_000);
 
         changeFilter = new CoarseChangeFilter(bibDatabaseContext);
         changeFilter.registerListener(this);
@@ -107,7 +107,7 @@ public static boolean backupFileDiffers(Path originalPath) {
         }
 
         try {
-            return Files.mismatch(originalPath, backupPath) != -1L;
+            return !Files.isSameFile(originalPath, backupPath);
         } catch (IOException e) {
             LOGGER.debug("Could not compare original file and backup file.", e);
             // User has to investigate in this case
@@ -134,13 +134,13 @@ private Optional<Path> determineBackupPath() {
     }
 
     private void performBackup(Path backupPath) {
-        try {
-            Charset charset = bibDatabaseContext.getMetaData().getEncoding().orElse(StandardCharsets.UTF_8);
-            GeneralPreferences generalPreferences = preferences.getGeneralPreferences();
-            SavePreferences savePreferences = preferences.getSavePreferences()
-                                                         .withMakeBackup(false);
-            Writer writer = new AtomicFileWriter(backupPath, charset);
-            BibWriter bibWriter = new BibWriter(writer, OS.NEWLINE);
+        // code similar to org.jabref.gui.exporter.SaveDatabaseAction.saveDatabase
+        GeneralPreferences generalPreferences = preferences.getGeneralPreferences();
+        SavePreferences savePreferences = preferences.getSavePreferences()
+                                                     .withMakeBackup(false);
+        Charset encoding = bibDatabaseContext.getMetaData().getEncoding().orElse(StandardCharsets.UTF_8);
+        try (AtomicFileWriter fileWriter = new AtomicFileWriter(backupPath, encoding)) {
+            BibWriter bibWriter = new BibWriter(fileWriter, bibDatabaseContext.getDatabase().getNewLineSeparator());
             new BibtexDatabaseWriter(bibWriter, generalPreferences, savePreferences, entryTypesManager)
                     .saveDatabase(bibDatabaseContext);
         } catch (IOException e) {
@@ -157,7 +157,7 @@ private void logIfCritical(Path backupPath, IOException e) {
 
         // do not print errors in field values into the log during autosave
         if (!isErrorInField) {
-            LOGGER.error("Error while saving to file" + backupPath, e);
+            LOGGER.error("Error while saving to file {}", backupPath, e);
         }
     }
 

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.io.FileUtil;
 
@@ -24,30 +24,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 BACKUP_EXTENSION = ".bak";
+    static final String BACKUP_EXTENSION = ".bak";
+    private static final String OLD_EXTENSION = ".old";
 
     /**
      * The file we want to create/replace.
@@ -58,26 +57,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 = getPathOfBackupFile(path);
-        this.keepBackup = keepBackup;
+        this.backupFile = FileUtil.getPathOfBackupFile(path, BACKUP_EXTENSION);
+
+        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)
@@ -92,20 +98,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);
-    }
-
-    private static Path getPathOfBackupFile(Path targetFile) {
-        return FileUtil.addExtension(targetFile, BACKUP_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;
     }
 
     /**
@@ -116,14 +120,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;
         }
     }
@@ -137,23 +142,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);
         }
     }
 
@@ -174,43 +173,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);
         }
     }
 
@@ -230,6 +229,7 @@ public void write(int b) throws IOException {
             super.write(b);
         } catch (IOException exception) {
             cleanup();
+            errorDuringWrite = true;
             throw exception;
         }
     }