ChangeRebuilderImpl: Skip patch sets greater than current

AOSP has some changes[1] dating from before we used transactions
properly, where there is a PatchSet entity with an ID higher than the
currentPatchSetId in the Change entity. In NoteDb we don't record the
current patch set ID explicitly, we just use the greatest
(non-deleted) ID found in the meta graph, so we can't represent this
discrepancy.

Just don't convert these newer patch sets or their associated
entities, and teach ChangeBundle to ignore this diff. This means we
might end up discarding some comments of the form "What's the deal
with this extra patch set?", which seems acceptable.

[1] https://android-review.googlesource.com/31041

Change-Id: I30e55f6455fe8db3dac3d2502e5e5eafc5bc2438

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java

@@ -36,6 +36,7 @@ import com.google.gerrit.reviewdb.client.PatchLineComment;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.PatchLineCommentsUtil;
@@ -532,6 +533,28 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     assertThat(notes.getComments()).isEmpty();
   }
 
+  @Test
+  public void skipPatchSetsGreaterThanCurrentPatchSet() throws Exception {
+    PushOneCommit.Result r = createChange();
+    Change change = r.getChange().change();
+    Change.Id id = change.getId();
+
+    PatchSet badPs =
+        new PatchSet(new PatchSet.Id(id, change.currentPatchSetId().get() + 1));
+    badPs.setCreatedOn(TimeUtil.nowTs());
+    badPs.setUploader(new Account.Id(12345));
+    badPs.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
+    db.patchSets().insert(Collections.singleton(badPs));
+    indexer.index(db, change.getProject(), id);
+
+    checker.rebuildAndCheckChanges(id);
+
+    notesMigration.setAllEnabled(true);
+    ChangeNotes notes = notesFactory.create(db, project, id);
+    assertThat(notes.getPatchSets().keySet())
+        .containsExactly(change.currentPatchSetId());
+  }
+
   private void setInvalidNoteDbState(Change.Id id) throws Exception {
     ReviewDb db = unwrapDb();
     Change c = db.changes().get(id);

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java

@@ -395,6 +395,17 @@ public class ChangeBundle {
         });
   }
 
+  private Map<PatchSet.Id, PatchSet> filterPatchSets() {
+    final int max = change.currentPatchSetId().get();
+    return Maps.filterKeys(patchSets,
+        new Predicate<PatchSet.Id>() {
+          @Override
+          public boolean apply(PatchSet.Id in) {
+            return in.get() <= max;
+          }
+        });
+  }
+
   private static void diffChanges(List<String> diffs, ChangeBundle bundleA,
       ChangeBundle bundleB) {
     Change a = bundleA.change;
@@ -601,6 +612,14 @@ public class ChangeBundle {
       ChangeBundle bundleB) {
     Map<PatchSet.Id, PatchSet> as = bundleA.patchSets;
     Map<PatchSet.Id, PatchSet> bs = bundleB.patchSets;
+    // Filter out patch sets from ReviewDb side that are greater than latest.
+    if (bundleA.getSource() == REVIEW_DB && bundleB.getSource() == NOTE_DB) {
+      as = bundleA.filterPatchSets();
+    } else if (bundleA.getSource() == NOTE_DB
+        && bundleB.getSource() == REVIEW_DB) {
+      bs = bundleB.filterPatchSets();
+    }
+
     for (PatchSet.Id id : diffKeySets(diffs, as, bs)) {
       PatchSet a = as.get(id);
       PatchSet b = bs.get(id);

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java

@@ -228,6 +228,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
   private void buildUpdates(NoteDbUpdateManager manager, ChangeBundle bundle)
       throws IOException, OrmException {
     Change change = new Change(bundle.getChange());
+    PatchSet.Id currPsId = change.currentPatchSetId();
     // We will rebuild all events, except for draft comments, in buckets based
     // on author and timestamp.
     List<Event> events = new ArrayList<>();
@@ -245,6 +246,12 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
         Sets.newHashSetWithExpectedSize(bundle.getPatchSets().size());
 
     for (PatchSet ps : bundle.getPatchSets()) {
+      if (ps.getId().get() > currPsId.get()) {
+        log.info(
+            "Skipping patch set {}, which is higher than current patch set {}",
+            ps.getId(), currPsId);
+        continue;
+      }
       psIds.add(ps.getId());
       events.add(new PatchSetEvent(
           change, ps, manager.getChangeRepo().rw));

gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java

@@ -705,6 +705,48 @@ public class ChangeBundleTest {
     assertNoDiffs(b2, b1);
   }
 
+  @Test
+  public void diffPatchSetsIgnoresGreaterThanCurrentFromReviewDb()
+      throws Exception {
+    Change c = TestChanges.newChange(project, accountId);
+
+    PatchSet ps1 = new PatchSet(new PatchSet.Id(c.getId(), 1));
+    ps1.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
+    ps1.setUploader(accountId);
+    ps1.setCreatedOn(TimeUtil.nowTs());
+    PatchSet ps2 = new PatchSet(new PatchSet.Id(c.getId(), 2));
+    ps2.setRevision(new RevId("badc0feebadc0feebadc0feebadc0feebadc0fee"));
+    ps2.setUploader(accountId);
+    ps2.setCreatedOn(TimeUtil.nowTs());
+    assertThat(ps2.getId().get()).isGreaterThan(c.currentPatchSetId().get());
+
+    // Both ReviewDb, exact match is required.
+    ChangeBundle b1 = new ChangeBundle(c, messages(), patchSets(ps1),
+        approvals(), comments(), REVIEW_DB);
+    ChangeBundle b2 = new ChangeBundle(c, messages(), patchSets(ps1, ps2),
+        approvals(), comments(), REVIEW_DB);
+    assertDiffs(b1, b2,
+        "PatchSet.Id sets differ:"
+            + " [] only in A; [" + c.getId() + ",2] only in B");
+
+    // Both NoteDb, exact match is required.
+    b1 = new ChangeBundle(c, messages(), patchSets(ps1), approvals(),
+        comments(), NOTE_DB);
+    b2 = new ChangeBundle(c, messages(), patchSets(ps1, ps2), approvals(),
+        comments(), NOTE_DB);
+    assertDiffs(b1, b2,
+        "PatchSet.Id sets differ:"
+            + " [] only in A; [" + c.getId() + ",2] only in B");
+
+    // PS2 is in ReviewDb but not NoteDb, ok.
+    b1 = new ChangeBundle(c, messages(), patchSets(ps1), approvals(),
+        comments(), NOTE_DB);
+    b2 = new ChangeBundle(c, messages(), patchSets(ps1, ps2), approvals(),
+        comments(), REVIEW_DB);
+    assertNoDiffs(b1, b2);
+    assertNoDiffs(b2, b1);
+  }
+
   @Test
   public void diffPatchSetApprovalKeySets() throws Exception {
     Change c = TestChanges.newChange(project, accountId);