diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index e09d936d30..f2f5c15503 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -2039,6 +2039,10 @@ entries. The entries in the map are sorted by file path, and the comments for each path are sorted by patch set number. Each comment has the `patch_set` and `author` fields set. +If the `enable_context` request parameter is set to true, the comment entries +will contain a list of link:#context-line[ContextLine] containing the lines of +the source file where the comment was written. + .Request ---- GET /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/comments HTTP/1.0 @@ -6676,6 +6680,11 @@ message that this comment is linked to. |`commit_id` |optional| Hex commit SHA1 (40 characters string) of the commit of the patchset to which this comment applies. +|`context_lines` |optional| +A list of link:#context-line[ContextLine] containing the lines of the source +file where the comment was written. Available only if the "enable_context" +parameter (see link:#list-change-comments[List Change Comments]) is set. + |=========================== [[comment-input]] @@ -6747,6 +6756,18 @@ to 0 will not include any characters on line 5, |`end_character` ||The character position in the end line. (0-based) |=========================== +[[context-line]] +=== ContextLine +The `ContextLine` entity contains the line number and line text of a single +line of the source file content. + +[options="header",cols="1,6"] +|=========================== +|Field Name |Description +|`line_number` |The line number of the source line. +|`context_line` |String containing the line text. +|=========================== + [[commit-info]] === CommitInfo The `CommitInfo` entity contains information about a commit. diff --git a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java index e8b58f9aca..3b3a5fc90e 100644 --- a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java +++ b/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> comments() throws RestApiException; + @Deprecated + default Map> 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 commentsAsList() throws RestApiException; + @Deprecated + default List 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> 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 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> comments() throws RestApiException { throw new NotImplementedException(); } @Override + @Deprecated public List commentsAsList() throws RestApiException { throw new NotImplementedException(); } + @Override + public CommentsRequest commentsRequest() throws RestApiException { + throw new NotImplementedException(); + } + @Override public Map> robotComments() throws RestApiException { throw new NotImplementedException(); diff --git a/java/com/google/gerrit/extensions/api/changes/CommentInput.java b/java/com/google/gerrit/extensions/api/changes/CommentInput.java new file mode 100644 index 0000000000..4e2f033eaa --- /dev/null +++ b/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; +} diff --git a/java/com/google/gerrit/extensions/common/CommentInfo.java b/java/com/google/gerrit/extensions/common/CommentInfo.java index 19e002ae56..19d721be69 100644 --- a/java/com/google/gerrit/extensions/common/CommentInfo.java +++ b/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 contextLines; + @Override public boolean equals(Object o) { if (super.equals(o)) { diff --git a/java/com/google/gerrit/extensions/common/ContextLineInfo.java b/java/com/google/gerrit/extensions/common/ContextLineInfo.java new file mode 100644 index 0000000000..3062e85b7b --- /dev/null +++ b/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); + } +} diff --git a/java/com/google/gerrit/server/CommentContextLoader.java b/java/com/google/gerrit/server/CommentContextLoader.java new file mode 100644 index 0000000000..813dad7f11 --- /dev/null +++ b/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> 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}. + * + *

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 getContext(CommentInfo comment) { + ContextData key = + ContextData.create( + comment.id, + ObjectId.fromString(comment.commitId), + comment.path, + getStartAndEndLines(comment)); + List 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> 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 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 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) { + return new AutoValue_CommentContextLoader_ContextData(id, commitId, path, range); + } + + abstract String id(); + + abstract ObjectId commitId(); + + abstract String path(); + + abstract Optional range(); + } +} diff --git a/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java index b4a5da75b5..0992bcdf11 100644 --- a/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java +++ b/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 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 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> 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> 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 commentsAsList() throws RestApiException { - try { - return listComments.getComments(change); - } catch (Exception e) { - throw asRestApiException("Cannot get comments", e); - } + @Override + public List 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 diff --git a/java/com/google/gerrit/server/restapi/change/CommentJson.java b/java/com/google/gerrit/server/restapi/change/CommentJson.java index 20c4b489f9..67049e8ff9 100644 --- a/java/com/google/gerrit/server/restapi/change/CommentJson.java +++ b/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) { diff --git a/java/com/google/gerrit/server/restapi/change/ListChangeComments.java b/java/com/google/gerrit/server/restapi/change/ListChangeComments.java index b842f55f08..fec7bdcaaf 100644 --- a/java/com/google/gerrit/server/restapi/change/ListChangeComments.java +++ b/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 { private final ChangeMessagesUtil changeMessagesUtil; private final ChangeData.Factory changeDataFactory; private final Provider 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, 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 { private ImmutableList getAsList(Iterable comments, ChangeResource rsrc) throws PermissionBackendException { - ImmutableList commentInfos = getCommentFormatter().formatAsList(comments); + ImmutableList commentInfos = + getCommentFormatter(rsrc.getProject()).formatAsList(comments); List changeMessages = changeMessagesUtil.byChange(rsrc.getNotes()); CommentsUtil.linkCommentsToChangeMessages(commentInfos, changeMessages, true); return commentInfos; @@ -78,7 +97,8 @@ public class ListChangeComments implements RestReadView { private Map> getAsMap( Iterable comments, ChangeResource rsrc) throws PermissionBackendException { - Map> commentInfosMap = getCommentFormatter().format(comments); + Map> commentInfosMap = + getCommentFormatter(rsrc.getProject()).format(comments); List commentInfos = commentInfosMap.values().stream().flatMap(List::stream).collect(toList()); List changeMessages = changeMessagesUtil.byChange(rsrc.getNotes()); @@ -86,7 +106,12 @@ public class ListChangeComments implements RestReadView { 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(); } } diff --git a/java/com/google/gerrit/server/restapi/change/Module.java b/java/com/google/gerrit/server/restapi/change/Module.java index 62d89ee7c3..69e2788767 100644 --- a/java/com/google/gerrit/server/restapi/change/Module.java +++ b/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); diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java index 4f61d7999c..72453fd71e 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java @@ -30,6 +30,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; +import com.google.common.collect.MoreCollectors; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.PushOneCommit; @@ -53,6 +54,7 @@ import com.google.gerrit.extensions.client.Comment; import com.google.gerrit.extensions.client.Side; import com.google.gerrit.extensions.common.ChangeInfo; 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.BadRequestException; import com.google.gerrit.extensions.restapi.IdString; @@ -79,6 +81,7 @@ import java.util.function.Function; import java.util.function.Supplier; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.stream.Collectors; import org.eclipse.jgit.lib.ObjectReader; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; @@ -1019,12 +1022,16 @@ public class CommentsIT extends AbstractDaemonTest { addComment(r1, "nit: trailing whitespace"); addComment(r2, "typo: content"); - Map> actual = gApi.changes().id(r2.getChangeId()).comments(); + Map> actual = + gApi.changes().id(r2.getChangeId()).commentsRequest().get(); assertThat(actual.keySet()).containsExactly(FILE_NAME); List comments = actual.get(FILE_NAME); assertThat(comments).hasSize(2); + // Comment context is disabled by default + assertThat(comments.stream().filter(c -> c.contextLines != null)).isEmpty(); + CommentInfo c1 = comments.get(0); assertThat(c1.author._accountId).isEqualTo(user.id().get()); assertThat(c1.patchSet).isEqualTo(1); @@ -1040,6 +1047,61 @@ public class CommentsIT extends AbstractDaemonTest { assertThat(c2.line).isEqualTo(1); } + @Test + public void listChangeCommentsWithContextEnabled() throws Exception { + PushOneCommit.Result r1 = createChange(); + + ImmutableList.Builder content = ImmutableList.builder(); + for (int i = 1; i <= 10; i++) { + content.add("line_" + i); + } + + PushOneCommit.Result r2 = + pushFactory + .create( + admin.newIdent(), + testRepo, + SUBJECT, + FILE_NAME, + content.build().stream().collect(Collectors.joining("\n")), + r1.getChangeId()) + .to("refs/for/master"); + + addCommentOnLine(r2, "nit: please fix", 1); + addCommentOnRange(r2, "looks good", commentRangeInLines(2, 5)); + + List comments = + gApi.changes().id(r2.getChangeId()).commentsRequest().withContext(true).getAsList(); + + assertThat(comments).hasSize(2); + + assertThat( + comments.stream() + .filter(c -> c.message.equals("nit: please fix")) + .collect(MoreCollectors.onlyElement()) + .contextLines) + .containsExactlyElementsIn(contextLines("1", "line_1")); + + assertThat( + comments.stream() + .filter(c -> c.message.equals("looks good")) + .collect(MoreCollectors.onlyElement()) + .contextLines) + .containsExactlyElementsIn( + contextLines("2", "line_2", "3", "line_3", "4", "line_4", "5", "line_5")); + } + + private List contextLines(String... args) { + List result = new ArrayList<>(); + for (int i = 0; i < args.length; i += 2) { + int lineNbr = Integer.parseInt(args[i]); + String contextLine = args[i + 1]; + ContextLineInfo info = new ContextLineInfo(lineNbr, contextLine); + result.add(info); + } + return result; + } + @Test public void listChangeCommentsAnonymousDoesNotRequireAuth() throws Exception { PushOneCommit.Result r1 = createChange(); @@ -1052,12 +1114,12 @@ public class CommentsIT extends AbstractDaemonTest { addComment(r1, "nit: trailing whitespace"); addComment(r2, "typo: content"); - List comments = gApi.changes().id(r1.getChangeId()).commentsAsList(); + List comments = gApi.changes().id(r1.getChangeId()).commentsRequest().getAsList(); assertThat(comments.stream().map(c -> c.message).collect(toList())) .containsExactly("nit: trailing whitespace", "typo: content"); requestScopeOperations.setApiUserAnonymous(); - comments = gApi.changes().id(r1.getChangeId()).commentsAsList(); + comments = gApi.changes().id(r1.getChangeId()).commentsRequest().getAsList(); assertThat(comments.stream().map(c -> c.message).collect(toList())) .containsExactly("nit: trailing whitespace", "typo: content"); } @@ -1713,7 +1775,23 @@ public class CommentsIT extends AbstractDaemonTest { } private void addComment(PushOneCommit.Result r, String message) throws Exception { - addComment(r, message, false, false, null); + addComment(r, message, false, false, null, 1, null); + } + + private void addCommentOnLine(PushOneCommit.Result r, String message, int line) throws Exception { + addComment(r, message, false, false, null, line, null); + } + + private void addCommentOnRange(PushOneCommit.Result r, String message, Comment.Range range) + throws Exception { + addComment(r, message, false, false, null, null, range); + } + + private Comment.Range commentRangeInLines(int startLine, int endLine) { + Comment.Range range = new Comment.Range(); + range.startLine = startLine; + range.endLine = endLine; + return range; } private void addComment( @@ -1723,12 +1801,25 @@ public class CommentsIT extends AbstractDaemonTest { Boolean unresolved, String inReplyTo) throws Exception { + addComment(r, message, omitDuplicateComments, unresolved, inReplyTo, 1, null); + } + + private void addComment( + PushOneCommit.Result r, + String message, + boolean omitDuplicateComments, + Boolean unresolved, + String inReplyTo, + Integer line, + Comment.Range range) + throws Exception { CommentInput c = new CommentInput(); c.line = 1; c.message = message; c.path = FILE_NAME; c.unresolved = unresolved; c.inReplyTo = inReplyTo; + c.range = range; ReviewInput in = newInput(c); in.omitDuplicateComments = omitDuplicateComments; gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(in); @@ -1766,11 +1857,11 @@ public class CommentsIT extends AbstractDaemonTest { } private List getPublishedCommentsAsList(String changeId) throws Exception { - return gApi.changes().id(changeId).commentsAsList(); + return gApi.changes().id(changeId).commentsRequest().getAsList(); } private List getPublishedCommentsAsList(Change.Id changeId) throws Exception { - return gApi.changes().id(changeId.get()).commentsAsList(); + return gApi.changes().id(changeId.get()).commentsRequest().getAsList(); } private Map> getDraftComments(String changeId, String revId)