ChangeRebuilderImpl: Handle ChangeMessages with null PatchSet.Id
Long ago we made the design decision in NoteDb to associate every
commit in the meta graph with a patch set ID. The actual reason for
this was somewhat obscure: we need to have at least one footer in
every commit so we can tell where the ChangeMessage text (the body of
the commit) ends and the footers begin.
A consequence of this decision is that we cannot represent
ChangeMessages that have null PatchSet.Id fields in NoteDb. In fact,
trying to convert those fields was resulting in NPE, similar to
I3723b0c9, but the fix is a little more complicated.
Note that we have pretty much eliminated ChangeMessages with null
patch set IDs from the code, since we already enforce that every
ChangeUpdate is associated with a PatchSet.Id. (The one remaining
exception is in MergeOp#abandonAllOpenChangesForDeletedProject, which
is moot since in a NoteDb world such changes just get deleted.) So
this change is just about converting old messages that may for
whatever reason be lacking the field.
In ChangeRebuilderImpl, after sorting events but before combining them
into ChangeUpdates, ensure that all psId fields are populated, using
the latest patch set of the change encountered so far. This is
analogous to the behavior in the UI where if you navigate to a change
screen, it defaults to the latest patch set; then if you add a
ChangeMessage, it likewise is associated with the latest patch set.
ChangeBundle is also modified to skip comparing the patch set ID field
when the one in ReviewDb is null.
Change-Id: Id2722541f128e77342f2185910ceea33552d1524
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 2e8804d..f15ff36 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.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 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,13 +167,49 @@
// 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<String, PatchSet.Id> 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();
Change.Id id = r.getPatchSetId().getParentKey();
@@ -404,9 +442,8 @@
}
}
- 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 ad57e24..459ba1f 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.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 @@
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<ChangeMessage> as = bundleA.changeMessages;
@@ -349,8 +349,25 @@
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<String> 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 @@
private static <T> void diffColumnsExcluding(List<String> diffs,
Class<T> clazz, String desc, ChangeBundle bundleA, T a,
ChangeBundle bundleB, T b, String... exclude) {
- Set<String> toExclude = Sets.newLinkedHashSet(Arrays.asList(exclude));
+ diffColumnsExcluding(diffs, clazz, desc, bundleA, a, bundleB, b,
+ Arrays.asList(exclude));
+ }
+
+ private static <T> void diffColumnsExcluding(List<String> diffs,
+ Class<T> clazz, String desc, ChangeBundle bundleA, T a,
+ ChangeBundle bundleB, T b, Iterable<String> exclude) {
+ Set<String> 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 92f33ee..c9a14e9 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 @@
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 @@
}).toSortedList(PatchLineCommentsUtil.PLC_ORDER);
}
+ private void sortEvents(Change.Id changeId, List<Event> 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<Event> events, Change change) throws OrmException, IOException {
if (events.isEmpty()) {
@@ -371,7 +388,7 @@
.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 @@
// 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 @@
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 @@
}
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 2ed95d3..c4f5e19 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
@@ -356,6 +356,45 @@
}
@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);
TestChanges.incrementPatchSet(c);
