diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java index f4ad11e790..78239528f4 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java +++ b/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); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java index 22255b5f8d..6a1065bc0a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java @@ -395,6 +395,17 @@ public class ChangeBundle { }); } + private Map filterPatchSets() { + final int max = change.currentPatchSetId().get(); + return Maps.filterKeys(patchSets, + new Predicate() { + @Override + public boolean apply(PatchSet.Id in) { + return in.get() <= max; + } + }); + } + private static void diffChanges(List diffs, ChangeBundle bundleA, ChangeBundle bundleB) { Change a = bundleA.change; @@ -601,6 +612,14 @@ public class ChangeBundle { ChangeBundle bundleB) { Map as = bundleA.patchSets; Map 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); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java index 87155d4780..1a44f08153 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java +++ b/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 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)); diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java index f57fba4ca6..12ed117318 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java +++ b/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);