Merge "ChangeRebuilderImpl: Handle createdOn > lastUpdatedOn" into stable-2.15

4 files changed, 200 insertions, 8 deletions

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 0324ffa254..7e71cd7dc3 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 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
   }
 
   @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 200e0d6bec..a9663c735e 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.LinkedList;
 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 @@ public class ChangeBundle {
     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 @@ public class ChangeBundle {
     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 @@ public class ChangeBundle {
     //
     // 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 @@ public class ChangeBundle {
           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 @@ public class ChangeBundle {
       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 @@ public class ChangeBundle {
     // 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 @@ public class ChangeBundle {
       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 166d8a9c53..576a8dd41b 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 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
     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 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
     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 90e680026c..80a8ab94c4 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 @@ public class ChangeBundleTest extends GerritBaseTests {
   }
 
   @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 @@ public class ChangeBundleTest extends GerritBaseTests {
   }
 
   @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();