From 8c369c6c21649a265e6de4e261f4f1f5d4742653 Mon Sep 17 00:00:00 2001 From: Marcin Cylke Date: Wed, 2 Jul 2014 08:26:32 +0200 Subject: [PATCH 1/9] Test showing multiple comments added in the same line --- .../connector/stash/StashConnector.java | 2 +- .../sputnik/connector/stash/StashFacade.java | 15 +++++--- .../java/pl/touk/sputnik/review/Review.java | 4 +-- .../connector/stash/StashFacadeTest.java | 36 ++++++++++++++++++- src/test/resources/json/stash-diff-empty.json | 5 +++ 5 files changed, 54 insertions(+), 8 deletions(-) create mode 100644 src/test/resources/json/stash-diff-empty.json 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..914e45f4 100644 --- a/src/main/java/pl/touk/sputnik/connector/stash/StashConnector.java +++ b/src/main/java/pl/touk/sputnik/connector/stash/StashConnector.java @@ -59,7 +59,7 @@ 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); 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 21ce4bdb..0977852e 100644 --- a/src/main/java/pl/touk/sputnik/connector/stash/StashFacade.java +++ b/src/main/java/pl/touk/sputnik/connector/stash/StashFacade.java @@ -64,9 +64,11 @@ public void setReview(ReviewInput reviewInput) { 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); + if (noCommentExists(changes, lineComment.line)) { + String json = objectMapper.writeValueAsString( + toFileComment(review.getKey(), lineComment, getChangeType(changes, lineComment.line))); + stashConnector.sendReview(json); + } } } // Add comment with number of violations @@ -77,8 +79,13 @@ public void setReview(ReviewInput reviewInput) { } } + private boolean noCommentExists(SingleFileChanges changes, Integer line) { + //if (changes.getChangesMap() != null && changes.getChangesMap().containsKey(line)) + return true; + } + private ChangeType getChangeType(SingleFileChanges changes, Integer line) { - if (changes.getChangesMap().containsKey(line)) { + if (changes.getChangesMap() != null && changes.getChangesMap().containsKey(line)) { return changes.getChangesMap().get(line); } return ChangeType.CONTEXT; 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/test/java/pl/touk/sputnik/connector/stash/StashFacadeTest.java b/src/test/java/pl/touk/sputnik/connector/stash/StashFacadeTest.java index 6cbfb801..7a5339fa 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,7 @@ package pl.touk.sputnik.connector.stash; 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 +9,9 @@ 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.connector.gerrit.json.ReviewInput; +import pl.touk.sputnik.connector.gerrit.json.ReviewLineComment; +import pl.touk.sputnik.review.*; import java.util.List; import java.util.Map; @@ -70,4 +73,35 @@ 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"; + + stubFor(get(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-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)); + + stubFor(get(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"))))); + + fixture.setReview(review.toReviewInput(5)); + + /* verify(2, 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))));*/ + } } 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 From 8f3d1c79c7e65d3817095bb187a989bb1a53b621 Mon Sep 17 00:00:00 2001 From: Marcin Cylke Date: Wed, 2 Jul 2014 11:51:47 +0200 Subject: [PATCH 2/9] Do not add duplicate comments to the same line --- .../gerrit/json/ReviewFileComment.java | 2 + .../sputnik/connector/stash/CrcMessage.java | 29 ++++++++++ .../connector/stash/SingleFileChanges.java | 27 +++++++++ .../sputnik/connector/stash/StashFacade.java | 19 ++++--- .../connector/stash/StashFacadeTest.java | 56 ++++++++++--------- src/test/resources/json/stash-diff.json | 2 +- 6 files changed, 100 insertions(+), 35 deletions(-) create mode 100644 src/main/java/pl/touk/sputnik/connector/stash/CrcMessage.java 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/CrcMessage.java b/src/main/java/pl/touk/sputnik/connector/stash/CrcMessage.java new file mode 100644 index 00000000..680c25b3 --- /dev/null +++ b/src/main/java/pl/touk/sputnik/connector/stash/CrcMessage.java @@ -0,0 +1,29 @@ +package pl.touk.sputnik.connector.stash; + +import org.apache.commons.codec.binary.Base64; +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\ncrc:%s", message, getCrc()); + } + + public String getCrc() { + return base64(); + } + + private String base64() { + byte[] encodedBytes = Base64.encodeBase64(String.format("%s %s", line, message).getBytes()); + + StringBuilder sb = new StringBuilder(); + for (byte encodedByte : encodedBytes) { + sb.append(Integer.toHexString(encodedByte)); + } + return sb.toString(); + } +} 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..2092d141 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.beust.jcommander.internal.Sets; import com.google.common.collect.Maps; import lombok.Data; import lombok.experimental.Builder; +import java.util.List; import java.util.Map; +import java.util.Set; @Data @Builder @@ -12,6 +15,7 @@ public class SingleFileChanges { private String filename; private Map changesMap; + private Set commentsCrcSet; public void addChange(int line, ChangeType changeType) { getMap().put(line, changeType); @@ -23,4 +27,27 @@ private Map getMap() { } return changesMap; } + + private Set getSet() { + if (commentsCrcSet == null) { + commentsCrcSet = Sets.newHashSet(); + } + return commentsCrcSet; + } + + public void setComments(List comments) { + for (String comment : comments) { + getSet().add(extractCrc(comment)); + } + } + + private String extractCrc(String comment) { + String[] commentLines = comment.split("\n"); + String lastLine = commentLines[commentLines.length - 1]; + + if (lastLine.startsWith("crc:")) { + return lastLine.replace("crc:", ""); + } + return ""; + } } 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 0977852e..17d24ea0 100644 --- a/src/main/java/pl/touk/sputnik/connector/stash/StashFacade.java +++ b/src/main/java/pl/touk/sputnik/connector/stash/StashFacade.java @@ -63,8 +63,8 @@ public void setReview(ReviewInput reviewInput) { log.info("{} : {}", review.getKey(), Joiner.on(", ").join(review.getValue())); SingleFileChanges changes = changesForSingleFile(review.getKey()); for (ReviewFileComment comment : review.getValue()) { - ReviewLineComment lineComment = (ReviewLineComment) comment; - if (noCommentExists(changes, lineComment.line)) { + CrcMessage lineComment = new CrcMessage((ReviewLineComment) comment); + if (noCommentExists(changes, lineComment)) { String json = objectMapper.writeValueAsString( toFileComment(review.getKey(), lineComment, getChangeType(changes, lineComment.line))); stashConnector.sendReview(json); @@ -79,9 +79,10 @@ public void setReview(ReviewInput reviewInput) { } } - private boolean noCommentExists(SingleFileChanges changes, Integer line) { - //if (changes.getChangesMap() != null && changes.getChangesMap().containsKey(line)) - return true; + private boolean noCommentExists(SingleFileChanges changes, CrcMessage lineComment) { + return changes.getChangesMap() == null + || !changes.getChangesMap().containsKey(lineComment.line) + || !changes.getCommentsCrcSet().contains(lineComment.getCrc()); } private ChangeType getChangeType(SingleFileChanges changes, Integer line) { @@ -93,7 +94,7 @@ private ChangeType getChangeType(SingleFileChanges changes, Integer line) { 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). @@ -119,9 +120,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/test/java/pl/touk/sputnik/connector/stash/StashFacadeTest.java b/src/test/java/pl/touk/sputnik/connector/stash/StashFacadeTest.java index 7a5339fa..5396ba61 100644 --- a/src/test/java/pl/touk/sputnik/connector/stash/StashFacadeTest.java +++ b/src/test/java/pl/touk/sputnik/connector/stash/StashFacadeTest.java @@ -1,5 +1,6 @@ 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; @@ -44,14 +45,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); @@ -59,13 +55,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"); @@ -78,30 +70,42 @@ public void shouldReturnDiffAsMapOfLines() throws Exception { public void shouldNotAddTheSameCommentMoreThanOnce() throws Exception { String filename = "src/main/java/Main.java"; - 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-empty.json"))))); + 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)); - 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))) + 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) .willReturn(aResponse() .withStatus(200) .withHeader("Content-Type", "application/json") - .withBody(IOUtils.toString(getClass().getResourceAsStream("/json/stash-diff.json"))))); - - fixture.setReview(review.toReviewInput(5)); + .withBody(IOUtils.toString(getClass().getResourceAsStream(responseFile))))); + } - /* verify(2, 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 stubPost(UrlMatchingStrategy url, String responseFile) throws Exception { + stubFor(post(url) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody(IOUtils.toString(getClass().getResourceAsStream(responseFile))))); } } diff --git a/src/test/resources/json/stash-diff.json b/src/test/resources/json/stash-diff.json index 42e74e9c..fd5dafae 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": "An insightful comment.\ncrc:4d53426263324e686247467a64486c735a56306752564a535431493649475679636d39794947316c63334e685a32553d", "author": { "name": "jcitizen", "emailAddress": "jane@example.com", From f468b6dfdf8eb0a01e9f4db30e3e2a531b6d8a2a Mon Sep 17 00:00:00 2001 From: Marcin Cylke Date: Wed, 2 Jul 2014 13:05:07 +0200 Subject: [PATCH 3/9] config path, fixes #19 --- src/main/resources/example.properties | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/main/resources/example.properties b/src/main/resources/example.properties index 888ff59b..733a0d4f 100644 --- a/src/main/resources/example.properties +++ b/src/main/resources/example.properties @@ -1,18 +1,13 @@ -#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.repositorySlug= +connector.projectKey= -stash.repositorySlug= -stash.projectKey= checkstyle.enabled=true checkstyle.configurationFile=sun_checks.xml From f851f6936dbc6b680147159af9e88bcb0440cf04 Mon Sep 17 00:00:00 2001 From: Karol Lassak Date: Wed, 2 Jul 2014 17:03:27 +0200 Subject: [PATCH 4/9] Some fixes in multiple comments - Shortened crc - Fixed NPE when no comments found --- .../touk/sputnik/connector/stash/CrcMessage.java | 15 ++------------- .../connector/stash/SingleFileChanges.java | 14 ++------------ .../touk/sputnik/connector/stash/StashFacade.java | 2 +- .../sputnik/connector/stash/StashFacadeTest.java | 2 -- src/test/resources/json/stash-diff.json | 2 +- 5 files changed, 6 insertions(+), 29 deletions(-) diff --git a/src/main/java/pl/touk/sputnik/connector/stash/CrcMessage.java b/src/main/java/pl/touk/sputnik/connector/stash/CrcMessage.java index 680c25b3..395fcb0e 100644 --- a/src/main/java/pl/touk/sputnik/connector/stash/CrcMessage.java +++ b/src/main/java/pl/touk/sputnik/connector/stash/CrcMessage.java @@ -1,6 +1,5 @@ package pl.touk.sputnik.connector.stash; -import org.apache.commons.codec.binary.Base64; import pl.touk.sputnik.connector.gerrit.json.ReviewLineComment; public class CrcMessage extends ReviewLineComment { @@ -10,20 +9,10 @@ public CrcMessage(ReviewLineComment comment) { @Override public String getMessage() { - return String.format("%s\ncrc:%s", message, getCrc()); + return String.format("%s (%s)", message, getCrc()); } public String getCrc() { - return base64(); - } - - private String base64() { - byte[] encodedBytes = Base64.encodeBase64(String.format("%s %s", line, message).getBytes()); - - StringBuilder sb = new StringBuilder(); - for (byte encodedByte : encodedBytes) { - sb.append(Integer.toHexString(encodedByte)); - } - return sb.toString(); + return Integer.toHexString(String.format("%s %s", line, message).hashCode() % 1000); } } 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 2092d141..8d720f13 100644 --- a/src/main/java/pl/touk/sputnik/connector/stash/SingleFileChanges.java +++ b/src/main/java/pl/touk/sputnik/connector/stash/SingleFileChanges.java @@ -28,7 +28,7 @@ private Map getMap() { return changesMap; } - private Set getSet() { + public Set getCommentsCrcSet() { if (commentsCrcSet == null) { commentsCrcSet = Sets.newHashSet(); } @@ -37,17 +37,7 @@ private Set getSet() { public void setComments(List comments) { for (String comment : comments) { - getSet().add(extractCrc(comment)); + getCommentsCrcSet().add(comment); } } - - private String extractCrc(String comment) { - String[] commentLines = comment.split("\n"); - String lastLine = commentLines[commentLines.length - 1]; - - if (lastLine.startsWith("crc:")) { - return lastLine.replace("crc:", ""); - } - return ""; - } } 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 17d24ea0..63759808 100644 --- a/src/main/java/pl/touk/sputnik/connector/stash/StashFacade.java +++ b/src/main/java/pl/touk/sputnik/connector/stash/StashFacade.java @@ -82,7 +82,7 @@ public void setReview(ReviewInput reviewInput) { private boolean noCommentExists(SingleFileChanges changes, CrcMessage lineComment) { return changes.getChangesMap() == null || !changes.getChangesMap().containsKey(lineComment.line) - || !changes.getCommentsCrcSet().contains(lineComment.getCrc()); + || !changes.getCommentsCrcSet().contains(lineComment.getMessage()); } private ChangeType getChangeType(SingleFileChanges changes, Integer line) { 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 5396ba61..5a7b6167 100644 --- a/src/test/java/pl/touk/sputnik/connector/stash/StashFacadeTest.java +++ b/src/test/java/pl/touk/sputnik/connector/stash/StashFacadeTest.java @@ -10,8 +10,6 @@ import org.junit.Test; import pl.touk.sputnik.configuration.ConfigurationSetup; import pl.touk.sputnik.connector.FacadeConfigUtil; -import pl.touk.sputnik.connector.gerrit.json.ReviewInput; -import pl.touk.sputnik.connector.gerrit.json.ReviewLineComment; import pl.touk.sputnik.review.*; import java.util.List; diff --git a/src/test/resources/json/stash-diff.json b/src/test/resources/json/stash-diff.json index fd5dafae..b4c9c243 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.\ncrc:4d53426263324e686247467a64486c735a56306752564a535431493649475679636d39794947316c63334e685a32553d", + "text": "[scalastyle] ERROR: error message (163)", "author": { "name": "jcitizen", "emailAddress": "jane@example.com", From 910bf748199c4593a8b4d862861aaf57b90896d7 Mon Sep 17 00:00:00 2001 From: Karol Lassak Date: Wed, 2 Jul 2014 17:17:10 +0200 Subject: [PATCH 5/9] Fixed format - Now numbers < 0 should look way better - Uppercased all letters --- src/main/java/pl/touk/sputnik/connector/stash/CrcMessage.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/pl/touk/sputnik/connector/stash/CrcMessage.java b/src/main/java/pl/touk/sputnik/connector/stash/CrcMessage.java index 395fcb0e..ef28fd61 100644 --- a/src/main/java/pl/touk/sputnik/connector/stash/CrcMessage.java +++ b/src/main/java/pl/touk/sputnik/connector/stash/CrcMessage.java @@ -13,6 +13,6 @@ public String getMessage() { } public String getCrc() { - return Integer.toHexString(String.format("%s %s", line, message).hashCode() % 1000); + return Integer.toHexString(Math.abs(String.format("%s %s", line, message).hashCode()) % 1000).toUpperCase(); } } From 33458638931ae6009fb8a4e4b3ad9ef6a4f5ab90 Mon Sep 17 00:00:00 2001 From: Karol Lassak Date: Wed, 2 Jul 2014 17:54:42 +0200 Subject: [PATCH 6/9] Added option for commenting only changed lines - Will create test tumorrow --- .../sputnik/configuration/GeneralOption.java | 1 + .../sputnik/connector/stash/ChangeType.java | 9 ++++++- .../connector/stash/SingleFileChanges.java | 4 +-- .../connector/stash/StashConnector.java | 1 - .../sputnik/connector/stash/StashFacade.java | 27 ++++++++++--------- 5 files changed, 26 insertions(+), 16 deletions(-) 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/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/SingleFileChanges.java b/src/main/java/pl/touk/sputnik/connector/stash/SingleFileChanges.java index 8d720f13..f99b9aa0 100644 --- a/src/main/java/pl/touk/sputnik/connector/stash/SingleFileChanges.java +++ b/src/main/java/pl/touk/sputnik/connector/stash/SingleFileChanges.java @@ -18,10 +18,10 @@ public class SingleFileChanges { 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(); } 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 914e45f4..299f0ec6 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; 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 63759808..e557f933 100644 --- a/src/main/java/pl/touk/sputnik/connector/stash/StashFacade.java +++ b/src/main/java/pl/touk/sputnik/connector/stash/StashFacade.java @@ -6,14 +6,11 @@ import com.jayway.jsonpath.JsonPath; import lombok.extern.slf4j.Slf4j; import net.minidev.json.JSONObject; -import org.apache.http.client.protocol.HttpClientContext; -import org.apache.http.impl.client.CloseableHttpClient; import org.jetbrains.annotations.NotNull; import pl.touk.sputnik.connector.ConnectorFacade; import pl.touk.sputnik.connector.gerrit.json.ReviewFileComment; import pl.touk.sputnik.connector.gerrit.json.ReviewInput; import pl.touk.sputnik.connector.gerrit.json.ReviewLineComment; -import pl.touk.sputnik.connector.http.HttpConnector; import pl.touk.sputnik.connector.stash.json.*; import pl.touk.sputnik.review.ReviewFile; @@ -23,6 +20,8 @@ import java.util.List; import java.util.Map; import pl.touk.sputnik.Connectors; +import pl.touk.sputnik.configuration.ConfigurationHolder; +import pl.touk.sputnik.configuration.GeneralOption; @Slf4j public class StashFacade implements ConnectorFacade { @@ -58,6 +57,7 @@ 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())); @@ -65,9 +65,13 @@ public void setReview(ReviewInput reviewInput) { for (ReviewFileComment comment : review.getValue()) { CrcMessage lineComment = new CrcMessage((ReviewLineComment) comment); if (noCommentExists(changes, lineComment)) { - String json = objectMapper.writeValueAsString( - toFileComment(review.getKey(), lineComment, getChangeType(changes, lineComment.line))); - stashConnector.sendReview(json); + 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); + } } } } @@ -80,16 +84,15 @@ public void setReview(ReviewInput reviewInput) { } private boolean noCommentExists(SingleFileChanges changes, CrcMessage lineComment) { - return changes.getChangesMap() == null - || !changes.getChangesMap().containsKey(lineComment.line) - || !changes.getCommentsCrcSet().contains(lineComment.getMessage()); + return !changes.getChangesMap().containsKey(lineComment.line) + || !changes.getCommentsCrcSet().contains(lineComment.getMessage()); } private ChangeType getChangeType(SingleFileChanges changes, Integer line) { - if (changes.getChangesMap() != null && changes.getChangesMap().containsKey(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) { @@ -99,7 +102,7 @@ private FileComment toFileComment(String key, ReviewLineComment comment, ChangeT path(key). srcPath(key). line(comment.line). - lineType(changeType.name()). + lineType(changeType.getNameForStash()). build(); fileComment.setAnchor(anchor); return fileComment; From 10e1da0fef51b5702b4f17c77808c816e1dbf7ea Mon Sep 17 00:00:00 2001 From: Karol Lassak Date: Wed, 2 Jul 2014 18:08:49 +0200 Subject: [PATCH 7/9] Simplest methods are always the best - Crc is not needed we are comparing whole line --- .../java/pl/touk/sputnik/connector/stash/CrcMessage.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/main/java/pl/touk/sputnik/connector/stash/CrcMessage.java b/src/main/java/pl/touk/sputnik/connector/stash/CrcMessage.java index ef28fd61..a6a4fd59 100644 --- a/src/main/java/pl/touk/sputnik/connector/stash/CrcMessage.java +++ b/src/main/java/pl/touk/sputnik/connector/stash/CrcMessage.java @@ -9,10 +9,6 @@ public CrcMessage(ReviewLineComment comment) { @Override public String getMessage() { - return String.format("%s (%s)", message, getCrc()); - } - - public String getCrc() { - return Integer.toHexString(Math.abs(String.format("%s %s", line, message).hashCode()) % 1000).toUpperCase(); + return String.format("%s (%s)", message, line); } } From 869b92d71ff578d0aa5ecad48a2a2d43687e89e6 Mon Sep 17 00:00:00 2001 From: Karol Lassak Date: Wed, 2 Jul 2014 19:25:05 +0200 Subject: [PATCH 8/9] Update stash-diff.json - Repaired tests --- src/test/resources/json/stash-diff.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/resources/json/stash-diff.json b/src/test/resources/json/stash-diff.json index b4c9c243..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": "[scalastyle] ERROR: error message (163)", + "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 +} From d0098a2fbd53e31f31c8e371b215d9cd96e33932 Mon Sep 17 00:00:00 2001 From: Marcin Cylke Date: Thu, 3 Jul 2014 07:30:44 +0200 Subject: [PATCH 9/9] Testing and changes: fix #13, fix #22 --- .../pl/touk/sputnik/connector/stash/SingleFileChanges.java | 2 +- .../java/pl/touk/sputnik/connector/stash/StashConnector.java | 3 --- src/main/resources/example.properties | 1 + .../java/pl/touk/sputnik/connector/stash/StashFacadeTest.java | 2 ++ 4 files changed, 4 insertions(+), 4 deletions(-) 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 f99b9aa0..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,7 +1,7 @@ package pl.touk.sputnik.connector.stash; -import com.beust.jcommander.internal.Sets; import com.google.common.collect.Maps; +import com.google.common.collect.Sets; import lombok.Data; import lombok.experimental.Builder; 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 299f0ec6..42e07672 100644 --- a/src/main/java/pl/touk/sputnik/connector/stash/StashConnector.java +++ b/src/main/java/pl/touk/sputnik/connector/stash/StashConnector.java @@ -38,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); } @@ -60,7 +58,6 @@ public String getDiffByLine(String filename) throws URISyntaxException, IOExcept new BasicNameValuePair("srcPath", filename), 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/resources/example.properties b/src/main/resources/example.properties index 733a0d4f..e957126a 100644 --- a/src/main/resources/example.properties +++ b/src/main/resources/example.properties @@ -4,6 +4,7 @@ connector.port=443 connector.useHttps=true connector.username=username connector.password=password +connector.path= connector.repositorySlug= connector.projectKey= 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 5a7b6167..50aafef6 100644 --- a/src/test/java/pl/touk/sputnik/connector/stash/StashFacadeTest.java +++ b/src/test/java/pl/touk/sputnik/connector/stash/StashFacadeTest.java @@ -93,6 +93,7 @@ public void shouldNotAddTheSameCommentMoreThanOnce() throws Exception { 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") @@ -101,6 +102,7 @@ private void stubGet(UrlMatchingStrategy url, String responseFile) throws Except 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")