Add the comment context to the ListChangeComments RestReadView

Comment context lines are returned from the comments API if the
parameter "enable-context" is set. Each CommentInfo will include an
additional contextLines field, which is a list of {line_number,
line_text}, which are some lines of the source file where the comment
was written.

In this change, the context lines are exactly the lines highlighted by
the user when the comment was written. In a later change, the
"enable-context" parameter will be changed to an integer parameter. The
caller will decide how many lines to retrieve.

Change-Id: I76928de54818fa63b5b49b8e89244c5289361af4

java/com/google/gerrit/extensions/api/changes/ChangeApi.java

@@ -326,8 +326,12 @@ public interface ChangeApi {
    * @return comments in a map keyed by path; comments have the {@code revision} field set to
    *     indicate their patch set.
    * @throws RestApiException
+   * @deprecated Callers should use {@link #commentsRequest()} instead
    */
-  Map<String, List<CommentInfo>> comments() throws RestApiException;
+  @Deprecated
+  default Map<String, List<CommentInfo>> comments() throws RestApiException {
+    return commentsRequest().get();
+  }
 
   /**
    * Get all published comments on a change as a list.
@@ -335,8 +339,21 @@ public interface ChangeApi {
    * @return comments as a list; comments have the {@code revision} field set to indicate their
    *     patch set.
    * @throws RestApiException
+   * @deprecate Callers should use {@link #commentsRequest()} instead
    */
-  List<CommentInfo> commentsAsList() throws RestApiException;
+  @Deprecated
+  default List<CommentInfo> commentsAsList() throws RestApiException {
+    return commentsRequest().getAsList();
+  }
+
+  /**
+   * Get a {@link CommentsRequest} entity that can be used to retrieve published comments.
+   *
+   * @return A {@link CommentsRequest} entity that can be used to retrieve the comments using the
+   *     {@link CommentsRequest#get()} or {@link CommentsRequest#getAsList()}.
+   * @throws RestApiException
+   */
+  CommentsRequest commentsRequest() throws RestApiException;
 
   /**
    * Get all robot comments on a change.
@@ -395,6 +412,42 @@ public interface ChangeApi {
    */
   ChangeMessageApi message(String id) throws RestApiException;
 
+  abstract class CommentsRequest {
+    private boolean enableContext;
+
+    /**
+     * Get all published comments on a change.
+     *
+     * @return comments in a map keyed by path; comments have the {@code revision} field set to
+     *     indicate their patch set.
+     * @throws RestApiException
+     */
+    public abstract Map<String, List<CommentInfo>> get() throws RestApiException;
+
+    /**
+     * Get all published comments on a change as a list.
+     *
+     * @return comments as a list; comments have the {@code revision} field set to indicate their
+     *     patch set.
+     * @throws RestApiException
+     */
+    public abstract List<CommentInfo> getAsList() throws RestApiException;
+
+    public CommentsRequest withContext(boolean enableContext) {
+      this.enableContext = enableContext;
+      return this;
+    }
+
+    public CommentsRequest withContext() {
+      this.enableContext = true;
+      return this;
+    }
+
+    public boolean getContext() {
+      return enableContext;
+    }
+  }
+
   abstract class SuggestedReviewersRequest {
     private String query;
     private int limit;
@@ -603,15 +656,22 @@ public interface ChangeApi {
     }
 
     @Override
+    @Deprecated
     public Map<String, List<CommentInfo>> comments() throws RestApiException {
       throw new NotImplementedException();
     }
 
     @Override
+    @Deprecated
     public List<CommentInfo> commentsAsList() throws RestApiException {
       throw new NotImplementedException();
     }
 
+    @Override
+    public CommentsRequest commentsRequest() throws RestApiException {
+      throw new NotImplementedException();
+    }
+
     @Override
     public Map<String, List<RobotCommentInfo>> robotComments() throws RestApiException {
       throw new NotImplementedException();

java/com/google/gerrit/extensions/api/changes/CommentInput.java

@@ -0,0 +1,20 @@
+// 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.extensions.api.changes;
+
+/** Input to the {@link ChangeApi#comments(CommentInput)}. */
+public class CommentInput {
+  public boolean enableContext;
+}

java/com/google/gerrit/extensions/common/CommentInfo.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.extensions.common;
 
 import com.google.gerrit.extensions.client.Comment;
+import java.util.List;
 import java.util.Objects;
 
 public class CommentInfo extends Comment {
@@ -22,6 +23,12 @@ public class CommentInfo extends Comment {
   public String tag;
   public String changeMessageId;
 
+  /**
+   * A list of {@link ContextLineInfo}, that is, a list of pairs of {line_num, line_text} of the
+   * actual source file content surrounding and including the lines where the comment was written.
+   */
+  public List<ContextLineInfo> contextLines;
+
   @Override
   public boolean equals(Object o) {
     if (super.equals(o)) {

java/com/google/gerrit/extensions/common/ContextLineInfo.java

@@ -0,0 +1,47 @@
+// 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.extensions.common;
+
+import java.util.Objects;
+
+/**
+ * An entity class representing 1 line of context {line number, line text} of the source file where
+ * a comment was written.
+ */
+public class ContextLineInfo {
+  public int lineNumber;
+  public String contextLine;
+
+  public ContextLineInfo() {}
+
+  public ContextLineInfo(int lineNumber, String contextLine) {
+    this.lineNumber = lineNumber;
+    this.contextLine = contextLine;
+  }
+
+  @Override
+  public boolean equals(Object o) {
+    if (o instanceof ContextLineInfo) {
+      ContextLineInfo l = (ContextLineInfo) o;
+      return lineNumber == l.lineNumber && contextLine.equals(l.contextLine);
+    }
+    return false;
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(lineNumber, contextLine);
+  }
+}

java/com/google/gerrit/server/CommentContextLoader.java

@@ -0,0 +1,164 @@
+// 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;
+
+import static java.util.stream.Collectors.groupingBy;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.extensions.common.CommentInfo;
+import com.google.gerrit.extensions.common.ContextLineInfo;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.patch.Text;
+import com.google.inject.Inject;
+import com.google.inject.assistedinject.Assisted;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.treewalk.TreeWalk;
+
+/**
+ * Computes the list of {@link ContextLineInfo} for a given comment, that is, the lines of the
+ * source file surrounding and including the area where the comment was written.
+ */
+public class CommentContextLoader {
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+  private final GitRepositoryManager repoManager;
+  private final Project.NameKey project;
+  private Map<ContextData, List<ContextLineInfo>> candidates;
+
+  public interface Factory {
+    CommentContextLoader create(Project.NameKey project);
+  }
+
+  @Inject
+  public CommentContextLoader(GitRepositoryManager repoManager, @Assisted Project.NameKey project) {
+    this.repoManager = repoManager;
+    this.project = project;
+    this.candidates = new HashMap<>();
+  }
+
+  /**
+   * Returns an empty list of {@link ContextLineInfo}. Clients are expected to call this method one
+   * or more times. Each call returns a reference to an empty {@link List<ContextLineInfo>}.
+   *
+   * <p>A single call to {@link #fill()} will cause all list references returned from this method to
+   * be populated. If a client calls this method again with a comment that was passed before calling
+   * {@link #fill()}, the new populated list will be returned.
+   *
+   * @param comment the comment entity for which we want to load the context
+   * @return a list of {@link ContextLineInfo}
+   */
+  public List<ContextLineInfo> getContext(CommentInfo comment) {
+    ContextData key =
+        ContextData.create(
+            comment.id,
+            ObjectId.fromString(comment.commitId),
+            comment.path,
+            getStartAndEndLines(comment));
+    List<ContextLineInfo> context = candidates.get(key);
+    if (context == null) {
+      context = new ArrayList<>();
+      candidates.put(key, context);
+    }
+    return context;
+  }
+
+  /**
+   * A call to this method loads the context for all comments stored in {@link
+   * CommentContextLoader#candidates}. This is useful so that the repository is opened once for all
+   * comments.
+   */
+  public void fill() {
+    // Group comments by commit ID so that each commit is parsed only once
+    Map<ObjectId, List<ContextData>> commentsByCommitId =
+        candidates.keySet().stream().collect(groupingBy(ContextData::commitId));
+
+    try (Repository repo = repoManager.openRepository(project);
+        RevWalk rw = new RevWalk(repo)) {
+      for (ObjectId commitId : commentsByCommitId.keySet()) {
+        RevCommit commit = rw.parseCommit(commitId);
+        for (ContextData k : commentsByCommitId.get(commitId)) {
+          if (!k.range().isPresent()) {
+            continue;
+          }
+          try (TreeWalk tw = TreeWalk.forPath(rw.getObjectReader(), k.path(), commit.getTree())) {
+            if (tw == null) {
+              logger.atWarning().log(
+                  "Failed to find path %s in the git tree of ID %s.",
+                  k.path(), commit.getTree().getId());
+              continue;
+            }
+            ObjectId id = tw.getObjectId(0);
+            Text src = new Text(repo.open(id, Constants.OBJ_BLOB));
+            List<ContextLineInfo> contextLines = candidates.get(k);
+            Range r = k.range().get();
+            for (int i = r.start(); i <= r.end(); i++) {
+              contextLines.add(new ContextLineInfo(i, src.getString(i - 1)));
+            }
+          }
+        }
+      }
+    } catch (IOException e) {
+      throw new StorageException("Failed to load the comment context", e);
+    }
+  }
+
+  private static Optional<Range> getStartAndEndLines(CommentInfo comment) {
+    if (comment.range != null) {
+      return Optional.of(Range.create(comment.range.startLine, comment.range.endLine));
+    } else if (comment.line != null) {
+      return Optional.of(Range.create(comment.line, comment.line));
+    }
+    return Optional.empty();
+  }
+
+  @AutoValue
+  abstract static class Range {
+    static Range create(int start, int end) {
+      return new AutoValue_CommentContextLoader_Range(start, end);
+    }
+
+    abstract int start();
+
+    abstract int end();
+  }
+
+  @AutoValue
+  abstract static class ContextData {
+    static ContextData create(String id, ObjectId commitId, String path, Optional<Range> range) {
+      return new AutoValue_CommentContextLoader_ContextData(id, commitId, path, range);
+    }
+
+    abstract String id();
+
+    abstract ObjectId commitId();
+
+    abstract String path();
+
+    abstract Optional<Range> range();
+  }
+}

java/com/google/gerrit/server/api/changes/ChangeApiImpl.java

@@ -158,7 +158,7 @@ class ChangeApiImpl implements ChangeApi {
   private final GetAssignee getAssignee;
   private final GetPastAssignees getPastAssignees;
   private final DeleteAssignee deleteAssignee;
-  private final ListChangeComments listComments;
+  private final Provider<ListChangeComments> listCommentsProvider;
   private final ListChangeRobotComments listChangeRobotComments;
   private final ListChangeDrafts listDrafts;
   private final ChangeEditApiImpl.Factory changeEditApi;
@@ -211,7 +211,7 @@ class ChangeApiImpl implements ChangeApi {
       GetAssignee getAssignee,
       GetPastAssignees getPastAssignees,
       DeleteAssignee deleteAssignee,
-      ListChangeComments listComments,
+      Provider<ListChangeComments> listCommentsProvider,
       ListChangeRobotComments listChangeRobotComments,
       ListChangeDrafts listDrafts,
       ChangeEditApiImpl.Factory changeEditApi,
@@ -262,7 +262,7 @@ class ChangeApiImpl implements ChangeApi {
     this.getAssignee = getAssignee;
     this.getPastAssignees = getPastAssignees;
     this.deleteAssignee = deleteAssignee;
-    this.listComments = listComments;
+    this.listCommentsProvider = listCommentsProvider;
     this.listChangeRobotComments = listChangeRobotComments;
     this.listDrafts = listDrafts;
     this.changeEditApi = changeEditApi;
@@ -599,21 +599,30 @@ class ChangeApiImpl implements ChangeApi {
   }
 
   @Override
-  public Map<String, List<CommentInfo>> comments() throws RestApiException {
-    try {
-      return listComments.apply(change).value();
-    } catch (Exception e) {
-      throw asRestApiException("Cannot get comments", e);
-    }
-  }
+  public CommentsRequest commentsRequest() throws RestApiException {
+    return new CommentsRequest() {
+      @Override
+      public Map<String, List<CommentInfo>> get() throws RestApiException {
+        try {
+          ListChangeComments listComments = listCommentsProvider.get();
+          listComments.setContext(this.getContext());
+          return listComments.apply(change).value();
+        } catch (Exception e) {
+          throw asRestApiException("Cannot get comments", e);
+        }
+      }
 
-  @Override
-  public List<CommentInfo> commentsAsList() throws RestApiException {
-    try {
-      return listComments.getComments(change);
-    } catch (Exception e) {
-      throw asRestApiException("Cannot get comments", e);
-    }
+      @Override
+      public List<CommentInfo> getAsList() throws RestApiException {
+        try {
+          ListChangeComments listComments = listCommentsProvider.get();
+          listComments.setContext(this.getContext());
+          return listComments.getComments(change);
+        } catch (Exception e) {
+          throw asRestApiException("Cannot get comments", e);
+        }
+      }
+    };
   }
 
   @Override

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

@@ -26,6 +26,7 @@ import com.google.gerrit.entities.Comment;
 import com.google.gerrit.entities.FixReplacement;
 import com.google.gerrit.entities.FixSuggestion;
 import com.google.gerrit.entities.HumanComment;
+import com.google.gerrit.entities.Project;
 import com.google.gerrit.entities.RobotComment;
 import com.google.gerrit.extensions.client.Comment.Range;
 import com.google.gerrit.extensions.client.Side;
@@ -34,6 +35,7 @@ import com.google.gerrit.extensions.common.FixReplacementInfo;
 import com.google.gerrit.extensions.common.FixSuggestionInfo;
 import com.google.gerrit.extensions.common.RobotCommentInfo;
 import com.google.gerrit.extensions.restapi.Url;
+import com.google.gerrit.server.CommentContextLoader;
 import com.google.gerrit.server.account.AccountLoader;
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.inject.Inject;
@@ -48,10 +50,15 @@ public class CommentJson {
 
   private boolean fillAccounts = true;
   private boolean fillPatchSet;
+  private CommentContextLoader.Factory commentContextLoaderFactory;
+  private CommentContextLoader commentContextLoader;
 
   @Inject
-  CommentJson(AccountLoader.Factory accountLoaderFactory) {
+  CommentJson(
+      AccountLoader.Factory accountLoaderFactory,
+      CommentContextLoader.Factory commentContextLoaderFactory) {
     this.accountLoaderFactory = accountLoaderFactory;
+    this.commentContextLoaderFactory = commentContextLoaderFactory;
   }
 
   CommentJson setFillAccounts(boolean fillAccounts) {
@@ -64,6 +71,13 @@ public class CommentJson {
     return this;
   }
 
+  CommentJson setEnableContext(boolean enableContext, Project.NameKey project) {
+    if (enableContext) {
+      this.commentContextLoader = commentContextLoaderFactory.create(project);
+    }
+    return this;
+  }
+
   public HumanCommentFormatter newHumanCommentFormatter() {
     return new HumanCommentFormatter();
   }
@@ -79,6 +93,9 @@ public class CommentJson {
       if (loader != null) {
         loader.fill();
       }
+      if (commentContextLoader != null) {
+        commentContextLoader.fill();
+      }
       return info;
     }
 
@@ -103,6 +120,9 @@ public class CommentJson {
       if (loader != null) {
         loader.fill();
       }
+      if (commentContextLoader != null) {
+        commentContextLoader.fill();
+      }
       return out;
     }
 
@@ -118,6 +138,9 @@ public class CommentJson {
       if (loader != null) {
         loader.fill();
       }
+      if (commentContextLoader != null) {
+        commentContextLoader.fill();
+      }
       return out;
     }
 
@@ -148,6 +171,9 @@ public class CommentJson {
         r.author = loader.get(c.author.getId());
       }
       r.commitId = c.getCommitId().getName();
+      if (commentContextLoader != null) {
+        r.contextLines = commentContextLoader.getContext(r);
+      }
     }
 
     protected Range toRange(Comment.Range commentRange) {

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

@@ -19,38 +19,56 @@ import static java.util.stream.Collectors.toList;
 import com.google.common.collect.ImmutableList;
 import com.google.gerrit.entities.ChangeMessage;
 import com.google.gerrit.entities.HumanComment;
+import com.google.gerrit.entities.Project;
 import com.google.gerrit.extensions.common.CommentInfo;
+import com.google.gerrit.extensions.common.ContextLineInfo;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.gerrit.server.ChangeMessagesUtil;
+import com.google.gerrit.server.CommentContextLoader;
 import com.google.gerrit.server.CommentsUtil;
 import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
-import com.google.inject.Singleton;
 import java.util.List;
 import java.util.Map;
+import org.kohsuke.args4j.Option;
 
-@Singleton
 public class ListChangeComments implements RestReadView<ChangeResource> {
   private final ChangeMessagesUtil changeMessagesUtil;
   private final ChangeData.Factory changeDataFactory;
   private final Provider<CommentJson> commentJson;
   private final CommentsUtil commentsUtil;
+  private final CommentContextLoader.Factory commentContextFactory;
+
+  private boolean includeContext;
+
+  /**
+   * Optional parameter. If set, the contextLines field of the {@link ContextLineInfo} of the
+   * response will contain the lines of the source file where the comment was written.
+   *
+   * @param context If true, comment context will be attached to the response
+   */
+  @Option(name = "--enable-context")
+  public void setContext(boolean context) {
+    this.includeContext = context;
+  }
 
   @Inject
   ListChangeComments(
       ChangeData.Factory changeDataFactory,
       Provider<CommentJson> commentJson,
       CommentsUtil commentsUtil,
-      ChangeMessagesUtil changeMessagesUtil) {
+      ChangeMessagesUtil changeMessagesUtil,
+      CommentContextLoader.Factory commentContextFactory) {
     this.changeDataFactory = changeDataFactory;
     this.commentJson = commentJson;
     this.commentsUtil = commentsUtil;
     this.changeMessagesUtil = changeMessagesUtil;
+    this.commentContextFactory = commentContextFactory;
   }
 
   @Override
@@ -70,7 +88,8 @@ public class ListChangeComments implements RestReadView<ChangeResource> {
 
   private ImmutableList<CommentInfo> getAsList(Iterable<HumanComment> comments, ChangeResource rsrc)
       throws PermissionBackendException {
-    ImmutableList<CommentInfo> commentInfos = getCommentFormatter().formatAsList(comments);
+    ImmutableList<CommentInfo> commentInfos =
+        getCommentFormatter(rsrc.getProject()).formatAsList(comments);
     List<ChangeMessage> changeMessages = changeMessagesUtil.byChange(rsrc.getNotes());
     CommentsUtil.linkCommentsToChangeMessages(commentInfos, changeMessages, true);
     return commentInfos;
@@ -78,7 +97,8 @@ public class ListChangeComments implements RestReadView<ChangeResource> {
 
   private Map<String, List<CommentInfo>> getAsMap(
       Iterable<HumanComment> comments, ChangeResource rsrc) throws PermissionBackendException {
-    Map<String, List<CommentInfo>> commentInfosMap = getCommentFormatter().format(comments);
+    Map<String, List<CommentInfo>> commentInfosMap =
+        getCommentFormatter(rsrc.getProject()).format(comments);
     List<CommentInfo> commentInfos =
         commentInfosMap.values().stream().flatMap(List::stream).collect(toList());
     List<ChangeMessage> changeMessages = changeMessagesUtil.byChange(rsrc.getNotes());
@@ -86,7 +106,12 @@ public class ListChangeComments implements RestReadView<ChangeResource> {
     return commentInfosMap;
   }
 
-  private CommentJson.HumanCommentFormatter getCommentFormatter() {
-    return commentJson.get().setFillAccounts(true).setFillPatchSet(true).newHumanCommentFormatter();
+  private CommentJson.HumanCommentFormatter getCommentFormatter(Project.NameKey project) {
+    return commentJson
+        .get()
+        .setFillAccounts(true)
+        .setFillPatchSet(true)
+        .setEnableContext(includeContext, project)
+        .newHumanCommentFormatter();
   }
 }

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

@@ -29,6 +29,7 @@ import static com.google.gerrit.server.change.VoteResource.VOTE_KIND;
 
 import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.extensions.restapi.RestApiModule;
+import com.google.gerrit.server.CommentContextLoader;
 import com.google.gerrit.server.account.AccountLoader;
 import com.google.gerrit.server.change.AddReviewersOp;
 import com.google.gerrit.server.change.AddToAttentionSetOp;
@@ -205,6 +206,7 @@ public class Module extends RestApiModule {
     factory(AccountLoader.Factory.class);
     factory(ChangeInserter.Factory.class);
     factory(ChangeResource.Factory.class);
+    factory(CommentContextLoader.Factory.class);
     factory(DeleteChangeOp.Factory.class);
     factory(DeleteReviewerByEmailOp.Factory.class);
     factory(DeleteReviewerOp.Factory.class);