Return empty context if the comment range is outside file boundaries
This is replacing the current behaviour of throwing an exception. See
attached bug where a file in the UI contained an extra line at the end
of file. This is due to our custom handling of newlines in the diff
endpoint. This change mitigates this issue until I come up with a better
solution.
We don't need to increase the cache version since no entries should be
invalidated. Previously failing cache loads will now succeed and should
be cached with this fix.
Bug: Issue 14102
Change-Id: I076437df2aae1495ab2c6f9d44fbd36c60147b77
diff --git a/java/com/google/gerrit/server/comment/CommentContextLoader.java b/java/com/google/gerrit/server/comment/CommentContextLoader.java
index 63bb8d0..c93f4b1 100644
--- a/java/com/google/gerrit/server/comment/CommentContextLoader.java
+++ b/java/com/google/gerrit/server/comment/CommentContextLoader.java
@@ -26,7 +26,6 @@
import com.google.gerrit.entities.Comment;
import com.google.gerrit.entities.CommentContext;
import com.google.gerrit.entities.Project;
-import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.common.ContextLineInfo;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.ComparisonType;
@@ -158,12 +157,10 @@
private static CommentContext createContext(Text src, Range commentRange, int contextPadding) {
if (commentRange.start() < 1 || commentRange.end() - 1 > src.size()) {
- throw new StorageException(
- "Invalid comment range "
- + commentRange
- + ". Text only contains "
- + src.size()
- + " lines.");
+ // TODO(ghareeb): We should throw an exception in this case. See
+ // https://bugs.chromium.org/p/gerrit/issues/detail?id=14102 which is an example where the
+ // diff contains an extra line not in the original file.
+ return CommentContext.empty();
}
commentRange = adjustRange(commentRange, contextPadding, src.size());
ImmutableMap.Builder<Integer, String> context =
diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentContextIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentContextIT.java
index adfe56d..548e3fe 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/CommentContextIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/CommentContextIT.java
@@ -19,11 +19,9 @@
import static com.google.gerrit.entities.Patch.COMMIT_MSG;
import static com.google.gerrit.entities.Patch.MERGE_LIST;
import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
-import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.MoreCollectors;
-import com.google.common.util.concurrent.UncheckedExecutionException;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
@@ -148,12 +146,10 @@
CommentsUtil.newComment(COMMIT_MSG, Side.REVISION, 100, "comment", false);
CommentsUtil.addComments(gApi, changeId, ps1, comment);
- Throwable thrown =
- assertThrows(
- UncheckedExecutionException.class,
- () -> gApi.changes().id(changeId).commentsRequest().withContext(true).getAsList());
-
- assertThat(thrown).hasCauseThat().hasMessageThat().contains("Invalid comment range");
+ List<CommentInfo> comments =
+ gApi.changes().id(changeId).commentsRequest().withContext(true).getAsList();
+ assertThat(comments).hasSize(1);
+ assertThat(comments.get(0).contextLines).isEmpty();
}
@Test
