Restoring a Unix Backup on Windows is not working well - Reopen (#150)

- Changes change detector to always use the selected permission comparison strategy - Removes the file create datetime from the metadata change detection criteria as it is unreliable - Updates tests Resolves #94 {patch} Signed-off-by: Esta Nagy <[email protected]>
nagyesta · Feb 20, 2024 · 1c723b9 · 1c723b9
1 parent f80576a
commit 1c723b9
Show file tree

Hide file tree

Showing 12 changed files with 44 additions and 34 deletions.
diff --git a/...ore/src/main/java/com/github/nagyesta/filebarj/core/backup/pipeline/BackupController.java b/...ore/src/main/java/com/github/nagyesta/filebarj/core/backup/pipeline/BackupController.java
@@ -4,10 +4,7 @@
 import com.github.nagyesta.filebarj.core.backup.worker.DefaultBackupScopePartitioner;
 import com.github.nagyesta.filebarj.core.backup.worker.FileMetadataParser;
 import com.github.nagyesta.filebarj.core.backup.worker.FileMetadataParserFactory;
-import com.github.nagyesta.filebarj.core.common.FileMetadataChangeDetector;
-import com.github.nagyesta.filebarj.core.common.FileMetadataChangeDetectorFactory;
-import com.github.nagyesta.filebarj.core.common.ManifestManager;
-import com.github.nagyesta.filebarj.core.common.ManifestManagerImpl;
+import com.github.nagyesta.filebarj.core.common.*;
 import com.github.nagyesta.filebarj.core.config.BackupJobConfiguration;
 import com.github.nagyesta.filebarj.core.model.BackupIncrementManifest;
 import com.github.nagyesta.filebarj.core.model.BackupPath;
@@ -139,7 +136,8 @@ private void calculateBackupDelta() {
         final var previousFiles = new TreeMap<String, Map<UUID, FileMetadata>>();
         previousManifests.forEach((key, value) -> previousFiles.put(value.getFileNamePrefix(), value.getFiles()));
         if (!previousManifests.isEmpty()) {
-            changeDetector = FileMetadataChangeDetectorFactory.create(manifest.getConfiguration(), previousFiles);
+            changeDetector = FileMetadataChangeDetectorFactory
+                    .create(manifest.getConfiguration(), previousFiles, PermissionComparisonStrategy.STRICT);
             log.info("Trying to find unchanged files in previous backup increments");
             threadPool.submit(() -> this.filesFound.parallelStream()
                     .forEach(this::findPreviousVersionToReuseOrAddToBackupFileSet)).join();

diff --git a/...rc/main/java/com/github/nagyesta/filebarj/core/common/BaseFileMetadataChangeDetector.java b/...rc/main/java/com/github/nagyesta/filebarj/core/common/BaseFileMetadataChangeDetector.java
@@ -4,9 +4,7 @@
 import com.github.nagyesta.filebarj.core.model.FileMetadata;
 import com.github.nagyesta.filebarj.core.model.enums.Change;
 import com.github.nagyesta.filebarj.core.model.enums.FileType;
-import lombok.Getter;
 import lombok.NonNull;
-import lombok.Setter;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
@@ -22,24 +20,24 @@ public abstract class BaseFileMetadataChangeDetector<T> implements FileMetadataC
     private final SortedMap<String, Map<UUID, FileMetadata>> filesFromManifests;
     private final SortedMap<String, Map<T, List<FileMetadata>>> contentIndex;
     private final Map<String, FileMetadata> nameIndex;
-    @NonNull
-    @Getter
-    @Setter
-    private PermissionComparisonStrategy permissionComparisonStrategy = PermissionComparisonStrategy.STRICT;
+    private final PermissionComparisonStrategy permissionComparisonStrategy;
 
     /**
      * Creates a new instance with the previous manifests.
      *
      * @param filesFromManifests The files found in the previous manifests
+     * @param permissionStrategy The permission comparison strategy
      */
     protected BaseFileMetadataChangeDetector(
-            @NotNull final Map<String, Map<UUID, FileMetadata>> filesFromManifests) {
+            @NotNull final Map<String, Map<UUID, FileMetadata>> filesFromManifests,
+            @Nullable final PermissionComparisonStrategy permissionStrategy) {
         this.filesFromManifests = new TreeMap<>(filesFromManifests);
         final SortedMap<String, Map<T, List<FileMetadata>>> contentIndex = new TreeMap<>();
         final Map<String, FileMetadata> nameIndex = new TreeMap<>();
         index(this.filesFromManifests, contentIndex, nameIndex);
         this.contentIndex = contentIndex;
         this.nameIndex = nameIndex;
+        this.permissionComparisonStrategy = Objects.requireNonNullElse(permissionStrategy, PermissionComparisonStrategy.STRICT);
     }
 
     @Override
@@ -49,8 +47,7 @@ public boolean hasMetadataChanged(
         final var permissionsChanged = !permissionComparisonStrategy.matches(previousMetadata, currentMetadata);
         final var hiddenStatusChanged = currentMetadata.getHidden() != previousMetadata.getHidden();
         final var timesChanged = currentMetadata.getFileType() != FileType.SYMBOLIC_LINK
-                && (!Objects.equals(currentMetadata.getCreatedUtcEpochSeconds(), previousMetadata.getCreatedUtcEpochSeconds())
-                || !Objects.equals(currentMetadata.getLastModifiedUtcEpochSeconds(), previousMetadata.getLastModifiedUtcEpochSeconds()));
+                && !Objects.equals(currentMetadata.getLastModifiedUtcEpochSeconds(), previousMetadata.getLastModifiedUtcEpochSeconds());
         return hiddenStatusChanged || permissionsChanged || timesChanged;
     }
 

diff --git a/...main/java/com/github/nagyesta/filebarj/core/common/FileMetadataChangeDetectorFactory.java b/...main/java/com/github/nagyesta/filebarj/core/common/FileMetadataChangeDetectorFactory.java
@@ -4,6 +4,7 @@
 import com.github.nagyesta.filebarj.core.config.enums.HashAlgorithm;
 import com.github.nagyesta.filebarj.core.model.FileMetadata;
 import lombok.NonNull;
+import org.jetbrains.annotations.Nullable;
 
 import java.util.Map;
 import java.util.UUID;
@@ -18,18 +19,20 @@ public class FileMetadataChangeDetectorFactory {
      *
      * @param configuration      The backup configuration
      * @param filesFromManifests The previous manifests
+     * @param permissionStrategy The permission comparison strategy
      * @return The new instance
      */
     public static FileMetadataChangeDetector create(
             @NonNull final BackupJobConfiguration configuration,
-            @NonNull final Map<String, Map<UUID, FileMetadata>> filesFromManifests) {
+            @NonNull final Map<String, Map<UUID, FileMetadata>> filesFromManifests,
+            @Nullable final PermissionComparisonStrategy permissionStrategy) {
         if (filesFromManifests.isEmpty()) {
             throw new IllegalArgumentException("Previous manifests cannot be empty");
         }
         if (configuration.getHashAlgorithm() == HashAlgorithm.NONE) {
-            return new SimpleFileMetadataChangeDetector(filesFromManifests);
+            return new SimpleFileMetadataChangeDetector(filesFromManifests, permissionStrategy);
         } else {
-            return new HashingFileMetadataChangeDetector(filesFromManifests);
+            return new HashingFileMetadataChangeDetector(filesFromManifests, permissionStrategy);
         }
     }
 }
diff --git a/...main/java/com/github/nagyesta/filebarj/core/common/HashingFileMetadataChangeDetector.java b/...main/java/com/github/nagyesta/filebarj/core/common/HashingFileMetadataChangeDetector.java
@@ -3,6 +3,7 @@
 import com.github.nagyesta.filebarj.core.model.FileMetadata;
 import lombok.NonNull;
 import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
 
 import java.util.Map;
 import java.util.Objects;
@@ -17,10 +18,12 @@ public class HashingFileMetadataChangeDetector extends BaseFileMetadataChangeDet
      * Creates a new instance with the previous manifests.
      *
      * @param filesFromManifests The files found in the previous manifests
+     * @param permissionStrategy The permission comparison strategy
      */
     protected HashingFileMetadataChangeDetector(
-            @NotNull final Map<String, Map<UUID, FileMetadata>> filesFromManifests) {
-        super(filesFromManifests);
+            @NotNull final Map<String, Map<UUID, FileMetadata>> filesFromManifests,
+            @Nullable final PermissionComparisonStrategy permissionStrategy) {
+        super(filesFromManifests, permissionStrategy);
     }
 
     @Override

diff --git a/.../main/java/com/github/nagyesta/filebarj/core/common/SimpleFileMetadataChangeDetector.java b/.../main/java/com/github/nagyesta/filebarj/core/common/SimpleFileMetadataChangeDetector.java
@@ -3,6 +3,7 @@
 import com.github.nagyesta.filebarj.core.model.FileMetadata;
 import lombok.NonNull;
 import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
 
 import java.util.Map;
 import java.util.Objects;
@@ -17,10 +18,12 @@ public class SimpleFileMetadataChangeDetector extends BaseFileMetadataChangeDete
      * Creates a new instance with the previous manifests.
      *
      * @param filesFromManifests The files found in the previous manifests
+     * @param permissionStrategy The permission comparison strategy
      */
     protected SimpleFileMetadataChangeDetector(
-            @NotNull final Map<String, Map<UUID, FileMetadata>> filesFromManifests) {
-        super(filesFromManifests);
+            @NotNull final Map<String, Map<UUID, FileMetadata>> filesFromManifests,
+            @Nullable final PermissionComparisonStrategy permissionStrategy) {
+        super(filesFromManifests, permissionStrategy);
     }
 
     @Override

diff --git a/...ore/src/main/java/com/github/nagyesta/filebarj/core/restore/pipeline/RestorePipeline.java b/...ore/src/main/java/com/github/nagyesta/filebarj/core/restore/pipeline/RestorePipeline.java
@@ -81,7 +81,8 @@ public RestorePipeline(@NonNull final RestoreManifest manifest,
         if (manifest.getMaximumAppVersion().compareTo(new AppVersion()) > 0) {
             throw new IllegalArgumentException("Manifests were saved with a newer version of the application");
         }
-        this.changeDetector = FileMetadataChangeDetectorFactory.create(manifest.getConfiguration(), manifest.getFiles());
+        this.changeDetector = FileMetadataChangeDetectorFactory
+                .create(manifest.getConfiguration(), manifest.getFiles(), permissionStrategy);
         this.manifest = manifest;
         this.backupDirectory = backupDirectory;
         this.restoreTargets = restoreTargets;

diff --git a/...thub/nagyesta/filebarj/core/common/AbstractFileMetadataChangeDetectorIntegrationTest.java b/...thub/nagyesta/filebarj/core/common/AbstractFileMetadataChangeDetectorIntegrationTest.java
@@ -134,11 +134,11 @@ public static Stream<Arguments> commonFileContentProvider() {
     }
 
     protected SimpleFileMetadataChangeDetector getDefaultSimpleFileMetadataChangeDetector(final FileMetadata prev) {
-        return new SimpleFileMetadataChangeDetector(Map.of("test", Map.of(prev.getId(), prev)));
+        return new SimpleFileMetadataChangeDetector(Map.of("test", Map.of(prev.getId(), prev)), null);
     }
 
     protected HashingFileMetadataChangeDetector getDefaultHashingFileMetadataChangeDetector(final FileMetadata prev) {
-        return new HashingFileMetadataChangeDetector(Map.of("test", Map.of(prev.getId(), prev)));
+        return new HashingFileMetadataChangeDetector(Map.of("test", Map.of(prev.getId(), prev)), null);
     }
 
     protected FileMetadata createMetadata(

diff --git a/.../java/com/github/nagyesta/filebarj/core/common/FileMetadataChangeDetectorFactoryTest.java b/.../java/com/github/nagyesta/filebarj/core/common/FileMetadataChangeDetectorFactoryTest.java
@@ -23,7 +23,8 @@ void testCreateShouldThrowExceptionWhenCalledWithNullConfiguration() {
 
         //when
         Assertions.assertThrows(IllegalArgumentException.class,
-                () -> FileMetadataChangeDetectorFactory.create(null, Map.of("key", Map.of())));
+                () -> FileMetadataChangeDetectorFactory
+                        .create(null, Map.of("key", Map.of()), PermissionComparisonStrategy.STRICT));
 
         //then + exception
     }
@@ -35,7 +36,8 @@ void testCreateShouldThrowExceptionWhenCalledWithNullFiles() {
 
         //when
         Assertions.assertThrows(IllegalArgumentException.class,
-                () -> FileMetadataChangeDetectorFactory.create(mock(BackupJobConfiguration.class), null));
+                () -> FileMetadataChangeDetectorFactory
+                        .create(mock(BackupJobConfiguration.class), null, PermissionComparisonStrategy.STRICT));
 
         //then + exception
     }
@@ -55,7 +57,8 @@ void testCreateShouldReturnSimpleFileMetadataChangeDetectorWhenCalledWithNoneHas
                 .build();
 
         //when
-        final var actual = FileMetadataChangeDetectorFactory.create(config, Map.of("key", Map.of()));
+        final var actual = FileMetadataChangeDetectorFactory
+                .create(config, Map.of("key", Map.of()), PermissionComparisonStrategy.STRICT);
 
         //then
         Assertions.assertInstanceOf(SimpleFileMetadataChangeDetector.class, actual);
@@ -76,7 +79,8 @@ void testCreateShouldReturnHashingFileMetadataChangeDetectorWhenCalledWithSha256
                 .build();
 
         //when
-        final var actual = FileMetadataChangeDetectorFactory.create(config, Map.of("key", Map.of()));
+        final var actual = FileMetadataChangeDetectorFactory
+                .create(config, Map.of("key", Map.of()), PermissionComparisonStrategy.STRICT);
 
         //then
         Assertions.assertInstanceOf(HashingFileMetadataChangeDetector.class, actual);

diff --git a/...ithub/nagyesta/filebarj/core/common/HashingFileMetadataChangeDetectorIntegrationTest.java b/...ithub/nagyesta/filebarj/core/common/HashingFileMetadataChangeDetectorIntegrationTest.java
@@ -68,7 +68,7 @@ void testHashingChangeDetectorShouldDetectChangesWhenContentWasRolledBack()
         waitASecond();
         final var curr = createMetadata("file.txt", "content-1", FileType.REGULAR_FILE, "rwxrwxrwx", true);
         final var manifests = Map.of("1", Map.of(orig.getId(), orig), "2", Map.of(prev.getId(), prev));
-        final var underTest = new HashingFileMetadataChangeDetector(manifests);
+        final var underTest = new HashingFileMetadataChangeDetector(manifests, null);
 
         //when
         final var relevant = underTest.findMostRelevantPreviousVersion(curr);
@@ -94,7 +94,7 @@ void testFindMostRelevantPreviousVersionByContentShouldFallbackToFilePathWhenCon
         waitASecond();
         final var curr = createMetadata("file.txt", "content-3", FileType.REGULAR_FILE, "rwxrwxrwx", true);
         final var manifests = Map.of("1", Map.of(orig.getId(), orig), "2", Map.of(prev.getId(), prev));
-        final var underTest = new HashingFileMetadataChangeDetector(manifests);
+        final var underTest = new HashingFileMetadataChangeDetector(manifests, null);
 
         //when
         final var actual = underTest.findMostRelevantPreviousVersion(curr);

diff --git a/...github/nagyesta/filebarj/core/common/SimpleFileMetadataChangeDetectorIntegrationTest.java b/...github/nagyesta/filebarj/core/common/SimpleFileMetadataChangeDetectorIntegrationTest.java
@@ -77,7 +77,7 @@ void testSimpleChangeDetectorShouldDetectChangesWhenContentWasRolledBack()
         FileMetadataSetterFactory.newInstance(restoreTargets, null).setTimestamps(orig);
         final var restored = PARSER.parse(curr.getAbsolutePath().toFile(), CONFIGURATION);
         final var manifests = Map.of("1", Map.of(orig.getId(), orig), "2", Map.of(prev.getId(), prev));
-        final var underTest = new SimpleFileMetadataChangeDetector(manifests);
+        final var underTest = new SimpleFileMetadataChangeDetector(manifests, null);
 
         //when
         final var relevant = underTest.findMostRelevantPreviousVersion(restored);
@@ -103,7 +103,7 @@ void testFindMostRelevantPreviousVersionByContentShouldFallbackToFilePathWhenCon
         waitASecond();
         final var curr = createMetadata("file.txt", "content-3", FileType.REGULAR_FILE, "rwxrwxrwx", true);
         final var manifests = Map.of("1", Map.of(orig.getId(), orig), "2", Map.of(prev.getId(), prev));
-        final var underTest = new SimpleFileMetadataChangeDetector(manifests);
+        final var underTest = new SimpleFileMetadataChangeDetector(manifests, null);
 
         //when
         final var actual = underTest.findMostRelevantPreviousVersion(curr);

diff --git a/...m/github/nagyesta/filebarj/core/restore/pipeline/CrossPlatformRestoreIntegrationTest.java b/...m/github/nagyesta/filebarj/core/restore/pipeline/CrossPlatformRestoreIntegrationTest.java
@@ -1,6 +1,7 @@
 package com.github.nagyesta.filebarj.core.restore.pipeline;
 
 import com.github.nagyesta.filebarj.core.TempFileAwareTest;
+import com.github.nagyesta.filebarj.core.common.PermissionComparisonStrategy;
 import com.github.nagyesta.filebarj.core.config.RestoreTarget;
 import com.github.nagyesta.filebarj.core.config.RestoreTargets;
 import com.github.nagyesta.filebarj.core.config.RestoreTask;
@@ -36,6 +37,7 @@ void testRestoreShouldRestoreContentWhenRestoringABackupMadeOnWindows() throws I
                 .restoreTargets(new RestoreTargets(Set.of(r, u)))
                 .dryRun(false)
                 .threads(1)
+                .permissionComparisonStrategy(PermissionComparisonStrategy.RELAXED)
                 .build();
 
         //when
@@ -66,6 +68,7 @@ void testRestoreShouldRestoreContentWhenRestoringABackupMadeOnUnix() throws IOEx
                 .restoreTargets(new RestoreTargets(Set.of(r, u)))
                 .dryRun(false)
                 .threads(1)
+                .permissionComparisonStrategy(PermissionComparisonStrategy.RELAXED)
                 .build();
 
         //when

diff --git a/.../com/github/nagyesta/filebarj/core/restore/pipeline/RestoreControllerIntegrationTest.java b/.../com/github/nagyesta/filebarj/core/restore/pipeline/RestoreControllerIntegrationTest.java
@@ -620,8 +620,6 @@ private static void assertFileMetadataMatches(
                 "File should have correct permissions: " + restoredFile);
         Assertions.assertEquals(expectedMetadata.getLastModifiedUtcEpochSeconds(), actualMetadata.getLastModifiedUtcEpochSeconds(),
                 "File should have correct last modified time: " + restoredFile);
-        Assertions.assertEquals(expectedMetadata.getCreatedUtcEpochSeconds(), actualMetadata.getCreatedUtcEpochSeconds(),
-                "File should have correct creation time: " + restoredFile);
         Assertions.assertEquals(expectedMetadata.getOriginalSizeBytes(), actualMetadata.getOriginalSizeBytes(),
                 "File should have correct size: " + restoredFile);
         Assertions.assertEquals(expectedMetadata.getOriginalHash(), actualMetadata.getOriginalHash(),