More conservative write

CHANGELOG.md

@@ -17,6 +17,9 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 ### Changed
 
 - We improved the Latex2Unicode conversion [#8639](https://github.com/JabRef/jabref/pull/8639)
+- 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 changed the writing method of a `.bib` file: Existing files should have been modified instead of being replaced.
 - Writing BibTeX data into a PDF (XMP) removes braces. [#8452](https://github.com/JabRef/jabref/issues/8452)
 - Writing BibTeX data into a PDF (XMP) does not write the `file` field.
 - Writing BibTeX data into a PDF (XMP) considers the configured keyword separator (and does not use "," as default any more)

build.gradle

@@ -219,6 +219,12 @@ dependencies {
     testImplementation "org.testfx:testfx-junit5:4.0.17-alpha-SNAPSHOT"
     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.1'
     // 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'

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

@@ -3,17 +3,18 @@
 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 org.jabref.logic.util.BuildInfo;
 import org.jabref.logic.util.io.FileUtil;
 
+import net.harawata.appdirs.AppDirsFactory;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -58,13 +59,18 @@ public class AtomicFileOutputStream extends FilterOutputStream {
      * The file to which writes are redirected to.
      */
     private final Path temporaryFile;
+
     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.
      *
@@ -79,6 +85,11 @@ public AtomicFileOutputStream(Path path, boolean keepBackup) throws IOException
         this.backupFile = getPathOfBackupFile(path);
         this.keepBackup = keepBackup;
 
+        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)
             if (out instanceof FileOutputStream) {
@@ -100,12 +111,36 @@ public AtomicFileOutputStream(Path path) throws IOException {
         this(path, false);
     }
 
-    private static Path getPathOfTemporaryFile(Path targetFile) {
-        return FileUtil.addExtension(targetFile, TEMPORARY_EXTENSION);
+    private static Path getBibsPath() {
+        // We choose the cache dir as it is a local-to-app temporary directory
+        Path directory = Path.of(AppDirsFactory.getInstance().getUserCacheDir(
+                                     "jabref",
+                                     new BuildInfo().version.toString(),
+                                     "org.jabref"))
+                             .resolve("bibs");
+        return directory;
+    }
+
+    private static Path getPathOfSecondFile(Path targetFile, String extension) {
+        Path directory = getBibsPath();
+        try {
+            Files.createDirectories(directory);
+        } catch (IOException e) {
+            Path result = FileUtil.addExtension(targetFile, extension);
+            LOGGER.warn("Could not create bib writing directory {}, using {} as file", directory, result, e);
+            return result;
+        }
+        // By using a part of the hex string, we keep some old versions, but not all
+        String fileName = FileUtil.getBaseName(targetFile) + "-" + Integer.toHexString(targetFile.hashCode()).substring(0, 2) + extension + ".bib";
+        return directory.resolve(fileName);
     }
 
-    private static Path getPathOfBackupFile(Path targetFile) {
-        return FileUtil.addExtension(targetFile, BACKUP_EXTENSION);
+    static Path getPathOfTemporaryFile(Path targetFile) {
+        return getPathOfSecondFile(targetFile, TEMPORARY_EXTENSION);
+    }
+
+    static Path getPathOfBackupFile(Path targetFile) {
+        return getPathOfSecondFile(targetFile, BACKUP_EXTENSION);
     }
 
     /**
@@ -116,14 +151,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 +173,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,46 +204,44 @@ 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_COMPILANT) {
-                    try {
-                        oldFilePermissions = Files.getPosixFilePermissions(targetFile);
-                    } catch (IOException exception) {
-                        LOGGER.warn("Error getting file permissions for file {}.", targetFile, exception);
-                    }
-                }
-            }
-
-            // 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_COMPILANT) {
-                try {
-                    Files.setPosixFilePermissions(targetFile, oldFilePermissions);
-                } catch (IOException exception) {
-                    LOGGER.warn("Error writing file permissions to file {}.", targetFile, exception);
-                }
+            if (!errorDuringWrite) {
+                // We successfully wrote everything to the temporary file, let's copy it to the correct place
+                replaceOriginalFileByWrittenFile();
             }
 
             if (!keepBackup) {
-                // Remove backup file
                 Files.deleteIfExists(backupFile);
             }
         } finally {
-            // Remove temporary file (but not the backup!)
             cleanup();
         }
     }
 
+    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);
+             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");
+            }
+            // we rethrow the original error to indicate that writing (somehow) went wrong
+            throw e;
+        }
+    }
+
     @Override
     public void flush() throws IOException {
         try {
@@ -230,6 +258,7 @@ public void write(int b) throws IOException {
             super.write(b);
         } catch (IOException exception) {
             cleanup();
+            errorDuringWrite = true;
             throw exception;
         }
     }

src/test/java/org/jabref/logic/exporter/AtomicFileOutputStreamTest.java

@@ -0,0 +1,52 @@
+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 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.*;
+
+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, false);
+            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), AtomicFileOutputStream.getPathOfBackupFile(testFile));
+    }
+}

src/test/java/org/jabref/logic/exporter/AtomicFileWriterTest.java

@@ -0,0 +1,68 @@
+package org.jabref.logic.exporter;
+
+import java.io.StringWriter;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Set;
+
+import org.jabref.logic.util.OS;
+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();
+    }
+}