Populate createdOn and lastUpdatedOn fields of a change from notedb

createdOn/lastUpdatedOn is set to the timestamp of the first/last
commit on the notes branch.

The initial update of the change notes must create a commit even when
it is empty so that the createdOn timestamp is recorded. This initial
update is done when the change is created.

To reflect this is in AbstractChangeNotesTest the newChange method
which creates a change is updated to also make an initial update to
the change notes. As consequence of this all tests that assert
timestamps must be adapted because the creation of the initial update
of the change notes consumes one timestamp.

Also the newUpdate method in AbstractChangeNotesTest is adapted to
load the ChangeUpdate that it creates so that its revision field gets
populated. Due to this tests that expected that the revision was null
had to be changed as well.

The multipleUpdatesIncludingComments test started to fail because the
same RevWalk instance was passed two times into ChangeNotesParser.
ChangeNotesParser walks the commits and for the second
ChangeNotesParser instance the walk was already done. Instead of
resetting the RevWalk, create a new one since this is easier to read.

PostReview must take care to set the new lastUpdatedOn timestamp on
the change only after the lastUpdatedOn field was populated from
notedb, otherwise the lastUpdatedOn timestamp in the database will not
be updated.

Change-Id: I24e385e32052fc3286fab50c7377686f425baf1f
Signed-off-by: Edwin Kempin <ekempin@google.com>

gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java

@@ -523,6 +523,10 @@ public final class Change {
     return createdOn;
   }
 
+  public void setCreatedOn(Timestamp ts) {
+    createdOn = ts;
+  }
+
   public Timestamp getLastUpdatedOn() {
     return lastUpdatedOn;
   }

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

@@ -354,14 +354,14 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
         throws OrmException, ResourceConflictException {
       user = ctx.getUser().asIdentifiedUser();
       change = ctx.getChange();
-      if (change.getLastUpdatedOn().before(ctx.getWhen())) {
-        change.setLastUpdatedOn(ctx.getWhen());
-      }
       ps = ctx.getDb().patchSets().get(psId);
       boolean dirty = false;
       dirty |= insertComments(ctx);
       dirty |= updateLabels(ctx);
       dirty |= insertMessage(ctx);
+      if (change.getLastUpdatedOn().before(ctx.getWhen())) {
+        change.setLastUpdatedOn(ctx.getWhen());
+      }
       if (dirty) {
         ctx.saveChange();
       }

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

@@ -266,6 +266,8 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
       comments = ImmutableListMultimap.copyOf(parser.comments);
       noteMap = parser.commentNoteMap;
       change.setTopic(Strings.emptyToNull(parser.topic));
+      change.setCreatedOn(parser.createdOn);
+      change.setLastUpdatedOn(parser.lastUpdatedOn);
 
       if (parser.hashtags != null) {
         hashtags = ImmutableSet.copyOf(parser.hashtags);

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

@@ -81,6 +81,8 @@ class ChangeNotesParser implements AutoCloseable {
   Change.Status status;
   String topic;
   Set<String> hashtags;
+  Timestamp createdOn;
+  Timestamp lastUpdatedOn;
 
   private final Change.Id changeId;
   private final ObjectId tip;
@@ -153,6 +155,10 @@ class ChangeNotesParser implements AutoCloseable {
   }
 
   private void parse(RevCommit commit) throws ConfigInvalidException {
+    createdOn = getCommitTime(commit);
+    if (lastUpdatedOn == null) {
+      lastUpdatedOn = getCommitTime(commit);
+    }
     if (status == null) {
       status = parseStatus(commit);
     }
@@ -291,7 +297,7 @@ class ChangeNotesParser implements AutoCloseable {
     ChangeMessage changeMessage = new ChangeMessage(
         new ChangeMessage.Key(psId.getParentKey(), commit.name()),
         accountId,
-        new Timestamp(commit.getCommitterIdent().getWhen().getTime()),
+        getCommitTime(commit),
         psId);
     changeMessage.setMessage(changeMsgString);
     changeMessagesByPatchSet.put(psId, changeMessage);
@@ -348,7 +354,7 @@ class ChangeNotesParser implements AutoCloseable {
               accountId,
               new LabelId(l.label())),
           l.value(),
-          new Timestamp(commit.getCommitterIdent().getWhen().getTime()))));
+          getCommitTime(commit))));
     }
   }
 
@@ -516,6 +522,10 @@ class ChangeNotesParser implements AutoCloseable {
     }
   }
 
+  private static Timestamp getCommitTime(RevCommit commit) {
+    return new Timestamp(commit.getCommitterIdent().getWhen().getTime());
+  }
+
   private ConfigInvalidException parseException(String fmt, Object... args) {
     return ChangeNotes.parseException(changeId, fmt, args);
   }

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

@@ -418,7 +418,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
 
   @Override
   protected boolean onSave(CommitBuilder commit) {
-    if (isEmpty()) {
+    if (getRevision() != null && isEmpty()) {
       return false;
     }
     commit.setAuthor(newIdent(getUser().getAccount(), when));

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

@@ -61,12 +61,15 @@ import com.google.inject.Inject;
 import com.google.inject.Injector;
 import com.google.inject.util.Providers;
 
+import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.Repository;
 import org.junit.After;
 import org.junit.Before;
 
+import java.io.IOException;
 import java.sql.Timestamp;
 import java.util.TimeZone;
 
@@ -159,14 +162,21 @@ public class AbstractChangeNotesTest extends GerritBaseTests {
     System.setProperty("user.timezone", systemTimeZone);
   }
 
-  protected Change newChange() {
-    return TestChanges.newChange(project, changeOwner.getAccountId());
+  protected Change newChange()
+      throws IOException, OrmException, ConfigInvalidException {
+    Change c = TestChanges.newChange(project, changeOwner.getAccountId());
+    newUpdate(c, changeOwner).commit();
+    return c;
   }
 
   protected ChangeUpdate newUpdate(Change c, IdentifiedUser user)
-      throws OrmException {
-    return TestChanges.newUpdate(
+      throws OrmException, IOException, ConfigInvalidException {
+    ChangeUpdate update = TestChanges.newUpdate(
         injector, repoManager, MIGRATION, c, allUsers, user);
+    try (Repository repo = repoManager.openMetadataRepository(c.getProject())) {
+      update.load(repo);
+    }
+    return update;
   }
 
   protected ChangeNotes newNotes(Change c) throws OrmException {

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

@@ -78,7 +78,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     assertThat(psas.get(0).getAccountId().get()).isEqualTo(1);
     assertThat(psas.get(0).getLabel()).isEqualTo("Code-Review");
     assertThat(psas.get(0).getValue()).isEqualTo((short) -1);
-    assertThat(psas.get(0).getGranted()).isEqualTo(truncate(after(c, 1000)));
+    assertThat(psas.get(0).getGranted()).isEqualTo(truncate(after(c, 2000)));
 
     assertThat(psas.get(1).getPatchSetId()).isEqualTo(c.currentPatchSetId());
     assertThat(psas.get(1).getAccountId().get()).isEqualTo(1);
@@ -110,14 +110,14 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     assertThat(psa1.getAccountId().get()).isEqualTo(1);
     assertThat(psa1.getLabel()).isEqualTo("Code-Review");
     assertThat(psa1.getValue()).isEqualTo((short) -1);
-    assertThat(psa1.getGranted()).isEqualTo(truncate(after(c, 1000)));
+    assertThat(psa1.getGranted()).isEqualTo(truncate(after(c, 2000)));
 
     PatchSetApproval psa2 = Iterables.getOnlyElement(psas.get(ps2));
     assertThat(psa2.getPatchSetId()).isEqualTo(ps2);
     assertThat(psa2.getAccountId().get()).isEqualTo(1);
     assertThat(psa2.getLabel()).isEqualTo("Code-Review");
     assertThat(psa2.getValue()).isEqualTo((short) +1);
-    assertThat(psa2.getGranted()).isEqualTo(truncate(after(c, 2000)));
+    assertThat(psa2.getGranted()).isEqualTo(truncate(after(c, 3000)));
   }
 
   @Test
@@ -166,13 +166,13 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     assertThat(psas.get(0).getAccountId().get()).isEqualTo(1);
     assertThat(psas.get(0).getLabel()).isEqualTo("Code-Review");
     assertThat(psas.get(0).getValue()).isEqualTo((short) -1);
-    assertThat(psas.get(0).getGranted()).isEqualTo(truncate(after(c, 1000)));
+    assertThat(psas.get(0).getGranted()).isEqualTo(truncate(after(c, 2000)));
 
     assertThat(psas.get(1).getPatchSetId()).isEqualTo(c.currentPatchSetId());
     assertThat(psas.get(1).getAccountId().get()).isEqualTo(2);
     assertThat(psas.get(1).getLabel()).isEqualTo("Code-Review");
     assertThat(psas.get(1).getValue()).isEqualTo((short) 1);
-    assertThat(psas.get(1).getGranted()).isEqualTo(truncate(after(c, 2000)));
+    assertThat(psas.get(1).getGranted()).isEqualTo(truncate(after(c, 3000)));
   }
 
   @Test
@@ -399,9 +399,18 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
 
   @Test
   public void emptyChangeUpdate() throws Exception {
-    ChangeUpdate update = newUpdate(newChange(), changeOwner);
+    Change c = newChange();
+
+    // The initial empty update creates a commit which is needed to track the
+    // creation time of the change.
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    ObjectId revision = update.getRevision();
+    assertThat(revision).isNotNull();
+
+    // Any further empty update doesn't create a new commit.
+    update = newUpdate(c, changeOwner);
     update.commit();
-    assertThat(update.getRevision()).isNull();
+    assertThat(update.getRevision()).isEqualTo(revision);
   }
 
   @Test
@@ -474,6 +483,37 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     assertThat(notes.getChange().getTopic()).isNull();
   }
 
+  @Test
+  public void createdOnChangeNotes() throws Exception {
+    Change c = newChange();
+
+    Timestamp createdOn = newNotes(c).getChange().getCreatedOn();
+    assertThat(createdOn).isNotNull();
+
+    // An update doesn't affect the createdOn timestamp.
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.setTopic("topic"); // Change something to get a new commit.
+    update.commit();
+    assertThat(newNotes(c).getChange().getCreatedOn()).isEqualTo(createdOn);
+  }
+
+  @Test
+  public void lastUpdatedOnChangeNotes() throws Exception {
+    Change c = newChange();
+
+    ChangeNotes notes = newNotes(c);
+    Timestamp lastUpdatedOn = notes.getChange().getLastUpdatedOn();
+    assertThat(lastUpdatedOn).isNotNull();
+    assertThat(lastUpdatedOn).isEqualTo(notes.getChange().getCreatedOn());
+
+    // An update creates a new lastUpdatedOn timestamp.
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.setTopic("topic"); // Change something to get a new commit.
+    update.commit();
+    assertThat(newNotes(c).getChange().getLastUpdatedOn())
+        .isGreaterThan(lastUpdatedOn);
+  }
+
   @Test
   public void emptyExceptSubject() throws Exception {
     ChangeUpdate update = newUpdate(newChange(), changeOwner);
@@ -537,17 +577,24 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     update2.putApproval("Code-Review", (short) 2);
     update2.writeCommit(batch);
 
+    RevCommit tipCommit;
     try (RevWalk rw = new RevWalk(repo)) {
       batch.commit();
       bru.execute(rw, NullProgressMonitor.INSTANCE);
 
       ChangeNotes notes = newNotes(c);
       ObjectId tip = notes.getRevision();
-      RevCommit commitWithApprovals = rw.parseCommit(tip);
+      tipCommit = rw.parseCommit(tip);
+    } finally {
+      batch.close();
+    }
+
+    RevCommit commitWithApprovals = tipCommit;
     assertThat(commitWithApprovals).isNotNull();
     RevCommit commitWithComments = commitWithApprovals.getParent(0);
     assertThat(commitWithComments).isNotNull();
 
+    try (RevWalk rw = new RevWalk(repo)) {
       try (ChangeNotesParser notesWithComments =
           new ChangeNotesParser(c, commitWithComments.copy(), rw, repoManager)) {
         notesWithComments.parseAll();
@@ -556,7 +603,9 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         assertThat(approvals1).isEmpty();
         assertThat(notesWithComments.comments).hasSize(1);
       }
+    }
 
+    try (RevWalk rw = new RevWalk(repo)) {
       try (ChangeNotesParser notesWithApprovals =
           new ChangeNotesParser(c, commitWithApprovals.copy(), rw, repoManager)) {
         notesWithApprovals.parseAll();
@@ -565,8 +614,6 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         assertThat(approvals2).hasSize(1);
         assertThat(notesWithApprovals.comments).hasSize(1);
       }
-    } finally {
-      batch.close();
     }
   }
 
@@ -588,12 +635,12 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
       batch1 = update1.openUpdateInBatch(bru);
       update1.writeCommit(batch1);
       batch1.commit();
-      assertThat(repo.exactRef(update1.getRefName())).isNull();
+      assertThat(repo.exactRef(update1.getRefName())).isNotNull();
 
       batch2 = update2.openUpdateInBatch(bru);
       update2.writeCommit(batch2);
       batch2.commit();
-      assertThat(repo.exactRef(update2.getRefName())).isNull();
+      assertThat(repo.exactRef(update2.getRefName())).isNotNull();
     } finally {
       if (batch1 != null) {
         batch1.close();

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

@@ -134,7 +134,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
     assertThat(author.getName()).isEqualTo("Change Owner");
     assertThat(author.getEmailAddress()).isEqualTo("1@gerrit");
     assertThat(author.getWhen())
-        .isEqualTo(new Date(c.getCreatedOn().getTime() + 1000));
+        .isEqualTo(new Date(c.getCreatedOn().getTime() + 2000));
     assertThat(author.getTimeZone())
         .isEqualTo(TimeZone.getTimeZone("GMT-7:00"));