Ignore comments on non-existing patchsets when porting
This change is similar to I7a27b09ba. Instead of running into an
internal server error, we simply drop such comments. As this is a corner
case and we didn't want to introduce additional complexity, we opted
for ignoring instead of using the fallback location for such comments.
This can be improved in the future if this case turns out to be more
frequent than anticipated.
Change-Id: I5682bf9b60d5aab33aefd4f77dd4307bb0acb382
diff --git a/java/com/google/gerrit/server/restapi/change/CommentPorter.java b/java/com/google/gerrit/server/restapi/change/CommentPorter.java
index 05241e2..d663af9 100644
--- a/java/com/google/gerrit/server/restapi/change/CommentPorter.java
+++ b/java/com/google/gerrit/server/restapi/change/CommentPorter.java
@@ -131,13 +131,21 @@
ImmutableList<HumanComment> patchsetComments = commentsPerPatchset.get(originalPatchsetId);
PatchSet originalPatchset =
notes.getPatchSets().get(PatchSet.id(notes.getChangeId(), originalPatchsetId));
- portedComments.addAll(
- portSamePatchset(
- notes.getProjectName(),
- notes.getChange(),
- originalPatchset,
- targetPatchset,
- patchsetComments));
+ if (originalPatchset != null) {
+ portedComments.addAll(
+ portSamePatchset(
+ notes.getProjectName(),
+ notes.getChange(),
+ originalPatchset,
+ targetPatchset,
+ patchsetComments));
+ } else {
+ logger.atWarning().log(
+ String.format(
+ "Some comments which should be ported refer to the non-existent patchset %s of"
+ + " change %d. Omitting %d affected comments.",
+ originalPatchsetId, notes.getChangeId().get(), patchsetComments.size()));
+ }
}
return portedComments.build();
}
diff --git a/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java b/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
index 9c30fc9..b6ec50c 100644
--- a/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
+++ b/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
@@ -154,13 +154,7 @@
HumanComment comment1 = createComment(patchset1.id(), "myFile");
HumanComment comment2 = createComment(patchset2.id(), "myFile");
when(patchListCache.getOldId(any(), any(), any())).thenReturn(dummyObjectId);
- PatchList emptyDiff =
- new PatchList(
- dummyObjectId,
- dummyObjectId,
- false,
- ComparisonType.againstOtherPatchSet(),
- new PatchListEntry[0]);
+ PatchList emptyDiff = getEmptyDiff();
// Throw an exception on the first diff request but return an actual value on the second.
when(patchListCache.get(any(PatchListKey.class), any(Project.NameKey.class)))
.thenThrow(IllegalStateException.class)
@@ -173,6 +167,28 @@
assertThat(portedComments).comparingElementsUsing(hasFilePath()).contains("myFile");
}
+ @Test
+ public void commentsWithInvalidPatchsetsAreIgnored() throws Exception {
+ Project.NameKey project = Project.nameKey("myProject");
+ Change.Id changeId = Change.id(1);
+ Change change = createChange(project, changeId);
+ PatchSet patchset1 = createPatchset(PatchSet.id(changeId, 1));
+ PatchSet patchset2 = createPatchset(PatchSet.id(changeId, 2));
+ // Leave out patchset 1 (e.g. reserved for draft patchsets in the past).
+ ChangeNotes changeNotes = mockChangeNotes(project, change, patchset2);
+
+ CommentPorter commentPorter = new CommentPorter(patchListCache);
+ HumanComment comment = createComment(patchset1.id(), "myFile");
+ when(patchListCache.getOldId(any(), any(), any())).thenReturn(dummyObjectId);
+ PatchList emptyDiff = getEmptyDiff();
+ when(patchListCache.get(any(PatchListKey.class), any(Project.NameKey.class)))
+ .thenReturn(emptyDiff);
+ ImmutableList<HumanComment> portedComments =
+ commentPorter.portComments(changeNotes, patchset2, ImmutableList.of(comment));
+
+ assertThat(portedComments).isEmpty();
+ }
+
private Change createChange(Project.NameKey project, Change.Id changeId) {
return new Change(
Change.key("changeKey"),
@@ -224,4 +240,13 @@
private Correspondence<HumanComment, String> hasFilePath() {
return NullAwareCorrespondence.transforming(comment -> comment.key.filename, "hasFilePath");
}
+
+ private PatchList getEmptyDiff() {
+ return new PatchList(
+ dummyObjectId,
+ dummyObjectId,
+ false,
+ ComparisonType.againstOtherPatchSet(),
+ new PatchListEntry[0]);
+ }
}
