opendev/gerrit
Code Issues Proposed changes

Merge changes I705bce78,I2d44fb85,I0ba61957

Browse Source 
* changes:
  Configure maximum allowed value for the number of context padding lines
  Add the context-padding parameter to the comment context
  Refactor CommentContextLoader to only take the required fields
This commit is contained in:
Youssef Elghareeb
2021-02-09 10:01:54 +00:00
committed by Gerrit Code Review
parent eb029c4da6 80bc394ddc
commit 31780a1e56
11 changed files with 293 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
Documentation/rest-api-changes.txt
View File

@@ -2056,6 +2056,10 @@ 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.
The `context-padding` request parameter can be used to specify an extra number
of context lines to be added before and after the comment range. This parameter
only works if `enable-context` is set to true.
.Request
----
GET /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/comments HTTP/1.0

10
java/com/google/gerrit/extensions/api/changes/ChangeApi.java
View File

@@ -413,6 +413,7 @@ public interface ChangeApi {
abstract class CommentsRequest {
private boolean enableContext;
private int contextPadding;
/**
* Get all published comments on a change.
@@ -436,6 +437,11 @@ public interface ChangeApi {
return this;
}
public CommentsRequest contextPadding(int contextPadding) {
this.contextPadding = contextPadding;
return this;
}
public CommentsRequest withContext() {
this.enableContext = true;
return this;
@@ -444,6 +450,10 @@ public interface ChangeApi {
public boolean getContext() {
return enableContext;
}
public int getContextPadding() {
return contextPadding;
}
}
abstract class SuggestedReviewersRequest {

2
java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
View File

@@ -612,6 +612,7 @@ class ChangeApiImpl implements ChangeApi {
try {
ListChangeComments listComments = listCommentsProvider.get();
listComments.setContext(this.getContext());
listComments.setContextPadding(this.getContextPadding());
return listComments.apply(change).value();
} catch (Exception e) {
throw asRestApiException("Cannot get comments", e);
@@ -623,6 +624,7 @@ class ChangeApiImpl implements ChangeApi {
try {
ListChangeComments listComments = listCommentsProvider.get();
listComments.setContext(this.getContext());
listComments.setContextPadding(this.getContextPadding());
return listComments.getComments(change);
} catch (Exception e) {
throw asRestApiException("Cannot get comments", e);

46
java/com/google/gerrit/server/comment/CommentContextCacheImpl.java
View File

@@ -16,6 +16,7 @@ package com.google.gerrit.server.comment;
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.cache.Weigher;
@@ -23,6 +24,7 @@ import com.google.common.collect.ImmutableList;
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.common.hash.Hashing;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Comment;
@@ -36,6 +38,7 @@ import com.google.gerrit.server.cache.CacheModule;
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;
@@ -50,14 +53,22 @@ import java.util.stream.Collectors;
/** Implementation of {@link CommentContextCache}. */
public class CommentContextCacheImpl implements CommentContextCache {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private static final String CACHE_NAME = "comment_context";
/**
* Comment context is expected to contain just few lines of code to be displayed beside the
* comment. Setting an upper bound of 100 for padding.
*/
@VisibleForTesting public static final int MAX_CONTEXT_PADDING = 50;
public static Module module() {
return new CacheModule() {
@Override
protected void configure() {
persist(CACHE_NAME, CommentContextKey.class, CommentContext.class)
.version(1)
.version(2)
.diskLimit(1 << 30) // limit the total cache size to 1 GB
.maximumWeight(1 << 23) // Limit the size of the in-memory cache to 8 MB
.weigher(CommentContextWeigher.class)
@@ -88,9 +99,14 @@ public class CommentContextCacheImpl implements CommentContextCache {
Iterable<CommentContextKey> inputKeys) {
ImmutableMap.Builder<CommentContextKey, CommentContext> result = ImmutableMap.builder();
List<CommentContextKey> adjustedKeys =
Streams.stream(inputKeys)
.map(CommentContextCacheImpl::adjustMaxContextPadding)
.collect(ImmutableList.toImmutableList());
// Convert the input keys to the same keys but with their file paths hashed
Map<CommentContextKey, CommentContextKey> keysToCacheKeys =
Streams.stream(inputKeys)
adjustedKeys.stream()
.collect(
Collectors.toMap(
Function.identity(),
@@ -101,7 +117,7 @@ public class CommentContextCacheImpl implements CommentContextCache {
contextCache.getAll(keysToCacheKeys.values());
for (CommentContextKey inputKey : inputKeys) {
CommentContextKey cacheKey = keysToCacheKeys.get(inputKey);
CommentContextKey cacheKey = keysToCacheKeys.get(adjustMaxContextPadding(inputKey));
result.put(inputKey, allContext.get(cacheKey));
}
return result.build();
@@ -110,6 +126,23 @@ public class CommentContextCacheImpl implements CommentContextCache {
}
}
private static CommentContextKey adjustMaxContextPadding(CommentContextKey key) {
if (key.contextPadding() < 0) {
logger.atWarning().log(
"Cannot set context padding to a negative number %d. Adjusting the number to 0",
key.contextPadding());
return key.toBuilder().contextPadding(0).build();
}
if (key.contextPadding() > MAX_CONTEXT_PADDING) {
logger.atWarning().log(
"Number of requested context lines is %d and exceeding the configured maximum of %d."
+ " Adjusting the number to the maximum.",
key.contextPadding(), MAX_CONTEXT_PADDING);
return key.toBuilder().contextPadding(MAX_CONTEXT_PADDING).build();
}
return key;
}
public enum CommentContextSerializer implements CacheSerializer<CommentContext> {
INSTANCE;
@@ -216,11 +249,12 @@ public class CommentContextCacheImpl implements CommentContextCache {
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.contextPadding()), 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));
}

7
java/com/google/gerrit/server/comment/CommentContextKey.java
View File

@@ -28,6 +28,9 @@ public abstract class CommentContextKey {
abstract Integer patchset();
/** Number of extra lines of context that should be added before and after the comment range. */
abstract int contextPadding();
abstract Builder toBuilder();
public static Builder builder() {
@@ -47,6 +50,8 @@ public abstract class CommentContextKey {
public abstract Builder patchset(Integer patchset);
public abstract Builder contextPadding(Integer numLines);
public abstract CommentContextKey build();
}
@@ -62,6 +67,7 @@ public abstract class CommentContextKey {
.setPatchset(key.patchset())
.setPathHash(key.path())
.setCommentId(key.id())
.setContextPadding(key.contextPadding())
.build());
}
@@ -75,6 +81,7 @@ public abstract class CommentContextKey {
.patchset(proto.getPatchset())
.id(proto.getCommentId())
.path(proto.getPathHash())
.contextPadding(proto.getContextPadding())
.build();
}
}

156
java/com/google/gerrit/server/comment/CommentContextLoader.java
View File

@@ -21,8 +21,8 @@ import static java.util.stream.Collectors.groupingBy;
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.gerrit.server.patch.Text;
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,51 @@ public class CommentContextLoader {
}
/**
* 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(), contextInput.contextPadding()));
break;
case MERGE_LIST:
result.put(
comment, getContextForMergeList(rw.getObjectReader(), commit, range.get()));
contextInput,
getContextForMergeList(
rw.getObjectReader(), commit, range.get(), contextInput.contextPadding()));
break;
default:
result.put(comment, getContextForFilePath(repo, rw, commit, filePath, range.get()));
result.put(
contextInput,
getContextForFilePath(
repo, rw, commit, filePath, range.get(), contextInput.contextPadding()));
}
}
}
@@ -111,20 +120,27 @@ public class CommentContextLoader {
}
private CommentContext getContextForCommitMessage(
ObjectReader reader, RevCommit commit, Range range) throws IOException {
ObjectReader reader, RevCommit commit, Range commentRange, int contextPadding)
throws IOException {
Text text = Text.forCommit(reader, commit);
return createContext(text, range);
return createContext(text, commentRange, contextPadding);
}
private CommentContext getContextForMergeList(ObjectReader reader, RevCommit commit, Range range)
private CommentContext getContextForMergeList(
ObjectReader reader, RevCommit commit, Range commentRange, int contextPadding)
throws IOException {
ComparisonType cmp = ComparisonType.againstParent(1);
Text text = Text.forMergeList(cmp, reader, commit);
return createContext(text, range);
return createContext(text, commentRange, contextPadding);
}
private CommentContext getContextForFilePath(
Repository repo, RevWalk rw, RevCommit commit, String filePath, Range range)
Repository repo,
RevWalk rw,
RevCommit commit,
String filePath,
Range commentRange,
int contextPadding)
throws IOException {
// TODO(ghareeb): We can further group the comments by file paths to avoid opening
// the same file multiple times.
@@ -136,28 +152,43 @@ public class CommentContextLoader {
}
ObjectId id = tw.getObjectId(0);
Text src = new Text(repo.open(id, Constants.OBJ_BLOB));
return createContext(src, range);
return createContext(src, commentRange, contextPadding);
}
}
private static CommentContext createContext(Text src, Range range) {
if (range.start() < 1 || range.end() > src.size()) {
private static CommentContext createContext(Text src, Range commentRange, int contextPadding) {
if (commentRange.start() < 1 || commentRange.end() - 1 > src.size()) {
throw new StorageException(
"Invalid comment range " + range + ". Text only contains " + src.size() + " lines.");
"Invalid comment range "
+ commentRange
+ ". Text only contains "
+ src.size()
+ " lines.");
}
commentRange = adjustRange(commentRange, contextPadding, src.size());
ImmutableMap.Builder<Integer, String> context =
ImmutableMap.builderWithExpectedSize(range.end() - range.start());
for (int i = range.start(); i < range.end(); i++) {
ImmutableMap.builderWithExpectedSize(commentRange.end() - commentRange.start());
for (int i = commentRange.start(); i < commentRange.end(); i++) {
context.put(i, src.getString(i - 1));
}
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));
/**
* Adjust the {@code commentRange} parameter by adding {@code contextPadding} lines before and
* after the comment range.
*/
private static Range adjustRange(Range commentRange, int contextPadding, int fileLines) {
int newStartLine = commentRange.start() - contextPadding;
int newEndLine = commentRange.end() + contextPadding;
return Range.create(Math.max(1, newStartLine), Math.min(fileLines + 1, newEndLine));
}
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();
}
@@ -173,5 +204,62 @@ public class CommentContextLoader {
/** End line of the comment (exclusive). */
abstract int end();
/** Number of lines covered by this range. */
int size() {
return end() - start();
}
}
/** This entity only contains comment fields needed to load the comment context. */
@AutoValue
abstract static class ContextInput {
static ContextInput fromComment(Comment comment, int contextPadding) {
return new AutoValue_CommentContextLoader_ContextInput.Builder()
.commitId(comment.getCommitId())
.filePath(comment.key.filename)
.range(comment.range)
.lineNumber(comment.lineNbr)
.contextPadding(contextPadding)
.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();
/** Number of extra lines of context that should be added before and after the comment range. */
abstract Integer contextPadding();
@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 Builder contextPadding(Integer contextPadding);
public abstract ContextInput build();
}
}
}

7
java/com/google/gerrit/server/restapi/change/CommentJson.java
View File

@@ -61,6 +61,7 @@ public class CommentJson {
private boolean fillAccounts = true;
private boolean fillPatchSet;
private boolean fillCommentContext;
private int contextPadding;
@Inject
CommentJson(AccountLoader.Factory accountLoaderFactory, CommentContextCache commentContextCache) {
@@ -83,6 +84,11 @@ public class CommentJson {
return this;
}
CommentJson setContextPadding(int contextPadding) {
this.contextPadding = contextPadding;
return this;
}
CommentJson setProjectKey(Project.NameKey project) {
this.project = project;
return this;
@@ -184,6 +190,7 @@ public class CommentJson {
.id(r.id)
.path(r.path)
.patchset(r.patchSet)
.contextPadding(contextPadding)
.build();
}

12
java/com/google/gerrit/server/restapi/change/ListChangeComments.java
View File

@@ -42,6 +42,7 @@ public class ListChangeComments implements RestReadView<ChangeResource> {
private final CommentsUtil commentsUtil;
private boolean includeContext;
private int contextPadding;
/**
* Optional parameter. If set, the contextLines field of the {@link ContextLineInfo} of the
@@ -54,6 +55,16 @@ public class ListChangeComments implements RestReadView<ChangeResource> {
this.includeContext = context;
}
/**
* Optional parameter. Works only if {@link #includeContext} is set to true. If {@link
* #contextPadding} is set, the context lines in the response will be padded with {@link
* #contextPadding} extra lines before and after the comment range.
*/
@Option(name = "--context-padding")
public void setContextPadding(int contextPadding) {
this.contextPadding = contextPadding;
}
@Inject
ListChangeComments(
ChangeData.Factory changeDataFactory,
@@ -105,6 +116,7 @@ public class ListChangeComments implements RestReadView<ChangeResource> {
.setFillAccounts(true)
.setFillPatchSet(true)
.setFillCommentContext(includeContext)
.setContextPadding(contextPadding)
.setProjectKey(rsrc.getProject())
.setChangeId(rsrc.getId())
.newHumanCommentFormatter();

85
javatests/com/google/gerrit/acceptance/server/change/CommentContextIT.java
View File

@@ -37,6 +37,7 @@ import com.google.inject.Inject;
import java.util.ArrayList;
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import org.junit.Before;
import org.junit.Test;
@@ -286,6 +287,90 @@ public class CommentContextIT extends AbstractDaemonTest {
assertThat(comments.get(0).contextLines).isEmpty();
}
@Test
public void commentContextWithZeroPadding() throws Exception {
String changeId = createChangeWithComment(3, 4);
assertContextLines(changeId, /* contextPadding= */ 0, ImmutableList.of(3, 4));
}
@Test
public void commentContextWithSmallPadding() throws Exception {
String changeId = createChangeWithComment(3, 4);
assertContextLines(changeId, /* contextPadding= */ 1, ImmutableList.of(2, 3, 4, 5));
}
@Test
public void commentContextWithSmallPaddingAtTheBeginningOfFile() throws Exception {
String changeId = createChangeWithComment(1, 2);
assertContextLines(changeId, /* contextPadding= */ 2, ImmutableList.of(1, 2, 3, 4));
}
@Test
public void commentContextWithPaddingLargerThanFileSize() throws Exception {
String changeId = createChangeWithComment(3, 3);
assertContextLines(
changeId,
/* contextPadding= */ 20,
ImmutableList.of(1, 2, 3, 4, 5, 6)); // file only contains six lines.
}
@Test
public void commentContextWithLargePaddingReturnsAdjustedMaximumPadding() throws Exception {
String changeId = createChangeWithCommentLarge(250, 250);
assertContextLines(
changeId,
/* contextPadding= */ 300,
IntStream.range(200, 301).boxed().collect(ImmutableList.toImmutableList()));
}
private String createChangeWithComment(int startLine, int endLine) throws Exception {
PushOneCommit.Result result =
createChange(testRepo, "master", SUBJECT, FILE_NAME, FILE_CONTENT, "topic");
String changeId = result.getChangeId();
String ps1 = result.getCommit().name();
Comment.Range commentRange = createCommentRange(startLine, endLine);
CommentInput comment =
CommentsUtil.newComment(FILE_NAME, Side.REVISION, commentRange, "comment", false);
CommentsUtil.addComments(gApi, changeId, ps1, comment);
return changeId;
}
private String createChangeWithCommentLarge(int startLine, int endLine) throws Exception {
StringBuilder largeContent = new StringBuilder();
for (int i = 0; i < 1000; i++) {
largeContent.append("line " + i + "\n");
}
PushOneCommit.Result result =
createChange(testRepo, "master", SUBJECT, FILE_NAME, largeContent.toString(), "topic");
String changeId = result.getChangeId();
String ps1 = result.getCommit().name();
Comment.Range commentRange = createCommentRange(startLine, endLine);
CommentInput comment =
CommentsUtil.newComment(FILE_NAME, Side.REVISION, commentRange, "comment", false);
CommentsUtil.addComments(gApi, changeId, ps1, comment);
return changeId;
}
private void assertContextLines(
String changeId, int contextPadding, ImmutableList<Integer> expectedLines) throws Exception {
List<CommentInfo> comments =
gApi.changes()
.id(changeId)
.commentsRequest()
.withContext(true)
.contextPadding(contextPadding)
.getAsList();
assertThat(comments).hasSize(1);
assertThat(
comments.get(0).contextLines.stream()
.map(c -> c.lineNumber)
.collect(Collectors.toList()))
.containsExactlyElementsIn(expectedLines);
}
private Comment.Range createCommentRange(int startLine, int endLine) {
Comment.Range range = new Comment.Range();
range.startLine = startLine;

1
javatests/com/google/gerrit/server/cache/serialize/CommentContextSerializerTest.java vendored
View File

@@ -34,6 +34,7 @@ public class CommentContextSerializerTest {
.id("commentId")
.path("pathHash")
.patchset(1)
.contextPadding(3)
.build();
byte[] serialized = CommentContextKey.Serializer.INSTANCE.serialize(k);
assertThat(k).isEqualTo(CommentContextKey.Serializer.INSTANCE.deserialize(serialized));

4
proto/cache.proto
View File

@@ -511,7 +511,7 @@ message ProjectCacheKeyProto {
}
// Serialized form of com.google.gerrit.server.comment.CommentContextCacheImpl.Key
// Next ID: 6
// Next ID: 7
message CommentContextKeyProto {
string project = 1;
string change_id = 2;
@@ -520,6 +520,8 @@ message CommentContextKeyProto {
// hashed with the murmur3_128 hash function
string path_hash = 5;
int32 context_padding = 6;
}
// Serialized form of a list of com.google.gerrit.extensions.common.ContextLineInfo