opendev/gerrit
Code Issues Proposed changes

ChangeRebuilderImpl: Handle non-monotonic patch set timestamps

Browse Source 
Some data created by old versions of Gerrit has timestamps out of order
in ReviewDb. Naively converting these to NoteDb results in out-of-order
commits in the meta graph. Since at least I099fd703 (possibly earlier),
ChangeNotesParser depends on ordering in the meta graph to set the
currentPatchSetId properly, so these changes were showing up with the
current patch set being the latest patch set by creation time.

The easiest way to fix this is to use the dependency mechanism in
ChangeRebuilderImpl to ensure that each patch set has a dependency on
the previous patch set. However, this breaks the createdOn timestamp,
replacing the bogus old timestamp with a newer one. Handle this case in
ChangeBundle by ignoring createdOn if ReviewDb is out of order.

Change-Id: I1a245ff528cd96b6549c237bb9073b1820af93de
This commit is contained in:
Dave Borowitz
2017-02-06 12:45:21 -05:00
committed by Edwin Kempin
parent 72f61b20fc
commit c6db35e1dc
4 changed files with 203 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

44
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java
View File

@@ -790,7 +790,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
PushOneCommit.FILE_NAME,
"new contents",
r.getChangeId())
.to("refs/heads/master");
.to("refs/for/master");
r.assertOkStatus();
PatchSet.Id psId = r.getPatchSetId();
@@ -1232,6 +1232,48 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
checker.rebuildAndCheckChanges(c.getId());
}
@Test
public void patchSetsOutOfOrder() throws Exception {
String id = createChange().getChangeId();
amendChange(id);
PushOneCommit.Result r = amendChange(id);
ChangeData cd = r.getChange();
PatchSet.Id psId3 = cd.change().currentPatchSetId();
assertThat(psId3.get()).isEqualTo(3);
PatchSet ps1 = db.patchSets().get(new PatchSet.Id(cd.getId(), 1));
PatchSet ps3 = db.patchSets().get(psId3);
assertThat(ps1.getCreatedOn()).isLessThan(ps3.getCreatedOn());
// Simulate an old Gerrit bug by setting the created timestamp of the latest
// patch set ID to the timestamp of PS1.
ps3.setCreatedOn(ps1.getCreatedOn());
db.patchSets().update(Collections.singleton(ps3));
checker.rebuildAndCheckChanges(cd.getId());
setNotesMigration(true, true);
cd = changeDataFactory.create(db, project, cd.getId());
assertThat(cd.change().currentPatchSetId()).isEqualTo(psId3);
List<PatchSet> patchSets = ImmutableList.copyOf(cd.patchSets());
assertThat(patchSets).hasSize(3);
PatchSet newPs1 = patchSets.get(0);
assertThat(newPs1.getId()).isEqualTo(ps1.getId());
assertThat(newPs1.getCreatedOn()).isEqualTo(ps1.getCreatedOn());
PatchSet newPs2 = patchSets.get(1);
assertThat(newPs2.getCreatedOn()).isGreaterThan(newPs1.getCreatedOn());
PatchSet newPs3 = patchSets.get(2);
assertThat(newPs3.getId()).isEqualTo(ps3.getId());
// Migrated with a newer timestamp than the original, to preserve ordering.
assertThat(newPs3.getCreatedOn()).isAtLeast(newPs2.getCreatedOn());
assertThat(newPs3.getCreatedOn()).isGreaterThan(ps1.getCreatedOn());
}
private void assertChangesReadOnly(RestApiException e) throws Exception {
Throwable cause = e.getCause();
assertThat(cause).isInstanceOf(UpdateException.class);

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

@@ -22,6 +22,7 @@ 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;
import static java.util.stream.Collectors.toList;
import com.google.auto.value.AutoValue;
import com.google.common.base.CharMatcher;
@@ -50,6 +51,7 @@ import com.google.gerrit.reviewdb.client.Patch;
import com.google.gerrit.reviewdb.client.PatchLineComment;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.notedb.rebuild.ChangeRebuilderImpl;
@@ -648,7 +650,26 @@ public class ChangeBundle {
List<String> diffs, ChangeBundle bundleA, ChangeBundle bundleB) {
Map<PatchSet.Id, PatchSet> as = bundleA.patchSets;
Map<PatchSet.Id, PatchSet> bs = bundleB.patchSets;
for (PatchSet.Id id : diffKeySets(diffs, as, bs)) {
Set<PatchSet.Id> ids = diffKeySets(diffs, as, bs);
// Old versions of Gerrit had a bug that created patch sets during
// rebase or submission with a createdOn timestamp earlier than the patch
// set it was replacing. (In the cases I examined, it was equal to createdOn
// for the change, but we're not counting on this exact behavior.)
//
// ChangeRebuilder ensures patch set events come out in order, but it's hard
// to predict what the resulting timestamps would look like. So, completely
// ignore the createdOn timestamps if both:
// * ReviewDb timestamps are non-monotonic.
// * NoteDb timestamps are monotonic.
boolean excludeCreatedOn = false;
if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
excludeCreatedOn = !createdOnIsMonotonic(as, ids) && createdOnIsMonotonic(bs, ids);
} else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
excludeCreatedOn = createdOnIsMonotonic(as, ids) && !createdOnIsMonotonic(bs, ids);
}
for (PatchSet.Id id : ids) {
PatchSet a = as.get(id);
PatchSet b = bs.get(id);
String desc = describe(id);
@@ -662,6 +683,9 @@ public class ChangeBundle {
}
List<String> exclude = Lists.newArrayList(pushCertField);
if (excludeCreatedOn) {
exclude.add("createdOn");
}
if (excludeDesc) {
exclude.add("description");
}
@@ -678,6 +702,18 @@ public class ChangeBundle {
return CharMatcher.is('\n').trimTrailingFrom(ps.getPushCertificate());
}
private static boolean createdOnIsMonotonic(
Map<?, PatchSet> patchSets, Set<PatchSet.Id> limitToIds) {
List<PatchSet> orderedById =
patchSets
.values()
.stream()
.filter(ps -> limitToIds.contains(ps.getId()))
.sorted(ChangeUtil.PS_ID_ORDER)
.collect(toList());
return Ordering.natural().onResultOf(PatchSet::getCreatedOn).isOrdered(orderedById);
}
private static void diffPatchSetApprovals(
List<String> diffs, ChangeBundle bundleA, ChangeBundle bundleB) {
Map<PatchSetApproval.Key, PatchSetApproval> as = bundleA.filterPatchSetApprovals();

22
gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java
View File

@@ -27,7 +27,6 @@ import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.MultimapBuilder;
import com.google.common.collect.Ordering;
import com.google.common.collect.Sets;
@@ -77,11 +76,12 @@ import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.TreeMap;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
@@ -304,8 +304,8 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
deleteDraftRefs(change, manager.getAllUsersRepo());
Integer minPsNum = getMinPatchSetNum(bundle);
Map<PatchSet.Id, PatchSetEvent> patchSetEvents =
Maps.newHashMapWithExpectedSize(bundle.getPatchSets().size());
TreeMap<PatchSet.Id, PatchSetEvent> patchSetEvents =
new TreeMap<>(ReviewDbUtil.intKeyOrdering());
for (PatchSet ps : bundle.getPatchSets()) {
PatchSetEvent pse = new PatchSetEvent(change, ps, manager.getChangeRepo().rw);
@@ -320,6 +320,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
draftCommentEvents.put(c.author.getId(), e);
}
}
ensurePatchSetOrder(patchSetEvents);
for (PatchSetApproval psa : bundle.getPatchSetApprovals()) {
PatchSetEvent pse = patchSetEvents.get(psa.getPatchSetId());
@@ -394,6 +395,19 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
return minPsNum;
}
private static void ensurePatchSetOrder(TreeMap<PatchSet.Id, PatchSetEvent> events) {
if (events.isEmpty()) {
return;
}
Iterator<PatchSetEvent> it = events.values().iterator();
PatchSetEvent curr = it.next();
while (it.hasNext()) {
PatchSetEvent next = it.next();
next.addDep(curr);
curr = next;
}
}
private static List<Comment> getComments(
ChangeBundle bundle, String serverId, PatchLineComment.Status status, PatchSet ps) {
return bundle

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

@@ -1290,6 +1290,111 @@ public class ChangeBundleTest extends GerritBaseTests {
b1, b2, "description differs for PatchSet.Id " + ps1.getId() + ":" + " { abc } != {cba}");
}
@Test
public void diffPatchSetsIgnoresCreatedOnWhenReviewDbIsNonMonotonic() throws Exception {
Change c = TestChanges.newChange(project, accountId);
Timestamp beforePs1 = TimeUtil.nowTs();
PatchSet goodPs1 = new PatchSet(new PatchSet.Id(c.getId(), 1));
goodPs1.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
goodPs1.setUploader(accountId);
goodPs1.setCreatedOn(TimeUtil.nowTs());
PatchSet goodPs2 = new PatchSet(new PatchSet.Id(c.getId(), 2));
goodPs2.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
goodPs2.setUploader(accountId);
goodPs2.setCreatedOn(TimeUtil.nowTs());
assertThat(goodPs2.getCreatedOn()).isGreaterThan(goodPs1.getCreatedOn());
PatchSet badPs2 = clone(goodPs2);
badPs2.setCreatedOn(beforePs1);
assertThat(badPs2.getCreatedOn()).isLessThan(goodPs1.getCreatedOn());
// Both ReviewDb, exact match required.
ChangeBundle b1 =
new ChangeBundle(
c,
messages(),
patchSets(goodPs1, goodPs2),
approvals(),
comments(),
reviewers(),
REVIEW_DB);
ChangeBundle b2 =
new ChangeBundle(
c,
messages(),
patchSets(goodPs1, badPs2),
approvals(),
comments(),
reviewers(),
REVIEW_DB);
assertDiffs(
b1,
b2,
"createdOn differs for PatchSet.Id "
+ badPs2.getId()
+ ":"
+ " {2009-09-30 17:00:18.0} != {2009-09-30 17:00:06.0}");
// Non-monotonic in ReviewDb but monotonic in NoteDb, timestamps are
// ignored, including for ps1.
PatchSet badPs1 = clone(goodPs1);
badPs1.setCreatedOn(TimeUtil.nowTs());
b1 =
new ChangeBundle(
c,
messages(),
patchSets(badPs1, badPs2),
approvals(),
comments(),
reviewers(),
REVIEW_DB);
b2 =
new ChangeBundle(
c,
messages(),
patchSets(goodPs1, goodPs2),
approvals(),
comments(),
reviewers(),
NOTE_DB);
assertNoDiffs(b1, b2);
assertNoDiffs(b2, b1);
// Non-monotonic in NoteDb but monotonic in ReviewDb, timestamps are not
// ignored.
b1 =
new ChangeBundle(
c,
messages(),
patchSets(goodPs1, goodPs2),
approvals(),
comments(),
reviewers(),
REVIEW_DB);
b2 =
new ChangeBundle(
c,
messages(),
patchSets(badPs1, badPs2),
approvals(),
comments(),
reviewers(),
NOTE_DB);
assertDiffs(
b1,
b2,
"createdOn differs for PatchSet.Id "
+ badPs1.getId()
+ " in NoteDb vs. ReviewDb:"
+ " {2009-09-30 17:00:24.0} != {2009-09-30 17:00:12.0}",
"createdOn differs for PatchSet.Id "
+ badPs2.getId()
+ " in NoteDb vs. ReviewDb:"
+ " {2009-09-30 17:00:06.0} != {2009-09-30 17:00:18.0}");
}
@Test
public void diffPatchSetApprovalKeySets() throws Exception {
Change c = TestChanges.newChange(project, accountId);