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 2e8804d5f5..f15ff36b2f 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 @@ -40,6 +40,7 @@ import com.google.gerrit.server.change.Rebuild; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.notedb.ChangeBundle; import com.google.gerrit.server.notedb.ChangeNoteUtil; +import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.NoteDbChangeState; import com.google.gerrit.server.schema.DisabledChangesReviewDbWrapper; import com.google.gerrit.testutil.NoteDbChecker; @@ -59,6 +60,7 @@ import org.junit.Test; import java.sql.Timestamp; import java.util.Collections; import java.util.HashMap; +import java.util.Map; import java.util.concurrent.TimeUnit; public class ChangeRebuilderIT extends AbstractDaemonTest { @@ -165,12 +167,48 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { // Events need to be otherwise identical for the account ID to be compared. ChangeMessage msg1 = - insertMessage(psId, user.getId(), TimeUtil.nowTs(), "message 1"); - insertMessage(psId, null, msg1.getWrittenOn(), "message 2"); + insertMessage(id, psId, user.getId(), TimeUtil.nowTs(), "message 1"); + insertMessage(id, psId, null, msg1.getWrittenOn(), "message 2"); checker.rebuildAndCheckChanges(id); } + @Test + public void nullPatchSetId() throws Exception { + PushOneCommit.Result r = createChange(); + PatchSet.Id psId1 = r.getPatchSetId(); + Change.Id id = psId1.getParentKey(); + + // Events need to be otherwise identical for the PatchSet.ID to be compared. + ChangeMessage msg1 = + insertMessage(id, null, user.getId(), TimeUtil.nowTs(), "message 1"); + insertMessage(id, null, user.getId(), msg1.getWrittenOn(), "message 2"); + + PatchSet.Id psId2 = amendChange(r.getChangeId()).getPatchSetId(); + + ChangeMessage msg3 = + insertMessage(id, null, user.getId(), TimeUtil.nowTs(), "message 3"); + insertMessage(id, null, user.getId(), msg3.getWrittenOn(), "message 4"); + + checker.rebuildAndCheckChanges(id); + + notesMigration.setWriteChanges(true); + notesMigration.setReadChanges(true); + + ChangeNotes notes = notesFactory.create(db, project, id); + Map psIds = new HashMap<>(); + for (ChangeMessage msg : notes.getChangeMessages()) { + PatchSet.Id psId = msg.getPatchSetId(); + assertThat(psId).named("patchset for " + msg).isNotNull(); + psIds.put(msg.getMessage(), psId); + } + // Patch set IDs were replaced during conversion process. + assertThat(psIds).containsEntry("message 1", psId1); + assertThat(psIds).containsEntry("message 2", psId1); + assertThat(psIds).containsEntry("message 3", psId2); + assertThat(psIds).containsEntry("message 4", psId2); + } + @Test public void noWriteToNewRef() throws Exception { PushOneCommit.Result r = createChange(); @@ -404,9 +442,8 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { } } - private ChangeMessage insertMessage(PatchSet.Id psId, Account.Id author, - Timestamp ts, String message) throws Exception { - Change.Id id = psId.getParentKey(); + private ChangeMessage insertMessage(Change.Id id, PatchSet.Id psId, + Account.Id author, Timestamp ts, String message) throws Exception { ChangeMessage msg = new ChangeMessage( new ChangeMessage.Key(id, ChangeUtil.messageUUID(db)), author, ts, psId); 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 ad57e24215..459ba1fc04 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 @@ -28,6 +28,7 @@ import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; +import com.google.common.collect.Lists; import com.google.common.collect.Ordering; import com.google.common.collect.Sets; import com.google.gerrit.reviewdb.client.Change; @@ -332,8 +333,7 @@ public class ChangeBundle { return; } - // At least one is from NoteDb, so we need to ignore UUIDs for both, and - // allow timestamp slop if the sources differ. + // At least one is from NoteDb, so comparisons are inexact as noted below. Change.Id id = bundleA.getChange().getId(); checkArgument(id.equals(bundleB.getChange().getId())); List as = bundleA.changeMessages; @@ -349,8 +349,25 @@ public class ChangeBundle { ChangeMessage a = as.get(i); ChangeMessage b = bs.get(i); String desc = "ChangeMessage on " + id + " at index " + i; + + // Ignore null PatchSet.Id on a ReviewDb change; all entities in NoteDb + // have a PatchSet.Id. + boolean checkPsId = true; + if (bundleA.source == REVIEW_DB) { + checkPsId = a.getPatchSetId() != null; + } else if (bundleB.source == REVIEW_DB) { + checkPsId = b.getPatchSetId() != null; + } + + // Ignore UUIDs for both sides. + List exclude = Lists.newArrayList("key"); + if (!checkPsId) { + exclude.add("patchset"); + } + + // Normal column-wise diff also allows timestamp slop. diffColumnsExcluding(diffs, ChangeMessage.class, desc, bundleA, a, - bundleB, b, "key"); + bundleB, b, exclude); } } @@ -417,7 +434,14 @@ public class ChangeBundle { private static void diffColumnsExcluding(List diffs, Class clazz, String desc, ChangeBundle bundleA, T a, ChangeBundle bundleB, T b, String... exclude) { - Set toExclude = Sets.newLinkedHashSet(Arrays.asList(exclude)); + diffColumnsExcluding(diffs, clazz, desc, bundleA, a, bundleB, b, + Arrays.asList(exclude)); + } + + private static void diffColumnsExcluding(List diffs, + Class clazz, String desc, ChangeBundle bundleA, T a, + ChangeBundle bundleB, T b, Iterable exclude) { + Set toExclude = Sets.newLinkedHashSet(exclude); for (Field f : clazz.getDeclaredFields()) { Column col = f.getAnnotation(Column.class); if (col == null) { 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 92f33ee9c1..c9a14e9472 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 @@ -15,6 +15,7 @@ package com.google.gerrit.server.notedb; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static com.google.gerrit.server.PatchLineCommentsUtil.setCommentRevId; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_HASHTAGS; @@ -215,7 +216,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { new ChangeMessageEvent(msg, noteDbChange, change.getCreatedOn())); } - Collections.sort(events, EVENT_ORDER); + sortEvents(change.getId(), events); events.add(new FinalUpdatesEvent(change, noteDbChange)); @@ -253,6 +254,22 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { }).toSortedList(PatchLineCommentsUtil.PLC_ORDER); } + private void sortEvents(Change.Id changeId, List events) { + Collections.sort(events, EVENT_ORDER); + + // Fill in any missing patch set IDs using the latest patch set of the + // change at the time of the event. This is as if a user added a + // ChangeMessage on the change by replying from the latest patch set. + int ps = 1; + for (Event e : events) { + if (e.psId == null) { + e.psId = new PatchSet.Id(changeId, ps); + } else { + ps = Math.max(ps, e.psId.get()); + } + } + } + private void flushEventsToUpdate(NoteDbUpdateManager manager, EventList events, Change change) throws OrmException, IOException { if (events.isEmpty()) { @@ -371,7 +388,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { .compareTrueFirst(a.predatesChange, b.predatesChange) .compare(a.when, b.when) .compare(a.who, b.who, ReviewDbUtil.intKeyOrdering()) - .compare(a.psId.get(), b.psId.get()) + .compare(a.psId, b.psId, ReviewDbUtil.intKeyOrdering().nullsLast()) .result(); } }; @@ -380,10 +397,10 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { // NOTE: EventList only supports direct subclasses, not an arbitrary // hierarchy. - final PatchSet.Id psId; final Account.Id who; final Timestamp when; final boolean predatesChange; + PatchSet.Id psId; protected Event(PatchSet.Id psId, Account.Id who, Timestamp when, Timestamp changeCreatedOn) { @@ -449,7 +466,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { Event last = getLast(); if (!Objects.equals(e.who, last.who) - || !Objects.equals(e.psId, last.psId)) { + || !e.psId.equals(last.psId)) { return false; // Different patch set or author. } @@ -482,9 +499,9 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { } PatchSet.Id getPatchSetId() { - PatchSet.Id id = get(0).psId; + PatchSet.Id id = checkNotNull(get(0).psId); for (int i = 1; i < size(); i++) { - checkState(Objects.equals(id, get(i).psId), + checkState(get(i).psId.equals(id), "mismatched patch sets in EventList: %s != %s", id, get(i).psId); } return id; 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 2ed95d39d8..c4f5e19da2 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 @@ -355,6 +355,45 @@ public class ChangeBundleTest { assertDiffs(b3, b1, msg); } + @Test + public void diffChangeMessagesAllowsNullPatchSetIdFromReviewDb() + throws Exception { + Change c = TestChanges.newChange(project, accountId); + int id = c.getId().get(); + ChangeMessage cm1 = new ChangeMessage( + new ChangeMessage.Key(c.getId(), "uuid"), + accountId, TimeUtil.nowTs(), c.currentPatchSetId()); + cm1.setMessage("message 1"); + ChangeMessage cm2 = clone(cm1); + cm2.setPatchSetId(null); + + ChangeBundle b1 = new ChangeBundle(c, messages(cm1), patchSets(), + approvals(), comments(), REVIEW_DB); + ChangeBundle b2 = new ChangeBundle(c, messages(cm2), patchSets(), + approvals(), comments(), REVIEW_DB); + + // Both are ReviewDb, exact patch set ID match is required. + assertDiffs(b1, b2, + "patchset differs for ChangeMessage.Key " + c.getId() + ",uuid:" + + " {" + id + ",1} != {null}"); + + // Null patch set ID on ReviewDb is ignored. + b1 = new ChangeBundle(c, messages(cm1), patchSets(), approvals(), + comments(), NOTE_DB); + b2 = new ChangeBundle(c, messages(cm2), patchSets(), approvals(), + comments(), REVIEW_DB); + assertNoDiffs(b1, b2); + + // Null patch set ID on NoteDb is not ignored (but is not realistic). + b1 = new ChangeBundle(c, messages(cm1), patchSets(), approvals(), + comments(), REVIEW_DB); + b2 = new ChangeBundle(c, messages(cm2), patchSets(), approvals(), + comments(), NOTE_DB); + assertDiffs(b1, b2, + "patchset differs for ChangeMessage on " + id + " at index 0:" + + " {" + id + ",1} != {null}"); + } + @Test public void diffPatchSetIdSets() throws Exception { Change c = TestChanges.newChange(project, accountId);