opendev/gerrit
Code Issues Proposed changes

ChangeRebuilderImpl: Handle ChangeMessages with null PatchSet.Id

Browse Source 
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
This commit is contained in:
Dave Borowitz
2016-04-15 12:44:50 -04:00
parent 8a867d6414
commit b7db0a0f18
4 changed files with 132 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -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<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();
@@ -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);

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

@@ -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<ChangeMessage> 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<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 @@ public class ChangeBundle {
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) {

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

@@ -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<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 @@ 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;

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

@@ -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);