#377: Allow decorating Gitlab Mono Repository Merge Requests

The Gitlab decorator treated any comment posted by the current Gitlab user as having been submitted for the current Sonarqube project, and therefore resolved any threads that no active issue could be found for in the current analysis. This prevented the decorator being used for a mono-repo as issues from decorations for other projects based in the repo would be resolved as subsequent projects completed their analysis.

The decorator has been changed to include the project ID in the details extracted from the existing comments, and to filter out any comments with project keys that do not match the project key the analysis is being performed for, before closing any remaining discussions, so that comments from other Sonarqube projects are not prematurely closed.

build.gradle

@@ -56,7 +56,7 @@ dependencies {
     compileOnly fileTree(dir: sonarLibraries, include: '**/*.jar', exclude: 'extensions/*.jar')
     testCompile fileTree(dir: sonarLibraries, include: '**/*.jar', exclude: 'extensions/*.jar')
     testCompile group: 'org.mockito', name: 'mockito-core', version: '3.1.0'
-    testCompile group: 'org.assertj', name: 'assertj-core', version: '3.13.2'
+    testCompile group: 'org.assertj', name: 'assertj-core', version: '3.20.2'
     testCompile group: 'com.github.tomakehurst', name: 'wiremock', version: '2.24.1'
     zip "sonarqube:sonarqube:${sonarqubeVersion}@zip"
     compile 'org.bouncycastle:bcpkix-jdk15on:1.62'

src/main/java/com/github/mc1arke/sonarqube/plugin/ce/pullrequest/AnalysisDetails.java

@@ -127,21 +127,34 @@ public String getIssueUrl(PostAnalysisIssueVisitor.LightIssue issue) {
         }
     }
 
-    public Optional<String> parseIssueIdFromUrl(String issueUrl) {
+    public Optional<ProjectIssueIdentifier> parseIssueIdFromUrl(String issueUrl) {
         URI url = URI.create(issueUrl);
         List<NameValuePair> parameters = URLEncodedUtils.parse(url, StandardCharsets.UTF_8);
+        Optional<String> optionalProjectId = parameters.stream()
+                .filter(parameter -> "id".equals(parameter.getName()))
+                .map(NameValuePair::getValue)
+                .findFirst();
+
+        if (!optionalProjectId.isPresent()) {
+            return Optional.empty();
+        }
+
+        String projectId = optionalProjectId.get();
+
         if (url.getPath().endsWith("/dashboard")) {
-            return Optional.of("decorator-summary-comment");
+            return Optional.of(new ProjectIssueIdentifier(projectId, "decorator-summary-comment"));
         } else if (url.getPath().endsWith("security_hotspots")) {
             return parameters.stream()
                     .filter(parameter -> "hotspots".equals(parameter.getName()))
                     .map(NameValuePair::getValue)
-                    .findFirst();
+                    .findFirst()
+                    .map(issueId -> new ProjectIssueIdentifier(projectId, issueId));
         } else {
             return parameters.stream()
                     .filter(parameter -> "issues".equals(parameter.getName()))
                     .map(NameValuePair::getValue)
-                    .findFirst();
+                    .findFirst()
+                    .map(issueId -> new ProjectIssueIdentifier(projectId, issueId));
         }
     }
 
@@ -466,4 +479,23 @@ private String getImageName() {
         }
     }
 
+    public static class ProjectIssueIdentifier {
+
+        private final String projectKey;
+        private final String issueKey;
+
+        public ProjectIssueIdentifier(String projectKey, String issueKey) {
+            this.projectKey = projectKey;
+            this.issueKey = issueKey;
+        }
+
+        public String getProjectKey() {
+            return projectKey;
+        }
+
+        public String getIssueKey() {
+            return issueKey;
+        }
+    }
+
 }

src/main/java/com/github/mc1arke/sonarqube/plugin/ce/pullrequest/gitlab/GitlabMergeRequestDecorator.java

@@ -117,10 +117,13 @@ public DecorationResult decorateQualityGateStatus(AnalysisDetails analysis, AlmS
                 .filter(i -> OPEN_ISSUE_STATUSES.contains(i.getIssue().getStatus()))
                 .collect(Collectors.toList());
 
-        List<Triple<Discussion, Note, Optional<String>>> currentProjectSonarqueComments = findOpenSonarqubeComments(gitlabClient,
+        List<Triple<Discussion, Note, Optional<AnalysisDetails.ProjectIssueIdentifier>>> currentProjectSonarqueComments = findOpenSonarqubeComments(gitlabClient,
                 mergeRequest,
                 user.getUsername(),
-                analysis);
+                analysis)
+                .stream()
+                .filter(comment -> commentIsFromCurrentProject(comment, analysis.getAnalysisProjectKey()))
+                .collect(Collectors.toList());
 
         List<String> commentKeysForOpenComments = closeOldDiscussionsAndExtractRemainingKeys(gitlabClient,
                 user.getUsername(),
@@ -155,6 +158,10 @@ public DecorationResult decorateQualityGateStatus(AnalysisDetails analysis, AlmS
         return DecorationResult.builder().withPullRequestUrl(mergeRequestHtmlUrl).build();
     }
 
+    private static boolean commentIsFromCurrentProject(Triple<Discussion, Note, Optional<AnalysisDetails.ProjectIssueIdentifier>> comment, String projectId) {
+        return comment.getRight().filter(projectIssueIdentifier -> projectId.equals(projectIssueIdentifier.getProjectKey())).isPresent();
+    }
+
     @Override
     public List<ALM> alm() {
         return Collections.singletonList(ALM.GITLAB);
@@ -283,10 +290,10 @@ private static void submitSummaryComment(GitlabClient gitlabClient, AnalysisDeta
 
     }
 
-    private static List<Triple<Discussion, Note, Optional<String>>> findOpenSonarqubeComments(GitlabClient gitlabClient,
-                                                                                              MergeRequest mergeRequest,
-                                                                                              String currentUsername,
-                                                                                              AnalysisDetails analysisDetails) {
+    private static List<Triple<Discussion, Note, Optional<AnalysisDetails.ProjectIssueIdentifier>>> findOpenSonarqubeComments(GitlabClient gitlabClient,
+                                                                                                                              MergeRequest mergeRequest,
+                                                                                                                              String currentUsername,
+                                                                                                                              AnalysisDetails analysisDetails) {
         try {
             return gitlabClient.getMergeRequestDiscussions(mergeRequest.getSourceProjectId(), mergeRequest.getIid()).stream()
                     .map(discussion -> discussion.getNotes().stream()
@@ -304,30 +311,34 @@ private static List<Triple<Discussion, Note, Optional<String>>> findOpenSonarqub
     }
 
     private static List<String> closeOldDiscussionsAndExtractRemainingKeys(GitlabClient gitlabClient, String currentUsername,
-                                                                               List<Triple<Discussion, Note, Optional<String>>> openSonarqubeComments,
-                                                                               List<PostAnalysisIssueVisitor.ComponentIssue> openIssues,
-                                                                               MergeRequest mergeRequest) {
+                                                                                                   List<Triple<Discussion, Note, Optional<AnalysisDetails.ProjectIssueIdentifier>>> openSonarqubeComments,
+                                                                                                   List<PostAnalysisIssueVisitor.ComponentIssue> openIssues,
+                                                                                                   MergeRequest mergeRequest) {
         List<String> openIssueKeys = openIssues.stream()
                 .map(issue -> issue.getIssue().key())
                 .collect(Collectors.toList());
 
         List<String> remainingCommentKeys = new ArrayList<>();
 
-        for (Triple<Discussion, Note, Optional<String>> openSonarqubeComment : openSonarqubeComments) {
-            Optional<String> issueKey = openSonarqubeComment.getRight();
+        for (Triple<Discussion, Note, Optional<AnalysisDetails.ProjectIssueIdentifier>> openSonarqubeComment : openSonarqubeComments) {
+            Optional<AnalysisDetails.ProjectIssueIdentifier> noteIdentifier = openSonarqubeComment.getRight();
             Discussion discussion = openSonarqubeComment.getLeft();
-            if (!issueKey.isPresent()) {
+            if (!noteIdentifier.isPresent()) {
                 LOGGER.warn("Note {} was found on discussion {} in Merge Request {} posted by Sonarqube user {}, " +
                         "but Sonarqube issue details could not be parsed from it. " +
                         "This discussion will therefore will not be cleaned up.",
                         openSonarqubeComment.getMiddle().getId(),
                         openSonarqubeComment.getLeft().getId(),
                         mergeRequest.getIid(),
                         currentUsername);
-            } else if (!openIssueKeys.contains(issueKey.get())) {
+                continue;
+            }
+
+            String issueKey = noteIdentifier.get().getIssueKey();
+            if (!openIssueKeys.contains(issueKey)) {
                 resolveOrPlaceFinalCommentOnDiscussion(gitlabClient, currentUsername, discussion, mergeRequest);
             } else {
-                remainingCommentKeys.add(issueKey.get());
+                remainingCommentKeys.add(issueKey);
             }
         }
 
@@ -355,7 +366,7 @@ private static void resolveOrPlaceFinalCommentOnDiscussion(GitlabClient gitlabCl
         }
     }
 
-    private static Optional<String> parseIssueDetails(Note note, AnalysisDetails analysisDetails) {
+    private static Optional<AnalysisDetails.ProjectIssueIdentifier> parseIssueDetails(Note note, AnalysisDetails analysisDetails) {
         try (BufferedReader reader = new BufferedReader(new StringReader(note.getBody()))) {
             return reader.lines()
                     .filter(line -> line.contains("View in SonarQube"))
@@ -368,7 +379,7 @@ private static Optional<String> parseIssueDetails(Note note, AnalysisDetails ana
         }
     }
 
-    private static Optional<String> parseIssueLineDetails(String noteLine, AnalysisDetails analysisDetails) {
+    private static Optional<AnalysisDetails.ProjectIssueIdentifier> parseIssueLineDetails(String noteLine, AnalysisDetails analysisDetails) {
         Matcher identifierMatcher = NOTE_VIEW_LINK_PATTERN.matcher(noteLine);
 
         if (identifierMatcher.matches()) {

src/test/java/com/github/mc1arke/sonarqube/plugin/ce/pullrequest/AnalysisDetailsTest.java

@@ -861,25 +861,29 @@ public void testFakeIdReturnedForSummaryComment() {
         AnalysisDetails analysisDetails = new AnalysisDetails(mock(AnalysisDetails.BranchDetails.class), mock(PostAnalysisIssueVisitor.class),
                 mock(QualityGate.class), mock(AnalysisDetails.MeasuresHolder.class), mock(Analysis.class), mock(Project.class),
                 mock(Configuration.class),"", mock(ScannerContext.class));
-        assertThat(analysisDetails.parseIssueIdFromUrl("https://sonarqube.dummy/path/dashboard?pullRequest=123"))
-                .isEqualTo(Optional.of("decorator-summary-comment"));
+        assertThat(analysisDetails.parseIssueIdFromUrl("https://sonarqube.dummy/path/dashboard?id=project&pullRequest=123"))
+                .get()
+                .usingRecursiveComparison()
+                .isEqualTo(new AnalysisDetails.ProjectIssueIdentifier("project", "decorator-summary-comment"));
     }
 
     @Test
     public void testIssueIdReturnedForHotspotUrl() {
         AnalysisDetails analysisDetails = new AnalysisDetails(mock(AnalysisDetails.BranchDetails.class), mock(PostAnalysisIssueVisitor.class),
                 mock(QualityGate.class), mock(AnalysisDetails.MeasuresHolder.class), mock(Analysis.class), mock(Project.class),
                 mock(Configuration.class),"", mock(ScannerContext.class));
-        assertThat(analysisDetails.parseIssueIdFromUrl("http://subdomain.sonarqube.dummy/path/security_hotspots?hotspots=A1B2-Z9Y8X7"))
-                .isEqualTo(Optional.of("A1B2-Z9Y8X7"));
+        assertThat(analysisDetails.parseIssueIdFromUrl("http://subdomain.sonarqube.dummy/path/security_hotspots?id=projectIdentifier&hotspots=A1B2-Z9Y8X7"))
+                .get()
+                .usingRecursiveComparison()
+                .isEqualTo(new AnalysisDetails.ProjectIssueIdentifier("projectIdentifier", "A1B2-Z9Y8X7"));
     }
 
     @Test
     public void testNoIssueIdReturnedForHotspotUrlWithoutId() {
         AnalysisDetails analysisDetails = new AnalysisDetails(mock(AnalysisDetails.BranchDetails.class), mock(PostAnalysisIssueVisitor.class),
                 mock(QualityGate.class), mock(AnalysisDetails.MeasuresHolder.class), mock(Analysis.class), mock(Project.class),
                 mock(Configuration.class),"", mock(ScannerContext.class));
-        assertThat(analysisDetails.parseIssueIdFromUrl("http://subdomain.sonarqube.dummy/path/security_hotspots?other_parameter=ABC"))
+        assertThat(analysisDetails.parseIssueIdFromUrl("http://subdomain.sonarqube.dummy/path/security_hotspots?id=projectId&other_parameter=ABC"))
                 .isEmpty();
     }
 
@@ -888,15 +892,17 @@ public void testIssueIdReturnedForIssueUrl() {
         AnalysisDetails analysisDetails = new AnalysisDetails(mock(AnalysisDetails.BranchDetails.class), mock(PostAnalysisIssueVisitor.class),
                 mock(QualityGate.class), mock(AnalysisDetails.MeasuresHolder.class), mock(Analysis.class), mock(Project.class),
                 mock(Configuration.class),"", mock(ScannerContext.class));
-        assertThat(analysisDetails.parseIssueIdFromUrl("http://subdomain.sonarqube.dummy/path/issue?issues=XXX-YYY-ZZZ"))
-                .isEqualTo(Optional.of("XXX-YYY-ZZZ"));
+        assertThat(analysisDetails.parseIssueIdFromUrl("http://subdomain.sonarqube.dummy/path/issue?id=projectId&issues=XXX-YYY-ZZZ"))
+                .get()
+                .usingRecursiveComparison()
+                .isEqualTo(new AnalysisDetails.ProjectIssueIdentifier("projectId", "XXX-YYY-ZZZ"));
     }
 
     @Test
     public void testNoIssueIdReturnedForIssueUrlWithoutId() {
         AnalysisDetails analysisDetails = new AnalysisDetails(mock(AnalysisDetails.BranchDetails.class), mock(PostAnalysisIssueVisitor.class),
                 mock(QualityGate.class), mock(AnalysisDetails.MeasuresHolder.class), mock(Analysis.class), mock(Project.class),
                 mock(Configuration.class),"", mock(ScannerContext.class));
-        assertThat(analysisDetails.parseIssueIdFromUrl("http://subdomain.sonarqube.dummy/path/issue?other_parameter=123")).isEmpty();
+        assertThat(analysisDetails.parseIssueIdFromUrl("http://subdomain.sonarqube.dummy/path/issue?id=projectId&other_parameter=123")).isEmpty();
     }
 }

src/test/java/com/github/mc1arke/sonarqube/plugin/ce/pullrequest/gitlab/GitlabMergeRequestDecoratorIntegrationTest.java

@@ -178,6 +178,9 @@ private void decorateQualityGateStatus(QualityGate.Status status) {
                         discussionPostResponseBody(discussionId + 6,
                                 discussionNote(noteId + 9, user, "Resolved Sonarqube issue with response comment from other user so discussion can't be closed\\n[View in SonarQube](https://sonarqube.dummy/project/issues?id=" + projectKey + "&pullRequest=1234&issues=oldid&open=oldid)", true, false),
                                 discussionNote(noteId + 10, "other", "Comment from other user", true, false)) +
+                        "," +
+                        discussionPostResponseBody(discussionId + 7,
+                                discussionNote(noteId + 11, user, "Sonarqube issue for anther project\\n[View in SonarQube](https://sonarqube.dummy/project/issues?id=abcd-" + projectKey + "&pullRequest=1234&issues=oldid&open=oldid)", true, false)) +
                         "]")));
 
         wireMockRule.stubFor(post(urlPathEqualTo("/api/v4/projects/" + sourceProjectId + "/merge_requests/" + mergeRequestIid + "/discussions/" + discussionId + "/notes"))