Merge "NoteDbMigrator: Totally skip migration for orphan changes" into stable-2.16

2 files changed, 53 insertions, 4 deletions

diff --git a/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java b/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java
index ebec9c59df..d6daae16b2 100644
--- a/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java
+++ b/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java
@@ -72,6 +72,7 @@ import com.google.gerrit.server.notedb.PrimaryStorageMigrator.NoNoteDbStateExcep
 import com.google.gerrit.server.notedb.RepoSequence;
 import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder.NoPatchSetsException;
 import com.google.gerrit.server.project.NoSuchChangeException;
+import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.update.ChainedReceiveCommands;
 import com.google.gerrit.server.update.RefUpdateUtil;
 import com.google.gerrit.server.util.ManualRequestContext;
@@ -164,6 +165,7 @@ public class NoteDbMigrator implements AutoCloseable {
     private final MutableNotesMigration globalNotesMigration;
     private final PrimaryStorageMigrator primaryStorageMigrator;
     private final DynamicSet<NotesMigrationStateListener> listeners;
+    private final ProjectCache projectCache;
 
     private int threads;
     private ImmutableList<Project.NameKey> projects = ImmutableList.of();
@@ -193,7 +195,8 @@ public class NoteDbMigrator implements AutoCloseable {
         WorkQueue workQueue,
         MutableNotesMigration globalNotesMigration,
         PrimaryStorageMigrator primaryStorageMigrator,
-        DynamicSet<NotesMigrationStateListener> listeners) {
+        DynamicSet<NotesMigrationStateListener> listeners,
+        ProjectCache projectCache) {
       // Reload gerrit.config/notedb.config on each migrator invocation, in case a previous
       // migration in the same process modified the on-disk contents. This ensures the defaults for
       // trial/autoMigrate get set correctly below.
@@ -213,6 +216,7 @@ public class NoteDbMigrator implements AutoCloseable {
       this.globalNotesMigration = globalNotesMigration;
       this.primaryStorageMigrator = primaryStorageMigrator;
       this.listeners = listeners;
+      this.projectCache = projectCache;
       this.trial = getTrialMode(cfg);
       this.autoMigrate = getAutoMigrate(cfg);
     }
@@ -400,6 +404,7 @@ public class NoteDbMigrator implements AutoCloseable {
           changes,
           progressOut,
           stopAtState,
+          projectCache,
           trial,
           forceRebuild,
           sequenceGap >= 0 ? sequenceGap : Sequences.getChangeSequenceGap(cfg),
@@ -429,6 +434,7 @@ public class NoteDbMigrator implements AutoCloseable {
   private final ImmutableList<Change.Id> changes;
   private final OutputStream progressOut;
   private final NotesMigrationState stopAtState;
+  private final ProjectCache projectCache;
   private final boolean trial;
   private final boolean forceRebuild;
   private final int sequenceGap;
@@ -455,6 +461,7 @@ public class NoteDbMigrator implements AutoCloseable {
       ImmutableList<Change.Id> changes,
       OutputStream progressOut,
       NotesMigrationState stopAtState,
+      ProjectCache projectCache,
       boolean trial,
       boolean forceRebuild,
       int sequenceGap,
@@ -489,6 +496,7 @@ public class NoteDbMigrator implements AutoCloseable {
     this.changes = changes;
     this.progressOut = progressOut;
     this.stopAtState = stopAtState;
+    this.projectCache = projectCache;
     this.trial = trial;
     this.forceRebuild = forceRebuild;
     this.sequenceGap = sequenceGap;
@@ -702,11 +710,13 @@ public class NoteDbMigrator implements AutoCloseable {
    * of the NoteDb migration code, which is too risky to attempt in the stable branch where this bug
    * had to be fixed.
    *
-   * <p>As of this writing, the only case where this happens is when a change has no patch sets.
+   * <p>As of this writing, there are only two cases where this happens: when a change has no patch
+   * sets, or the project doesn't exist.
    */
-  private static boolean canSkipPrimaryStorageMigration(ReviewDb db, Change.Id id) {
+  private boolean canSkipPrimaryStorageMigration(ReviewDb db, Change.Id id) {
     try {
-      return Iterables.isEmpty(unwrapDb(db).patchSets().byChange(id));
+      return Iterables.isEmpty(unwrapDb(db).patchSets().byChange(id))
+          || projectCache.get(unwrapDb(db).changes().get(id).getProject()) == null;
     } catch (Exception e) {
       logger.atSevere().withCause(e).log(
           "Error checking if change %s can be skipped, assuming no", id);
diff --git a/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java b/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java
index c2499737d9..b9ed0f3ac7 100644
--- a/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java
@@ -35,6 +35,8 @@ import static org.easymock.EasyMock.verify;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSortedSet;
+import com.google.common.io.MoreFiles;
+import com.google.common.io.RecursiveDeleteOption;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.GerritConfig;
 import com.google.gerrit.acceptance.NoHttpd;
@@ -84,6 +86,7 @@ import org.eclipse.jgit.lib.ObjectReader;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.RefUpdate;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.lib.RepositoryCache.FileKey;
 import org.eclipse.jgit.storage.file.FileBasedConfig;
 import org.eclipse.jgit.util.FS;
 import org.junit.After;
@@ -511,6 +514,42 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
   }
 
   @Test
+  public void fullMigrationOneChangeWithNoProject() throws Exception {
+    PushOneCommit.Result r1 = createChange();
+    Change.Id id1 = r1.getChange().getId();
+
+    Project.NameKey p2 = createProject("project2");
+    TestRepository<?> tr2 = cloneProject(p2, admin);
+    PushOneCommit.Result r2 = pushFactory.create(db, admin.getIdent(), tr2).to("refs/for/master");
+    Change.Id id2 = r2.getChange().getId();
+
+    // TODO(davido): Find an easier way to wipe out a repository from the file system.
+    MoreFiles.deleteRecursively(
+        FileKey.lenient(
+                sitePaths
+                    .resolve(cfg.getString("gerrit", null, "basePath"))
+                    .resolve(p2.get())
+                    .toFile(),
+                FS.DETECTED)
+            .getFile()
+            .toPath(),
+        RecursiveDeleteOption.ALLOW_INSECURE);
+
+    migrate(b -> b);
+    assertNotesMigrationState(NOTE_DB, false, false);
+
+    try (ReviewDb db = schemaFactory.open();
+        Repository repo = repoManager.openRepository(project)) {
+      assertThat(repo.exactRef(RefNames.changeMetaRef(id1))).isNotNull();
+      assertThat(db.changes().get(id1).getNoteDbState()).isEqualTo(NOTE_DB_PRIMARY_STATE);
+    }
+
+    // A change without project is so corrupt that it is completely skipped by the migration
+    // process.
+    assertThat(db.changes().get(id2).getNoteDbState()).isNull();
+  }
+
+  @Test
   public void fullMigrationMissingPatchSetRefs() throws Exception {
     PushOneCommit.Result r = createChange();
     Change.Id id = r.getChange().getId();