Version7Migrator will now perform migration _after_ visiting a direct…

…ory to avoid changes to elements while still iterating
cryptomator · Oct 11, 2019 · b266343 · b266343
1 parent ca063e9
commit b266343
Show file tree

Hide file tree

Showing 4 changed files with 116 additions and 29 deletions.
diff --git a/src/main/java/org/cryptomator/cryptofs/migration/v7/FilePathMigration.java b/src/main/java/org/cryptomator/cryptofs/migration/v7/FilePathMigration.java
@@ -173,6 +173,10 @@ Path getTargetPath(String attemptSuffix) {
 			}
 		}
 	}
+
+	public Path getOldPath() {
+		return oldPath;
+	}
 
 	// visible for testing
 	String getOldCanonicalName() {

diff --git a/src/main/java/org/cryptomator/cryptofs/migration/v7/MigratingVisitor.java b/src/main/java/org/cryptomator/cryptofs/migration/v7/MigratingVisitor.java
@@ -0,0 +1,66 @@
+package org.cryptomator.cryptofs.migration.v7;
+
+import org.cryptomator.cryptofs.migration.api.MigrationProgressListener;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.nio.file.FileAlreadyExistsException;
+import java.nio.file.FileVisitResult;
+import java.nio.file.Path;
+import java.nio.file.SimpleFileVisitor;
+import java.nio.file.attribute.BasicFileAttributes;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Optional;
+
+class MigratingVisitor extends SimpleFileVisitor<Path> {
+
+	private static final Logger LOG = LoggerFactory.getLogger(MigratingVisitor.class);
+
+	private final Path vaultRoot;
+	private final MigrationProgressListener progressListener;
+	private final long estimatedTotalFiles;
+
+	public MigratingVisitor(Path vaultRoot, MigrationProgressListener progressListener, long estimatedTotalFiles) {
+		this.vaultRoot = vaultRoot;
+		this.progressListener = progressListener;
+		this.estimatedTotalFiles = estimatedTotalFiles;
+	}
+
+	private Collection<FilePathMigration> migrationsInCurrentDir = new ArrayList<>();
+	private long migratedFiles = 0;
+
+	// Step 1: Collect files to be migrated
+	@Override
+	public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
+		final Optional<FilePathMigration> migration;
+		try {
+			migration = FilePathMigration.parse(vaultRoot, file);
+		} catch (UninflatableFileException e) {
+			LOG.warn("SKIP {} because inflation failed.", file);
+			return FileVisitResult.CONTINUE;
+		}
+		migration.ifPresent(migrationsInCurrentDir::add);
+		return FileVisitResult.CONTINUE;
+	}
+
+	// Step 2: Only after visiting this dir, we will perform any changes to avoid "ConcurrentModificationExceptions"
+	@Override
+	public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException {
+		for (FilePathMigration migration : migrationsInCurrentDir) {
+			migratedFiles++;
+			progressListener.update(MigrationProgressListener.ProgressState.MIGRATING, (double) migratedFiles / estimatedTotalFiles);
+			try {
+				Path migratedFile = migration.migrate();
+				LOG.info("MOVED {} to {}", migration.getOldPath(), migratedFile);
+			} catch (FileAlreadyExistsException e) {
+				LOG.error("Failed to migrate " + migration.getOldPath() + " due to FileAlreadyExistsException. Already migrated on a different machine?.", e);
+				return FileVisitResult.TERMINATE;
+			}
+		}
+		migrationsInCurrentDir.clear();
+		return FileVisitResult.CONTINUE;
+	}
+
+}
diff --git a/src/main/java/org/cryptomator/cryptofs/migration/v7/Version7Migrator.java b/src/main/java/org/cryptomator/cryptofs/migration/v7/Version7Migrator.java
@@ -20,7 +20,6 @@
 
 import javax.inject.Inject;
 import java.io.IOException;
-import java.nio.file.FileAlreadyExistsException;
 import java.nio.file.FileVisitOption;
 import java.nio.file.FileVisitResult;
 import java.nio.file.Files;
@@ -30,7 +29,6 @@
 import java.nio.file.StandardOpenOption;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.util.EnumSet;
-import java.util.Optional;
 import java.util.concurrent.atomic.LongAdder;
 
 public class Version7Migrator implements Migrator {
@@ -92,33 +90,7 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
 	private void migrateFileNames(Path vaultRoot, MigrationProgressListener progressListener, long totalFiles) throws IOException {
 		assert totalFiles > 0;
 		Path dataDir = vaultRoot.resolve("d");
-		Files.walkFileTree(dataDir, EnumSet.noneOf(FileVisitOption.class), 3, new SimpleFileVisitor<Path>() {
-
-			long migratedFiles = 0;
-
-			@Override
-			public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
-				migratedFiles++;
-				progressListener.update(MigrationProgressListener.ProgressState.MIGRATING, (double) migratedFiles / totalFiles);
-				final Optional<FilePathMigration> migration;
-				try {
-					migration = FilePathMigration.parse(vaultRoot, file);
-				} catch (UninflatableFileException e) {
-					LOG.warn("SKIP {} because inflation failed.", file);
-					return FileVisitResult.CONTINUE;
-				}
-				if (migration.isPresent()) {
-					try {
-						Path migratedFile = migration.get().migrate();
-						LOG.info("MOVED {} to {}", file, migratedFile);
-					} catch (FileAlreadyExistsException e) {
-						LOG.error("Failed to migrate " + file + " due to FileAlreadyExistsException. Already migrated?.", e);
-						return FileVisitResult.TERMINATE;
-					}
-				}
-				return FileVisitResult.CONTINUE;
-			}
-		});
+		Files.walkFileTree(dataDir, EnumSet.noneOf(FileVisitOption.class), 3, new MigratingVisitor(vaultRoot, progressListener, totalFiles));
 	}
 
 }
diff --git a/src/test/java/org/cryptomator/cryptofs/migration/v7/Version7MigratorTest.java b/src/test/java/org/cryptomator/cryptofs/migration/v7/Version7MigratorTest.java
@@ -69,4 +69,49 @@ public void testMDirectoryGetsDeleted() throws IOException {
 		Assertions.assertFalse(Files.exists(metaDir));
 	}
 
+	@Test
+	public void testMigrationOfNormalFile() throws IOException {
+		Path dir = dataDir.resolve("AA/BBBBBCCCCCDDDDDEEEEEFFFFFGGGGG");
+		Files.createDirectories(dir);
+		Path fileBeforeMigration = dir.resolve("MZUWYZLOMFWWK===");
+		Path fileAfterMigration = dir.resolve("ZmlsZW5hbWU=.c9r");
+		Files.createFile(fileBeforeMigration);
+
+		Migrator migrator = new Version7Migrator(cryptorProvider);
+		migrator.migrate(vaultRoot, "masterkey.cryptomator", "test");
+
+		Assertions.assertFalse(Files.exists(fileBeforeMigration));
+		Assertions.assertTrue(Files.exists(fileAfterMigration));
+	}
+
+	@Test
+	public void testMigrationOfNormalDirectory() throws IOException {
+		Path dir = dataDir.resolve("AA/BBBBBCCCCCDDDDDEEEEEFFFFFGGGGG");
+		Files.createDirectories(dir);
+		Path fileBeforeMigration = dir.resolve("0MZUWYZLOMFWWK===");
+		Path fileAfterMigration = dir.resolve("ZmlsZW5hbWU=.c9r/dir.c9r");
+		Files.createFile(fileBeforeMigration);
+
+		Migrator migrator = new Version7Migrator(cryptorProvider);
+		migrator.migrate(vaultRoot, "masterkey.cryptomator", "test");
+
+		Assertions.assertFalse(Files.exists(fileBeforeMigration));
+		Assertions.assertTrue(Files.exists(fileAfterMigration));
+	}
+
+	@Test
+	public void testMigrationOfNormalSymlink() throws IOException {
+		Path dir = dataDir.resolve("AA/BBBBBCCCCCDDDDDEEEEEFFFFFGGGGG");
+		Files.createDirectories(dir);
+		Path fileBeforeMigration = dir.resolve("1SMZUWYZLOMFWWK===");
+		Path fileAfterMigration = dir.resolve("ZmlsZW5hbWU=.c9r/symlink.c9r");
+		Files.createFile(fileBeforeMigration);
+
+		Migrator migrator = new Version7Migrator(cryptorProvider);
+		migrator.migrate(vaultRoot, "masterkey.cryptomator", "test");
+
+		Assertions.assertFalse(Files.exists(fileBeforeMigration));
+		Assertions.assertTrue(Files.exists(fileAfterMigration));
+	}
+
 }