opendev/gerrit
Code Issues Proposed changes

ChangeBundle: Support ChangeMessages from NoteDb

Browse Source 
NoteDb doesn't store the original ChangeMessage UUID; instead, it just
populates UUIDs by using the SHA-1 of the commit that added the
message, since a commit can add at most one message. Thus when
comparing bundles from ReviewDb and NoteDb, we need to ignore the
UUID.

Add a new AutoValue type that represents a normalized ChangeMessage
instance, and check that both bundles have the same multiset of
normalized ChangeMessages. We need to use a Multiset as it is valid in
the database to have two ChangeMessages that have all fields exactly
identical except for the UUID. Therefore bundles are only equivalent
if each unique ChangeMessage appears the correct number of times.

The diff output in this case is slightly less helpful than in other
cases, as we don't do a field-by-field diff. An implementation that
tries to match up mostly-identical ChangeMessages for a field-by-field
diff would be much more complicated of an implementation.

Also round tiemstamps when comparing bundles that are not both from
ReviewDb, since at least one of them will have a rounded timestamp
from NoteDb.

Change-Id: Ie180d70fb749a0c27f2ebcbba135e18c72a8fbea
This commit is contained in:
Dave Borowitz
2016-02-25 12:50:04 -05:00
parent b0cfc534c5
commit 019e3ac40f
3 changed files with 212 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/ChangeMessage.java
View File

@@ -50,7 +50,7 @@ public final class ChangeMessage {
}
@Override
protected void set(String newValue) {
public void set(String newValue) {
uuid = newValue;
}
}
@@ -105,6 +105,10 @@ public final class ChangeMessage {
return writtenOn;
}
public void setWrittenOn(Timestamp ts) {
writtenOn = ts;
}
public String getMessage() {
return message;
}
@@ -120,4 +124,4 @@ public final class ChangeMessage {
public void setPatchSetId(PatchSet.Id id) {
patchset = id;
}
}
}

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

@@ -17,13 +17,21 @@ 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.notedb.ChangeBundle.Source.REVIEW_DB;
import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ComparisonChain;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Multiset;
import com.google.common.collect.Ordering;
import com.google.common.collect.Sets;
import com.google.common.collect.TreeMultiset;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.Patch;
@@ -33,9 +41,11 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.PatchLineCommentsUtil;
import com.google.gwtorm.client.Column;
import com.google.gwtorm.client.IntKey;
import com.google.gwtorm.server.OrmException;
import java.lang.reflect.Field;
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Comparator;
@@ -54,6 +64,10 @@ import java.util.TreeSet;
* the minor implementation differences between ReviewDb and NoteDb.
*/
public class ChangeBundle {
public enum Source {
REVIEW_DB, NOTE_DB;
}
public static ChangeBundle fromReviewDb(ReviewDb db, Change.Id id)
throws OrmException {
db.changes().beginTransaction(id);
@@ -63,7 +77,8 @@ public class ChangeBundle {
db.changeMessages().byChange(id),
db.patchSets().byChange(id),
db.patchSetApprovals().byChange(id),
db.patchComments().byChange(id));
db.patchComments().byChange(id),
Source.REVIEW_DB);
} finally {
db.rollback();
}
@@ -78,7 +93,8 @@ public class ChangeBundle {
notes.getApprovals().values(),
Iterables.concat(
plcUtil.draftByChange(null, notes),
plcUtil.publishedByChange(null, notes)));
plcUtil.publishedByChange(null, notes)),
Source.NOTE_DB);
}
private static Map<ChangeMessage.Key, ChangeMessage> changeMessageMap(
@@ -193,6 +209,7 @@ public class ChangeBundle {
patchSetApprovals;
private final ImmutableMap<PatchLineComment.Key, PatchLineComment>
patchLineComments;
private final Source source;
@VisibleForTesting
ChangeBundle(
@@ -200,7 +217,8 @@ public class ChangeBundle {
Iterable<ChangeMessage> changeMessages,
Iterable<PatchSet> patchSets,
Iterable<PatchSetApproval> patchSetApprovals,
Iterable<PatchLineComment> patchLineComments) {
Iterable<PatchLineComment> patchLineComments,
Source source) {
this.change = checkNotNull(change);
this.changeMessages = ImmutableMap.copyOf(changeMessageMap(changeMessages));
this.patchSets = ImmutableMap.copyOf(patchSetMap(patchSets));
@@ -208,6 +226,7 @@ public class ChangeBundle {
ImmutableMap.copyOf(patchSetApprovalMap(patchSetApprovals));
this.patchLineComments =
ImmutableMap.copyOf(patchLineCommentMap(patchLineComments));
this.source = checkNotNull(source);
for (ChangeMessage.Key k : this.changeMessages.keySet()) {
checkArgument(k.getParentKey().equals(change.getId()));
@@ -244,14 +263,40 @@ public class ChangeBundle {
private static void diffChangeMessages(List<String> diffs,
ChangeBundle bundleA, ChangeBundle bundleB) {
Map<ChangeMessage.Key, ChangeMessage> as = bundleA.changeMessages;
Map<ChangeMessage.Key, ChangeMessage> bs = bundleB.changeMessages;
for (ChangeMessage.Key k : diffKeySets(diffs, as, bs)) {
diffColumns(
diffs, ChangeMessage.class, describe(k), as.get(k), bs.get(k));
if (bundleA.source == REVIEW_DB && bundleB.source == REVIEW_DB) {
// Both came from ReviewDb: check all fields exactly.
Map<ChangeMessage.Key, ChangeMessage> as = bundleA.changeMessages;
Map<ChangeMessage.Key, ChangeMessage> bs = bundleB.changeMessages;
for (ChangeMessage.Key k : diffKeySets(diffs, as, bs)) {
diffColumns(
diffs, ChangeMessage.class, describe(k), as.get(k), bs.get(k));
}
} else {
// At least one is from NoteDb, so we need to normalize UUIDs and
// timestamps for both.
Multiset<NormalizedChangeMessage> as = bundleA.normalizeChangeMessages();
Multiset<NormalizedChangeMessage> bs = bundleB.normalizeChangeMessages();
Set<NormalizedChangeMessage> union = new TreeSet<>();
for (NormalizedChangeMessage m
: Iterables.concat(as.elementSet(), bs.elementSet())) {
union.add(m);
}
for (NormalizedChangeMessage m : union) {
int ac = as.count(m);
int bc = bs.count(m);
if (ac != bc) {
diffs.add("ChangeMessage present "
+ times(ac) + " in A but " + times(bc) + " in B: " + m);
}
}
}
}
private static String times(int n) {
return n + " time" + (n != 1 ? "s" : "");
}
private static void diffPatchSets(List<String> diffs, ChangeBundle bundleA,
ChangeBundle bundleB) {
Map<PatchSet.Id, PatchSet> as = bundleA.patchSets;
@@ -336,6 +381,51 @@ public class ChangeBundle {
return clazz.getEnclosingClass().getSimpleName() + "." + name;
}
@AutoValue
static abstract class NormalizedChangeMessage
implements Comparable<NormalizedChangeMessage> {
private static final Ordering<Comparable<?>> NULLS_FIRST =
Ordering.natural().nullsFirst();
private static NormalizedChangeMessage create(ChangeMessage msg) {
return new AutoValue_ChangeBundle_NormalizedChangeMessage(
msg.getKey().getParentKey(),
msg.getAuthor(),
TimeUtil.roundToSecond(msg.getWrittenOn()),
msg.getMessage(),
msg.getPatchSetId());
}
private static Integer intKey(IntKey<?> k) {
return k != null ? k.get() : null;
}
abstract Change.Id changeId();
@Nullable abstract Account.Id author();
abstract Timestamp writtenOn();
@Nullable abstract String message();
@Nullable abstract PatchSet.Id patchset();
@Override
public int compareTo(NormalizedChangeMessage o) {
return ComparisonChain.start()
.compare(changeId().get(), o.changeId().get())
.compare(intKey(patchset()), intKey(o.patchset()), NULLS_FIRST)
.compare(writtenOn(), o.writtenOn())
.compare(intKey(author()), intKey(o.author()), NULLS_FIRST)
.compare(message(), o.message(), NULLS_FIRST)
.result();
}
}
private Multiset<NormalizedChangeMessage> normalizeChangeMessages() {
Multiset<NormalizedChangeMessage> normalized = TreeMultiset.create();
for (ChangeMessage msg : changeMessages.values()) {
normalized.add(NormalizedChangeMessage.create(msg));
}
return normalized;
}
@Override
public boolean equals(Object o) {
if (!(o instanceof ChangeBundle)) {
@@ -348,7 +438,7 @@ public class ChangeBundle {
public int hashCode() {
return Objects.hash(
change.getId(),
changeMessages.keySet(),
normalizeChangeMessages(),
patchSets.keySet(),
patchSetApprovals.keySet(),
patchLineComments.keySet());

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

@@ -15,7 +15,9 @@
package com.google.gerrit.server.notedb;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.server.notedb.ChangeBundle.Source.NOTE_DB;
import static com.google.gerrit.server.notedb.ChangeBundle.Source.REVIEW_DB;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;
import com.google.gerrit.common.TimeUtil;
@@ -93,9 +95,9 @@ public class ChangeBundleTest {
Change c2 = TestChanges.newChange(project, accountId);
int id2 = c2.getId().get();
ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
comments());
comments(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
comments());
comments(), REVIEW_DB);
assertDiffs(b1, b2,
"changeId differs for Changes: {" + id1 + "} != {" + id2 + "}",
@@ -111,9 +113,9 @@ public class ChangeBundleTest {
new Project.NameKey("project"), new Account.Id(100));
Change c2 = clone(c1);
ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
comments());
comments(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
comments());
comments(), REVIEW_DB);
assertNoDiffs(b1, b2);
@@ -133,9 +135,9 @@ public class ChangeBundleTest {
new ChangeMessage.Key(c.getId(), "uuid2"),
accountId, TimeUtil.nowTs(), c.currentPatchSetId());
ChangeBundle b1 = new ChangeBundle(c, messages(cm1), patchSets(),
approvals(), comments());
approvals(), comments(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(cm2), patchSets(),
approvals(), comments());
approvals(), comments(), REVIEW_DB);
assertDiffs(b1, b2,
"ChangeMessage.Key sets differ:"
@@ -151,9 +153,9 @@ public class ChangeBundleTest {
cm1.setMessage("message 1");
ChangeMessage cm2 = clone(cm1);
ChangeBundle b1 = new ChangeBundle(c, messages(cm1), patchSets(),
approvals(), comments());
approvals(), comments(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(cm2), patchSets(),
approvals(), comments());
approvals(), comments(), REVIEW_DB);
assertNoDiffs(b1, b2);
@@ -163,6 +165,90 @@ public class ChangeBundleTest {
+ " {message 1} != {message 2}");
}
@Test
public void diffChangeMessagesIgnoresUuids() throws Exception {
Change c = TestChanges.newChange(project, accountId);
int id = c.getId().get();
ChangeMessage cm1 = new ChangeMessage(
new ChangeMessage.Key(c.getId(), "uuid1"),
accountId, TimeUtil.nowTs(), c.currentPatchSetId());
cm1.setMessage("message 1");
ChangeMessage cm2 = clone(cm1);
cm2.getKey().set("uuid2");
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 UUID match is required.
assertDiffs(b1, b2,
"ChangeMessage.Key sets differ:"
+ " [" + id + ",uuid1] only in A; [" + id + ",uuid2] only in B");
// One NoteDb, UUIDs are ignored.
b1 = new ChangeBundle(c, messages(cm1), patchSets(), approvals(),
comments(), REVIEW_DB);
b2 = new ChangeBundle(c, messages(cm2), patchSets(), approvals(),
comments(), NOTE_DB);
assertNoDiffs(b1, b2);
}
@Test
public void diffChangeMessagesMixedSourcesWithDifferences() throws Exception {
Change c = TestChanges.newChange(project, accountId);
int id = c.getId().get();
ChangeMessage cm1 = new ChangeMessage(
new ChangeMessage.Key(c.getId(), "uuid1"),
accountId, TimeUtil.nowTs(), c.currentPatchSetId());
cm1.setMessage("message 1");
ChangeMessage cm2 = clone(cm1);
cm2.setMessage("message 2");
ChangeMessage cm3 = clone(cm1);
cm3.getKey().set("uuid2"); // Differs only in UUID.
ChangeBundle b1 = new ChangeBundle(c, messages(cm1, cm3), patchSets(),
approvals(), comments(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(cm2, cm3), patchSets(),
approvals(), comments(), NOTE_DB);
assertDiffs(b1, b2,
"ChangeMessage present 2 times in A but 1 time in B:"
+ " NormalizedChangeMessage{changeId=" + id + ", author=100,"
+ " writtenOn=2009-09-30 17:00:01.0, message=message 1,"
+ " patchset=" + id + ",1}",
"ChangeMessage present 0 times in A but 1 time in B:"
+ " NormalizedChangeMessage{changeId=" + id + ", author=100,"
+ " writtenOn=2009-09-30 17:00:01.0, message=message 2,"
+ " patchset=" + id + ",1}");
}
@Test
public void diffChangeMessagesMixedSourcesRoundsTimestamp() throws Exception {
TestTimeUtil.resetWithClockStep(100, MILLISECONDS);
Change c = TestChanges.newChange(project, accountId);
ChangeMessage cm1 = new ChangeMessage(
new ChangeMessage.Key(c.getId(), "uuid1"),
accountId, TimeUtil.nowTs(), c.currentPatchSetId());
ChangeMessage cm2 = clone(cm1);
cm2.setWrittenOn(TimeUtil.nowTs());
// Both are ReviewDb, exact timestamp match is required.
ChangeBundle b1 = new ChangeBundle(c, messages(cm1), patchSets(),
approvals(), comments(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(cm2), patchSets(),
approvals(), comments(), REVIEW_DB);
assertDiffs(b1, b2,
"writtenOn differs for ChangeMessage.Key " + c.getId() + ",uuid1:"
+ " {2009-09-30 17:00:00.1} != {2009-09-30 17:00:00.2}");
// One NoteDb, timestamp is rounded.
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);
}
@Test
public void diffPatchSetIdSets() throws Exception {
Change c = TestChanges.newChange(project, accountId);
@@ -178,9 +264,9 @@ public class ChangeBundleTest {
ps2.setCreatedOn(TimeUtil.nowTs());
ChangeBundle b1 = new ChangeBundle(c, messages(), patchSets(ps2),
approvals(), comments());
approvals(), comments(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(), patchSets(ps1, ps2),
approvals(), comments());
approvals(), comments(), REVIEW_DB);
assertDiffs(b1, b2,
"PatchSet.Id sets differ:"
@@ -196,9 +282,9 @@ public class ChangeBundleTest {
ps1.setCreatedOn(TimeUtil.nowTs());
PatchSet ps2 = clone(ps1);
ChangeBundle b1 = new ChangeBundle(c, messages(), patchSets(ps1),
approvals(), comments());
approvals(), comments(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(), patchSets(ps2),
approvals(), comments());
approvals(), comments(), REVIEW_DB);
assertNoDiffs(b1, b2);
@@ -225,9 +311,9 @@ public class ChangeBundleTest {
TimeUtil.nowTs());
ChangeBundle b1 = new ChangeBundle(c, messages(), patchSets(),
approvals(a1), comments());
approvals(a1), comments(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(), patchSets(),
approvals(a2), comments());
approvals(a2), comments(), REVIEW_DB);
assertDiffs(b1, b2,
"PatchSetApproval.Key sets differ:"
@@ -245,9 +331,9 @@ public class ChangeBundleTest {
TimeUtil.nowTs());
PatchSetApproval a2 = clone(a1);
ChangeBundle b1 = new ChangeBundle(c, messages(), patchSets(),
approvals(a1), comments());
approvals(a1), comments(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(), patchSets(),
approvals(a2), comments());
approvals(a2), comments(), REVIEW_DB);
assertNoDiffs(b1, b2);
@@ -271,9 +357,9 @@ public class ChangeBundleTest {
5, accountId, null, TimeUtil.nowTs());
ChangeBundle b1 = new ChangeBundle(c, messages(), patchSets(),
approvals(), comments(c1));
approvals(), comments(c1), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(), patchSets(),
approvals(), comments(c2));
approvals(), comments(c2), REVIEW_DB);
assertDiffs(b1, b2,
"PatchLineComment.Key sets differ:"
@@ -290,9 +376,9 @@ public class ChangeBundleTest {
5, accountId, null, TimeUtil.nowTs());
PatchLineComment c2 = clone(c1);
ChangeBundle b1 = new ChangeBundle(c, messages(), patchSets(),
approvals(), comments(c1));
approvals(), comments(c1), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(), patchSets(),
approvals(), comments(c2));
approvals(), comments(c2), REVIEW_DB);
assertNoDiffs(b1, b2);