From 1c723b9fb44490e3a53b0aed4160d5b6a8df6523 Mon Sep 17 00:00:00 2001 From: Esta Nagy Date: Tue, 20 Feb 2024 22:23:02 +0100 Subject: [PATCH] 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 --- .../core/backup/pipeline/BackupController.java | 8 +++----- .../common/BaseFileMetadataChangeDetector.java | 15 ++++++--------- .../common/FileMetadataChangeDetectorFactory.java | 9 ++++++--- .../common/HashingFileMetadataChangeDetector.java | 7 +++++-- .../common/SimpleFileMetadataChangeDetector.java | 7 +++++-- .../core/restore/pipeline/RestorePipeline.java | 3 ++- ...FileMetadataChangeDetectorIntegrationTest.java | 4 ++-- .../FileMetadataChangeDetectorFactoryTest.java | 12 ++++++++---- ...FileMetadataChangeDetectorIntegrationTest.java | 4 ++-- ...FileMetadataChangeDetectorIntegrationTest.java | 4 ++-- .../CrossPlatformRestoreIntegrationTest.java | 3 +++ .../RestoreControllerIntegrationTest.java | 2 -- 12 files changed, 44 insertions(+), 34 deletions(-) diff --git a/file-barj-core/src/main/java/com/github/nagyesta/filebarj/core/backup/pipeline/BackupController.java b/file-barj-core/src/main/java/com/github/nagyesta/filebarj/core/backup/pipeline/BackupController.java index e381a4d..5322e45 100644 --- a/file-barj-core/src/main/java/com/github/nagyesta/filebarj/core/backup/pipeline/BackupController.java +++ b/file-barj-core/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>(); 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/file-barj-core/src/main/java/com/github/nagyesta/filebarj/core/common/BaseFileMetadataChangeDetector.java b/file-barj-core/src/main/java/com/github/nagyesta/filebarj/core/common/BaseFileMetadataChangeDetector.java index 1e4b068..7715c9b 100644 --- a/file-barj-core/src/main/java/com/github/nagyesta/filebarj/core/common/BaseFileMetadataChangeDetector.java +++ b/file-barj-core/src/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 implements FileMetadataC private final SortedMap> filesFromManifests; private final SortedMap>> contentIndex; private final Map 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> filesFromManifests) { + @NotNull final Map> filesFromManifests, + @Nullable final PermissionComparisonStrategy permissionStrategy) { this.filesFromManifests = new TreeMap<>(filesFromManifests); final SortedMap>> contentIndex = new TreeMap<>(); final Map 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/file-barj-core/src/main/java/com/github/nagyesta/filebarj/core/common/FileMetadataChangeDetectorFactory.java b/file-barj-core/src/main/java/com/github/nagyesta/filebarj/core/common/FileMetadataChangeDetectorFactory.java index af33cc4..7c5f91f 100644 --- a/file-barj-core/src/main/java/com/github/nagyesta/filebarj/core/common/FileMetadataChangeDetectorFactory.java +++ b/file-barj-core/src/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> filesFromManifests) { + @NonNull final Map> 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/file-barj-core/src/main/java/com/github/nagyesta/filebarj/core/common/HashingFileMetadataChangeDetector.java b/file-barj-core/src/main/java/com/github/nagyesta/filebarj/core/common/HashingFileMetadataChangeDetector.java index 143cc9d..a77e960 100644 --- a/file-barj-core/src/main/java/com/github/nagyesta/filebarj/core/common/HashingFileMetadataChangeDetector.java +++ b/file-barj-core/src/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> filesFromManifests) { - super(filesFromManifests); + @NotNull final Map> filesFromManifests, + @Nullable final PermissionComparisonStrategy permissionStrategy) { + super(filesFromManifests, permissionStrategy); } @Override diff --git a/file-barj-core/src/main/java/com/github/nagyesta/filebarj/core/common/SimpleFileMetadataChangeDetector.java b/file-barj-core/src/main/java/com/github/nagyesta/filebarj/core/common/SimpleFileMetadataChangeDetector.java index ad34385..599e221 100644 --- a/file-barj-core/src/main/java/com/github/nagyesta/filebarj/core/common/SimpleFileMetadataChangeDetector.java +++ b/file-barj-core/src/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> filesFromManifests) { - super(filesFromManifests); + @NotNull final Map> filesFromManifests, + @Nullable final PermissionComparisonStrategy permissionStrategy) { + super(filesFromManifests, permissionStrategy); } @Override diff --git a/file-barj-core/src/main/java/com/github/nagyesta/filebarj/core/restore/pipeline/RestorePipeline.java b/file-barj-core/src/main/java/com/github/nagyesta/filebarj/core/restore/pipeline/RestorePipeline.java index 8d77448..45639c6 100644 --- a/file-barj-core/src/main/java/com/github/nagyesta/filebarj/core/restore/pipeline/RestorePipeline.java +++ b/file-barj-core/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/file-barj-core/src/test/java/com/github/nagyesta/filebarj/core/common/AbstractFileMetadataChangeDetectorIntegrationTest.java b/file-barj-core/src/test/java/com/github/nagyesta/filebarj/core/common/AbstractFileMetadataChangeDetectorIntegrationTest.java index 0ed41cd..0f0eb82 100644 --- a/file-barj-core/src/test/java/com/github/nagyesta/filebarj/core/common/AbstractFileMetadataChangeDetectorIntegrationTest.java +++ b/file-barj-core/src/test/java/com/github/nagyesta/filebarj/core/common/AbstractFileMetadataChangeDetectorIntegrationTest.java @@ -134,11 +134,11 @@ public static Stream 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/file-barj-core/src/test/java/com/github/nagyesta/filebarj/core/common/FileMetadataChangeDetectorFactoryTest.java b/file-barj-core/src/test/java/com/github/nagyesta/filebarj/core/common/FileMetadataChangeDetectorFactoryTest.java index 960ba74..c77b31f 100644 --- a/file-barj-core/src/test/java/com/github/nagyesta/filebarj/core/common/FileMetadataChangeDetectorFactoryTest.java +++ b/file-barj-core/src/test/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/file-barj-core/src/test/java/com/github/nagyesta/filebarj/core/common/HashingFileMetadataChangeDetectorIntegrationTest.java b/file-barj-core/src/test/java/com/github/nagyesta/filebarj/core/common/HashingFileMetadataChangeDetectorIntegrationTest.java index c0ee59a..fd6f021 100644 --- a/file-barj-core/src/test/java/com/github/nagyesta/filebarj/core/common/HashingFileMetadataChangeDetectorIntegrationTest.java +++ b/file-barj-core/src/test/java/com/github/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/file-barj-core/src/test/java/com/github/nagyesta/filebarj/core/common/SimpleFileMetadataChangeDetectorIntegrationTest.java b/file-barj-core/src/test/java/com/github/nagyesta/filebarj/core/common/SimpleFileMetadataChangeDetectorIntegrationTest.java index e9dbcdf..8014186 100644 --- a/file-barj-core/src/test/java/com/github/nagyesta/filebarj/core/common/SimpleFileMetadataChangeDetectorIntegrationTest.java +++ b/file-barj-core/src/test/java/com/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/file-barj-core/src/test/java/com/github/nagyesta/filebarj/core/restore/pipeline/CrossPlatformRestoreIntegrationTest.java b/file-barj-core/src/test/java/com/github/nagyesta/filebarj/core/restore/pipeline/CrossPlatformRestoreIntegrationTest.java index 9780340..5b07a9a 100644 --- a/file-barj-core/src/test/java/com/github/nagyesta/filebarj/core/restore/pipeline/CrossPlatformRestoreIntegrationTest.java +++ b/file-barj-core/src/test/java/com/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/file-barj-core/src/test/java/com/github/nagyesta/filebarj/core/restore/pipeline/RestoreControllerIntegrationTest.java b/file-barj-core/src/test/java/com/github/nagyesta/filebarj/core/restore/pipeline/RestoreControllerIntegrationTest.java index a5ea365..1e0680c 100644 --- a/file-barj-core/src/test/java/com/github/nagyesta/filebarj/core/restore/pipeline/RestoreControllerIntegrationTest.java +++ b/file-barj-core/src/test/java/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(),