diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index 6314777f11..0908d9474a 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/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 diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java index e3a85727c1..9160a45ebf 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java @@ -279,7 +279,6 @@ public class PostReviewers implements RestModifyView 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 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 exclude = Lists.newArrayList("rowVersion", "noteDbState"); + String updatedField = "lastUpdatedOn"; + List 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 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 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 tempDiffs = new ArrayList<>(1); + diffTimestamps(tempDiffs, "temp", bundleA, ta, bundleB, tb, "temp"); + return !tempDiffs.isEmpty(); + } + private static void diffTimestamps(List diffs, String desc, Timestamp tsFromNoteDb, Timestamp tsFromReviewDb, String field) { // Because ChangeRebuilder may batch events together that are several 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 604c866caf..4bf45081fb 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 @@ -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 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)) { - lastUpdatedOn = ts; - } + if (lastUpdatedOn == null || ts.after(lastUpdatedOn)) { + 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() diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java index 8afc0b7410..977ac325f2 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java +++ b/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() - + " in NoteDb vs. ReviewDb:" + String msg = "effective last updated time differs for Change.Id " + + 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); 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 a796f9c91b..c73d972eab 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 @@ -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);