Merge "Add the comment context to the ListChangeComments RestReadView"

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);