diff --git a/java/com/google/gerrit/server/restapi/change/CommentPorter.java b/java/com/google/gerrit/server/restapi/change/CommentPorter.java new file mode 100644 index 0000000000..9a009d172f --- /dev/null +++ b/java/com/google/gerrit/server/restapi/change/CommentPorter.java @@ -0,0 +1,208 @@ +// 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.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static java.util.stream.Collectors.groupingBy; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.google.gerrit.entities.Comment.Range; +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.extensions.client.DiffPreferencesInfo.Whitespace; +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.Mapping; +import com.google.gerrit.server.patch.GitPositionTransformer.Position; +import com.google.gerrit.server.patch.GitPositionTransformer.PositionedEntity; +import com.google.gerrit.server.patch.PatchList; +import com.google.gerrit.server.patch.PatchListCache; +import com.google.gerrit.server.patch.PatchListKey; +import com.google.gerrit.server.patch.PatchListNotAvailableException; +import com.google.inject.Inject; +import com.google.inject.Singleton; +import java.util.List; +import java.util.Map; +import java.util.Optional; + +/** + * Container for all logic necessary to port comments to a target patchset. + * + *

A ported comment is a comment which was left on an earlier patchset and is shown on a later + * patchset. If a comment eligible for porting (e.g. before target patchset) can't be matched to its + * exact position in the target patchset, we'll map it to its next best location. This can also + * include a transformation of a line comment into a file comment. + */ +@Singleton +public class CommentPorter { + + private final GitPositionTransformer positionTransformer = + new GitPositionTransformer(BestPositionOnConflict.INSTANCE); + private final PatchListCache patchListCache; + + @Inject + public CommentPorter(PatchListCache patchListCache) { + this.patchListCache = patchListCache; + } + + /** + * Ports the given comments to the target patchset. + * + *

Not all given comments are ported. Only those fulfilling some criteria (e.g. before target + * patchset) are considered eligible for porting. + * + *

The returned comments represent the ported version. They don't bear any indication to which + * patchset they were ported. This is intentional as the target patchset should be obvious from + * the API or the used REST resources. The returned comments still have the patchset field filled. + * It contains the reference to the patchset on which the comment was originally left. That + * patchset number can vary among the returned comments as all comments before the target patchset + * are potentially eligible for porting. + * + *

The number of returned comments can be smaller (-> only eligible ones are ported!) or larger + * compared to the provided comments. The latter happens when files appear as copied in the target + * patchset. In such a situation, the same comment UUID will occur more than once in the returned + * comments. + * + * @param changeNotes the {@link ChangeNotes} of the change to which the comments belong + * @param targetPatchset the patchset to which the comments should be ported + * @param comments the original comments + * @return the ported comments, in no particular order + */ + public ImmutableList portComments( + ChangeNotes changeNotes, PatchSet targetPatchset, List comments) + throws PatchListNotAvailableException { + + ImmutableList relevantComments = filterToRelevant(comments, targetPatchset); + return portToTargetPatchset(changeNotes, targetPatchset, relevantComments); + } + + private ImmutableList filterToRelevant( + List allComments, PatchSet targetPatchset) { + return allComments.stream() + .filter(comment -> comment.key.patchSetId < targetPatchset.number()) + // TODO(aliceks): Also support comments on parent revisions. + .filter(comment -> comment.side > 0) + .collect(toImmutableList()); + } + + private ImmutableList portToTargetPatchset( + ChangeNotes notes, PatchSet targetPatchset, List comments) + throws PatchListNotAvailableException { + Map> commentsPerPatchset = + comments.stream().collect(groupingBy(comment -> comment.key.patchSetId, toImmutableList())); + + ImmutableList.Builder portedComments = + ImmutableList.builderWithExpectedSize(comments.size()); + for (Integer originalPatchsetId : commentsPerPatchset.keySet()) { + ImmutableList patchsetComments = commentsPerPatchset.get(originalPatchsetId); + PatchSet originalPatchset = + notes.getPatchSets().get(PatchSet.id(notes.getChangeId(), originalPatchsetId)); + portedComments.addAll( + portToTargetPatchset( + notes.getProjectName(), originalPatchset, targetPatchset, patchsetComments)); + } + return portedComments.build(); + } + + private ImmutableList portToTargetPatchset( + Project.NameKey project, + PatchSet originalPatchset, + PatchSet targetPatchset, + ImmutableList comments) + throws PatchListNotAvailableException { + ImmutableSet mappings = + loadPatchsetMappings(project, originalPatchset, targetPatchset); + ImmutableList> positionedComments = + comments.stream().map(this::toPositionedEntity).collect(toImmutableList()); + return positionTransformer.transform(positionedComments, mappings).stream() + .map(PositionedEntity::getEntityAtUpdatedPosition) + .collect(toImmutableList()); + } + + private ImmutableSet loadPatchsetMappings( + Project.NameKey project, PatchSet originalPatchset, PatchSet targetPatchset) + throws PatchListNotAvailableException { + PatchList patchList = + patchListCache.get( + PatchListKey.againstCommit( + originalPatchset.commitId(), targetPatchset.commitId(), Whitespace.IGNORE_NONE), + project); + return patchList.getPatches().stream().map(DiffMappings::toMapping).collect(toImmutableSet()); + } + + private PositionedEntity toPositionedEntity(HumanComment comment) { + return PositionedEntity.create( + comment, CommentPorter::extractPosition, CommentPorter::createCommentAtNewPosition); + } + + private static Position extractPosition(HumanComment comment) { + Position.Builder positionBuilder = Position.builder(); + // Patchset-level comments don't have a file path. The transformation logic still works when + // using the magic file path but it doesn't hurt to use the actual representation for "no file" + // internally. + if (!Patch.PATCHSET_LEVEL.equals(comment.key.filename)) { + positionBuilder.filePath(comment.key.filename); + } + return positionBuilder.lineRange(extractLineRange(comment)).build(); + } + + private static Optional extractLineRange(HumanComment comment) { + // Line specifications in comment are 1-based. Line specifications in Position are 0-based. + if (comment.range != null) { + // The combination of (line, charOffset) is exclusive and must be mapped to an exclusive line. + int exclusiveEndLine = + comment.range.endChar > 0 ? comment.range.endLine : comment.range.endLine - 1; + return Optional.of( + GitPositionTransformer.Range.create(comment.range.startLine - 1, exclusiveEndLine)); + } + if (comment.lineNbr > 0) { + return Optional.of(GitPositionTransformer.Range.create(comment.lineNbr - 1, comment.lineNbr)); + } + // File comment -> no range. + return Optional.empty(); + } + + private static HumanComment createCommentAtNewPosition( + HumanComment originalComment, Position newPosition) { + HumanComment portedComment = new HumanComment(originalComment); + portedComment.key.filename = newPosition.filePath().orElse(Patch.PATCHSET_LEVEL); + if (portedComment.range != null && newPosition.lineRange().isPresent()) { + // Comment was a range comment and also stayed one. + portedComment.range = + toRange( + newPosition.lineRange().get(), + portedComment.range.startChar, + portedComment.range.endChar); + portedComment.lineNbr = portedComment.range.endLine; + } else { + portedComment.range = null; + // No line -> use 0 = file comment or any other comment type without an explicit line. + portedComment.lineNbr = newPosition.lineRange().map(range -> range.start() + 1).orElse(0); + } + return portedComment; + } + + private static Range toRange( + GitPositionTransformer.Range lineRange, int originalStartChar, int originalEndChar) { + int adjustedEndLine = originalEndChar > 0 ? lineRange.end() : lineRange.end() + 1; + return new Range(lineRange.start() + 1, originalStartChar, adjustedEndLine, originalEndChar); + } +} diff --git a/java/com/google/gerrit/server/restapi/change/ListPortedComments.java b/java/com/google/gerrit/server/restapi/change/ListPortedComments.java index a3be9eb180..6494d30d6e 100644 --- a/java/com/google/gerrit/server/restapi/change/ListPortedComments.java +++ b/java/com/google/gerrit/server/restapi/change/ListPortedComments.java @@ -14,34 +14,14 @@ package com.google.gerrit.server.restapi.change; -import static com.google.common.collect.ImmutableList.toImmutableList; -import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static java.util.stream.Collectors.groupingBy; - import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; -import com.google.gerrit.entities.Comment; -import com.google.gerrit.entities.Comment.Range; 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.extensions.client.DiffPreferencesInfo.Whitespace; import com.google.gerrit.extensions.common.CommentInfo; 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.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.Mapping; -import com.google.gerrit.server.patch.GitPositionTransformer.Position; -import com.google.gerrit.server.patch.GitPositionTransformer.PositionedEntity; -import com.google.gerrit.server.patch.PatchList; -import com.google.gerrit.server.patch.PatchListCache; -import com.google.gerrit.server.patch.PatchListKey; import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.inject.Inject; @@ -49,23 +29,20 @@ import com.google.inject.Provider; import com.google.inject.Singleton; import java.util.List; import java.util.Map; -import java.util.Optional; @Singleton public class ListPortedComments implements RestReadView { - private final GitPositionTransformer positionTransformer = - new GitPositionTransformer(BestPositionOnConflict.INSTANCE); private final CommentsUtil commentsUtil; + private final CommentPorter commentPorter; private final Provider commentJson; - private final PatchListCache patchListCache; @Inject public ListPortedComments( - Provider commentJson, CommentsUtil commentsUtil, PatchListCache patchListCache) { + Provider commentJson, CommentsUtil commentsUtil, CommentPorter commentPorter) { this.commentJson = commentJson; this.commentsUtil = commentsUtil; - this.patchListCache = patchListCache; + this.commentPorter = commentPorter; } @Override @@ -73,132 +50,13 @@ public class ListPortedComments implements RestReadView { throws PermissionBackendException, PatchListNotAvailableException { PatchSet targetPatchset = revisionResource.getPatchSet(); - List allComments = loadAllPublishedComments(revisionResource); - ImmutableList relevantComments = filterToRelevant(allComments, targetPatchset); + List allComments = + commentsUtil.publishedHumanCommentsByChange(revisionResource.getNotes()); ImmutableList portedComments = - portToTargetPatchset( - revisionResource.getChangeResource().getNotes(), targetPatchset, relevantComments); + commentPorter.portComments(revisionResource.getNotes(), targetPatchset, allComments); return Response.ok(format(portedComments)); } - private List loadAllPublishedComments(RevisionResource revisionResource) { - return commentsUtil.publishedHumanCommentsByChange(revisionResource.getNotes()); - } - - private ImmutableList filterToRelevant( - List allComments, PatchSet targetPatchset) { - return allComments.stream() - .filter(comment -> comment.key.patchSetId < targetPatchset.number()) - // TODO(aliceks): Also support comments on parent revisions. - .filter(comment -> comment.side > 0) - .collect(toImmutableList()); - } - - private ImmutableList portToTargetPatchset( - ChangeNotes notes, PatchSet targetPatchset, List comments) - throws PatchListNotAvailableException { - Map> commentsPerPatchset = - comments.stream().collect(groupingBy(comment -> comment.key.patchSetId, toImmutableList())); - - ImmutableList.Builder portedComments = - ImmutableList.builderWithExpectedSize(comments.size()); - for (Integer originalPatchsetId : commentsPerPatchset.keySet()) { - ImmutableList patchsetComments = commentsPerPatchset.get(originalPatchsetId); - PatchSet originalPatchset = - notes.getPatchSets().get(PatchSet.id(notes.getChangeId(), originalPatchsetId)); - portedComments.addAll( - portToTargetPatchset( - notes.getProjectName(), originalPatchset, targetPatchset, patchsetComments)); - } - return portedComments.build(); - } - - private ImmutableList portToTargetPatchset( - Project.NameKey project, - PatchSet originalPatchset, - PatchSet targetPatchset, - ImmutableList comments) - throws PatchListNotAvailableException { - ImmutableSet mappings = - loadPatchsetMappings(project, originalPatchset, targetPatchset); - ImmutableList> positionedComments = - comments.stream().map(this::toPositionedEntity).collect(toImmutableList()); - return positionTransformer.transform(positionedComments, mappings).stream() - .map(PositionedEntity::getEntityAtUpdatedPosition) - .collect(toImmutableList()); - } - - private ImmutableSet loadPatchsetMappings( - Project.NameKey project, PatchSet originalPatchset, PatchSet targetPatchset) - throws PatchListNotAvailableException { - PatchList patchList = - patchListCache.get( - PatchListKey.againstCommit( - originalPatchset.commitId(), targetPatchset.commitId(), Whitespace.IGNORE_NONE), - project); - return patchList.getPatches().stream().map(DiffMappings::toMapping).collect(toImmutableSet()); - } - - private PositionedEntity toPositionedEntity(HumanComment comment) { - return PositionedEntity.create( - comment, - ListPortedComments::extractPosition, - ListPortedComments::createCommentAtNewPosition); - } - - private static Position extractPosition(HumanComment comment) { - Position.Builder positionBuilder = Position.builder(); - // Patchset-level comments don't have a file path. The transformation logic still works when - // using the magic file path but it doesn't hurt to use the actual representation for "no file" - // internally. - if (!Patch.PATCHSET_LEVEL.equals(comment.key.filename)) { - positionBuilder.filePath(comment.key.filename); - } - return positionBuilder.lineRange(extractLineRange(comment)).build(); - } - - private static Optional extractLineRange(HumanComment comment) { - // Line specifications in comment are 1-based. Line specifications in Position are 0-based. - if (comment.range != null) { - // The combination of (line, charOffset) is exclusive and must be mapped to an exclusive line. - int exclusiveEndLine = - comment.range.endChar > 0 ? comment.range.endLine : comment.range.endLine - 1; - return Optional.of( - GitPositionTransformer.Range.create(comment.range.startLine - 1, exclusiveEndLine)); - } - if (comment.lineNbr > 0) { - return Optional.of(GitPositionTransformer.Range.create(comment.lineNbr - 1, comment.lineNbr)); - } - // File comment -> no range. - return Optional.empty(); - } - - private static HumanComment createCommentAtNewPosition( - HumanComment originalComment, Position newPosition) { - HumanComment portedComment = new HumanComment(originalComment); - portedComment.key.filename = newPosition.filePath().orElse(Patch.PATCHSET_LEVEL); - if (portedComment.range != null && newPosition.lineRange().isPresent()) { - // Comment was a range comment and also stayed one. - portedComment.range = - toRange( - newPosition.lineRange().get(), - portedComment.range.startChar, - portedComment.range.endChar); - portedComment.lineNbr = portedComment.range.endLine; - } else { - portedComment.range = null; - // No line -> use 0 = file comment or any other comment type without an explicit line. - portedComment.lineNbr = newPosition.lineRange().map(range -> range.start() + 1).orElse(0); - } - return portedComment; - } - - private static Comment.Range toRange( - GitPositionTransformer.Range lineRange, int originalStartChar, int originalEndChar) { - int adjustedEndLine = originalEndChar > 0 ? lineRange.end() : lineRange.end() + 1; - return new Range(lineRange.start() + 1, originalStartChar, adjustedEndLine, originalEndChar); - } - private Map> format(List comments) throws PermissionBackendException { return commentJson