ChangeRebuilder: Add dependencies on PatchSetEvent

This ensures events for patch sets don't get sorted before the
PatchSetEvent that created them. This can happen due to timestamp
skew, or the corner case described in If8ef0726.

Change-Id: I45b16437518fb396e1d42d4aedb21a42258364ac

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

@@ -715,6 +715,8 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     rin.message = "comment";
 
     Timestamp ts = new Timestamp(c.getCreatedOn().getTime() + 2000);
+    assertThat(ts).isGreaterThan(c.getCreatedOn());
+    assertThat(ts).isLessThan(db.patchSets().get(psId).getCreatedOn());
     RevisionResource revRsrc = parseCurrentRevisionResource(r.getChangeId());
     postReview.get().apply(revRsrc, rin, ts);
 

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

@@ -624,17 +624,31 @@ public class ChangeBundle {
     List<String> tempDiffs = new ArrayList<>();
     String temp = "temp";
 
+    // ReviewDb allows timestamps before patch set was created, but NoteDb
+    // truncates this to the patch set creation timestamp.
+    Timestamp ta = a.getWrittenOn();
+    Timestamp tb = b.getWrittenOn();
+    PatchSet psa = bundleA.patchSets.get(a.getPatchSetId());
+    PatchSet psb = bundleB.patchSets.get(b.getPatchSetId());
     boolean excludePatchSet = false;
+    boolean excludeWrittenOn = false;
     if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
       excludePatchSet = a.getPatchSetId() == null;
+      excludeWrittenOn = psa != null && psb != null
+          && ta.before(psa.getCreatedOn()) && tb.equals(psb.getCreatedOn());
     } else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
       excludePatchSet = b.getPatchSetId() == null;
+      excludeWrittenOn = psa != null && psb != null
+          && tb.before(psb.getCreatedOn()) && ta.equals(psa.getCreatedOn());
     }
 
     List<String> exclude = Lists.newArrayList("key");
     if (excludePatchSet) {
       exclude.add("patchset");
     }
+    if (excludeWrittenOn) {
+      exclude.add("writtenOn");
+    }
 
     diffColumnsExcluding(
         tempDiffs, ChangeMessage.class, temp, bundleA, a, bundleB, b, exclude);
@@ -673,7 +687,28 @@ public class ChangeBundle {
       PatchSetApproval a = as.get(k);
       PatchSetApproval b = bs.get(k);
       String desc = describe(k);
-      diffColumns(diffs, PatchSetApproval.class, desc, bundleA, a, bundleB, b);
+
+      // ReviewDb allows timestamps before patch set was created, but NoteDb
+      // truncates this to the patch set creation timestamp.
+      Timestamp ta = a.getGranted();
+      Timestamp tb = b.getGranted();
+      PatchSet psa = checkNotNull(bundleA.patchSets.get(a.getPatchSetId()));
+      PatchSet psb = checkNotNull(bundleB.patchSets.get(b.getPatchSetId()));
+      boolean excludeGranted = false;
+      List<String> exclude = new ArrayList<>(1);
+      if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
+        excludeGranted =
+            ta.before(psa.getCreatedOn()) && tb.equals(psb.getCreatedOn());
+      } else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
+        excludeGranted =
+            tb.before(psb.getCreatedOn()) && ta.equals(psa.getCreatedOn());
+      }
+      if (excludeGranted) {
+        exclude.add("granted");
+      }
+
+      diffColumnsExcluding(
+          diffs, PatchSetApproval.class, desc, bundleA, a, bundleB, b, exclude);
     }
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java

@@ -29,6 +29,7 @@ import com.google.common.base.Splitter;
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Maps;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.Ordering;
 import com.google.common.collect.Sets;
@@ -92,6 +93,7 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
+import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
 
@@ -309,8 +311,8 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
     deleteDraftRefs(change, manager.getAllUsersRepo());
 
     Integer minPsNum = getMinPatchSetNum(bundle);
-    Set<PatchSet.Id> psIds =
+    Map<PatchSet.Id, PatchSetEvent> patchSetEvents =
-        Sets.newHashSetWithExpectedSize(bundle.getPatchSets().size());
+        Maps.newHashMapWithExpectedSize(bundle.getPatchSets().size());
 
     for (PatchSet ps : bundle.getPatchSets()) {
       if (ps.getId().get() > currPsId.get()) {
@@ -319,14 +321,15 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
             ps.getId(), currPsId);
         continue;
       }
-      psIds.add(ps.getId());
+      PatchSetEvent pse =
-      events.add(new PatchSetEvent(
+          new PatchSetEvent(change, ps, manager.getChangeRepo().rw);
-          change, ps, manager.getChangeRepo().rw));
+      patchSetEvents.put(ps.getId(), pse);
+      events.add(pse);
       for (PatchLineComment c : getPatchLineComments(bundle, ps)) {
         PatchLineCommentEvent e =
             new PatchLineCommentEvent(c, change, ps, patchListCache);
         if (c.getStatus() == Status.PUBLISHED) {
-          events.add(e);
+          events.add(e.addDep(pse));
         } else {
           draftCommentEvents.put(c.getAuthor(), e);
         }
@@ -334,8 +337,9 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
     }
 
     for (PatchSetApproval psa : bundle.getPatchSetApprovals()) {
-      if (psIds.contains(psa.getPatchSetId())) {
+      PatchSetEvent pse = patchSetEvents.get(psa.getPatchSetId());
-        events.add(new ApprovalEvent(psa, change.getCreatedOn()));
+      if (pse != null) {
+        events.add(new ApprovalEvent(psa, change.getCreatedOn()).addDep(pse));
       }
     }
 
@@ -346,9 +350,18 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
 
     Change noteDbChange = new Change(null, null, null, null, null);
     for (ChangeMessage msg : bundle.getChangeMessages()) {
-      if (msg.getPatchSetId() == null || psIds.contains(msg.getPatchSetId())) {
+      if (msg.getPatchSetId() == null) {
+        // No dependency necessary; will get assigned to most recent patch set
+        // in sortAndFillEvents.
         events.add(
             new ChangeMessageEvent(msg, noteDbChange, change.getCreatedOn()));
+        continue;
+      }
+      PatchSetEvent pse = patchSetEvents.get(msg.getPatchSetId());
+      if (pse != null) {
+        events.add(
+            new ChangeMessageEvent(msg, noteDbChange, change.getCreatedOn())
+                .addDep(pse));
       }
     }
 

gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/Event.java

@@ -66,8 +66,9 @@ abstract class Event implements Comparable<Event> {
         who, update.getNullableAccountId());
   }
 
-  void addDep(Event e) {
+  Event addDep(Event e) {
     deps.add(e);
+    return this;
   }
 
   /**

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

@@ -1260,7 +1260,9 @@ public class ChangeBundleTest {
   }
 
   private static List<PatchSet> latest(Change c) {
-    return ImmutableList.of(new PatchSet(c.currentPatchSetId()));
+    PatchSet ps = new PatchSet(c.currentPatchSetId());
+    ps.setCreatedOn(c.getLastUpdatedOn());
+    return ImmutableList.of(ps);
   }
 
   private static List<PatchSetApproval> approvals(PatchSetApproval... ents) {