opendev
/
gerrit
Code Issues Proposed changes

Skip migrating inline comments on missing patch set parents 

In NoteDb, inline comments are stored on notes referring to the commit
containing that file. For a comment on the PARENT side of a patch set,
the migration process needs to look up the parent SHA-1; this lookup
will fail if the patch set SHA-1 is missing. The changes can be
otherwise migrated, although of course they may fail to render or
present other problems down the line.

This fix is slightly invasive because it changes the signature of
CommentsUtil#setCommentRevId to throw PatchListNotAvailableException,
allowing the downstream migration code to distinguish this case.

Bug: Issue 8552
Change-Id: I9bcd4f533f02fa20617d5c0b2b3264a8899e9f2c
This commit is contained in:
Dave Borowitz 2018-03-16 07:50:28 -04:00
parent 43a2a6e489
commit 7f2991dc13
11 changed files with 179 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -24,6 +24,7 @@ import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.concurrent.TimeUnit.DAYS;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;
import static java.util.stream.Collectors.joining;
import static java.util.stream.Collectors.toList;
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
import static org.junit.Assert.fail;
@ -54,6 +55,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.ChangeUtil;
@ -73,6 +75,8 @@ import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.notedb.NoteDbUpdateManager;
import com.google.gerrit.server.notedb.TestChangeRebuilderWrapper;
import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder.NoPatchSetsException;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
import com.google.gerrit.server.project.Util;
@ -97,6 +101,7 @@ import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.TimeUnit;
import java.util.stream.Stream;
import org.apache.http.Header;
import org.apache.http.message.BasicHeader;
import org.eclipse.jgit.junit.TestRepository;
@ -149,6 +154,8 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
@Inject private PatchSetInfoFactory patchSetInfoFactory;
@Inject private PatchListCache patchListCache;
@Before
public void setUp() throws Exception {
assume().that(NoteDbMode.get()).isEqualTo(NoteDbMode.OFF);
@ -1378,6 +1385,67 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
assertChangeUpToDate(true, id);
}
@Test
public void missingPatchSetCommitOkForCommentsNotOnParentSide() throws Exception {
PushOneCommit.Result r = createChange();
Change.Id id = r.getChange().getId();
putDraft(user, id, 1, "draft comment", null, Side.REVISION);
putComment(user, id, 1, "published comment", null, Side.REVISION);
ReviewDb db = getUnwrappedDb();
PatchSet ps = db.patchSets().get(new PatchSet.Id(id, 1));
ps.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
db.patchSets().update(Collections.singleton(ps));
try {
patchListCache.getOldId(db.changes().get(id), ps, null);
assert_().fail("Expected PatchListNotAvailableException");
} catch (PatchListNotAvailableException e) {
// Expected.
}
checker.rebuildAndCheckChanges(id);
}
@Test
public void missingPatchSetCommitOmitsCommentsOnParentSide() throws Exception {
PushOneCommit.Result r = createChange();
Change.Id id = r.getChange().getId();
CommentInfo draftInfo = putDraft(user, id, 1, "draft comment", null, Side.PARENT);
putComment(user, id, 1, "published comment", null, Side.PARENT);
CommentInfo commentInfo =
gApi.changes()
.id(id.get())
.comments()
.values()
.stream()
.flatMap(List::stream)
.findFirst()
.get();
ReviewDb db = getUnwrappedDb();
PatchSet ps = db.patchSets().get(new PatchSet.Id(id, 1));
ps.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
db.patchSets().update(Collections.singleton(ps));
try {
patchListCache.getOldId(db.changes().get(id), ps, null);
assert_().fail("Expected PatchListNotAvailableException");
} catch (PatchListNotAvailableException e) {
// Expected.
}
checker.rebuildAndCheckChange(
id,
Stream.of(draftInfo.id, commentInfo.id)
.sorted()
.map(c -> id + ",1," + PushOneCommit.FILE_NAME + "," + c)
.collect(
joining(", ", "PatchLineComment.Key sets differ: [", "] only in A; [] only in B")));
}
private void assertChangesReadOnly(RestApiException e) throws Exception {
Throwable cause = e.getCause();
assertThat(cause).isInstanceOf(UpdateException.class);
@ -1427,16 +1495,24 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
}
}
private void putDraft(TestAccount account, Change.Id id, int line, String msg, Boolean unresolved)
private CommentInfo putDraft(
TestAccount account, Change.Id id, int line, String msg, Boolean unresolved)
throws Exception {
return putDraft(account, id, line, msg, unresolved, Side.REVISION);
}
private CommentInfo putDraft(
TestAccount account, Change.Id id, int line, String msg, Boolean unresolved, Side side)
throws Exception {
DraftInput in = new DraftInput();
in.side = side;
in.line = line;
in.message = msg;
in.path = PushOneCommit.FILE_NAME;
in.unresolved = unresolved;
AcceptanceTestRequestScope.Context old = setApiUser(account);
try {
gApi.changes().id(id.get()).current().createDraft(in);
return gApi.changes().id(id.get()).current().createDraft(in).get();
} finally {
atrScope.set(old);
}
@ -1444,7 +1520,14 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
private void putComment(TestAccount account, Change.Id id, int line, String msg, String inReplyTo)
throws Exception {
putComment(account, id, line, msg, inReplyTo, Side.REVISION);
}
private void putComment(
TestAccount account, Change.Id id, int line, String msg, String inReplyTo, Side side)
throws Exception {
CommentInput in = new CommentInput();
in.side = side;
in.line = line;
in.message = msg;
in.inReplyTo = inReplyTo;

24
gerrit-server/src/main/java/com/google/gerrit/server/CommentsUtil.java
View File

@ -491,25 +491,21 @@ public class CommentsUtil {
}
public static void setCommentRevId(Comment c, PatchListCache cache, Change change, PatchSet ps)
throws OrmException {
throws PatchListNotAvailableException {
checkArgument(
c.key.patchSetId == ps.getId().get(),
"cannot set RevId for patch set %s on comment %s",
ps.getId(),
c);
if (c.revId == null) {
try {
if (Side.fromShort(c.side) == Side.PARENT) {
if (c.side < 0) {
c.revId = ObjectId.toString(cache.getOldId(change, ps, -c.side));
} else {
c.revId = ObjectId.toString(cache.getOldId(change, ps, null));
}
if (Side.fromShort(c.side) == Side.PARENT) {
if (c.side < 0) {
c.revId = ObjectId.toString(cache.getOldId(change, ps, -c.side));
} else {
c.revId = ps.getRevision().get();
c.revId = ObjectId.toString(cache.getOldId(change, ps, null));
}
} catch (PatchListNotAvailableException e) {
throw new OrmException(e);
} else {
c.revId = ps.getRevision().get();
}
}
}
@ -576,7 +572,11 @@ public class CommentsUtil {
// Draft may have been created by a different real user; copy the current real user. (Only
// applies to X-Gerrit-RunAs, since modifying drafts via on_behalf_of is not allowed.)
ctx.getUser().updateRealAccountId(d::setRealAuthor);
setCommentRevId(d, patchListCache, notes.getChange(), ps);
try {
setCommentRevId(d, patchListCache, notes.getChange(), ps);
} catch (PatchListNotAvailableException e) {
throw new OrmException(e);
}
}
putComments(ctx.getDb(), ctx.getUpdate(psId), PUBLISHED, drafts);
}

4
gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java
View File

@ -33,6 +33,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@ -107,7 +108,8 @@ public class CreateDraftComment
@Override
public boolean updateChange(ChangeContext ctx)
throws ResourceNotFoundException, OrmException, UnprocessableEntityException {
throws ResourceNotFoundException, OrmException, UnprocessableEntityException,
PatchListNotAvailableException {
PatchSet ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
if (ps == null) {
throw new ResourceNotFoundException("patch set not found: " + psId);

4
gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftComment.java
View File

@ -28,6 +28,7 @@ import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.DeleteDraftComment.Input;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@ -87,7 +88,8 @@ public class DeleteDraftComment
}
@Override
public boolean updateChange(ChangeContext ctx) throws ResourceNotFoundException, OrmException {
public boolean updateChange(ChangeContext ctx)
throws ResourceNotFoundException, OrmException, PatchListNotAvailableException {
Optional<Comment> maybeComment =
commentsUtil.getDraft(ctx.getDb(), ctx.getNotes(), ctx.getIdentifiedUser(), key);
if (!maybeComment.isPresent()) {

14
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
View File

@ -838,7 +838,8 @@ public class PostReview
@Override
public boolean updateChange(ChangeContext ctx)
throws OrmException, ResourceConflictException, UnprocessableEntityException, IOException {
throws OrmException, ResourceConflictException, UnprocessableEntityException, IOException,
PatchListNotAvailableException {
user = ctx.getIdentifiedUser();
notes = ctx.getNotes();
ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
@ -880,7 +881,7 @@ public class PostReview
}
private boolean insertComments(ChangeContext ctx)
throws OrmException, UnprocessableEntityException {
throws OrmException, UnprocessableEntityException, PatchListNotAvailableException {
Map<String, List<CommentInput>> map = in.comments;
if (map == null) {
map = Collections.emptyMap();
@ -945,7 +946,8 @@ public class PostReview
return !toDel.isEmpty() || !toPublish.isEmpty();
}
private boolean insertRobotComments(ChangeContext ctx) throws OrmException {
private boolean insertRobotComments(ChangeContext ctx)
throws OrmException, PatchListNotAvailableException {
if (in.robotComments == null) {
return false;
}
@ -956,7 +958,8 @@ public class PostReview
return !newRobotComments.isEmpty();
}
private List<RobotComment> getNewRobotComments(ChangeContext ctx) throws OrmException {
private List<RobotComment> getNewRobotComments(ChangeContext ctx)
throws OrmException, PatchListNotAvailableException {
List<RobotComment> toAdd = new ArrayList<>(in.robotComments.size());
Set<CommentSetEntry> existingIds =
@ -976,7 +979,8 @@ public class PostReview
}
private RobotComment createRobotCommentFromInput(
ChangeContext ctx, String path, RobotCommentInput robotCommentInput) throws OrmException {
ChangeContext ctx, String path, RobotCommentInput robotCommentInput)
throws PatchListNotAvailableException {
RobotComment robotComment =
commentsUtil.newRobotComment(
ctx,

4
gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java
View File

@ -32,6 +32,7 @@ import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@ -112,7 +113,8 @@ public class PutDraftComment
}
@Override
public boolean updateChange(ChangeContext ctx) throws ResourceNotFoundException, OrmException {
public boolean updateChange(ChangeContext ctx)
throws ResourceNotFoundException, OrmException, PatchListNotAvailableException {
Optional<Comment> maybeComment =
commentsUtil.getDraft(ctx.getDb(), ctx.getNotes(), ctx.getIdentifiedUser(), key);
if (!maybeComment.isPresent()) {

5
gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailProcessor.java
View File

@ -44,6 +44,7 @@ import com.google.gerrit.server.extensions.events.CommentAdded;
import com.google.gerrit.server.mail.MailFilter;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.update.BatchUpdate;
@ -241,7 +242,7 @@ public class MailProcessor {
@Override
public boolean updateChange(ChangeContext ctx)
throws OrmException, UnprocessableEntityException {
throws OrmException, UnprocessableEntityException, PatchListNotAvailableException {
patchSet = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
notes = ctx.getNotes();
if (patchSet == null) {
@ -338,7 +339,7 @@ public class MailProcessor {
private Comment persistentCommentFromMailComment(
ChangeContext ctx, MailComment mailComment, PatchSet patchSetForComment)
throws OrmException, UnprocessableEntityException {
throws OrmException, UnprocessableEntityException, PatchListNotAvailableException {
String fileName;
// The patch set that this comment is based on is different if this
// comment was sent in reply to a comment on a previous patch set.

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

@ -530,8 +530,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
}
private void flushEventsToDraftUpdate(
NoteDbUpdateManager manager, EventList<DraftCommentEvent> events, Change change)
throws OrmException {
NoteDbUpdateManager manager, EventList<DraftCommentEvent> events, Change change) {
if (events.isEmpty()) {
return;
}

19
gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/CommentEvent.java
View File

@ -24,9 +24,13 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gwtorm.server.OrmException;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
class CommentEvent extends Event {
private static final Logger log = LoggerFactory.getLogger(CommentEvent.class);
public final Comment c;
private final Change change;
private final PatchSet ps;
@ -57,10 +61,19 @@ class CommentEvent extends Event {
}
@Override
void apply(ChangeUpdate update) throws OrmException {
void apply(ChangeUpdate update) {
checkUpdate(update);
if (c.revId == null) {
setCommentRevId(c, cache, change, ps);
try {
setCommentRevId(c, cache, change, ps);
} catch (PatchListNotAvailableException e) {
log.warn(
"Unable to determine parent commit of patch set {} ({}); omitting inline comment",
ps.getId(),
ps.getRevision(),
c);
return;
}
}
update.putComment(PatchLineComment.Status.PUBLISHED, c);
}

19
gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/DraftCommentEvent.java
View File

@ -24,9 +24,13 @@ import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.notedb.ChangeDraftUpdate;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gwtorm.server.OrmException;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
class DraftCommentEvent extends Event {
private static final Logger log = LoggerFactory.getLogger(DraftCommentEvent.class);
public final Comment c;
private final Change change;
private final PatchSet ps;
@ -56,9 +60,18 @@ class DraftCommentEvent extends Event {
throw new UnsupportedOperationException();
}
void applyDraft(ChangeDraftUpdate draftUpdate) throws OrmException {
void applyDraft(ChangeDraftUpdate draftUpdate) {
if (c.revId == null) {
setCommentRevId(c, cache, change, ps);
try {
setCommentRevId(c, cache, change, ps);
} catch (PatchListNotAvailableException e) {
log.warn(
"Unable to determine parent commit of patch set {} ({}); omitting draft inline comment",
ps.getId(),
ps.getRevision(),
c);
return;
}
}
draftUpdate.putComment(c);
}

35
gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbChecker.java
View File

@ -19,6 +19,8 @@ import static java.util.Comparator.comparing;
import static java.util.stream.Collectors.toList;
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ListMultimap;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
@ -78,14 +80,17 @@ public class NoteDbChecker {
}
public void rebuildAndCheckAllChanges() throws Exception {
rebuildAndCheckChanges(getUnwrappedDb().changes().all().toList().stream().map(Change::getId));
rebuildAndCheckChanges(
getUnwrappedDb().changes().all().toList().stream().map(Change::getId),
ImmutableListMultimap.of());
}
public void rebuildAndCheckChanges(Change.Id... changeIds) throws Exception {
rebuildAndCheckChanges(Arrays.stream(changeIds));
rebuildAndCheckChanges(Arrays.stream(changeIds), ImmutableListMultimap.of());
}
private void rebuildAndCheckChanges(Stream<Change.Id> changeIds) throws Exception {
private void rebuildAndCheckChanges(
Stream<Change.Id> changeIds, ListMultimap<Change.Id, String> expectedDiffs) throws Exception {
ReviewDb db = getUnwrappedDb();
List<ChangeBundle> allExpected = readExpected(changeIds);
@ -105,7 +110,7 @@ public class NoteDbChecker {
}
}
checkActual(allExpected, msgs);
checkActual(allExpected, expectedDiffs, msgs);
} finally {
notesMigration.setReadChanges(oldRead);
notesMigration.setWriteChanges(oldWrite);
@ -113,7 +118,14 @@ public class NoteDbChecker {
}
public void checkChanges(Change.Id... changeIds) throws Exception {
checkActual(readExpected(Arrays.stream(changeIds)), new ArrayList<>());
checkActual(
readExpected(Arrays.stream(changeIds)), ImmutableListMultimap.of(), new ArrayList<>());
}
public void rebuildAndCheckChange(Change.Id changeId, String... expectedDiff) throws Exception {
ImmutableListMultimap.Builder<Change.Id, String> b = ImmutableListMultimap.builder();
b.putAll(changeId, Arrays.asList(expectedDiff));
rebuildAndCheckChanges(Stream.of(changeId), b.build());
}
public void assertNoChangeRef(Project.NameKey project, Change.Id changeId) throws Exception {
@ -160,7 +172,11 @@ public class NoteDbChecker {
}
}
private void checkActual(List<ChangeBundle> allExpected, List<String> msgs) throws Exception {
private void checkActual(
List<ChangeBundle> allExpected,
ListMultimap<Change.Id, String> expectedDiffs,
List<String> msgs)
throws Exception {
ReviewDb db = getUnwrappedDb();
boolean oldRead = notesMigration.readChanges();
boolean oldWrite = notesMigration.rawWriteChangesSetting();
@ -181,9 +197,14 @@ public class NoteDbChecker {
continue;
}
List<String> diff = expected.differencesFrom(actual);
if (!diff.isEmpty()) {
List<String> expectedDiff = expectedDiffs.get(c.getId());
if (!diff.equals(expectedDiff)) {
msgs.add("Differences between ReviewDb and NoteDb for " + c + ":");
msgs.addAll(diff);
if (!expectedDiff.isEmpty()) {
msgs.add("Expected differences:");
msgs.addAll(expectedDiff);
}
msgs.add("");
} else {
System.err.println("NoteDb conversion of change " + c.getId() + " successful");