Make porting comments resilient to diff failures

The diff computation can run into various errors, ranging from a diff
temporarily not being available due to replication lag in a multi-master
setup to the diff not existing as the auto-merge commit creation failed.
As comments are ported from various patchsets and sources
(e.g. patchset commits vs. parent commits vs. auto-merge commit), a
single failure would have meant that no ported comments were available
with the previous code even though some of them could have been
successfully ported.

This change increases the resilience of the ported comments endpoint by
catching individual diff errors and mapping only the affected comments
to a fallback position instead. At the moment, the fallback is the
patchset-level. If the diff errors don't occur on a later request
anymore, the corresponding comments will be ported as usual.

To test this error situation, we add unit tests. Unfortunately,
ChangeNotes is a very rich class referring to many other non-value
classes. Hence, we mock it. To simplify unit testing, we might consider
to refactor CommentPorter in the future to not use ChangeNotes as method
parameter. Instead, we could introduce a new value class which
provides the small amount of value classes (Change, Patchset instances,
Project.NameKey) CommentPorter needs.

Change-Id: I307c9b3a2d93668097b84d5f83345f8c9f49a1d4

java/com/google/gerrit/server/restapi/change/CommentPorter.java

@@ -20,6 +20,7 @@ import static java.util.stream.Collectors.groupingBy;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.entities.Change;
 import com.google.gerrit.entities.Comment.Range;
 import com.google.gerrit.entities.HumanComment;
@@ -32,6 +33,7 @@ import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.patch.DiffMappings;
 import com.google.gerrit.server.patch.GitPositionTransformer;
 import com.google.gerrit.server.patch.GitPositionTransformer.BestPositionOnConflict;
+import com.google.gerrit.server.patch.GitPositionTransformer.FileMapping;
 import com.google.gerrit.server.patch.GitPositionTransformer.Mapping;
 import com.google.gerrit.server.patch.GitPositionTransformer.Position;
 import com.google.gerrit.server.patch.GitPositionTransformer.PositionedEntity;
@@ -57,6 +59,7 @@ import org.eclipse.jgit.lib.ObjectId;
  */
 @Singleton
 public class CommentPorter {
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
   private final GitPositionTransformer positionTransformer =
       new GitPositionTransformer(BestPositionOnConflict.INSTANCE);
@@ -91,8 +94,7 @@ public class CommentPorter {
    * @return the ported comments, in no particular order
    */
   public ImmutableList<HumanComment> portComments(
-      ChangeNotes changeNotes, PatchSet targetPatchset, List<HumanComment> comments)
-      throws PatchListNotAvailableException {
+      ChangeNotes changeNotes, PatchSet targetPatchset, List<HumanComment> comments) {
 
     ImmutableList<HumanComment> relevantComments = filterToRelevant(comments, targetPatchset);
     return port(changeNotes, targetPatchset, relevantComments);
@@ -106,8 +108,7 @@ public class CommentPorter {
   }
 
   private ImmutableList<HumanComment> port(
-      ChangeNotes notes, PatchSet targetPatchset, List<HumanComment> comments)
-      throws PatchListNotAvailableException {
+      ChangeNotes notes, PatchSet targetPatchset, List<HumanComment> comments) {
     Map<Integer, ImmutableList<HumanComment>> commentsPerPatchset =
         comments.stream().collect(groupingBy(comment -> comment.key.patchSetId, toImmutableList()));
 
@@ -133,8 +134,7 @@ public class CommentPorter {
       Change change,
       PatchSet originalPatchset,
       PatchSet targetPatchset,
-      ImmutableList<HumanComment> comments)
-      throws PatchListNotAvailableException {
+      ImmutableList<HumanComment> comments) {
     Map<Short, List<HumanComment>> commentsPerSide =
         comments.stream().collect(groupingBy(comment -> comment.side));
     ImmutableList.Builder<HumanComment> portedComments = ImmutableList.builder();
@@ -157,10 +157,22 @@ public class CommentPorter {
       PatchSet originalPatchset,
       PatchSet targetPatchset,
       List<HumanComment> comments,
-      short side)
-      throws PatchListNotAvailableException {
-    ImmutableSet<Mapping> mappings =
-        loadMappings(project, change, originalPatchset, targetPatchset, side);
+      short side) {
+    ImmutableSet<Mapping> mappings;
+    try {
+      mappings = loadMappings(project, change, originalPatchset, targetPatchset, side);
+    } catch (Exception e) {
+      logger.atWarning().withCause(e).log(
+          "Could not determine some necessary diff mappings for porting comments on change %s from"
+              + " patchset %s to patchset %s. Mapping %d affected comments to the fallback"
+              + " destination.",
+          change.getChangeId(),
+          originalPatchset.id().getId(),
+          targetPatchset.id().getId(),
+          comments.size());
+      mappings = getFallbackMappings(comments);
+    }
+
     ImmutableList<PositionedEntity<HumanComment>> positionedComments =
         comments.stream().map(this::toPositionedEntity).collect(toImmutableList());
     return positionTransformer.transform(positionedComments, mappings).stream()
@@ -192,6 +204,17 @@ public class CommentPorter {
     return patchList.getPatches().stream().map(DiffMappings::toMapping).collect(toImmutableSet());
   }
 
+  private ImmutableSet<Mapping> getFallbackMappings(List<HumanComment> comments) {
+    // Consider all files as deleted. -> Comments will be ported to the fallback destination, which
+    // currently are patchset-level comments.
+    return comments.stream()
+        .map(comment -> comment.key.filename)
+        .distinct()
+        .map(FileMapping::forDeletedFile)
+        .map(fileMapping -> Mapping.create(fileMapping, ImmutableSet.of()))
+        .collect(toImmutableSet());
+  }
+
   private PositionedEntity<HumanComment> toPositionedEntity(HumanComment comment) {
     return PositionedEntity.create(
         comment, CommentPorter::extractPosition, CommentPorter::createCommentAtNewPosition);

java/com/google/gerrit/server/restapi/change/ListPortedComments.java

@@ -22,7 +22,6 @@ import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.gerrit.server.CommentsUtil;
 import com.google.gerrit.server.change.RevisionResource;
-import com.google.gerrit.server.patch.PatchListNotAvailableException;
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -47,7 +46,7 @@ public class ListPortedComments implements RestReadView<RevisionResource> {
 
   @Override
   public Response<Map<String, List<CommentInfo>>> apply(RevisionResource revisionResource)
-      throws PermissionBackendException, PatchListNotAvailableException {
+      throws PermissionBackendException {
     PatchSet targetPatchset = revisionResource.getPatchSet();
 
     List<HumanComment> allComments =

java/com/google/gerrit/server/restapi/change/ListPortedDrafts.java

@@ -22,7 +22,6 @@ import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.gerrit.server.CommentsUtil;
 import com.google.gerrit.server.change.RevisionResource;
-import com.google.gerrit.server.patch.PatchListNotAvailableException;
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -47,7 +46,7 @@ public class ListPortedDrafts implements RestReadView<RevisionResource> {
 
   @Override
   public Response<Map<String, List<CommentInfo>>> apply(RevisionResource revisionResource)
-      throws PermissionBackendException, PatchListNotAvailableException {
+      throws PermissionBackendException {
     PatchSet targetPatchset = revisionResource.getPatchSet();
 
     List<HumanComment> draftComments =

javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java

@@ -0,0 +1,221 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.restapi.change;
+
+import static com.google.common.truth.Truth.assertThat;
+import static java.util.Comparator.comparing;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSortedMap;
+import com.google.common.truth.Correspondence;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.Comment;
+import com.google.gerrit.entities.HumanComment;
+import com.google.gerrit.entities.Patch;
+import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.patch.ComparisonType;
+import com.google.gerrit.server.patch.PatchList;
+import com.google.gerrit.server.patch.PatchListCache;
+import com.google.gerrit.server.patch.PatchListEntry;
+import com.google.gerrit.server.patch.PatchListKey;
+import com.google.gerrit.server.patch.PatchListNotAvailableException;
+import com.google.gerrit.truth.NullAwareCorrespondence;
+import java.sql.Timestamp;
+import java.util.Arrays;
+import org.eclipse.jgit.lib.ObjectId;
+import org.junit.Rule;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnit;
+import org.mockito.junit.MockitoRule;
+
+public class CommentPorterTest {
+
+  private final ObjectId dummyObjectId =
+      ObjectId.fromString("0123456789012345678901234567890123456789");
+
+  @Rule public final MockitoRule mockito = MockitoJUnit.rule();
+
+  @Mock private PatchListCache patchListCache;
+
+  @Test
+  public void commentsAreNotDroppedWhenDiffNotAvailable() 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));
+    ChangeNotes changeNotes = mockChangeNotes(project, change, patchset1, patchset2);
+
+    CommentPorter commentPorter = new CommentPorter(patchListCache);
+    HumanComment comment = createComment(patchset1.id(), "myFile");
+    when(patchListCache.getOldId(any(), any(), any())).thenReturn(dummyObjectId);
+    when(patchListCache.get(any(PatchListKey.class), any(Project.NameKey.class)))
+        .thenThrow(PatchListNotAvailableException.class);
+    ImmutableList<HumanComment> portedComments =
+        commentPorter.portComments(changeNotes, patchset2, ImmutableList.of(comment));
+
+    assertThat(portedComments).isNotEmpty();
+  }
+
+  @Test
+  public void commentsAreNotDroppedWhenDiffHasUnexpectedError() 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));
+    ChangeNotes changeNotes = mockChangeNotes(project, change, patchset1, patchset2);
+
+    CommentPorter commentPorter = new CommentPorter(patchListCache);
+    HumanComment comment = createComment(patchset1.id(), "myFile");
+    when(patchListCache.getOldId(any(), any(), any())).thenReturn(dummyObjectId);
+    when(patchListCache.get(any(PatchListKey.class), any(Project.NameKey.class)))
+        .thenThrow(IllegalStateException.class);
+    ImmutableList<HumanComment> portedComments =
+        commentPorter.portComments(changeNotes, patchset2, ImmutableList.of(comment));
+
+    assertThat(portedComments).isNotEmpty();
+  }
+
+  @Test
+  public void commentsAreNotDroppedWhenRetrievingCommitSha1sHasUnexpectedError() 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));
+    ChangeNotes changeNotes = mockChangeNotes(project, change, patchset1, patchset2);
+
+    CommentPorter commentPorter = new CommentPorter(patchListCache);
+    HumanComment comment = createComment(patchset1.id(), "myFile");
+    when(patchListCache.getOldId(any(), any(), any())).thenThrow(IllegalStateException.class);
+    ImmutableList<HumanComment> portedComments =
+        commentPorter.portComments(changeNotes, patchset2, ImmutableList.of(comment));
+
+    assertThat(portedComments).isNotEmpty();
+  }
+
+  @Test
+  public void commentsAreMappedToPatchsetLevelOnDiffError() 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));
+    ChangeNotes changeNotes = mockChangeNotes(project, change, patchset1, patchset2);
+
+    CommentPorter commentPorter = new CommentPorter(patchListCache);
+    HumanComment comment = createComment(patchset1.id(), "myFile");
+    when(patchListCache.getOldId(any(), any(), any())).thenReturn(dummyObjectId);
+    when(patchListCache.get(any(PatchListKey.class), any(Project.NameKey.class)))
+        .thenThrow(IllegalStateException.class);
+    ImmutableList<HumanComment> portedComments =
+        commentPorter.portComments(changeNotes, patchset2, ImmutableList.of(comment));
+
+    assertThat(portedComments)
+        .comparingElementsUsing(hasFilePath())
+        .containsExactly(Patch.PATCHSET_LEVEL);
+  }
+
+  @Test
+  public void commentsAreStillPortedWhenDiffOfOtherCommentsHasError() 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));
+    PatchSet patchset3 = createPatchset(PatchSet.id(changeId, 3));
+    ChangeNotes changeNotes = mockChangeNotes(project, change, patchset1, patchset2, patchset3);
+
+    CommentPorter commentPorter = new CommentPorter(patchListCache);
+    // Place the comments on different patchsets to have two different diff requests.
+    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]);
+    // 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)
+        .thenReturn(emptyDiff);
+    ImmutableList<HumanComment> portedComments =
+        commentPorter.portComments(changeNotes, patchset3, ImmutableList.of(comment1, comment2));
+
+    // One of the comments should still be ported as usual. -> Keeps its file name as the diff was
+    // empty.
+    assertThat(portedComments).comparingElementsUsing(hasFilePath()).contains("myFile");
+  }
+
+  private Change createChange(Project.NameKey project, Change.Id changeId) {
+    return new Change(
+        Change.key("changeKey"),
+        changeId,
+        Account.id(123),
+        BranchNameKey.create(project, "myBranch"),
+        new Timestamp(12345));
+  }
+
+  private PatchSet createPatchset(PatchSet.Id id) {
+    return PatchSet.builder()
+        .id(id)
+        .commitId(dummyObjectId)
+        .uploader(Account.id(123))
+        .createdOn(new Timestamp(12345))
+        .build();
+  }
+
+  private ChangeNotes mockChangeNotes(
+      Project.NameKey project, Change change, PatchSet... patchsets) {
+    ChangeNotes changeNotes = mock(ChangeNotes.class);
+    when(changeNotes.getProjectName()).thenReturn(project);
+    when(changeNotes.getChange()).thenReturn(change);
+    when(changeNotes.getChangeId()).thenReturn(change.getId());
+    ImmutableSortedMap<PatchSet.Id, PatchSet> sortedPatchsets =
+        Arrays.stream(patchsets)
+            .collect(
+                ImmutableSortedMap.toImmutableSortedMap(
+                    comparing(PatchSet.Id::get), PatchSet::id, patchset -> patchset));
+    when(changeNotes.getPatchSets()).thenReturn(sortedPatchsets);
+    return changeNotes;
+  }
+
+  private HumanComment createComment(PatchSet.Id patchsetId, String filePath) {
+    return new HumanComment(
+        new Comment.Key("commentUuid", filePath, patchsetId.get()),
+        Account.id(100),
+        new Timestamp(1234),
+        (short) 1,
+        "Comment text",
+        "serverId",
+        true);
+  }
+
+  private Correspondence<HumanComment, String> hasFilePath() {
+    return NullAwareCorrespondence.transforming(comment -> comment.key.filename, "hasFilePath");
+  }
+}