Merge pull request TouK#22 from zygm0nt/GITHUB-19-dont-repeat-comments

Fix for duplicated comments
ingwarsw · Jul 3, 2014 · f22e60b · f22e60b
2 parents 95fe509 + 7f801c1
commit f22e60b
Show file tree

Hide file tree

Showing 12 changed files with 142 additions and 49 deletions.
diff --git 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: <stash|gerrit>", "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
@@ -1,12 +1,14 @@
 package pl.touk.sputnik.connector.gerrit.json;
 
+import lombok.Getter;
 import lombok.ToString;
 
 /**
  * Gerrit comment used with request for review input.
  * 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
@@ -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
@@ -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
@@ -1,26 +1,43 @@
 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
 public class SingleFileChanges {
 
     private String filename;
     private Map<Integer, ChangeType> changesMap;
+    private Set<String> commentsCrcSet;
 
     public void addChange(int line, ChangeType changeType) {
-        getMap().put(line, changeType);
+        getChangesMap().put(line, changeType);
     }
 
-    private Map<Integer, ChangeType> getMap() {
+    public Map<Integer, ChangeType> getChangesMap() {
         if (changesMap == null) {
             changesMap = Maps.newHashMap();
         }
         return changesMap;
     }
+
+    public Set<String> getCommentsCrcSet() {
+        if (commentsCrcSet == null) {
+            commentsCrcSet = Sets.newHashSet();
+        }
+        return commentsCrcSet;
+    }
+
+    public void setComments(List<String> 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
@@ -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
@@ -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<ReviewFile> listFiles() {
 
     @Override
     public void setReview(ReviewInput reviewInput) {
+        boolean commentOnlyChangedLines = Boolean.parseBoolean(ConfigurationHolder.instance().getProperty(GeneralOption.COMMENT_ONLY_CHANGED_LINES));
         try {
             for (Map.Entry<String, List<ReviewFileComment>> 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 <T> List<T> transform(List<JSONObject> jsonList, Class<T> someClass) {
     SingleFileChanges changesForSingleFile(String filename) {
         try {
             String response = stashConnector.getDiffByLine(filename);
-            List<JSONObject> jsonList = JsonPath.read(response, "$.diffs[*].hunks[*].segments[*]");
-            List<DiffSegment> segments = transform(jsonList, DiffSegment.class);
+            List<JSONObject> diffJsonList = JsonPath.read(response, "$.diffs[*].hunks[*].segments[*]");
+            List<String> lineList = JsonPath.read(response, "$.diffs[*].lineComments[*].text");
+            List<DiffSegment> 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
@@ -23,8 +23,8 @@ public class Review {
     private final List<ReviewFile> files;
     private int totalViolationsCount = 0;
 
-    public Review(List<ReviewFile> files, boolean revievTestFiles) {
-        if (revievTestFiles) {
+    public Review(List<ReviewFile> 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
@@ -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
@@ -1,14 +1,16 @@
 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;
 import org.junit.Rule;
 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,33 +43,69 @@ 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<ReviewFile> files = fixture.listFiles();
         assertThat(files).hasSize(4);
     }
 
     @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");
         assertThat(singleFileChanges.getChangesMap())
                 .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
@@ -0,0 +1,5 @@
+{
+    "diffs": [
+
+    ]
+}
diff --git 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
         }
     ]
-}
+}