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>
diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java
index 4154093..55b2574 100644
--- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java
+++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java
@@ -523,6 +523,10 @@
return createdOn;
}
+ public void setCreatedOn(Timestamp ts) {
+ createdOn = ts;
+ }
+
public Timestamp getLastUpdatedOn() {
return lastUpdatedOn;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
index 8c57a256..d49567d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
@@ -354,14 +354,14 @@
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();
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
index b5019e6..925da3e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -266,6 +266,8 @@
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);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
index 81e07b8..bb61103 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
@@ -81,6 +81,8 @@
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 @@
}
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 @@
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 @@
accountId,
new LabelId(l.label())),
l.value(),
- new Timestamp(commit.getCommitterIdent().getWhen().getTime()))));
+ getCommitTime(commit))));
}
}
@@ -516,6 +522,10 @@
}
}
+ 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);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
index d948a15..f2fb5e2 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
@@ -418,7 +418,7 @@
@Override
protected boolean onSave(CommitBuilder commit) {
- if (isEmpty()) {
+ if (getRevision() != null && isEmpty()) {
return false;
}
commit.setAuthor(newIdent(getUser().getAccount(), when));
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
index 07624a4..857f13a5 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
@@ -61,12 +61,15 @@
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 @@
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 {
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
index 231339fd..278f72a 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -78,7 +78,7 @@
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 @@
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 @@
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 @@
@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
@@ -475,6 +484,37 @@
}
@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);
update.setSubject("Create change");
@@ -537,17 +577,24 @@
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);
- assertThat(commitWithApprovals).isNotNull();
- RevCommit commitWithComments = commitWithApprovals.getParent(0);
- assertThat(commitWithComments).isNotNull();
+ 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 @@
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 @@
assertThat(approvals2).hasSize(1);
assertThat(notesWithApprovals.comments).hasSize(1);
}
- } finally {
- batch.close();
}
}
@@ -588,12 +635,12 @@
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();
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
index 811cd8a..4da294b 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
@@ -134,7 +134,7 @@
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"));
