Skip to content

Commit

Permalink
Implements commentOnlyChangedLines for Gerrit - by @garc33 (#124) (#202)
Browse files Browse the repository at this point in the history
Implements commentOnlyChangedLines for Gerrit - by @garc33 (#124)
  • Loading branch information
SpOOnman authored Oct 15, 2019
1 parent 180e754 commit 01f0303
Show file tree
Hide file tree
Showing 12 changed files with 332 additions and 45 deletions.
20 changes: 20 additions & 0 deletions src/main/java/pl/touk/sputnik/connector/gerrit/CommentFilter.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package pl.touk.sputnik.connector.gerrit;

public interface CommentFilter {

CommentFilter EMPTY_COMMENT_FILTER = new CommentFilter() {

@Override
public boolean include(String filePath, int line) {
return true;
}

@Override
public void init() {
}
};

boolean include(String filePath, int line);

void init();
}
30 changes: 30 additions & 0 deletions src/main/java/pl/touk/sputnik/connector/gerrit/FileDiff.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package pl.touk.sputnik.connector.gerrit;

import java.util.Collections;
import java.util.HashSet;
import java.util.Set;

public class FileDiff {

private final String fileName;

private final Set<Integer> modifiedLines = new HashSet<>();

public FileDiff(String fileName) {
this.fileName = fileName;
}

public void addHunk(int firstLine, int count) {
for (int i = firstLine; i < firstLine + count; i++) {
modifiedLines.add(i);
}
}

public String getFileName() {
return fileName;
}

public Set<Integer> getModifiedLines() {
return Collections.unmodifiableSet(modifiedLines);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package pl.touk.sputnik.connector.gerrit;

import com.google.gerrit.extensions.common.DiffInfo.ContentEntry;
import lombok.extern.slf4j.Slf4j;
import org.jetbrains.annotations.NotNull;

import java.util.List;

@Slf4j
public class FileDiffBuilder {

@NotNull
public FileDiff build(@NotNull String fileKey, @NotNull List<ContentEntry> content) {
FileDiff fileDiff = new FileDiff(fileKey);
int currentLine = 1;
for (ContentEntry diffHunk : content) {
if (diffHunk.skip != null) {
currentLine += diffHunk.skip;
}
if (diffHunk.ab != null) {
currentLine += diffHunk.ab.size();
}
if (diffHunk.b != null) {
fileDiff.addHunk(currentLine, diffHunk.b.size());
currentLine += diffHunk.b.size();
}
}
return fileDiff;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package pl.touk.sputnik.connector.gerrit;

import com.google.gerrit.extensions.api.GerritApi;
import com.google.gerrit.extensions.api.changes.FileApi;
import com.google.gerrit.extensions.api.changes.RevisionApi;
import com.google.gerrit.extensions.common.FileInfo;
import com.google.gerrit.extensions.restapi.RestApiException;
import lombok.extern.slf4j.Slf4j;

import java.util.HashMap;
import java.util.Map;

@Slf4j
public class GerritCommentFilter implements CommentFilter {

private final GerritApi gerritApi;

private final GerritPatchset patchset;

private Map<String, FileDiff> modifiedLines;

public GerritCommentFilter(GerritApi gerritApi, GerritPatchset patchset) {
this.gerritApi = gerritApi;
this.patchset = patchset;
}

@Override
public boolean include(String filePath, int line) {
FileDiff diff = modifiedLines == null ? null : modifiedLines.get(filePath);
return diff != null && diff.getModifiedLines().contains(line);
}

@Override
public void init() {
log.info("Query all file diffs to only include comments on modified lines");
modifiedLines = new HashMap<>();
FileDiffBuilder fileDiffBuilder = new FileDiffBuilder();
try {
RevisionApi revision = gerritApi.changes()
.id(patchset.getChangeId())
.revision(patchset.getRevisionId());

for (Map.Entry<String, FileInfo> file : revision.files().entrySet()) {
log.info("Query file diff for {}", file.getKey());
FileApi fileApi = revision.file(file.getKey());
FileDiff fileDiff = fileDiffBuilder.build(file.getKey(), fileApi.diff().content);
modifiedLines.put(fileDiff.getFileName(), fileDiff);
log.info("Query file diff for {} finished", file.getKey());
}
} catch (RestApiException e) {
throw new GerritException("Error when retrieve modified lines", e);
}
log.info("Query all file diffs finished");
}
}
26 changes: 10 additions & 16 deletions src/main/java/pl/touk/sputnik/connector/gerrit/GerritFacade.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@
import com.google.gerrit.extensions.api.GerritApi;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.common.FileInfo;
import lombok.AllArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.jetbrains.annotations.NotNull;
import pl.touk.sputnik.configuration.Configuration;
import pl.touk.sputnik.configuration.GeneralOption;
import pl.touk.sputnik.configuration.GeneralOptionNotSupportedException;
import pl.touk.sputnik.connector.ConnectorFacade;
import pl.touk.sputnik.connector.ConnectorValidator;
Expand All @@ -20,24 +20,23 @@
import java.util.Map;

@Slf4j
@AllArgsConstructor
public class GerritFacade implements ConnectorFacade, ConnectorValidator, ReviewPublisher {
private static final String COMMIT_MSG = "/COMMIT_MSG";

private final GerritApi gerritApi;
private final GerritPatchset gerritPatchset;

public GerritFacade(GerritApi gerritApi, GerritPatchset gerritPatchset) {
this.gerritApi = gerritApi;
this.gerritPatchset = gerritPatchset;
}
private final CommentFilter commentFilter;

@NotNull
@Override
public List<ReviewFile> listFiles() {
try {
List<ReviewFile> files = new ArrayList<ReviewFile>();
Map<String, FileInfo> changeFiles = gerritApi.changes()
.id(gerritPatchset.getChangeId()).revision(gerritPatchset.getRevisionId()).files();
.id(gerritPatchset.getChangeId())
.revision(gerritPatchset.getRevisionId())
.files();

for (Map.Entry<String, FileInfo> changeFileEntry : changeFiles.entrySet()) {
if (COMMIT_MSG.equals(changeFileEntry.getKey())) {
Expand All @@ -63,8 +62,10 @@ private boolean isDeleted(FileInfo fileInfo) {
public void publish(@NotNull Review review) {
try {
log.debug("Set review in Gerrit: {}", review);
ReviewInput reviewInput = new ReviewInputBuilder().toReviewInput(review);
gerritApi.changes().id(gerritPatchset.getChangeId()).revision(gerritPatchset.getRevisionId())
ReviewInput reviewInput = new ReviewInputBuilder(commentFilter).toReviewInput(review);
gerritApi.changes()
.id(gerritPatchset.getChangeId())
.revision(gerritPatchset.getRevisionId())
.review(reviewInput);
} catch (Throwable e) {
throw new GerritException("Error when setting review", e);
Expand All @@ -78,13 +79,6 @@ public Connectors name() {

@Override
public void validate(Configuration configuration) throws GeneralOptionNotSupportedException {
boolean commentOnlyChangedLines = Boolean.parseBoolean(configuration
.getProperty(GeneralOption.COMMENT_ONLY_CHANGED_LINES));

if (commentOnlyChangedLines) {
throw new GeneralOptionNotSupportedException("This connector does not support "
+ GeneralOption.COMMENT_ONLY_CHANGED_LINES.getKey());
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@
import com.urswolfer.gerrit.client.rest.GerritRestApiFactory;
import com.urswolfer.gerrit.client.rest.http.HttpClientBuilderExtension;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang3.BooleanUtils;
import org.apache.http.impl.client.HttpClientBuilder;
import org.jetbrains.annotations.NotNull;
import pl.touk.sputnik.configuration.CliOption;
import pl.touk.sputnik.configuration.Configuration;
import pl.touk.sputnik.configuration.GeneralOption;
import pl.touk.sputnik.connector.ConnectorDetails;
import pl.touk.sputnik.connector.http.HttpHelper;

Expand All @@ -30,6 +32,7 @@ public GerritFacade build(Configuration configuration) {
if (!Strings.isNullOrEmpty(connectorDetails.getPath())) {
hostUri += connectorDetails.getPath();
}

log.info("Using Gerrit URL: {}", hostUri);
GerritAuthData.Basic authData = new GerritAuthData.Basic(hostUri,
connectorDetails.getUsername(), connectorDetails.getPassword());
Expand All @@ -42,7 +45,9 @@ public HttpClientBuilder extend(HttpClientBuilder httpClientBuilder, GerritAuthD
}
});

return new GerritFacade(gerritApi, gerritPatchset);
CommentFilter commentFilter = buildCommentFilter(configuration, gerritPatchset, gerritApi);

return new GerritFacade(gerritApi, gerritPatchset, commentFilter);
}

@NotNull
Expand All @@ -55,4 +60,12 @@ private GerritPatchset buildGerritPatchset(Configuration configuration) {

return new GerritPatchset(changeId, revisionId);
}

@NotNull
private CommentFilter buildCommentFilter(Configuration configuration, GerritPatchset gerritPatchset, GerritApi gerritApi) {
boolean commentOnlyChangedLines = BooleanUtils.toBoolean(configuration.getProperty(GeneralOption.COMMENT_ONLY_CHANGED_LINES));
CommentFilter commentFilter = commentOnlyChangedLines ? new GerritCommentFilter(gerritApi, gerritPatchset) : CommentFilter.EMPTY_COMMENT_FILTER;
commentFilter.init();
return commentFilter;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import com.google.common.base.Joiner;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import lombok.AllArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.jetbrains.annotations.NotNull;
import pl.touk.sputnik.review.Comment;
import pl.touk.sputnik.review.Review;
Expand All @@ -11,8 +13,12 @@
import java.util.HashMap;
import java.util.List;

@AllArgsConstructor
@Slf4j
public class ReviewInputBuilder {

private final CommentFilter commentFilter;

@NotNull
public ReviewInput toReviewInput(@NotNull Review review) {
ReviewInput reviewInput = new ReviewInput();
Expand All @@ -22,6 +28,11 @@ public ReviewInput toReviewInput(@NotNull Review review) {
for (ReviewFile file : review.getFiles()) {
List<ReviewInput.CommentInput> comments = new ArrayList<ReviewInput.CommentInput>();
for (Comment comment : file.getComments()) {
if (!(commentFilter.include(file.getReviewFilename(), comment.getLine()))) {
log.info("Comment excluded in file {}: line {}, message {}",
file.getReviewFilename(), comment.getLine(), comment.getMessage());
continue;
}
ReviewInput.CommentInput commentInput = new ReviewInput.CommentInput();
commentInput.line = comment.getLine();
commentInput.message = comment.getMessage();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
package pl.touk.sputnik.connector.gerrit;

import com.google.gerrit.extensions.common.DiffInfo;
import org.junit.jupiter.api.Test;

import java.util.ArrayList;
import java.util.List;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

class FileDiffBuilderTest {

private static final String FILE_KEY = "test.java";

@Test
void shouldMoveWithHunkSkip() {
List<DiffInfo.ContentEntry> content = new ArrayList<>();
content.add(buildContentEntry(0, 0, 0, 10));
content.add(buildContentEntry(0, 0, 2, 0));

FileDiff fileDiff = new FileDiffBuilder().build(FILE_KEY, content);

assertThat(fileDiff.getFileName()).isEqualTo(FILE_KEY);
assertThat(fileDiff.getModifiedLines()).containsExactlyInAnyOrder(11, 12);
}

@Test
void shouldNotMoveWithHunkA() {
List<DiffInfo.ContentEntry> content = new ArrayList<>();
content.add(buildContentEntry(10, 0, 0, 0));
content.add(buildContentEntry(0, 0, 2, 0));

FileDiff fileDiff = new FileDiffBuilder().build(FILE_KEY, content);

assertThat(fileDiff.getFileName()).isEqualTo(FILE_KEY);
assertThat(fileDiff.getModifiedLines()).containsExactlyInAnyOrder(1, 2);
}

@Test
void shouldMoveWithHunkAB() {
List<DiffInfo.ContentEntry> content = new ArrayList<>();
content.add(buildContentEntry(0, 10, 0, 0));
content.add(buildContentEntry(0, 0, 2, 0));

FileDiff fileDiff = new FileDiffBuilder().build(FILE_KEY, content);

assertThat(fileDiff.getFileName()).isEqualTo(FILE_KEY);
assertThat(fileDiff.getModifiedLines()).containsExactlyInAnyOrder(11, 12);
}

@Test
void shouldComputeComplexHunks() {
List<DiffInfo.ContentEntry> content = new ArrayList<>();
content.add(buildContentEntry(10, 0, 5, 1));
content.add(buildContentEntry(3, 9, 2, 20));

FileDiff fileDiff = new FileDiffBuilder().build(FILE_KEY, content);

assertThat(fileDiff.getFileName()).isEqualTo(FILE_KEY);
assertThat(fileDiff.getModifiedLines()).containsExactlyInAnyOrder(2, 3, 4, 5, 6, 36, 37);
}

/**
* We build ContentEntry instance because it is a final class
*/
private DiffInfo.ContentEntry buildContentEntry(int aSize, int abSize, int bSize, int skip) {
DiffInfo.ContentEntry diffHunk = new DiffInfo.ContentEntry();
diffHunk.a = buildListMock(aSize);
diffHunk.ab = buildListMock(abSize);
diffHunk.b = buildListMock(bSize);
diffHunk.skip = skip;
return diffHunk;
}

private List<String> buildListMock(int size) {
List<String> list = mock(List.class);
when(list.size()).thenReturn(size);
return list;
}


}
Loading

0 comments on commit 01f0303

Please sign in to comment.