SLCORE-1589 Duplicate key exception upon analysis

Author: damien-urruty-sonarsource

backend/core/src/main/java/org/sonarsource/sonarlint/core/tracking/matching/IssueMatcher.java

@@ -19,7 +19,6 @@
  */
 package org.sonarsource.sonarlint.core.tracking.matching;
 
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
@@ -52,14 +51,16 @@ public class IssueMatcher<LEFT, RIGHT> {
     // 7. match issues with same rule and same line hash
     LineHashMatchingCriterion::new);
 
-  private final Map<MatchingCriterionFactory, Map<MatchingCriterion, List<RIGHT>>> rightIssuesByCriterion = new HashMap<>();
+  private final Map<MatchingCriterionFactory, Map<MatchingCriterion, RIGHT>> rightIssuesByCriterion = new HashMap<>();
+  private final MatchingAttributesMapper<RIGHT> rightMapper;
 
   public IssueMatcher(MatchingAttributesMapper<RIGHT> rightMapper, Collection<RIGHT> rightIssues) {
+    this.rightMapper = rightMapper;
     for (var matchingCriterion : MATCHING_CRITERIA) {
-      var issuesByCriterion = new HashMap<MatchingCriterion, List<RIGHT>>();
+      var issuesByCriterion = new HashMap<MatchingCriterion, RIGHT>();
       for (RIGHT right : rightIssues) {
         var criterionAppliedToIssue = matchingCriterion.build(right, rightMapper);
-        issuesByCriterion.computeIfAbsent(criterionAppliedToIssue, k -> new ArrayList<>()).add(right);
+        issuesByCriterion.put(criterionAppliedToIssue, right);
       }
       rightIssuesByCriterion.put(matchingCriterion, issuesByCriterion);
     }
@@ -81,17 +82,21 @@ public MatchingResult<LEFT, RIGHT> matchWith(MatchingAttributesMapper<LEFT> left
   private void matchWithCriterion(MatchingResult<LEFT, RIGHT> result, MatchingAttributesMapper<LEFT> leftMapper, MatchingCriterionFactory criterionFactory) {
     for (LEFT left : result.getUnmatchedLefts()) {
       var leftKey = criterionFactory.build(left, leftMapper);
-      var rightsCandidates = rightIssuesByCriterion.get(criterionFactory).get(leftKey);
-      if (rightsCandidates != null && !rightsCandidates.isEmpty()) {
+      var match = rightIssuesByCriterion.get(criterionFactory).get(leftKey);
+      if (match != null) {
         // TODO taking the first one. Could be improved if there are more than 2 issues on the same line.
         // Message could be checked to take the best one.
-        var match = rightsCandidates.iterator().next();
         result.recordMatch(left, match);
-        MATCHING_CRITERIA.forEach(criterion -> rightIssuesByCriterion.get(criterion).remove(criterion.build(left, leftMapper)));
+        MATCHING_CRITERIA.forEach(criterion -> rightIssuesByCriterion.get(criterion).remove(criterion.build(match, rightMapper)));
+        break;
       }
     }
   }
 
+  public int getUnmatchedIssuesCount() {
+    return rightIssuesByCriterion.entrySet().iterator().next().getValue().size();
+  }
+
   private interface MatchingCriterion {
   }
 

backend/core/src/main/java/org/sonarsource/sonarlint/core/tracking/matching/MatchingSession.java

@@ -37,8 +37,6 @@
 import org.sonarsource.sonarlint.core.tracking.TrackedIssue;
 
 public class MatchingSession {
-  private final Map<Path, List<KnownFinding>> remainingUnmatchedIssuesPerFile;
-  private final Map<Path, List<KnownFinding>> remainingUnmatchedSecurityHotspotsPerFile;
   private final Map<Path, IssueMatcher<RawIssue, KnownFinding>> issueMatchersByFile = new HashMap<>();
   private final Map<Path, IssueMatcher<RawIssue, KnownFinding>> hotspotMatchersByFile = new HashMap<>();
 
@@ -49,12 +47,12 @@ public class MatchingSession {
   private long newIssuesFound = 0;
 
   public MatchingSession(KnownFindings previousFindings, IntroductionDateProvider introductionDateProvider) {
-    this.remainingUnmatchedIssuesPerFile = previousFindings.getIssuesPerFile().entrySet().stream().map(entry -> Map.entry(entry.getKey(), new ArrayList<>(entry.getValue())))
+    var knownIssuesPerFile = previousFindings.getIssuesPerFile().entrySet().stream().map(entry -> Map.entry(entry.getKey(), new ArrayList<>(entry.getValue())))
       .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
-    this.remainingUnmatchedSecurityHotspotsPerFile = previousFindings.getSecurityHotspotsPerFile().entrySet().stream()
+    var knownSecurityHotspotsPerFile = previousFindings.getSecurityHotspotsPerFile().entrySet().stream()
       .map(entry -> Map.entry(entry.getKey(), new ArrayList<>(entry.getValue()))).collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
-    remainingUnmatchedIssuesPerFile.forEach((path, issues) -> issueMatchersByFile.put(path, new IssueMatcher<>(new KnownIssueMatchingAttributesMapper(), issues)));
-    remainingUnmatchedSecurityHotspotsPerFile.forEach((path, hotspots) -> hotspotMatchersByFile.put(path, new IssueMatcher<>(new KnownIssueMatchingAttributesMapper(), hotspots)));
+    knownIssuesPerFile.forEach((path, issues) -> issueMatchersByFile.put(path, new IssueMatcher<>(new KnownIssueMatchingAttributesMapper(), issues)));
+    knownSecurityHotspotsPerFile.forEach((path, hotspots) -> hotspotMatchersByFile.put(path, new IssueMatcher<>(new KnownIssueMatchingAttributesMapper(), hotspots)));
     this.introductionDateProvider = introductionDateProvider;
   }
 
@@ -71,7 +69,7 @@ public TrackedIssue matchWithKnownSecurityHotspot(Path relativePath, RawIssue ne
     if (hotspotMatcher == null) {
       throw new IllegalStateException("No hotspot matcher found for " + relativePath);
     }
-    var trackedSecurityHotspot = matchWithKnownFinding(relativePath, hotspotMatcher, remainingUnmatchedSecurityHotspotsPerFile, newSecurityHotspot);
+    var trackedSecurityHotspot = matchWithKnownFinding(relativePath, hotspotMatcher, newSecurityHotspot);
     securityHotspotsPerFile.computeIfAbsent(relativePath, f -> new ArrayList<>()).add(trackedSecurityHotspot);
     relativePathsInvolved.add(relativePath);
     return trackedSecurityHotspot;
@@ -83,23 +81,16 @@ private TrackedIssue matchWithKnownIssue(Path relativePath, RawIssue rawIssue) {
       throw new IllegalStateException("No issue matcher found for " + relativePath);
     }
 
-    var trackedIssue = matchWithKnownFinding(relativePath, issueMatcher, remainingUnmatchedIssuesPerFile, rawIssue);
+    var trackedIssue = matchWithKnownFinding(relativePath, issueMatcher, rawIssue);
     issuesPerFile.computeIfAbsent(relativePath, f -> new ArrayList<>()).add(trackedIssue);
     relativePathsInvolved.add(relativePath);
     return trackedIssue;
   }
 
-  private TrackedIssue matchWithKnownFinding(Path relativePath, IssueMatcher<RawIssue, KnownFinding> issueMatcher,
-    Map<Path, List<KnownFinding>> remainingUnmatchedKnownFindingsPerFile, RawIssue newFinding) {
-    var remainingUnmatchedKnownFindings = remainingUnmatchedKnownFindingsPerFile.get(relativePath);
+  private TrackedIssue matchWithKnownFinding(Path relativePath, IssueMatcher<RawIssue, KnownFinding> issueMatcher, RawIssue newFinding) {
     var localMatchingResult = issueMatcher.matchWith(new RawIssueFindingMatchingAttributeMapper(), List.of(newFinding));
     return localMatchingResult.getMatchOpt(newFinding)
-      .map(knownFinding -> {
-        if (remainingUnmatchedKnownFindings != null) {
-          remainingUnmatchedKnownFindings.remove(knownFinding);
-        }
-        return updateKnownFindingWithRawIssueData(knownFinding, newFinding);
-      })
+      .map(knownFinding -> updateKnownFindingWithRawIssueData(knownFinding, newFinding))
       .orElseGet(() -> newlyKnownIssue(relativePath, newFinding));
   }
 
@@ -113,7 +104,7 @@ public static TrackedIssue updateKnownFindingWithRawIssueData(KnownFinding known
   }
 
   private TrackedIssue newlyKnownIssue(Path relativePath, RawIssue rawFinding) {
-    newIssuesFound ++;
+    newIssuesFound++;
     var introductionDate = introductionDateProvider.determineIntroductionDate(relativePath, rawFinding.getLineNumbers());
     return IssueMapper.toTrackedIssue(rawFinding, introductionDate);
   }
@@ -135,6 +126,6 @@ public long countNewIssues() {
   }
 
   public long countRemainingUnmatchedIssues() {
-    return remainingUnmatchedIssuesPerFile.values().stream().mapToLong(List::size).sum();
+    return issueMatchersByFile.values().stream().mapToLong(IssueMatcher::getUnmatchedIssuesCount).sum();
   }
 }

medium-tests/src/test/java/mediumtest/tracking/IssueTrackingMediumTests.java

@@ -31,6 +31,7 @@
 import java.util.Set;
 import java.util.UUID;
 import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
 import org.eclipse.jgit.api.errors.GitAPIException;
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.extension.ExtendWith;
@@ -915,6 +916,42 @@ void it_should_notify_client_when_analysis_finishes_without_starting(SonarLintTe
     verify(client, never()).raiseIssues(eq(CONFIG_SCOPE_ID), any(), eq(true), any());
   }
 
+  @SonarLintTest
+  void it_should_not_match_the_same_issue_twice(SonarLintTestHarness harness, @TempDir Path baseDir) {
+    var filePath = createFile(baseDir, baseDir.resolve("file.py").toString(),
+      """
+        # TODO try this
+        # TODO try that
+        """);
+    var fileUri = filePath.toUri();
+    var client = harness.newFakeClient()
+      .withInitialFs(CONFIG_SCOPE_ID, baseDir, List.of(new ClientFileDto(fileUri, baseDir.relativize(filePath), CONFIG_SCOPE_ID, false, null, filePath, null, null, true)))
+      .build();
+    var backend = harness.newBackend()
+      .withUnboundConfigScope(CONFIG_SCOPE_ID)
+      .withStandaloneEmbeddedPluginAndEnabledLanguage(TestPlugin.PYTHON)
+      .start(client);
+
+    var raisedIssueDtos = analyzeFileAndGetAllIssues(backend, fileUri, client);
+    assertThat(raisedIssueDtos).hasSize(2);
+
+    changeFileContent(baseDir, filePath.getFileName().toString(), """
+        # TODO try that
+      """);
+    raisedIssueDtos = analyzeFileAndGetAllIssues(backend, fileUri, client);
+    assertThat(raisedIssueDtos).hasSize(1);
+
+    changeFileContent(baseDir, filePath.getFileName().toString(), """
+        # TODO try this
+        # TODO try that
+      """);
+    raisedIssueDtos = analyzeFileAndGetAllIssues(backend, fileUri, client);
+    assertThat(raisedIssueDtos)
+      .hasSize(2);
+    assertThat(raisedIssueDtos.stream().map(RaisedFindingDto::getId).collect(Collectors.toSet()))
+      .hasSize(2);
+  }
+
   private List<RaisedIssueDto> analyzeFileAndGetAllIssuesOfRule(SonarLintTestRpcServer backend, URI fileUri, SonarLintRpcClientDelegate client, String ruleKey) {
     var analysisId = UUID.randomUUID();
     var analysisResult = backend.getAnalysisService().analyzeFilesAndTrack(