diff --git a/src/main/java/pl/touk/sputnik/connector/gerrit/CommentFilter.java b/src/main/java/pl/touk/sputnik/connector/gerrit/CommentFilter.java new file mode 100644 index 00000000..f9a95c6e --- /dev/null +++ b/src/main/java/pl/touk/sputnik/connector/gerrit/CommentFilter.java @@ -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(); +} diff --git a/src/main/java/pl/touk/sputnik/connector/gerrit/FileDiff.java b/src/main/java/pl/touk/sputnik/connector/gerrit/FileDiff.java new file mode 100644 index 00000000..351c8121 --- /dev/null +++ b/src/main/java/pl/touk/sputnik/connector/gerrit/FileDiff.java @@ -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 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 getModifiedLines() { + return Collections.unmodifiableSet(modifiedLines); + } +} diff --git a/src/main/java/pl/touk/sputnik/connector/gerrit/FileDiffBuilder.java b/src/main/java/pl/touk/sputnik/connector/gerrit/FileDiffBuilder.java new file mode 100644 index 00000000..c124686a --- /dev/null +++ b/src/main/java/pl/touk/sputnik/connector/gerrit/FileDiffBuilder.java @@ -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 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; + } +} diff --git a/src/main/java/pl/touk/sputnik/connector/gerrit/GerritCommentFilter.java b/src/main/java/pl/touk/sputnik/connector/gerrit/GerritCommentFilter.java new file mode 100644 index 00000000..b7735cb3 --- /dev/null +++ b/src/main/java/pl/touk/sputnik/connector/gerrit/GerritCommentFilter.java @@ -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 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 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"); + } +} diff --git a/src/main/java/pl/touk/sputnik/connector/gerrit/GerritFacade.java b/src/main/java/pl/touk/sputnik/connector/gerrit/GerritFacade.java index 2432d7d0..f21f4927 100644 --- a/src/main/java/pl/touk/sputnik/connector/gerrit/GerritFacade.java +++ b/src/main/java/pl/touk/sputnik/connector/gerrit/GerritFacade.java @@ -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; @@ -20,16 +20,13 @@ 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 @@ -37,7 +34,9 @@ public List listFiles() { try { List files = new ArrayList(); Map changeFiles = gerritApi.changes() - .id(gerritPatchset.getChangeId()).revision(gerritPatchset.getRevisionId()).files(); + .id(gerritPatchset.getChangeId()) + .revision(gerritPatchset.getRevisionId()) + .files(); for (Map.Entry changeFileEntry : changeFiles.entrySet()) { if (COMMIT_MSG.equals(changeFileEntry.getKey())) { @@ -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); @@ -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 diff --git a/src/main/java/pl/touk/sputnik/connector/gerrit/GerritFacadeBuilder.java b/src/main/java/pl/touk/sputnik/connector/gerrit/GerritFacadeBuilder.java index 9458b447..f93cc187 100644 --- a/src/main/java/pl/touk/sputnik/connector/gerrit/GerritFacadeBuilder.java +++ b/src/main/java/pl/touk/sputnik/connector/gerrit/GerritFacadeBuilder.java @@ -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; @@ -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()); @@ -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 @@ -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; + } } diff --git a/src/main/java/pl/touk/sputnik/connector/gerrit/ReviewInputBuilder.java b/src/main/java/pl/touk/sputnik/connector/gerrit/ReviewInputBuilder.java index 2855aecd..e4facca9 100644 --- a/src/main/java/pl/touk/sputnik/connector/gerrit/ReviewInputBuilder.java +++ b/src/main/java/pl/touk/sputnik/connector/gerrit/ReviewInputBuilder.java @@ -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; @@ -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(); @@ -22,6 +28,11 @@ public ReviewInput toReviewInput(@NotNull Review review) { for (ReviewFile file : review.getFiles()) { List comments = new ArrayList(); 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(); diff --git a/src/test/java/pl/touk/sputnik/connector/gerrit/FileDiffBuilderTest.java b/src/test/java/pl/touk/sputnik/connector/gerrit/FileDiffBuilderTest.java new file mode 100644 index 00000000..d4754f8e --- /dev/null +++ b/src/test/java/pl/touk/sputnik/connector/gerrit/FileDiffBuilderTest.java @@ -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 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 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 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 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 buildListMock(int size) { + List list = mock(List.class); + when(list.size()).thenReturn(size); + return list; + } + + +} \ No newline at end of file diff --git a/src/test/java/pl/touk/sputnik/connector/gerrit/FileDiffTest.java b/src/test/java/pl/touk/sputnik/connector/gerrit/FileDiffTest.java new file mode 100644 index 00000000..5bc9eee3 --- /dev/null +++ b/src/test/java/pl/touk/sputnik/connector/gerrit/FileDiffTest.java @@ -0,0 +1,32 @@ +package pl.touk.sputnik.connector.gerrit; + +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +class FileDiffTest { + + private static final String FILE_NAME = "Book.java"; + + private FileDiff fileDiff = new FileDiff(FILE_NAME); + + @Test + void shouldAddHunk() { + fileDiff.addHunk(2, 5); + + assertThat(fileDiff.getModifiedLines()).containsExactlyInAnyOrder(2, 3, 4, 5, 6); + } + + @Test + void shouldAddManyHunks() { + fileDiff.addHunk(1, 2); + fileDiff.addHunk(100, 4); + fileDiff.addHunk(50, 3); + + assertThat(fileDiff.getModifiedLines()).containsExactlyInAnyOrder( + 1, 2, + 50, 51, 52, + 100, 101, 102, 103); + } + +} \ No newline at end of file diff --git a/src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeExceptionTest.java b/src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeExceptionTest.java index e2b979a5..2e504100 100644 --- a/src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeExceptionTest.java +++ b/src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeExceptionTest.java @@ -20,11 +20,14 @@ class GerritFacadeExceptionTest { @Mock private Changes changes; + @Mock + private CommentFilter commentFilter; + @Test void shouldWrapConnectorException() throws Exception { when(gerritApi.changes()).thenReturn(changes); when(changes.id("foo")).thenThrow(new RuntimeException("Connection refused")); - GerritFacade gerritFacade = new GerritFacade(gerritApi, new GerritPatchset("foo", "bar")); + GerritFacade gerritFacade = new GerritFacade(gerritApi, new GerritPatchset("foo", "bar"), commentFilter); Throwable thrown = catchThrowable(gerritFacade::listFiles); diff --git a/src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeTest.java b/src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeTest.java index cd0f84fa..8ed7925b 100644 --- a/src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeTest.java +++ b/src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeTest.java @@ -1,7 +1,6 @@ package pl.touk.sputnik.connector.gerrit; import com.google.common.base.Charsets; -import com.google.common.collect.ImmutableMap; import com.google.common.io.Resources; import com.google.gerrit.extensions.api.GerritApi; import com.google.gerrit.extensions.api.changes.ChangeApi; @@ -18,12 +17,6 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; -import pl.touk.sputnik.configuration.Configuration; -import pl.touk.sputnik.configuration.ConfigurationSetup; -import pl.touk.sputnik.configuration.GeneralOptionNotSupportedException; -import pl.touk.sputnik.connector.ConnectorFacade; -import pl.touk.sputnik.connector.ConnectorFacadeFactory; -import pl.touk.sputnik.connector.ConnectorType; import pl.touk.sputnik.review.ReviewFile; import java.io.IOException; @@ -31,7 +24,6 @@ import java.util.Map; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.catchThrowable; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -41,27 +33,14 @@ class GerritFacadeTest { @Mock private GerritApi gerritApi; + @Mock + private CommentFilter commentFilter; + private GerritFacade gerritFacade; @BeforeEach void setUp() { - gerritFacade = new GerritFacade(gerritApi, null); - } - - @Test - void shouldNotAllowCommentOnlyChangedLines() { - Configuration config = new ConfigurationSetup().setUp(ImmutableMap.of( - "cli.changeId", "abc", - "cli.revisionId", "def", - "global.commentOnlyChangedLines", Boolean.toString(true))); - - ConnectorFacadeFactory connectionFacade = new ConnectorFacadeFactory(); - - ConnectorFacade gerritFacade = connectionFacade.build(ConnectorType.GERRIT, config); - Throwable thrown = catchThrowable(() -> gerritFacade.validate(config)); - - assertThat(thrown).isInstanceOf(GeneralOptionNotSupportedException.class).hasMessage( - "This connector does not support global.commentOnlyChangedLines"); + gerritFacade = new GerritFacade(gerritApi, null, commentFilter); } @Test @@ -88,7 +67,7 @@ private GerritFacade createGerritFacade() throws IOException, RestApiException { RevisionApi revisionApi = mock(RevisionApi.class); when(changeApi.revision("revisionId")).thenReturn(revisionApi); when(revisionApi.files()).thenReturn(fileInfoMap); - return new GerritFacade(gerritApi, new GerritPatchset("changeId", "revisionId")); + return new GerritFacade(gerritApi, new GerritPatchset("changeId", "revisionId"), commentFilter); } } \ No newline at end of file diff --git a/src/test/java/pl/touk/sputnik/connector/gerrit/ReviewInputBuilderTest.java b/src/test/java/pl/touk/sputnik/connector/gerrit/ReviewInputBuilderTest.java index 1ecdc7a1..24566767 100644 --- a/src/test/java/pl/touk/sputnik/connector/gerrit/ReviewInputBuilderTest.java +++ b/src/test/java/pl/touk/sputnik/connector/gerrit/ReviewInputBuilderTest.java @@ -1,22 +1,41 @@ package pl.touk.sputnik.connector.gerrit; import com.google.gerrit.extensions.api.changes.ReviewInput; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; import pl.touk.sputnik.ReviewBuilder; import pl.touk.sputnik.configuration.Configuration; import pl.touk.sputnik.configuration.ConfigurationBuilder; import pl.touk.sputnik.review.Review; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.when; +@ExtendWith(MockitoExtension.class) class ReviewInputBuilderTest { + @Mock + private CommentFilter commentFilter; + + private ReviewInputBuilder reviewInputBuilder; + + @BeforeEach + void setUp() { + reviewInputBuilder = new ReviewInputBuilder(commentFilter); + } + @Test void shouldBuildReviewInput() { Configuration config = ConfigurationBuilder.initFromResource("test.properties"); Review review = ReviewBuilder.buildReview(config); + when(commentFilter.include(eq("filename1"), anyInt())).thenReturn(true); - ReviewInput reviewInput = new ReviewInputBuilder().toReviewInput(review); + ReviewInput reviewInput = reviewInputBuilder.toReviewInput(review); assertThat(reviewInput.message).isEqualTo("Total 8 violations found"); assertThat(reviewInput.comments).hasSize(4); @@ -25,4 +44,21 @@ void shouldBuildReviewInput() { assertThat(reviewInput.labels.get("Code-Review")).isEqualTo((short) 1); } + @Test + void shouldBuildReviewInputWithCommentFilter() { + Configuration config = ConfigurationBuilder.initFromResource("test.properties"); + Review review = ReviewBuilder.buildReview(config); + when(commentFilter.include("filename1", 0)).thenReturn(false); + when(commentFilter.include("filename1", 1)).thenReturn(true); + + ReviewInput reviewInput = reviewInputBuilder.toReviewInput(review); + + assertThat(reviewInput.message).isEqualTo("Total 8 violations found"); + assertThat(reviewInput.comments).hasSize(4); + assertThat(reviewInput.comments.get("filename1")).hasSize(1); + assertThat(reviewInput.comments.get("filename1").get(0).line).isEqualTo(1); + assertThat(reviewInput.comments.get("filename1").get(0).message).isEqualTo("test2"); + assertThat(reviewInput.labels.get("Code-Review")).isEqualTo((short) 1); + } + } \ No newline at end of file