Merge "ChangeRebuilderImpl: Handle createdOn > lastUpdatedOn" into stable-2.15
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 0324ffa..7e71cd7 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
@@ -868,6 +868,45 @@
}
@Test
+ public void allTimestampsExceptUpdatedAreEqualDueToBadMigration() throws Exception {
+ // https://bugs.chromium.org/p/gerrit/issues/detail?id=7397
+ PushOneCommit.Result r = createChange();
+ Change c = r.getChange().change();
+ Change.Id id = c.getId();
+ Timestamp ts = TimeUtil.nowTs();
+ Timestamp origUpdated = c.getLastUpdatedOn();
+
+ c.setCreatedOn(ts);
+ assertThat(c.getCreatedOn()).isGreaterThan(c.getLastUpdatedOn());
+ db.changes().update(Collections.singleton(c));
+
+ List<ChangeMessage> cm = db.changeMessages().byChange(id).toList();
+ cm.forEach(m -> m.setWrittenOn(ts));
+ db.changeMessages().update(cm);
+
+ List<PatchSet> ps = db.patchSets().byChange(id).toList();
+ ps.forEach(p -> p.setCreatedOn(ts));
+ db.patchSets().update(ps);
+
+ List<PatchSetApproval> psa = db.patchSetApprovals().byChange(id).toList();
+ psa.forEach(p -> p.setGranted(ts));
+ db.patchSetApprovals().update(psa);
+
+ List<PatchLineComment> plc = db.patchComments().byChange(id).toList();
+ plc.forEach(p -> p.setWrittenOn(ts));
+ db.patchComments().update(plc);
+
+ checker.rebuildAndCheckChanges(id);
+
+ setNotesMigration(true, true);
+ ChangeNotes notes = notesFactory.create(db, project, id);
+ assertThat(notes.getChange().getCreatedOn()).isEqualTo(origUpdated);
+ assertThat(notes.getChange().getLastUpdatedOn()).isAtLeast(origUpdated);
+ assertThat(notes.getPatchSets().get(new PatchSet.Id(id, 1)).getCreatedOn())
+ .isEqualTo(origUpdated);
+ }
+
+ @Test
public void createWithAutoRebuildingDisabled() throws Exception {
ReviewDb oldDb = db;
setNotesMigration(true, true);
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 200e0d6..a9663c7 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
@@ -68,6 +68,7 @@
import java.util.List;
import java.util.Map;
import java.util.Objects;
+import java.util.Optional;
import java.util.Set;
import java.util.TreeMap;
import java.util.TreeSet;
@@ -387,6 +388,8 @@
boolean excludeCreatedOn = false;
boolean excludeCurrentPatchSetId = false;
boolean excludeTopic = false;
+ Timestamp aCreated = a.getCreatedOn();
+ Timestamp bCreated = b.getCreatedOn();
Timestamp aUpdated = a.getLastUpdatedOn();
Timestamp bUpdated = b.getLastUpdatedOn();
@@ -397,8 +400,10 @@
String aSubj = Strings.nullToEmpty(a.getSubject());
String bSubj = Strings.nullToEmpty(b.getSubject());
- // Allow created timestamp in NoteDb to be either the created timestamp of
- // the change, or the timestamp of the first remaining patch set.
+ // Allow created timestamp in NoteDb to be any of:
+ // - The created timestamp of the change.
+ // - The timestamp of the first remaining patch set.
+ // - The last updated timestamp, if it is less than the created timestamp.
//
// Ignore subject if the NoteDb subject starts with the ReviewDb subject.
// The NoteDb subject is read directly from the commit, whereas the ReviewDb
@@ -434,8 +439,14 @@
//
// Use max timestamp of all ReviewDb entities when comparing with NoteDb.
if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
+ boolean createdOnMatchesFirstPs =
+ !timestampsDiffer(bundleA, bundleA.getFirstPatchSetTime(), bundleB, bCreated);
+ boolean createdOnMatchesLastUpdatedOn =
+ !timestampsDiffer(bundleA, aUpdated, bundleB, bCreated);
+ boolean createdAfterUpdated = aCreated.compareTo(aUpdated) > 0;
excludeCreatedOn =
- !timestampsDiffer(bundleA, bundleA.getFirstPatchSetTime(), bundleB, b.getCreatedOn());
+ createdOnMatchesFirstPs || (createdAfterUpdated && createdOnMatchesLastUpdatedOn);
+
aSubj = cleanReviewDbSubject(aSubj);
bSubj = cleanNoteDbSubject(bSubj);
excludeCurrentPatchSetId = !bundleA.validPatchSetPredicate().apply(a.currentPatchSetId());
@@ -446,8 +457,14 @@
Objects.equals(aTopic, b.getTopic()) || "".equals(aTopic) && b.getTopic() == null;
aUpdated = bundleA.getLatestTimestamp();
} else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
+ boolean createdOnMatchesFirstPs =
+ !timestampsDiffer(bundleA, aCreated, bundleB, bundleB.getFirstPatchSetTime());
+ boolean createdOnMatchesLastUpdatedOn =
+ !timestampsDiffer(bundleA, aCreated, bundleB, bUpdated);
+ boolean createdAfterUpdated = bCreated.compareTo(bUpdated) > 0;
excludeCreatedOn =
- !timestampsDiffer(bundleA, a.getCreatedOn(), bundleB, bundleB.getFirstPatchSetTime());
+ createdOnMatchesFirstPs || (createdAfterUpdated && createdOnMatchesLastUpdatedOn);
+
aSubj = cleanNoteDbSubject(aSubj);
bSubj = cleanReviewDbSubject(bSubj);
excludeCurrentPatchSetId = !bundleB.validPatchSetPredicate().apply(b.currentPatchSetId());
@@ -651,6 +668,8 @@
List<String> diffs, ChangeBundle bundleA, ChangeBundle bundleB) {
Map<PatchSet.Id, PatchSet> as = bundleA.patchSets;
Map<PatchSet.Id, PatchSet> bs = bundleB.patchSets;
+ Optional<PatchSet.Id> minA = as.keySet().stream().min(intKeyOrdering());
+ Optional<PatchSet.Id> minB = bs.keySet().stream().min(intKeyOrdering());
Set<PatchSet.Id> ids = diffKeySets(diffs, as, bs);
// Old versions of Gerrit had a bug that created patch sets during
@@ -663,11 +682,14 @@
// ignore the createdOn timestamps if both:
// * ReviewDb timestamps are non-monotonic.
// * NoteDb timestamps are monotonic.
- boolean excludeCreatedOn = false;
+ //
+ // Allow the timestamp of the first patch set to match the creation time of
+ // the change.
+ boolean excludeAllCreatedOn = false;
if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
- excludeCreatedOn = !createdOnIsMonotonic(as, ids) && createdOnIsMonotonic(bs, ids);
+ excludeAllCreatedOn = !createdOnIsMonotonic(as, ids) && createdOnIsMonotonic(bs, ids);
} else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
- excludeCreatedOn = createdOnIsMonotonic(as, ids) && !createdOnIsMonotonic(bs, ids);
+ excludeAllCreatedOn = createdOnIsMonotonic(as, ids) && !createdOnIsMonotonic(bs, ids);
}
for (PatchSet.Id id : ids) {
@@ -676,11 +698,16 @@
String desc = describe(id);
String pushCertField = "pushCertificate";
+ boolean excludeCreatedOn = excludeAllCreatedOn;
boolean excludeDesc = false;
if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
excludeDesc = Objects.equals(trimOrNull(a.getDescription()), b.getDescription());
+ excludeCreatedOn |=
+ Optional.of(id).equals(minB) && b.getCreatedOn().equals(bundleB.change.getCreatedOn());
} else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
excludeDesc = Objects.equals(a.getDescription(), trimOrNull(b.getDescription()));
+ excludeCreatedOn |=
+ Optional.of(id).equals(minA) && a.getCreatedOn().equals(bundleA.change.getCreatedOn());
}
List<String> exclude = Lists.newArrayList(pushCertField);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java
index 166d8a9..576a8dd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java
@@ -299,6 +299,13 @@
if (bundle.getPatchSets().isEmpty()) {
throw new NoPatchSetsException(change.getId());
}
+ if (change.getLastUpdatedOn().compareTo(change.getCreatedOn()) < 0) {
+ // A bug in data migration might set created_on to the time of the migration. The
+ // correct timestamps were lost, but we can at least set it so created_on is not after
+ // last_updated_on.
+ // See https://bugs.chromium.org/p/gerrit/issues/detail?id=7397
+ change.setCreatedOn(change.getLastUpdatedOn());
+ }
// We will rebuild all events, except for draft comments, in buckets based on author and
// timestamp.
@@ -428,9 +435,11 @@
new EventSorter(events).sort();
// Ensure the first event in the list creates the change, setting the author and any required
- // footers.
+ // footers. Also force the creation time of the first patch set to match the creation time of
+ // the change.
Event first = events.get(0);
if (first instanceof PatchSetEvent && change.getOwner().equals(first.user)) {
+ first.when = change.getCreatedOn();
((PatchSetEvent) first).createChange = true;
} else {
events.add(0, new CreateChangeEvent(change, minPsNum));
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 90e6800..80a8ab9 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
@@ -667,6 +667,39 @@
}
@Test
+ public void diffChangesAllowsCreatedToMatchLastUpdated() throws Exception {
+ Change c1 = TestChanges.newChange(new Project.NameKey("project"), new Account.Id(100));
+ c1.setCreatedOn(TimeUtil.nowTs());
+ assertThat(c1.getCreatedOn()).isGreaterThan(c1.getLastUpdatedOn());
+ Change c2 = clone(c1);
+ c2.setCreatedOn(c2.getLastUpdatedOn());
+
+ // Both ReviewDb.
+ ChangeBundle b1 =
+ new ChangeBundle(
+ c1, messages(), patchSets(), approvals(), comments(), reviewers(), REVIEW_DB);
+ ChangeBundle b2 =
+ new ChangeBundle(
+ c2, messages(), patchSets(), approvals(), comments(), reviewers(), REVIEW_DB);
+ assertDiffs(
+ b1,
+ b2,
+ "createdOn differs for Change.Id "
+ + c1.getId()
+ + ": {2009-09-30 17:00:06.0} != {2009-09-30 17:00:00.0}");
+
+ // One NoteDb.
+ b1 =
+ new ChangeBundle(
+ c1, messages(), patchSets(), approvals(), comments(), reviewers(), REVIEW_DB);
+ b2 =
+ new ChangeBundle(
+ c2, messages(), patchSets(), approvals(), comments(), reviewers(), NOTE_DB);
+ assertNoDiffs(b1, b2);
+ assertNoDiffs(b2, b1);
+ }
+
+ @Test
public void diffChangeMessageKeySets() throws Exception {
Change c = TestChanges.newChange(project, accountId);
int id = c.getId().get();
@@ -1393,6 +1426,90 @@
}
@Test
+ public void diffPatchSetsAllowsFirstPatchSetCreatedOnToMatchChangeCreatedOn() {
+ Change c = TestChanges.newChange(project, accountId);
+ c.setLastUpdatedOn(TimeUtil.nowTs());
+
+ PatchSet goodPs1 = new PatchSet(new PatchSet.Id(c.getId(), 1));
+ goodPs1.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
+ goodPs1.setUploader(accountId);
+ goodPs1.setCreatedOn(TimeUtil.nowTs());
+ assertThat(goodPs1.getCreatedOn()).isGreaterThan(c.getCreatedOn());
+
+ PatchSet ps1AtCreatedOn = clone(goodPs1);
+ ps1AtCreatedOn.setCreatedOn(c.getCreatedOn());
+
+ PatchSet goodPs2 = new PatchSet(new PatchSet.Id(c.getId(), 2));
+ goodPs2.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
+ goodPs2.setUploader(accountId);
+ goodPs2.setCreatedOn(TimeUtil.nowTs());
+
+ PatchSet ps2AtCreatedOn = clone(goodPs2);
+ ps2AtCreatedOn.setCreatedOn(c.getCreatedOn());
+
+ // Both ReviewDb, exact match required.
+ ChangeBundle b1 =
+ new ChangeBundle(
+ c,
+ messages(),
+ patchSets(goodPs1, goodPs2),
+ approvals(),
+ comments(),
+ reviewers(),
+ REVIEW_DB);
+ ChangeBundle b2 =
+ new ChangeBundle(
+ c,
+ messages(),
+ patchSets(ps1AtCreatedOn, ps2AtCreatedOn),
+ approvals(),
+ comments(),
+ reviewers(),
+ REVIEW_DB);
+ assertDiffs(
+ b1,
+ b2,
+ "createdOn differs for PatchSet.Id "
+ + c.getId()
+ + ",1: {2009-09-30 17:00:12.0} != {2009-09-30 17:00:00.0}",
+ "createdOn differs for PatchSet.Id "
+ + c.getId()
+ + ",2: {2009-09-30 17:00:18.0} != {2009-09-30 17:00:00.0}");
+
+ // One ReviewDb, PS1 is allowed to match change createdOn, but PS2 isn't.
+ b1 =
+ new ChangeBundle(
+ c,
+ messages(),
+ patchSets(goodPs1, goodPs2),
+ approvals(),
+ comments(),
+ reviewers(),
+ REVIEW_DB);
+ b2 =
+ new ChangeBundle(
+ c,
+ messages(),
+ patchSets(ps1AtCreatedOn, ps2AtCreatedOn),
+ approvals(),
+ comments(),
+ reviewers(),
+ NOTE_DB);
+ assertDiffs(
+ b1,
+ b2,
+ "createdOn differs for PatchSet.Id "
+ + c.getId()
+ + ",2 in NoteDb vs. ReviewDb: {2009-09-30 17:00:00.0} != {2009-09-30 17:00:18.0}");
+ assertDiffs(
+ b2,
+ b1,
+ "createdOn differs for PatchSet.Id "
+ + c.getId()
+ + ",2 in NoteDb vs. ReviewDb: {2009-09-30 17:00:00.0} != {2009-09-30 17:00:18.0}");
+ }
+
+ @Test
public void diffPatchSetApprovalKeySets() throws Exception {
Change c = TestChanges.newChange(project, accountId);
int id = c.getId().get();
