PostReview: Don't allow timestamps before change's CreatedOn date

Since createdOn is defined in NoteDb as the oldest timestamp in the
change meta DAG, using a very old timestamp will cause createdOn to
jump back when a change is converted to NoteDb. Avoid this by
truncating such timestamps in ChangeRebuilder.

Also avoid putting ourselves in this situation in ReviewDb in the
first place, by silently truncating the timestamp during PostReview.
This means new changes will not have this problem, but we still need
to accept it in ChangeRebuilder for converting old changes.

Change-Id: Ife53b120676ee8228873ddd2b7c658d8f26697f5

gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java

@@ -24,6 +24,7 @@ import com.google.auto.value.AutoValue;
 import com.google.common.base.Strings;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
+import com.google.common.collect.Ordering;
 import com.google.common.collect.Sets;
 import com.google.common.hash.HashCode;
 import com.google.common.hash.Hashing;
@@ -145,6 +146,8 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
 
   public Output apply(RevisionResource revision, ReviewInput input,
       Timestamp ts) throws RestApiException, UpdateException, OrmException {
+    // Respect timestamp, but truncate at change created-on time.
+    ts = Ordering.natural().max(ts, revision.getChange().getCreatedOn());
     if (revision.getEdit().isPresent()) {
       throw new ResourceConflictException("cannot post review on edit");
     }

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

@@ -18,6 +18,7 @@ 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.common.TimeUtil.roundToSecond;
+import static com.google.gerrit.reviewdb.server.ReviewDbUtil.intKeyOrdering;
 import static com.google.gerrit.server.notedb.ChangeBundle.Source.NOTE_DB;
 import static com.google.gerrit.server.notedb.ChangeBundle.Source.REVIEW_DB;
 
@@ -27,6 +28,7 @@ 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.Ordering;
 import com.google.common.collect.Sets;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
@@ -110,6 +112,33 @@ public class ChangeBundle {
     return out;
   }
 
+  private static ImmutableList<ChangeMessage> changeMessageList(
+      Iterable<ChangeMessage> in) {
+    // Unlike the *Map comparators, which are intended to make key lists
+    // diffable, this comparator sorts first on timestamp, then on every other
+    // field.
+    final Ordering<Comparable<?>> nullsFirst = Ordering.natural().nullsFirst();
+    return new Ordering<ChangeMessage>() {
+      @Override
+      public int compare(ChangeMessage a, ChangeMessage b) {
+        return ComparisonChain.start()
+            .compare(roundToSecond(a.getWrittenOn()),
+                roundToSecond(b.getWrittenOn()))
+            .compare(a.getKey().getParentKey().get(),
+                b.getKey().getParentKey().get())
+            .compare(psId(a), psId(b), nullsFirst)
+            .compare(a.getAuthor(), b.getAuthor(), intKeyOrdering())
+            .compare(a.getMessage(), b.getMessage(), nullsFirst)
+            .result();
+      }
+
+      private Integer psId(ChangeMessage m) {
+        return m.getPatchSetId() != null ? m.getPatchSetId().get() : null;
+      }
+    }.immutableSortedCopy(in);
+  }
+
+
   private static Map<PatchSet.Id, PatchSet> patchSetMap(Iterable<PatchSet> in) {
     Map<PatchSet.Id, PatchSet> out = new TreeMap<>(
         new Comparator<PatchSet.Id>() {
@@ -215,8 +244,7 @@ public class ChangeBundle {
       Iterable<PatchLineComment> patchLineComments,
       Source source) {
     this.change = checkNotNull(change);
-    this.changeMessages =
-        ChangeNotes.MESSAGE_BY_TIME.immutableSortedCopy(changeMessages);
+    this.changeMessages = changeMessageList(changeMessages);
     this.patchSets = ImmutableMap.copyOf(patchSetMap(patchSets));
     this.patchSetApprovals =
         ImmutableMap.copyOf(patchSetApprovalMap(patchSetApprovals));

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

@@ -186,12 +186,13 @@ public class ChangeRebuilder {
     }
 
     for (PatchSetApproval psa : db.patchSetApprovals().byChange(changeId)) {
-      events.add(new ApprovalEvent(psa));
+      events.add(new ApprovalEvent(psa, change.getCreatedOn()));
     }
 
     Change notedbChange = new Change(null, null, null, null, null);
     for (ChangeMessage msg : db.changeMessages().byChange(changeId)) {
-      events.add(new ChangeMessageEvent(msg, notedbChange));
+      events.add(
+          new ChangeMessageEvent(msg, notedbChange, change.getCreatedOn()));
     }
 
     Collections.sort(events, EVENT_ORDER);
@@ -301,7 +302,8 @@ public class ChangeRebuilder {
 
       Timestamp commitTime =
           new Timestamp(commit.getCommitterIdent().getWhen().getTime());
-      events.add(new HashtagsEvent(psId, authorId, commitTime, hashtags));
+      events.add(new HashtagsEvent(psId, authorId, commitTime, hashtags,
+            change.getCreatedOn()));
     }
     return events;
   }
@@ -344,6 +346,7 @@ public class ChangeRebuilder {
     @Override
     public int compare(Event a, Event b) {
       return ComparisonChain.start()
+          .compareTrueFirst(a.predatesChange, b.predatesChange)
           .compare(a.when, b.when)
           .compare(a.who.get(), b.who.get())
           .compare(a.psId.get(), b.psId.get())
@@ -358,11 +361,15 @@ public class ChangeRebuilder {
     final PatchSet.Id psId;
     final Account.Id who;
     final Timestamp when;
+    final boolean predatesChange;
 
-    protected Event(PatchSet.Id psId, Account.Id who, Timestamp when) {
+    protected Event(PatchSet.Id psId, Account.Id who, Timestamp when,
+        Timestamp changeCreatedOn) {
       this.psId = psId;
       this.who = who;
-      this.when = when;
+      // Truncate timestamps at the change's createdOn timestamp.
+      predatesChange = when.before(changeCreatedOn);
+      this.when = predatesChange ? changeCreatedOn : when;
     }
 
     protected void checkUpdate(AbstractChangeUpdate update) {
@@ -477,8 +484,9 @@ public class ChangeRebuilder {
   private static class ApprovalEvent extends Event {
     private PatchSetApproval psa;
 
-    ApprovalEvent(PatchSetApproval psa) {
-      super(psa.getPatchSetId(), psa.getAccountId(), psa.getGranted());
+    ApprovalEvent(PatchSetApproval psa, Timestamp changeCreatedOn) {
+      super(psa.getPatchSetId(), psa.getAccountId(), psa.getGranted(),
+          changeCreatedOn);
       this.psa = psa;
     }
 
@@ -500,7 +508,8 @@ public class ChangeRebuilder {
     private final RevWalk rw;
 
     PatchSetEvent(Change change, PatchSet ps, RevWalk rw) {
-      super(ps.getId(), ps.getUploader(), ps.getCreatedOn());
+      super(ps.getId(), ps.getUploader(), ps.getCreatedOn(),
+          change.getCreatedOn());
       this.change = change;
       this.ps = ps;
       this.rw = rw;
@@ -557,7 +566,8 @@ public class ChangeRebuilder {
 
     PatchLineCommentEvent(PatchLineComment c, Change change, PatchSet ps,
         PatchListCache cache) {
-      super(PatchLineCommentsUtil.getCommentPsId(c), c.getAuthor(), c.getWrittenOn());
+      super(PatchLineCommentsUtil.getCommentPsId(c), c.getAuthor(),
+          c.getWrittenOn(), change.getCreatedOn());
       this.c = c;
       this.change = change;
       this.ps = ps;
@@ -590,8 +600,8 @@ public class ChangeRebuilder {
     private final Set<String> hashtags;
 
     HashtagsEvent(PatchSet.Id psId, Account.Id who, Timestamp when,
-        Set<String> hashtags) {
-      super(psId, who, when);
+        Set<String> hashtags, Timestamp changeCreatdOn) {
+      super(psId, who, when, changeCreatdOn);
       this.hashtags = hashtags;
     }
 
@@ -624,9 +634,10 @@ public class ChangeRebuilder {
     private final ChangeMessage message;
     private final Change notedbChange;
 
-    ChangeMessageEvent(ChangeMessage message, Change notedbChange) {
+    ChangeMessageEvent(ChangeMessage message, Change notedbChange,
+        Timestamp changeCreatedOn) {
       super(message.getPatchSetId(), message.getAuthor(),
-          message.getWrittenOn());
+          message.getWrittenOn(), changeCreatedOn);
       this.message = message;
       this.notedbChange = notedbChange;
     }
@@ -689,11 +700,7 @@ public class ChangeRebuilder {
 
     FinalUpdatesEvent(Change change, Change notedbChange) {
       super(change.currentPatchSetId(), change.getOwner(),
-          // TODO(dborowitz): This should maybe be a synthetic timestamp just
-          // after the actual last update in the history. On the one hand using
-          // the commit updated time is reasonable, but on the other it might be
-          // non-monotonic, and who knows what would break then.
-          change.getLastUpdatedOn());
+          change.getLastUpdatedOn(), change.getCreatedOn());
       this.change = change;
       this.notedbChange = notedbChange;
     }