opendev/gerrit
Code Issues Proposed changes

ChangeRebuilderImpl: Ensure first event creates change

Browse Source 
There were some corner cases where the first event according to
EVENT_ORDER might not be a PatchSetEvent, or might not even be by the
change owner at all. This breaks ChangeNotesParser, which blindly
takes as the change owner the author of the first commit in the notes
graph.

One such corner case is where a historic comment is added prior to
change creation, which gets truncated to have the same timestamp as
change creation, and inserted first in the list. In this case it
should simply come after the corresponding PatchSetEvent.

Another corner case is more subtle: races can cause a historic comment
to get inserted between the createdOn timestamp of the change and the
createdOn timestamp of the first patch set (or first remaining patch
set). This would not cause the timestamp to get truncated, but would
predate any PatchSetEvent, so we can't depend on the tiebreaker used
in the previous corner case. As a fallback, double-check that the very
first event in the list, after sorting, includes the author and other
required fields. It ends up looking a little odd that this commit has
a patch set ID whose revision has not been parsed yet, but
ChangeNotesParser can handle this.

Change-Id: If8ef07268126c19b842cb4b226dc284c6fd4f2b4
This commit is contained in:
Dave Borowitz 2016-04-28 16:34:17 -04:00
parent 2c504d31c3
commit 1d96c9214a
2 changed files with 122 additions and 22 deletions
Show Stats 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

@ -36,7 +36,9 @@ import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.PatchLineCommentsUtil;
import com.google.gerrit.server.change.PostReview;
import com.google.gerrit.server.change.Rebuild;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.notedb.ChangeBundle;
import com.google.gerrit.server.notedb.ChangeNoteUtil;
@ -79,6 +81,9 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
@Inject
private PatchLineCommentsUtil plcUtil;
@Inject
private Provider<PostReview> postReview;
@Before
public void setUp() {
assume().that(NoteDbMode.readWrite()).isFalse();
@ -415,6 +420,45 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
assertThat(notes.getChange().getTopic()).isNull();
}
@Test
public void commentBeforeFirstPatchSet() throws Exception {
PushOneCommit.Result r = createChange();
PatchSet.Id psId = r.getPatchSetId();
Change.Id id = psId.getParentKey();
Change c = db.changes().get(id);
c.setCreatedOn(new Timestamp(c.getCreatedOn().getTime() - 5000));
db.changes().update(Collections.singleton(c));
indexer.index(db, project, id);
ReviewInput rin = new ReviewInput();
rin.message = "comment";
Timestamp ts = new Timestamp(c.getCreatedOn().getTime() + 2000);
RevisionResource revRsrc = parseCurrentRevisionResource(r.getChangeId());
postReview.get().apply(revRsrc, rin, ts);
checker.rebuildAndCheckChanges(id);
}
@Test
public void commentPredatingChangeBySomeoneOtherThanOwner() throws Exception {
PushOneCommit.Result r = createChange();
PatchSet.Id psId = r.getPatchSetId();
Change.Id id = psId.getParentKey();
Change c = db.changes().get(id);
ReviewInput rin = new ReviewInput();
rin.message = "comment";
Timestamp ts = new Timestamp(c.getCreatedOn().getTime() - 10000);
RevisionResource revRsrc = parseCurrentRevisionResource(r.getChangeId());
setApiUser(user);
postReview.get().apply(revRsrc, rin, ts);
checker.rebuildAndCheckChanges(id);
}
private void setInvalidNoteDbState(Change.Id id) throws Exception {
ReviewDb db = unwrapDb();
Change c = db.changes().get(id);

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

@ -240,16 +240,11 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
// Delete ref only after hashtags have been read
deleteRef(change, changeMetaRepo, manager.getChangeRepo().cmds);
Integer minPsNum = null;
for (PatchSet ps : bundle.getPatchSets()) {
int n = ps.getId().get();
if (minPsNum == null || n < minPsNum) {
minPsNum = n;
}
}
Integer minPsNum = getMinPatchSetNum(bundle);
for (PatchSet ps : bundle.getPatchSets()) {
events.add(new PatchSetEvent(
change, ps, checkNotNull(minPsNum), manager.getChangeRepo().rw));
change, ps, manager.getChangeRepo().rw));
for (PatchLineComment c : getPatchLineComments(bundle, ps)) {
PatchLineCommentEvent e =
new PatchLineCommentEvent(c, change, ps, patchListCache);
@ -271,9 +266,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
new ChangeMessageEvent(msg, noteDbChange, change.getCreatedOn()));
}
sortEvents(change.getId(), events, minPsNum);
events.add(new FinalUpdatesEvent(change, noteDbChange));
sortAndFillEvents(change, noteDbChange, events, minPsNum);
EventList<Event> el = new EventList<>();
for (Event e : events) {
@ -299,6 +292,17 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
}
}
private static Integer getMinPatchSetNum(ChangeBundle bundle) {
Integer minPsNum = null;
for (PatchSet ps : bundle.getPatchSets()) {
int n = ps.getId().get();
if (minPsNum == null || n < minPsNum) {
minPsNum = n;
}
}
return minPsNum;
}
private static List<PatchLineComment> getPatchLineComments(ChangeBundle bundle,
final PatchSet ps) {
return FluentIterable.from(bundle.getPatchLineComments())
@ -310,9 +314,19 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
}).toSortedList(PatchLineCommentsUtil.PLC_ORDER);
}
private void sortEvents(Change.Id changeId, List<Event> events,
Integer minPsNum) {
private void sortAndFillEvents(Change change, Change noteDbChange,
List<Event> events, Integer minPsNum) {
Collections.sort(events, EVENT_ORDER);
events.add(new FinalUpdatesEvent(change, noteDbChange));
// Ensure the first event in the list creates the change, setting the author
// and any required footers.
Event first = events.get(0);
if (first instanceof PatchSetEvent && change.getOwner().equals(first.who)) {
((PatchSetEvent) first).createChange = true;
} else {
events.add(0, new CreateChangeEvent(change, minPsNum));
}
// Fill in any missing patch set IDs using the latest patch set of the
// change at the time of the event, because NoteDb can't represent actions
@ -327,7 +341,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
int ps = firstNonNull(minPsNum, 1);
for (Event e : events) {
if (e.psId == null) {
e.psId = new PatchSet.Id(changeId, ps);
e.psId = new PatchSet.Id(change.getId(), ps);
} else {
ps = Math.max(ps, e.psId.get());
}
@ -457,12 +471,17 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
@Override
public int compare(Event a, Event b) {
return ComparisonChain.start()
.compareTrueFirst(a.predatesChange, b.predatesChange)
.compare(a.when, b.when)
.compareTrueFirst(isPatchSet(a), isPatchSet(b))
.compareTrueFirst(a.predatesChange, b.predatesChange)
.compare(a.who, b.who, ReviewDbUtil.intKeyOrdering())
.compare(a.psId, b.psId, ReviewDbUtil.intKeyOrdering().nullsLast())
.result();
}
private boolean isPatchSet(Event e) {
return e instanceof PatchSetEvent;
}
};
private abstract static class Event {
@ -606,6 +625,46 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
}
}
private static void createChange(ChangeUpdate update, Change change) {
update.setSubjectForCommit("Create change");
update.setChangeId(change.getKey().get());
update.setBranch(change.getDest().get());
update.setSubject(change.getOriginalSubject());
}
private static class CreateChangeEvent extends Event {
private final Change change;
private static PatchSet.Id psId(Change change, Integer minPsNum) {
int n;
if (minPsNum == null) {
// There were no patch sets for the change at all, so something is very
// wrong. Bail and use 1 as the patch set.
n = 1;
} else {
n = minPsNum;
}
return new PatchSet.Id(change.getId(), n);
}
CreateChangeEvent(Change change, Integer minPsNum) {
super(psId(change, minPsNum), change.getOwner(), change.getCreatedOn(),
change.getCreatedOn(), null);
this.change = change;
}
@Override
boolean uniquePerUpdate() {
return true;
}
@Override
void apply(ChangeUpdate update) throws IOException, OrmException {
checkUpdate(update);
createChange(update, change);
}
}
private static class ApprovalEvent extends Event {
private PatchSetApproval psa;
@ -630,16 +689,15 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
private static class PatchSetEvent extends Event {
private final Change change;
private final PatchSet ps;
private final boolean createChange;
private final RevWalk rw;
private boolean createChange;
PatchSetEvent(Change change, PatchSet ps, int minPsNum, RevWalk rw) {
PatchSetEvent(Change change, PatchSet ps, RevWalk rw) {
super(ps.getId(), ps.getUploader(), ps.getCreatedOn(),
change.getCreatedOn(), null);
this.change = change;
this.ps = ps;
this.rw = rw;
this.createChange = ps.getPatchSetId() == minPsNum;
}
@Override
@ -650,12 +708,10 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
@Override
void apply(ChangeUpdate update) throws IOException, OrmException {
checkUpdate(update);
update.setSubject(change.getSubject());
if (createChange) {
update.setSubjectForCommit("Create change");
update.setChangeId(change.getKey().get());
update.setBranch(change.getDest().get());
createChange(update, change);
} else {
update.setSubject(change.getSubject());
update.setSubjectForCommit("Create patch set " + ps.getPatchSetId());
}
setRevision(update, ps);