diff --git a/src/main/java/pl/touk/sputnik/configuration/GeneralOption.java b/src/main/java/pl/touk/sputnik/configuration/GeneralOption.java index 0d3bb6a2..04bf18d5 100644 --- a/src/main/java/pl/touk/sputnik/configuration/GeneralOption.java +++ b/src/main/java/pl/touk/sputnik/configuration/GeneralOption.java @@ -8,6 +8,7 @@ public enum GeneralOption implements ConfigurationOption { PROCESS_TEST_FILES("global.processTestFiles", "Process test files?", "true"), MAX_NUMBER_OF_COMMENTS("global.maxNumberOfComments", "Maximum number of comments to submit", "0"), + COMMENT_ONLY_CHANGED_LINES("global.commentOnlyChangedLines", "Comment only changed lines and context", "false"), CONNECTOR_TYPE("connector.type", "Connector: ", "gerrit"), HOST("connector.host", "Connector server host", "localhost"), diff --git a/src/main/java/pl/touk/sputnik/connector/gerrit/json/ReviewFileComment.java b/src/main/java/pl/touk/sputnik/connector/gerrit/json/ReviewFileComment.java index ac3a7687..f85a0b24 100644 --- a/src/main/java/pl/touk/sputnik/connector/gerrit/json/ReviewFileComment.java +++ b/src/main/java/pl/touk/sputnik/connector/gerrit/json/ReviewFileComment.java @@ -1,5 +1,6 @@ package pl.touk.sputnik.connector.gerrit.json; +import lombok.Getter; import lombok.ToString; /** @@ -7,6 +8,7 @@ * Used with JSON marshaller only. */ @ToString +@Getter public class ReviewFileComment { public String message; diff --git a/src/main/java/pl/touk/sputnik/connector/stash/ChangeType.java b/src/main/java/pl/touk/sputnik/connector/stash/ChangeType.java index ef273f61..46e388b8 100644 --- a/src/main/java/pl/touk/sputnik/connector/stash/ChangeType.java +++ b/src/main/java/pl/touk/sputnik/connector/stash/ChangeType.java @@ -1,5 +1,12 @@ package pl.touk.sputnik.connector.stash; public enum ChangeType { - ADDED, REMOVED, CONTEXT + ADDED, REMOVED, CONTEXT, NONE; + + public String getNameForStash() { + if (this.equals(NONE)) { + return CONTEXT.name(); + } + return name(); + } } diff --git a/src/main/java/pl/touk/sputnik/connector/stash/CrcMessage.java b/src/main/java/pl/touk/sputnik/connector/stash/CrcMessage.java new file mode 100644 index 00000000..a6a4fd59 --- /dev/null +++ b/src/main/java/pl/touk/sputnik/connector/stash/CrcMessage.java @@ -0,0 +1,14 @@ +package pl.touk.sputnik.connector.stash; + +import pl.touk.sputnik.connector.gerrit.json.ReviewLineComment; + +public class CrcMessage extends ReviewLineComment { + public CrcMessage(ReviewLineComment comment) { + super(comment.line, comment.message); + } + + @Override + public String getMessage() { + return String.format("%s (%s)", message, line); + } +} diff --git a/src/main/java/pl/touk/sputnik/connector/stash/SingleFileChanges.java b/src/main/java/pl/touk/sputnik/connector/stash/SingleFileChanges.java index 93929a61..350876fb 100644 --- a/src/main/java/pl/touk/sputnik/connector/stash/SingleFileChanges.java +++ b/src/main/java/pl/touk/sputnik/connector/stash/SingleFileChanges.java @@ -1,10 +1,13 @@ package pl.touk.sputnik.connector.stash; import com.google.common.collect.Maps; +import com.google.common.collect.Sets; import lombok.Data; import lombok.experimental.Builder; +import java.util.List; import java.util.Map; +import java.util.Set; @Data @Builder @@ -12,15 +15,29 @@ public class SingleFileChanges { private String filename; private Map changesMap; + private Set commentsCrcSet; public void addChange(int line, ChangeType changeType) { - getMap().put(line, changeType); + getChangesMap().put(line, changeType); } - private Map getMap() { + public Map getChangesMap() { if (changesMap == null) { changesMap = Maps.newHashMap(); } return changesMap; } + + public Set getCommentsCrcSet() { + if (commentsCrcSet == null) { + commentsCrcSet = Sets.newHashSet(); + } + return commentsCrcSet; + } + + public void setComments(List comments) { + for (String comment : comments) { + getCommentsCrcSet().add(comment); + } + } } diff --git a/src/main/java/pl/touk/sputnik/connector/stash/StashConnector.java b/src/main/java/pl/touk/sputnik/connector/stash/StashConnector.java index 99986ca2..42e07672 100644 --- a/src/main/java/pl/touk/sputnik/connector/stash/StashConnector.java +++ b/src/main/java/pl/touk/sputnik/connector/stash/StashConnector.java @@ -3,7 +3,6 @@ import lombok.AllArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.apache.http.HttpRequest; -import org.apache.http.NameValuePair; import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpGet; import org.apache.http.client.methods.HttpPost; @@ -39,8 +38,6 @@ public class StashConnector implements Connector { public String listFiles() throws URISyntaxException, IOException { URI uri = httpConnector.buildUri(createUrl(stashPatchset, CHANGES_URL_FORMAT)); HttpGet httpGet = new HttpGet(uri); - // Is this needed? -// addBasicAuthHeader(httpGet); CloseableHttpResponse httpResponse = httpConnector.logAndExecute(httpGet); return httpConnector.consumeAndLogEntity(httpResponse); } @@ -59,9 +56,8 @@ public String getDiffByLine(String filename) throws URISyntaxException, IOExcept URI uri = httpConnector.buildUri(createUrl(stashPatchset, DIFF_URL_FORMAT) + "/" + filename, new BasicNameValuePair("contextLines", "-1"), new BasicNameValuePair("srcPath", filename), - new BasicNameValuePair("withComments", "false")); + new BasicNameValuePair("withComments", "true")); HttpGet httpGet = new HttpGet(uri); - addBasicAuthHeader(httpGet); CloseableHttpResponse httpResponse = httpConnector.logAndExecute(httpGet); return httpConnector.consumeAndLogEntity(httpResponse); } diff --git a/src/main/java/pl/touk/sputnik/connector/stash/StashFacade.java b/src/main/java/pl/touk/sputnik/connector/stash/StashFacade.java index e28c134f..d074eb48 100644 --- a/src/main/java/pl/touk/sputnik/connector/stash/StashFacade.java +++ b/src/main/java/pl/touk/sputnik/connector/stash/StashFacade.java @@ -21,6 +21,9 @@ import java.util.List; import java.util.Map; +import pl.touk.sputnik.configuration.ConfigurationHolder; +import pl.touk.sputnik.configuration.GeneralOption; + @Slf4j public class StashFacade implements ConnectorFacade { private StashConnector stashConnector; @@ -55,15 +58,22 @@ public List listFiles() { @Override public void setReview(ReviewInput reviewInput) { + boolean commentOnlyChangedLines = Boolean.parseBoolean(ConfigurationHolder.instance().getProperty(GeneralOption.COMMENT_ONLY_CHANGED_LINES)); try { for (Map.Entry> review : reviewInput.comments.entrySet()) { log.info("{} : {}", review.getKey(), Joiner.on(", ").join(review.getValue())); SingleFileChanges changes = changesForSingleFile(review.getKey()); for (ReviewFileComment comment : review.getValue()) { - ReviewLineComment lineComment = (ReviewLineComment) comment; - String json = objectMapper.writeValueAsString( - toFileComment(review.getKey(), lineComment, getChangeType(changes, lineComment.line))); - stashConnector.sendReview(json); + CrcMessage lineComment = new CrcMessage((ReviewLineComment) comment); + if (noCommentExists(changes, lineComment)) { + ChangeType changeType = getChangeType(changes, lineComment.line); + if (changeType.equals(ChangeType.NONE) && commentOnlyChangedLines) { + log.info("Not posting out of context warning: {}", lineComment.message); + } else { + String json = objectMapper.writeValueAsString(toFileComment(review.getKey(), lineComment, changeType)); + stashConnector.sendReview(json); + } + } } } // Add comment with number of violations @@ -74,21 +84,26 @@ public void setReview(ReviewInput reviewInput) { } } + private boolean noCommentExists(SingleFileChanges changes, CrcMessage lineComment) { + return !changes.getChangesMap().containsKey(lineComment.line) + || !changes.getCommentsCrcSet().contains(lineComment.getMessage()); + } + private ChangeType getChangeType(SingleFileChanges changes, Integer line) { if (changes.getChangesMap().containsKey(line)) { return changes.getChangesMap().get(line); } - return ChangeType.CONTEXT; + return ChangeType.NONE; } private FileComment toFileComment(String key, ReviewLineComment comment, ChangeType changeType) { FileComment fileComment = new FileComment(); - fileComment.setText(comment.message); + fileComment.setText(comment.getMessage()); Anchor anchor = Anchor.builder(). path(key). srcPath(key). line(comment.line). - lineType(changeType.name()). + lineType(changeType.getNameForStash()). build(); fileComment.setAnchor(anchor); return fileComment; @@ -109,9 +124,11 @@ private List transform(List jsonList, Class someClass) { SingleFileChanges changesForSingleFile(String filename) { try { String response = stashConnector.getDiffByLine(filename); - List jsonList = JsonPath.read(response, "$.diffs[*].hunks[*].segments[*]"); - List segments = transform(jsonList, DiffSegment.class); + List diffJsonList = JsonPath.read(response, "$.diffs[*].hunks[*].segments[*]"); + List lineList = JsonPath.read(response, "$.diffs[*].lineComments[*].text"); + List segments = transform(diffJsonList, DiffSegment.class); SingleFileChanges changes = SingleFileChanges.builder().filename(filename).build(); + changes.setComments(lineList); for (DiffSegment segment : segments) { for (LineSegment line : segment.lines) { changes.addChange(line.destination, ChangeType.valueOf(segment.type)); diff --git a/src/main/java/pl/touk/sputnik/review/Review.java b/src/main/java/pl/touk/sputnik/review/Review.java index 85a3d046..47dd23d5 100644 --- a/src/main/java/pl/touk/sputnik/review/Review.java +++ b/src/main/java/pl/touk/sputnik/review/Review.java @@ -23,8 +23,8 @@ public class Review { private final List files; private int totalViolationsCount = 0; - public Review(List files, boolean revievTestFiles) { - if (revievTestFiles) { + public Review(List files, boolean reviewTestFiles) { + if (reviewTestFiles) { this.files = files; } else { // Filter test files diff --git a/src/main/resources/example.properties b/src/main/resources/example.properties index 888ff59b..e957126a 100644 --- a/src/main/resources/example.properties +++ b/src/main/resources/example.properties @@ -1,18 +1,14 @@ -#gerrit.changeId= -#gerrit.revisionId= -gerrit.host=your.host.com -gerrit.useHttps=false -gerrit.port=8080 -gerrit.username=sputnik -gerrit.password=PassWd -stash.host=your.host.com -stash.useHttps=true -stash.port=8080 -stash.username=sputnik -stash.password=PassWd +connector.type=stash +connector.host=localhost +connector.port=443 +connector.useHttps=true +connector.username=username +connector.password=password +connector.path= + +connector.repositorySlug= +connector.projectKey= -stash.repositorySlug= -stash.projectKey= checkstyle.enabled=true checkstyle.configurationFile=sun_checks.xml diff --git a/src/test/java/pl/touk/sputnik/connector/stash/StashFacadeTest.java b/src/test/java/pl/touk/sputnik/connector/stash/StashFacadeTest.java index 6cbfb801..50aafef6 100644 --- a/src/test/java/pl/touk/sputnik/connector/stash/StashFacadeTest.java +++ b/src/test/java/pl/touk/sputnik/connector/stash/StashFacadeTest.java @@ -1,6 +1,8 @@ package pl.touk.sputnik.connector.stash; +import com.github.tomakehurst.wiremock.client.UrlMatchingStrategy; import com.github.tomakehurst.wiremock.junit.WireMockRule; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import org.apache.commons.io.IOUtils; import org.junit.Before; @@ -8,7 +10,7 @@ import org.junit.Test; import pl.touk.sputnik.configuration.ConfigurationSetup; import pl.touk.sputnik.connector.FacadeConfigUtil; -import pl.touk.sputnik.review.ReviewFile; +import pl.touk.sputnik.review.*; import java.util.List; import java.util.Map; @@ -41,14 +43,9 @@ public void setUp() { @Test public void shouldGetChangeInfo() throws Exception { - stubFor(get(urlEqualTo(String.format( + stubGet(urlEqualTo(String.format( "%s/rest/api/1.0/projects/%s/repos/%s/pull-requests/%s/changes", - FacadeConfigUtil.PATH, SOME_PROJECT_KEY, SOME_REPOSITORY, SOME_PULL_REQUEST_ID))) - .withHeader("Authorization", equalTo("Basic dXNlcjpwYXNz")) - .willReturn(aResponse() - .withStatus(200) - .withHeader("Content-Type", "application/json") - .withBody(IOUtils.toString(getClass().getResourceAsStream("/json/stash-changes.json"))))); + FacadeConfigUtil.PATH, SOME_PROJECT_KEY, SOME_REPOSITORY, SOME_PULL_REQUEST_ID)), "/json/stash-changes.json"); List files = fixture.listFiles(); assertThat(files).hasSize(4); @@ -56,13 +53,9 @@ public void shouldGetChangeInfo() throws Exception { @Test public void shouldReturnDiffAsMapOfLines() throws Exception { - stubFor(get(urlMatching(String.format( + stubGet(urlMatching(String.format( "%s/rest/api/1.0/projects/%s/repos/%s/pull-requests/%s/diff.*", - FacadeConfigUtil.PATH, SOME_PROJECT_KEY, SOME_REPOSITORY, SOME_PULL_REQUEST_ID))) - .willReturn(aResponse() - .withStatus(200) - .withHeader("Content-Type", "application/json") - .withBody(IOUtils.toString(getClass().getResourceAsStream("/json/stash-diff.json"))))); + FacadeConfigUtil.PATH, SOME_PROJECT_KEY, SOME_REPOSITORY, SOME_PULL_REQUEST_ID)), "/json/stash-diff.json"); SingleFileChanges singleFileChanges = fixture.changesForSingleFile("src/main/java/Main.java"); assertThat(singleFileChanges.getFilename()).isEqualTo("src/main/java/Main.java"); @@ -70,4 +63,49 @@ public void shouldReturnDiffAsMapOfLines() throws Exception { .containsEntry(1, ChangeType.ADDED) .containsEntry(2, ChangeType.ADDED); } + + @Test + public void shouldNotAddTheSameCommentMoreThanOnce() throws Exception { + String filename = "src/main/java/Main.java"; + + stubGet(urlMatching(String.format( + "%s/rest/api/1.0/projects/%s/repos/%s/pull-requests/%s/diff.*", + FacadeConfigUtil.PATH, SOME_PROJECT_KEY, SOME_REPOSITORY, SOME_PULL_REQUEST_ID)), "/json/stash-diff-empty.json"); + + stubPost(urlMatching(String.format( + "%s/rest/api/1.0/projects/%s/repos/%s/pull-requests/%s/comments", + FacadeConfigUtil.PATH, SOME_PROJECT_KEY, SOME_REPOSITORY, SOME_PULL_REQUEST_ID)), "/json/stash-diff-empty.json"); + + Review review = new Review(ImmutableList.of(new ReviewFile(filename)), true); + review.addError("scalastyle", new Violation(filename, 1, "error message", Severity.ERROR)); + + fixture.setReview(review.toReviewInput(5)); + + stubGet(urlMatching(String.format( + "%s/rest/api/1.0/projects/%s/repos/%s/pull-requests/%s/diff.*", + FacadeConfigUtil.PATH, SOME_PROJECT_KEY, SOME_REPOSITORY, SOME_PULL_REQUEST_ID)), "/json/stash-diff.json"); + + fixture.setReview(review.toReviewInput(5)); + + verify(3, postRequestedFor(urlEqualTo(String.format("%s/rest/api/1.0/projects/%s/repos/%s/pull-requests/%s/comments", + FacadeConfigUtil.PATH, SOME_PROJECT_KEY, SOME_REPOSITORY, SOME_PULL_REQUEST_ID)))); + } + + private void stubGet(UrlMatchingStrategy url, String responseFile) throws Exception { + stubFor(get(url) + .withHeader("Authorization", equalTo("Basic dXNlcjpwYXNz")) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody(IOUtils.toString(getClass().getResourceAsStream(responseFile))))); + } + + private void stubPost(UrlMatchingStrategy url, String responseFile) throws Exception { + stubFor(post(url) + .withHeader("Authorization", equalTo("Basic dXNlcjpwYXNz")) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody(IOUtils.toString(getClass().getResourceAsStream(responseFile))))); + } } diff --git a/src/test/resources/json/stash-diff-empty.json b/src/test/resources/json/stash-diff-empty.json new file mode 100644 index 00000000..b2803869 --- /dev/null +++ b/src/test/resources/json/stash-diff-empty.json @@ -0,0 +1,5 @@ +{ + "diffs": [ + + ] +} \ No newline at end of file diff --git a/src/test/resources/json/stash-diff.json b/src/test/resources/json/stash-diff.json index 42e74e9c..5043f5ad 100644 --- a/src/test/resources/json/stash-diff.json +++ b/src/test/resources/json/stash-diff.json @@ -72,7 +72,7 @@ { "id": 1, "version": 1, - "text": "An insightful comment.", + "text": "[scalastyle] ERROR: error message (1)", "author": { "name": "jcitizen", "emailAddress": "jane@example.com", @@ -93,4 +93,4 @@ "truncated": false } ] -} \ No newline at end of file +}