Bump Change.lastUpdatedOn when adding reviewers

The agreement in a recent discussion[1] was that we want to add
ChangeMessages when adding reviewers to a change. At that point, we
would bump the lasUpdatedOn field in Change, since we always do that
when adding ChangeMessages. Thus there is no real reason to *not* bump
lastUpdatedOn when adding a reviewer; go ahead and do that now. (This
change is just about lastUpdatedOn, and does not actually add that
message.)

This change removes the only place other than draft comments where we
modify a change but don't bump lastUpdatedOn. We still need to deal
with converting old ReviewDb changes to NoteDb where there might be
entities in the bundle that are newer than the lastUpdatedOn
timestamp. Don't worry about messing around with timestamps in NoteDb;
instead just use the latest timestamp from across the whole entity
group (excluding draft comments) when comparing to a ReviewDb
ChangeBundle. This should get us the same results in NoteDb from
reading just the change meta graph.

[1] https://groups.google.com/d/topic/repo-discuss/aULNkde4DKc/discussion

Change-Id: I51978a0234b9c70d9d84a16fc7d30477bf6e3596

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -26,6 +26,7 @@ import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS
 import static com.google.gerrit.server.project.Util.blockLabel;
 import static com.google.gerrit.server.project.Util.category;
 import static com.google.gerrit.server.project.Util.value;
+import static java.util.concurrent.TimeUnit.SECONDS;
 
 import com.google.common.base.Function;
 import com.google.common.collect.ImmutableSet;
@@ -69,12 +70,15 @@ import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.Util;
 import com.google.gerrit.testutil.FakeEmailSender.Message;
 import com.google.gerrit.testutil.NoteDbMode;
+import com.google.gerrit.testutil.TestTimeUtil;
 
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
+import org.junit.After;
+import org.junit.Before;
 import org.junit.Test;
 
 import java.sql.Timestamp;
@@ -88,6 +92,18 @@ import java.util.Map;
 
 @NoHttpd
 public class ChangeIT extends AbstractDaemonTest {
+  private String systemTimeZone;
+
+  @Before
+  public void setTimeForTesting() {
+    systemTimeZone = System.setProperty("user.timezone", "US/Eastern");
+  }
+
+  @After
+  public void resetTime() {
+    TestTimeUtil.useSystemTime();
+    System.setProperty("user.timezone", systemTimeZone);
+  }
 
   @Test
   public void get() throws Exception {
@@ -503,6 +519,7 @@ public class ChangeIT extends AbstractDaemonTest {
 
   @Test
   public void addReviewer() throws Exception {
+    TestTimeUtil.resetWithClockStep(1, SECONDS);
     PushOneCommit.Result r = createChange();
     ChangeResource rsrc = parseResource(r);
     String oldETag = rsrc.getETag();
@@ -538,10 +555,10 @@ public class ChangeIT extends AbstractDaemonTest {
     assertThat(reviewers.iterator().next()._accountId)
         .isEqualTo(user.getId().get());
 
-    // Ensure ETag is updated but lastUpdatedOn isn't.
+    // Ensure ETag and lastUpdatedOn are updated.
     rsrc = parseResource(r);
     assertThat(rsrc.getETag()).isNotEqualTo(oldETag);
-    assertThat(rsrc.getChange().getLastUpdatedOn()).isEqualTo(oldTs);
+    assertThat(rsrc.getChange().getLastUpdatedOn()).isNotEqualTo(oldTs);
   }
 
   @Test

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

@@ -279,7 +279,6 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
         return false;
       }
       patchSet = psUtil.current(dbProvider.get(), rsrc.getNotes());
-      ctx.bumpLastUpdatedOn(false);
       return true;
     }
 

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

@@ -307,6 +307,27 @@ public class ChangeBundle {
     return ImmutableList.copyOf(diffs);
   }
 
+  private Timestamp getLatestTimestamp() {
+    Ordering<Timestamp> o = Ordering.natural();
+    Timestamp ts = change.getLastUpdatedOn();
+    for (ChangeMessage cm : getChangeMessages()) {
+      ts = o.max(ts, cm.getWrittenOn());
+    }
+    for (PatchSet ps : getPatchSets()) {
+      ts = o.max(ts, ps.getCreatedOn());
+    }
+    for (PatchSetApproval psa : getPatchSetApprovals()) {
+      ts = o.max(ts, psa.getGranted());
+    }
+    for (PatchLineComment plc : getPatchLineComments()) {
+      // Ignore draft comments, as they do not show up in the change meta graph.
+      if (plc.getStatus() != PatchLineComment.Status.DRAFT) {
+        ts = o.max(ts, plc.getWrittenOn());
+      }
+    }
+    return ts;
+  }
+
   private static void diffChanges(List<String> diffs, ChangeBundle bundleA,
       ChangeBundle bundleB) {
     Change a = bundleA.change;
@@ -315,19 +336,28 @@ public class ChangeBundle {
 
     boolean excludeOrigSubj = false;
     boolean excludeTopic = false;
+    Timestamp aUpdated = a.getLastUpdatedOn();
+    Timestamp bUpdated = b.getLastUpdatedOn();
+
     // Ignore null original subject on the ReviewDb side, as this field is
     // always set in NoteDb.
     //
     // Ignore empty topic on the ReviewDb side if it is null on the NoteDb side.
+    //
+    // Use max timestamp of all ReviewDb entities when comparing with NoteDb.
     if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
       excludeOrigSubj = a.getOriginalSubjectOrNull() == null;
       excludeTopic = "".equals(a.getTopic()) && b.getTopic() == null;
+      aUpdated = bundleA.getLatestTimestamp();
     } else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
       excludeOrigSubj = b.getOriginalSubjectOrNull() == null;
       excludeTopic = a.getTopic() == null && "".equals(b.getTopic());
+      bUpdated = bundleB.getLatestTimestamp();
     }
 
-    List<String> exclude = Lists.newArrayList("rowVersion", "noteDbState");
+    String updatedField = "lastUpdatedOn";
+    List<String> exclude =
+        Lists.newArrayList(updatedField, "noteDbState", "rowVersion");
     if (excludeOrigSubj) {
       exclude.add("originalSubject");
     }
@@ -336,6 +366,15 @@ public class ChangeBundle {
     }
     diffColumnsExcluding(diffs, Change.class, desc, bundleA, a, bundleB, b,
         exclude);
+
+    // Allow timestamps to either be exactly equal (within slop), or the NoteDb
+    // timestamp to be equal to the latest entity timestamp in the whole
+    // ReviewDb bundle (within slop).
+    if (timestampsDiffer(bundleA, a.getLastUpdatedOn(),
+          bundleB, b.getLastUpdatedOn())) {
+      diffTimestamps(diffs, desc, bundleA, aUpdated, bundleB, bUpdated,
+          "effective last updated time");
+    }
   }
 
   private static void diffChangeMessages(List<String> diffs,
@@ -516,15 +555,28 @@ public class ChangeBundle {
         | SecurityException e) {
       throw new IllegalArgumentException(e);
     }
+    diffTimestamps(diffs, desc, bundleA, ta, bundleB, tb, field);
+  }
+
+  private static void diffTimestamps(List<String> diffs, String desc,
+      ChangeBundle bundleA, Timestamp ta, ChangeBundle bundleB, Timestamp tb,
+      String fieldDesc) {
     if (bundleA.source == bundleB.source || ta == null || tb == null) {
-      diffValues(diffs, desc, ta, tb, field);
+      diffValues(diffs, desc, ta, tb, fieldDesc);
     } else if (bundleA.source == NOTE_DB) {
-      diffTimestamps(diffs, desc, ta, tb, field);
+      diffTimestamps(diffs, desc, ta, tb, fieldDesc);
     } else {
-      diffTimestamps(diffs, desc, tb, ta, field);
+      diffTimestamps(diffs, desc, tb, ta, fieldDesc);
     }
   }
 
+  private static boolean timestampsDiffer(ChangeBundle bundleA, Timestamp ta,
+      ChangeBundle bundleB, Timestamp tb) {
+    List<String> tempDiffs = new ArrayList<>(1);
+    diffTimestamps(tempDiffs, "temp", bundleA, ta, bundleB, tb, "temp");
+    return !tempDiffs.isEmpty();
+  }
+
   private static void diffTimestamps(List<String> diffs, String desc,
       Timestamp tsFromNoteDb, Timestamp tsFromReviewDb, String field) {
     // Because ChangeRebuilder may batch events together that are several

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

@@ -207,18 +207,14 @@ class ChangeNotesParser implements AutoCloseable {
     Timestamp ts =
         new Timestamp(commit.getCommitterIdent().getWhen().getTime());
 
-    boolean updateTs = commit.getParentCount() == 0;
     createdOn = ts;
     parseTag(commit);
-    updateTs |= tag != null;
 
     if (branch == null) {
       branch = parseBranch(commit);
-      updateTs |= branch != null;
     }
     if (status == null) {
       status = parseStatus(commit);
-      updateTs |= status != null;
     }
 
     PatchSet.Id psId = parsePatchSetId(commit);
@@ -238,7 +234,6 @@ class ChangeNotesParser implements AutoCloseable {
 
     if (changeId == null) {
       changeId = parseChangeId(commit);
-      updateTs |= changeId != null;
     }
 
     String currSubject = parseSubject(commit);
@@ -247,28 +242,22 @@ class ChangeNotesParser implements AutoCloseable {
         subject = currSubject;
       }
       originalSubject = currSubject;
-      updateTs = true;
     }
 
-    updateTs |= parseChangeMessage(psId, accountId, commit, ts) != null;
+    parseChangeMessage(psId, accountId, commit, ts);
     if (topic == null) {
       topic = parseTopic(commit);
-      updateTs |= topic != null;
     }
 
-    Set<String> oldHashtags = hashtags;
     parseHashtags(commit);
-    updateTs |= hashtags != oldHashtags;
 
     if (submissionId == null) {
       submissionId = parseSubmissionId(commit);
-      updateTs |= submissionId != null;
     }
 
     ObjectId currRev = parseRevision(commit);
     if (currRev != null) {
       parsePatchSet(psId, currRev, accountId, ts);
-      updateTs = true;
     }
     parseGroups(psId, commit);
 
@@ -276,12 +265,10 @@ class ChangeNotesParser implements AutoCloseable {
       // Only parse the most recent set of submit records; any older ones are
       // still there, but not currently used.
       parseSubmitRecords(commit.getFooterLineValues(FOOTER_SUBMITTED_WITH));
-      updateTs |= !submitRecords.isEmpty();
     }
 
     for (String line : commit.getFooterLineValues(FOOTER_LABEL)) {
       parseApproval(psId, accountId, ts, line);
-      updateTs = true;
     }
 
     for (ReviewerStateInternal state : ReviewerStateInternal.values()) {
@@ -292,10 +279,8 @@ class ChangeNotesParser implements AutoCloseable {
       // behavior.
     }
 
-    if (updateTs) {
+    if (lastUpdatedOn == null || ts.after(lastUpdatedOn)) {
-      if (lastUpdatedOn == null || ts.after(lastUpdatedOn)) {
+      lastUpdatedOn = ts;
-        lastUpdatedOn = ts;
-      }
     }
   }
 
@@ -476,7 +461,7 @@ class ChangeNotesParser implements AutoCloseable {
     throw invalidFooter(FOOTER_PATCH_SET, psIdLine);
   }
 
-  private ChangeMessage parseChangeMessage(PatchSet.Id psId,
+  private void parseChangeMessage(PatchSet.Id psId,
       Account.Id accountId, ChangeNotesCommit commit, Timestamp ts) {
     byte[] raw = commit.getRawBuffer();
     int size = raw.length;
@@ -484,12 +469,12 @@ class ChangeNotesParser implements AutoCloseable {
 
     int subjectStart = RawParseUtils.commitMessage(raw, 0);
     if (subjectStart < 0 || subjectStart >= size) {
-      return null;
+      return;
     }
 
     int subjectEnd = RawParseUtils.endOfParagraph(raw, subjectStart);
     if (subjectEnd == size) {
-      return null;
+      return;
     }
 
     int changeMessageStart;
@@ -499,7 +484,7 @@ class ChangeNotesParser implements AutoCloseable {
     } else if (raw[subjectEnd] == '\r') {
       changeMessageStart = subjectEnd + 4; //\r\n\r\n ends paragraph
     } else {
-      return null;
+      return;
     }
 
     int ptr = size - 1;
@@ -519,7 +504,7 @@ class ChangeNotesParser implements AutoCloseable {
     }
 
     if (ptr <= changeMessageStart) {
-      return null;
+      return;
     }
 
     String changeMsgString = RawParseUtils.decode(enc, raw,
@@ -533,7 +518,6 @@ class ChangeNotesParser implements AutoCloseable {
     changeMessage.setTag(tag);
     changeMessagesByPatchSet.put(psId, changeMessage);
     allChangeMessages.add(changeMessage);
-    return changeMessage;
   }
 
   private void parseNotes()

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

@@ -117,7 +117,7 @@ public class ChangeBundleTest {
         "changeId differs for Changes: {" + id1 + "} != {" + id2 + "}",
         "createdOn differs for Changes:"
             + " {2009-09-30 17:00:00.0} != {2009-09-30 17:00:06.0}",
-        "lastUpdatedOn differs for Changes:"
+        "effective last updated time differs for Changes:"
             + " {2009-09-30 17:00:00.0} != {2009-09-30 17:00:06.0}");
   }
 
@@ -155,7 +155,7 @@ public class ChangeBundleTest {
     assertDiffs(b1, b2,
         "createdOn differs for Change.Id " + c1.getId() + ":"
             + " {2009-09-30 17:00:01.0} != {2009-09-30 17:00:02.0}",
-        "lastUpdatedOn differs for Change.Id " + c1.getId() + ":"
+        "effective last updated time differs for Change.Id " + c1.getId() + ":"
             + " {2009-09-30 17:00:01.0} != {2009-09-30 17:00:03.0}");
 
     // One NoteDb, slop is allowed.
@@ -174,8 +174,8 @@ public class ChangeBundleTest {
         comments(), NOTE_DB);
     ChangeBundle b3 = new ChangeBundle(c3, messages(), patchSets(), approvals(),
         comments(), REVIEW_DB);
-    String msg = "lastUpdatedOn differs for Change.Id " + c1.getId()
+    String msg = "effective last updated time differs for Change.Id "
-        + " in NoteDb vs. ReviewDb:"
+        + c1.getId() + " in NoteDb vs. ReviewDb:"
         + " {2009-09-30 17:00:01.0} != {2009-09-30 17:00:10.0}";
     assertDiffs(b1, b3, msg);
     assertDiffs(b3, b1, msg);
@@ -272,6 +272,35 @@ public class ChangeBundleTest {
             + " {topic} != {null}");
   }
 
+  @Test
+  public void diffChangesTakesMaxEntityTimestampFromReviewDb()
+      throws Exception {
+    Change c1 = TestChanges.newChange(
+        new Project.NameKey("project"), new Account.Id(100));
+    PatchSetApproval a = new PatchSetApproval(
+        new PatchSetApproval.Key(
+            c1.currentPatchSetId(), accountId, new LabelId("Code-Review")),
+        (short) 1,
+        TimeUtil.nowTs());
+
+    Change c2 = clone(c1);
+    c2.setLastUpdatedOn(a.getGranted());
+
+    // Both ReviewDb, exact match required.
+    ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(),
+        approvals(a), comments(), REVIEW_DB);
+    ChangeBundle b2 = new ChangeBundle(c2, messages(), patchSets(),
+        approvals(a), comments(), REVIEW_DB);
+    assertDiffs(b1, b2,
+        "effective last updated time differs for Change.Id " + c1.getId() + ":"
+            + " {2009-09-30 17:00:00.0} != {2009-09-30 17:00:06.0}");
+
+    // NoteDb allows latest timestamp from all entities in bundle.
+    b2 = new ChangeBundle(c2, messages(), patchSets(),
+        approvals(a), comments(), NOTE_DB);
+    assertNoDiffs(b1, b2);
+  }
+
   @Test
   public void diffChangeMessageKeySets() throws Exception {
     Change c = TestChanges.newChange(project, accountId);

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

@@ -728,18 +728,17 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     Timestamp ts7 = newNotes(c).getChange().getLastUpdatedOn();
     assertThat(ts7).isGreaterThan(ts6);
 
-    // Updates that should not touch the timestamp.
     update = newUpdate(c, changeOwner);
     update.putReviewer(otherUser.getAccountId(), ReviewerStateInternal.REVIEWER);
     update.commit();
     Timestamp ts8 = newNotes(c).getChange().getLastUpdatedOn();
-    assertThat(ts8).isEqualTo(ts7);
+    assertThat(ts8).isGreaterThan(ts7);
 
     update = newUpdate(c, changeOwner);
     update.setGroups(ImmutableList.of("a", "b"));
     update.commit();
     Timestamp ts9 = newNotes(c).getChange().getLastUpdatedOn();
-    assertThat(ts9).isEqualTo(ts8);
+    assertThat(ts9).isGreaterThan(ts8);
 
     // Finish off by merging the change.
     update = newUpdate(c, changeOwner);