Refactor CommentContextLoader to only take the required fields Previously the comment context loader used to receive a list of Comment objects. This is not necessary since the loader only needs few fields, namely the commit ID, file path and comment range (start/end lines). This refactoring is also useful to support passing additional args to the loader, for example requesting a specific number of lines. I'll add this in a follow-up change. Change-Id: I0ba61957f40768f33c6f91b1e86630fedb6b85a4
diff --git a/java/com/google/gerrit/server/comment/CommentContextCacheImpl.java b/java/com/google/gerrit/server/comment/CommentContextCacheImpl.java index a0ab398..02a25b0 100644 --- a/java/com/google/gerrit/server/comment/CommentContextCacheImpl.java +++ b/java/com/google/gerrit/server/comment/CommentContextCacheImpl.java
@@ -36,6 +36,7 @@ import com.google.gerrit.server.cache.proto.Cache.AllCommentContextProto; import com.google.gerrit.server.cache.proto.Cache.AllCommentContextProto.CommentContextProto; import com.google.gerrit.server.cache.serialize.CacheSerializer; +import com.google.gerrit.server.comment.CommentContextLoader.ContextInput; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.inject.Inject; import com.google.inject.Module; @@ -216,11 +217,12 @@ ChangeNotes notes = notesFactory.createChecked(project, changeId); List<HumanComment> humanComments = commentsUtil.publishedHumanCommentsByChange(notes); CommentContextLoader loader = factory.create(project); - Map<Comment, CommentContextKey> commentsToKeys = new HashMap<>(); + Map<ContextInput, CommentContextKey> commentsToKeys = new HashMap<>(); for (CommentContextKey key : keys) { - commentsToKeys.put(getCommentForKey(humanComments, key), key); + Comment comment = getCommentForKey(humanComments, key); + commentsToKeys.put(ContextInput.fromComment(comment), key); } - Map<Comment, CommentContext> allContext = loader.getContext(commentsToKeys.keySet()); + Map<ContextInput, CommentContext> allContext = loader.getContext(commentsToKeys.keySet()); return allContext.entrySet().stream() .collect(Collectors.toMap(e -> commentsToKeys.get(e.getKey()), Map.Entry::getValue)); }
diff --git a/java/com/google/gerrit/server/comment/CommentContextLoader.java b/java/com/google/gerrit/server/comment/CommentContextLoader.java index f642438..46c4307 100644 --- a/java/com/google/gerrit/server/comment/CommentContextLoader.java +++ b/java/com/google/gerrit/server/comment/CommentContextLoader.java
@@ -21,8 +21,8 @@ import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; -import com.google.common.collect.Streams; import com.google.common.flogger.FluentLogger; +import com.google.gerrit.common.Nullable; import com.google.gerrit.entities.Comment; import com.google.gerrit.entities.CommentContext; import com.google.gerrit.entities.Project; @@ -34,6 +34,7 @@ import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; import java.io.IOException; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Optional; @@ -66,43 +67,46 @@ } /** - * Load the comment context for multiple comments at once. This method will open the repository - * and read the source files for all necessary comments' file paths. + * Load the comment context for multiple contextInputs at once. This method will open the + * repository and read the source files for all necessary contextInputs' file paths. * - * @param comments a list of comments. - * @return a Map where all entries consist of the input comments and the values are their + * @param contextInputs a list of contextInputs. + * @return a Map where all entries consist of the input contextInputs and the values are their * corresponding {@link CommentContext}. */ - public Map<Comment, CommentContext> getContext(Iterable<Comment> comments) throws IOException { - ImmutableMap.Builder<Comment, CommentContext> result = - ImmutableMap.builderWithExpectedSize(Iterables.size(comments)); + public Map<ContextInput, CommentContext> getContext(Collection<ContextInput> contextInputs) + throws IOException { + ImmutableMap.Builder<ContextInput, CommentContext> result = + ImmutableMap.builderWithExpectedSize(Iterables.size(contextInputs)); - // Group comments by commit ID so that each commit is parsed only once - Map<ObjectId, List<Comment>> commentsByCommitId = - Streams.stream(comments).collect(groupingBy(Comment::getCommitId)); + // Group contextInputs by commit ID so that each commit is parsed only once + Map<ObjectId, List<ContextInput>> commentsByCommitId = + contextInputs.stream().collect(groupingBy(ContextInput::commitId)); try (Repository repo = repoManager.openRepository(project); RevWalk rw = new RevWalk(repo)) { for (ObjectId commitId : commentsByCommitId.keySet()) { RevCommit commit = rw.parseCommit(commitId); - for (Comment comment : commentsByCommitId.get(commitId)) { - Optional<Range> range = getStartAndEndLines(comment); + for (ContextInput contextInput : commentsByCommitId.get(commitId)) { + Optional<Range> range = getStartAndEndLines(contextInput); if (!range.isPresent()) { - result.put(comment, CommentContext.empty()); + result.put(contextInput, CommentContext.empty()); continue; } - String filePath = comment.key.filename; + String filePath = contextInput.filePath(); switch (filePath) { case COMMIT_MSG: result.put( - comment, getContextForCommitMessage(rw.getObjectReader(), commit, range.get())); + contextInput, + getContextForCommitMessage(rw.getObjectReader(), commit, range.get())); break; case MERGE_LIST: result.put( - comment, getContextForMergeList(rw.getObjectReader(), commit, range.get())); + contextInput, getContextForMergeList(rw.getObjectReader(), commit, range.get())); break; default: - result.put(comment, getContextForFilePath(repo, rw, commit, filePath, range.get())); + result.put( + contextInput, getContextForFilePath(repo, rw, commit, filePath, range.get())); } } } @@ -153,11 +157,11 @@ return CommentContext.create(context.build()); } - private static Optional<Range> getStartAndEndLines(Comment comment) { - if (comment.range != null) { - return Optional.of(Range.create(comment.range.startLine, comment.range.endLine + 1)); - } else if (comment.lineNbr > 0) { - return Optional.of(Range.create(comment.lineNbr, comment.lineNbr + 1)); + private static Optional<Range> getStartAndEndLines(ContextInput comment) { + if (comment.range() != null) { + return Optional.of(Range.create(comment.range().startLine, comment.range().endLine + 1)); + } else if (comment.lineNumber() > 0) { + return Optional.of(Range.create(comment.lineNumber(), comment.lineNumber() + 1)); } return Optional.empty(); } @@ -174,4 +178,50 @@ /** End line of the comment (exclusive). */ abstract int end(); } + + /** This entity only contains comment fields needed to load the comment context. */ + @AutoValue + abstract static class ContextInput { + static ContextInput fromComment(Comment comment) { + return new AutoValue_CommentContextLoader_ContextInput.Builder() + .commitId(comment.getCommitId()) + .filePath(comment.key.filename) + .range(comment.range) + .lineNumber(comment.lineNbr) + .build(); + } + + /** 20 bytes SHA-1 of the patchset commit containing the file where the comment is written. */ + abstract ObjectId commitId(); + + /** File path where the comment is written. */ + abstract String filePath(); + + /** + * Position of the comment in the file (start line, start char, end line, end char). This field + * can be null if the range is not available for this comment. + */ + @Nullable + abstract Comment.Range range(); + + /** + * The 1-based line number where the comment is written. A value 0 means that the line number is + * not available for this comment. + */ + abstract Integer lineNumber(); + + @AutoValue.Builder + public abstract static class Builder { + + public abstract Builder commitId(ObjectId commitId); + + public abstract Builder filePath(String filePath); + + public abstract Builder range(@Nullable Comment.Range range); + + public abstract Builder lineNumber(Integer lineNumber); + + public abstract ContextInput build(); + } + } }