opendev/gerrit
Code Issues Proposed changes

Fix ChangeRebuilderImpl with null account IDs

Browse Source 
The Event comparator had a NPE when used with a null account ID,
which is otherwise supported. This only shows up after comparing a
few more fields, which may be why it hasn't shown up yet. (Or maybe
we didn't have any test cases at all with null IDs.)

Change-Id: I3723b0c9a90eac7ddd9b283d1e332ef92667c63b
This commit is contained in:
Dave Borowitz 2016-04-14 20:08:06 -04:00
parent ccc1ef19b1
commit c7cb3dd131
3 changed files with 38 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -27,11 +27,14 @@ import com.google.gerrit.extensions.api.changes.DraftInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
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.Rebuild;
import com.google.gerrit.server.config.AllUsersName;
@ -154,6 +157,20 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
checker.rebuildAndCheckChanges(id);
}
@Test
public void nullAccountId() throws Exception {
PushOneCommit.Result r = createChange();
PatchSet.Id psId = r.getPatchSetId();
Change.Id id = psId.getParentKey();
// Events need to be otherwise identical for the account ID to be compared.
ChangeMessage msg1 =
insertMessage(psId, user.getId(), TimeUtil.nowTs(), "message 1");
insertMessage(psId, null, msg1.getWrittenOn(), "message 2");
checker.rebuildAndCheckChanges(id);
}
@Test
public void noWriteToNewRef() throws Exception {
PushOneCommit.Result r = createChange();
@ -387,6 +404,24 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
}
}
private ChangeMessage insertMessage(PatchSet.Id psId, Account.Id author,
Timestamp ts, String message) throws Exception {
Change.Id id = psId.getParentKey();
ChangeMessage msg = new ChangeMessage(
new ChangeMessage.Key(id, ChangeUtil.messageUUID(db)),
author, ts, psId);
msg.setMessage(message);
db.changeMessages().insert(Collections.singleton(msg));
Change c = db.changes().get(id);
if (ts.compareTo(c.getLastUpdatedOn()) > 0) {
c.setLastUpdatedOn(ts);
db.changes().update(Collections.singleton(c));
}
return msg;
}
private ReviewDb unwrapDb() {
ReviewDb db = dbProvider.get();
if (db instanceof DisabledChangesReviewDbWrapper) {

2
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java
View File

@ -38,7 +38,7 @@ public class ReviewDbUtil {
};
private static final Ordering<? extends IntKey<?>> INT_KEY_ORDERING =
Ordering.natural().onResultOf(INT_KEY_FUNCTION);
Ordering.natural().nullsFirst().onResultOf(INT_KEY_FUNCTION).nullsFirst();
@SuppressWarnings("unchecked")
public static <K extends IntKey<?>> Ordering<K> intKeyOrdering() {

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

@ -42,6 +42,7 @@ import com.google.gerrit.reviewdb.client.PatchLineComment.Status;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.PatchLineCommentsUtil;
import com.google.gerrit.server.account.AccountCache;
@ -369,7 +370,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
return ComparisonChain.start()
.compareTrueFirst(a.predatesChange, b.predatesChange)
.compare(a.when, b.when)
.compare(a.who.get(), b.who.get())
.compare(a.who, b.who, ReviewDbUtil.intKeyOrdering())
.compare(a.psId.get(), b.psId.get())
.result();
}