From abb291c77fab71a2824f2ca57bcb38061c53820d Mon Sep 17 00:00:00 2001
From: Youssef Elghareeb <ghareeb@google.com>
Date: Thu, 18 Feb 2021 10:50:46 +0100
Subject: [PATCH] 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
---
 .../gerrit/server/comment/CommentContextLoader.java  | 11 ++++-------
 .../acceptance/server/change/CommentContextIT.java   | 12 ++++--------
 2 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/java/com/google/gerrit/server/comment/CommentContextLoader.java b/java/com/google/gerrit/server/comment/CommentContextLoader.java
index 63bb8d025b..c93f4b18fd 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.common.Nullable;
 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 @@ public class CommentContextLoader {
 
   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 adfe56db63..548e3fe6fe 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.acceptance.PushOneCommit.FILE_NAME;
 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 @@ public class CommentContextIT extends AbstractDaemonTest {
         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