diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/ChangeMessage.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/ChangeMessage.java index b98104a0ca..420aa2081e 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/ChangeMessage.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/ChangeMessage.java @@ -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; } -} \ No newline at end of file +} 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 2ae471f8ec..0f88384b35 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 @@ -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 changeMessageMap( @@ -193,6 +209,7 @@ public class ChangeBundle { patchSetApprovals; private final ImmutableMap patchLineComments; + private final Source source; @VisibleForTesting ChangeBundle( @@ -200,7 +217,8 @@ public class ChangeBundle { Iterable changeMessages, Iterable patchSets, Iterable patchSetApprovals, - Iterable patchLineComments) { + Iterable 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 diffs, ChangeBundle bundleA, ChangeBundle bundleB) { - Map as = bundleA.changeMessages; - Map 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 as = bundleA.changeMessages; + Map 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 as = bundleA.normalizeChangeMessages(); + Multiset bs = bundleB.normalizeChangeMessages(); + Set 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 diffs, ChangeBundle bundleA, ChangeBundle bundleB) { Map as = bundleA.patchSets; @@ -336,6 +381,51 @@ public class ChangeBundle { return clazz.getEnclosingClass().getSimpleName() + "." + name; } + @AutoValue + static abstract class NormalizedChangeMessage + implements Comparable { + private static final Ordering> 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 normalizeChangeMessages() { + Multiset 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()); 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 f4223ce390..3e7e81c39a 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 @@ -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);