From fdd609d2567e46fab56ed8b8e82aa0a7b290fa23 Mon Sep 17 00:00:00 2001 From: =?utf8?q?L=C3=A9o=20Geoffroy?= Date: Thu, 21 Dec 2023 13:37:39 +0100 Subject: [PATCH] SONAR-21259 Increase range of issue status tracked as issue fixed and refactor code --- .../issue/IntegrateIssuesVisitorIT.java | 30 ++++++++++++++-- .../issue/IntegrateIssuesVisitor.java | 36 +++++++++++++------ .../issue/IssueTrackingDelegator.java | 5 +-- .../projectanalysis/issue/IssueVisitor.java | 3 +- .../projectanalysis/issue/IssueVisitors.java | 5 +-- .../issue/PullRequestTrackerExecution.java | 10 +++--- .../issue/IssueTrackingDelegatorTest.java | 13 ++++--- .../PullRequestTrackerExecutionTest.java | 13 +++---- 8 files changed, 78 insertions(+), 37 deletions(-) diff --git a/server/sonar-ce-task-projectanalysis/src/it/java/org/sonar/ce/task/projectanalysis/issue/IntegrateIssuesVisitorIT.java b/server/sonar-ce-task-projectanalysis/src/it/java/org/sonar/ce/task/projectanalysis/issue/IntegrateIssuesVisitorIT.java index 1b9af4c9f34..7f87fe8baa7 100644 --- a/server/sonar-ce-task-projectanalysis/src/it/java/org/sonar/ce/task/projectanalysis/issue/IntegrateIssuesVisitorIT.java +++ b/server/sonar-ce-task-projectanalysis/src/it/java/org/sonar/ce/task/projectanalysis/issue/IntegrateIssuesVisitorIT.java @@ -55,6 +55,7 @@ import org.sonar.ce.task.projectanalysis.source.SourceLinesHashRepository; import org.sonar.core.issue.DefaultIssue; import org.sonar.core.issue.FieldDiffs; import org.sonar.core.issue.IssueChangeContext; +import org.sonar.core.issue.tracking.Input; import org.sonar.core.issue.tracking.Tracker; import org.sonar.db.DbClient; import org.sonar.db.DbTester; @@ -157,14 +158,14 @@ public class IntegrateIssuesVisitorIT { ClosedIssuesInputFactory closedIssuesInputFactory = new ClosedIssuesInputFactory(issuesLoader, dbClient, movedFilesRepository); TrackerExecution tracker = new TrackerExecution(baseInputFactory, closedIssuesInputFactory, new Tracker<>(), issuesLoader, analysisMetadataHolder); ReferenceBranchTrackerExecution mergeBranchTracker = new ReferenceBranchTrackerExecution(mergeInputFactory, new Tracker<>()); - PullRequestTrackerExecution prBranchTracker = new PullRequestTrackerExecution(baseInputFactory, targetInputFactory, new Tracker<>(), newLinesRepository); + PullRequestTrackerExecution prBranchTracker = new PullRequestTrackerExecution(baseInputFactory, new Tracker<>(), newLinesRepository); IssueTrackingDelegator trackingDelegator = new IssueTrackingDelegator(prBranchTracker, mergeBranchTracker, tracker, analysisMetadataHolder); treeRootHolder.setRoot(PROJECT); protoIssueCache = new ProtoIssueCache(temp.newFile(), System2.INSTANCE); when(issueFilter.accept(any(DefaultIssue.class), eq(FILE))).thenReturn(true); when(issueChangeContext.date()).thenReturn(new Date()); underTest = new IntegrateIssuesVisitor(protoIssueCache, rawInputFactory, baseInputFactory, issueLifecycle, issueVisitors, trackingDelegator, issueStatusCopier, - referenceBranchComponentUuids, mock(PullRequestSourceBranchMerger.class), fileStatuses, analysisMetadataHolder); + referenceBranchComponentUuids, mock(PullRequestSourceBranchMerger.class), fileStatuses, analysisMetadataHolder, targetInputFactory); } @Test @@ -229,6 +230,28 @@ public class IntegrateIssuesVisitorIT { assertThat(defaultIssueCaptor.getValue().ruleKey().rule()).isEqualTo("x1"); } + @Test + public void visitAny_whenIsPullRequest_shouldCallExpectedVisitorsRawIssues() { + when(analysisMetadataHolder.isPullRequest()).thenReturn(true); + when(targetBranchComponentUuids.hasTargetBranchAnalysis()).thenReturn(true); + + ruleRepositoryRule.add(RuleTesting.XOO_X1); + ScannerReport.Issue reportIssue = getReportIssue(RuleTesting.XOO_X1); + reportReader.putIssues(FILE_REF, singletonList(reportIssue)); + + IssueDto otherBranchIssueDto = addBaseIssueOnBranch(RuleTesting.XOO_X1); + when(targetBranchComponentUuids.getTargetBranchComponentUuid(FILE.getKey())).thenReturn(otherBranchIssueDto.getComponentUuid()); + + underTest.visitAny(FILE); + + ArgumentCaptor> targetInputCaptor = ArgumentCaptor.forClass(Input.class); + ArgumentCaptor> rawInputCaptor = ArgumentCaptor.forClass(Input.class); + verify(issueVisitor).onRawIssues(eq(FILE), rawInputCaptor.capture(), targetInputCaptor.capture()); + assertThat(rawInputCaptor.getValue().getIssues()).extracting(i -> i.ruleKey().rule()).containsExactly("x1"); + assertThat(targetInputCaptor.getValue().getIssues()).extracting(DefaultIssue::key).containsExactly(otherBranchIssueDto.getKee()); + + } + @Test public void close_unmatched_base_issue() { RuleKey ruleKey = RuleTesting.XOO_X1; @@ -334,7 +357,7 @@ public class IntegrateIssuesVisitorIT { dbTester.getSession().commit(); } - private void addBaseIssueOnBranch(RuleKey ruleKey) { + private IssueDto addBaseIssueOnBranch(RuleKey ruleKey) { ComponentDto project = ComponentTesting.newPrivateProjectDto(PROJECT_UUID_ON_BRANCH).setKey(PROJECT_KEY); ComponentDto file = ComponentTesting.newFileDto(project, null, FILE_UUID_ON_BRANCH).setKey(FILE_KEY); dbTester.components().insertComponents(project, file); @@ -349,6 +372,7 @@ public class IntegrateIssuesVisitorIT { .setChecksum(null); dbTester.getDbClient().issueDao().insert(dbTester.getSession(), issue); dbTester.getSession().commit(); + return issue; } @NotNull diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IntegrateIssuesVisitor.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IntegrateIssuesVisitor.java index 55f68ee47bb..854c57436c7 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IntegrateIssuesVisitor.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IntegrateIssuesVisitor.java @@ -26,6 +26,8 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.stream.Stream; +import javax.annotation.CheckForNull; +import javax.annotation.Nullable; import org.sonar.api.rule.RuleKey; import org.sonar.ce.task.projectanalysis.analysis.Analysis; import org.sonar.ce.task.projectanalysis.analysis.AnalysisMetadataHolder; @@ -54,6 +56,8 @@ public class IntegrateIssuesVisitor extends TypeAwareVisitorAdapter { private final PullRequestSourceBranchMerger pullRequestSourceBranchMerger; private final FileStatuses fileStatuses; private final AnalysisMetadataHolder analysisMetadataHolder; + private final TrackerTargetBranchInputFactory targetInputFactory; + public IntegrateIssuesVisitor( ProtoIssueCache protoIssueCache, @@ -66,7 +70,9 @@ public class IntegrateIssuesVisitor extends TypeAwareVisitorAdapter { ReferenceBranchComponentUuids referenceBranchComponentUuids, PullRequestSourceBranchMerger pullRequestSourceBranchMerger, FileStatuses fileStatuses, - AnalysisMetadataHolder analysisMetadataHolder) { + AnalysisMetadataHolder analysisMetadataHolder, + TrackerTargetBranchInputFactory targetInputFactory + ) { super(CrawlerDepthLimit.FILE, POST_ORDER); this.protoIssueCache = protoIssueCache; @@ -80,6 +86,7 @@ public class IntegrateIssuesVisitor extends TypeAwareVisitorAdapter { this.pullRequestSourceBranchMerger = pullRequestSourceBranchMerger; this.fileStatuses = fileStatuses; this.analysisMetadataHolder = analysisMetadataHolder; + this.targetInputFactory = targetInputFactory; } @Override @@ -87,9 +94,10 @@ public class IntegrateIssuesVisitor extends TypeAwareVisitorAdapter { try (CacheAppender cacheAppender = protoIssueCache.newAppender()) { issueVisitors.beforeComponent(component); Input rawInput = rawInputFactory.create(component); - List issues = getIssues(rawInput, component); + Input targetInput = createTargetInputIfExist(component); + List issues = getIssues(rawInput, targetInput, component); - issueVisitors.onRawIssues(component, rawInput); + issueVisitors.onRawIssues(component, rawInput, targetInput); processIssues(component, issues); issueVisitors.beforeCaching(component); @@ -100,21 +108,29 @@ public class IntegrateIssuesVisitor extends TypeAwareVisitorAdapter { } } - private List getIssues(Input rawInput, Component component) { + @CheckForNull + private Input createTargetInputIfExist(Component component) { + if (targetInputFactory.hasTargetBranchAnalysis()) { + return targetInputFactory.createForTargetBranch(component); + } + return null; + } + + private List getIssues(Input rawInput, @Nullable Input targetInput, Component component) { if (fileStatuses.isDataUnchanged(component)) { // we assume there's a previous analysis of the same branch - return getIssuesForUnchangedFile(rawInput, component); + return getIssuesForUnchangedFile(rawInput, targetInput, component); } else { - return getRawIssues(rawInput, component); + return getRawIssues(rawInput, targetInput, component); } } - private List getIssuesForUnchangedFile(Input rawInput, Component component) { + private List getIssuesForUnchangedFile(Input rawInput, @Nullable Input targetInput, Component component) { Input baseIssues = baseInputFactory.create(component); Collection issues = baseIssues.getIssues(); //In case of plugin update, issue impacts are potentially updated. We want to avoid incremental analysis in this case. return hasAnyInvolvedPluginChangedSinceLastAnalysis(issues) - ? getRawIssues(rawInput, component) + ? getRawIssues(rawInput, targetInput, component) : new LinkedList<>(issues); } @@ -134,8 +150,8 @@ public class IntegrateIssuesVisitor extends TypeAwareVisitorAdapter { .toList(); } - private List getRawIssues(Input rawInput, Component component) { - TrackingResult tracking = issueTracking.track(component, rawInput); + private List getRawIssues(Input rawInput, @Nullable Input targetInput, Component component) { + TrackingResult tracking = issueTracking.track(component, rawInput, targetInput); var newOpenIssues = fillNewOpenIssues(component, tracking.newIssues(), rawInput); var existingOpenIssues = fillExistingOpenIssues(tracking.issuesToMerge()); var closedIssues = closeIssues(tracking.issuesToClose()); diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueTrackingDelegator.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueTrackingDelegator.java index 008890585b6..a11f62c07d3 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueTrackingDelegator.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueTrackingDelegator.java @@ -19,6 +19,7 @@ */ package org.sonar.ce.task.projectanalysis.issue; +import javax.annotation.Nullable; import org.sonar.ce.task.projectanalysis.analysis.AnalysisMetadataHolder; import org.sonar.ce.task.projectanalysis.component.Component; import org.sonar.core.issue.DefaultIssue; @@ -42,9 +43,9 @@ public class IssueTrackingDelegator { this.analysisMetadataHolder = analysisMetadataHolder; } - public TrackingResult track(Component component, Input rawInput) { + public TrackingResult track(Component component, Input rawInput, @Nullable Input targetInput) { if (analysisMetadataHolder.isPullRequest()) { - return standardResult(pullRequestTracker.track(component, rawInput)); + return standardResult(pullRequestTracker.track(component, rawInput, targetInput)); } if (isFirstAnalysisSecondaryBranch()) { diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueVisitor.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueVisitor.java index d8fc94b4551..69d49daf8a8 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueVisitor.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueVisitor.java @@ -19,6 +19,7 @@ */ package org.sonar.ce.task.projectanalysis.issue; +import javax.annotation.Nullable; import org.sonar.ce.task.projectanalysis.component.Component; import org.sonar.core.issue.DefaultIssue; import org.sonar.core.issue.tracking.Input; @@ -48,7 +49,7 @@ public abstract class IssueVisitor { /** * This method is called for all raw issues of a component before tracking is done. */ - public void onRawIssues(Component component, Input rawIssues) { + public void onRawIssues(Component component, Input rawIssues, @Nullable Input baseIssues) { } diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueVisitors.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueVisitors.java index 6fadb057e7d..4b464da8ac6 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueVisitors.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueVisitors.java @@ -19,6 +19,7 @@ */ package org.sonar.ce.task.projectanalysis.issue; +import javax.annotation.Nullable; import org.sonar.ce.task.projectanalysis.component.Component; import org.sonar.core.issue.DefaultIssue; import org.sonar.core.issue.tracking.Input; @@ -55,9 +56,9 @@ public class IssueVisitors { } } - public void onRawIssues(Component component, Input rawIssues) { + public void onRawIssues(Component component, Input rawIssues, @Nullable Input targetIssues) { for (IssueVisitor visitor : visitors) { - visitor.onRawIssues(component, rawIssues); + visitor.onRawIssues(component, rawIssues, targetIssues); } } diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/PullRequestTrackerExecution.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/PullRequestTrackerExecution.java index b6bc6c41efd..d59f0dca386 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/PullRequestTrackerExecution.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/PullRequestTrackerExecution.java @@ -24,6 +24,7 @@ import java.util.Collections; import java.util.List; import java.util.Optional; import java.util.Set; +import javax.annotation.Nullable; import org.sonar.api.issue.Issue; import org.sonar.ce.task.projectanalysis.component.Component; import org.sonar.ce.task.projectanalysis.source.NewLinesRepository; @@ -36,25 +37,22 @@ public class PullRequestTrackerExecution { private final TrackerBaseInputFactory baseInputFactory; private final Tracker tracker; private final NewLinesRepository newLinesRepository; - private final TrackerTargetBranchInputFactory targetInputFactory; - public PullRequestTrackerExecution(TrackerBaseInputFactory baseInputFactory, TrackerTargetBranchInputFactory targetInputFactory, + public PullRequestTrackerExecution(TrackerBaseInputFactory baseInputFactory, Tracker tracker, NewLinesRepository newLinesRepository) { this.baseInputFactory = baseInputFactory; - this.targetInputFactory = targetInputFactory; this.tracker = tracker; this.newLinesRepository = newLinesRepository; } - public Tracking track(Component component, Input rawInput) { + public Tracking track(Component component, Input rawInput, @Nullable Input targetInput) { // Step 1: only keep issues on changed lines List filteredRaws = keepIssuesHavingAtLeastOneLocationOnChangedLines(component, rawInput.getIssues()); Input unmatchedRawsAfterChangedLineFiltering = createInput(rawInput, filteredRaws); // Step 2: remove issues that are resolved in the target branch Input unmatchedRawsAfterTargetResolvedTracking; - if (targetInputFactory.hasTargetBranchAnalysis()) { - Input targetInput = targetInputFactory.createForTargetBranch(component); + if (targetInput != null) { List resolvedTargetIssues = targetInput.getIssues().stream().filter(i -> Issue.STATUS_RESOLVED.equals(i.status())).toList(); Input resolvedTargetInput = createInput(targetInput, resolvedTargetIssues); Tracking prResolvedTracking = tracker.trackNonClosed(unmatchedRawsAfterChangedLineFiltering, resolvedTargetInput); diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/IssueTrackingDelegatorTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/IssueTrackingDelegatorTest.java index 6cc9a0efe72..8b63e4563ff 100644 --- a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/IssueTrackingDelegatorTest.java +++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/IssueTrackingDelegatorTest.java @@ -52,6 +52,9 @@ public class IssueTrackingDelegatorTest { @Mock private Input rawInput; + @Mock + private Input targetInput; + private IssueTrackingDelegator underTest; @Before @@ -60,14 +63,14 @@ public class IssueTrackingDelegatorTest { underTest = new IssueTrackingDelegator(prBranchTracker, mergeBranchTracker, tracker, analysisMetadataHolder); when(tracker.track(component, rawInput)).thenReturn(trackingResult); when(mergeBranchTracker.track(component, rawInput)).thenReturn(trackingResult); - when(prBranchTracker.track(component, rawInput)).thenReturn(trackingResult); + when(prBranchTracker.track(component, rawInput, targetInput)).thenReturn(trackingResult); } @Test public void delegate_regular_tracker() { when(analysisMetadataHolder.getBranch()).thenReturn(mock(Branch.class)); - underTest.track(component, rawInput); + underTest.track(component, rawInput, targetInput); verify(tracker).track(component, rawInput); verifyNoInteractions(prBranchTracker); @@ -82,7 +85,7 @@ public class IssueTrackingDelegatorTest { when(analysisMetadataHolder.getBranch()).thenReturn(branch); when(analysisMetadataHolder.isFirstAnalysis()).thenReturn(true); - underTest.track(component, rawInput); + underTest.track(component, rawInput, targetInput); verify(mergeBranchTracker).track(component, rawInput); verifyNoInteractions(tracker); @@ -97,9 +100,9 @@ public class IssueTrackingDelegatorTest { when(analysisMetadataHolder.getBranch()).thenReturn(mock(Branch.class)); when(analysisMetadataHolder.isPullRequest()).thenReturn(true); - underTest.track(component, rawInput); + underTest.track(component, rawInput, targetInput); - verify(prBranchTracker).track(component, rawInput); + verify(prBranchTracker).track(component, rawInput, targetInput); verifyNoInteractions(tracker); verifyNoInteractions(mergeBranchTracker); } diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/PullRequestTrackerExecutionTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/PullRequestTrackerExecutionTest.java index 67d8aba79e9..143d85967dd 100644 --- a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/PullRequestTrackerExecutionTest.java +++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/PullRequestTrackerExecutionTest.java @@ -65,15 +65,13 @@ public class PullRequestTrackerExecutionTest { private PullRequestTrackerExecution underTest; - private TrackerTargetBranchInputFactory targetFactory = mock(TrackerTargetBranchInputFactory.class); @Before public void setUp() { when(baseFactory.create(FILE)).thenReturn(createInput(baseIssues)); - when(targetFactory.createForTargetBranch(FILE)).thenReturn(createInput(targetIssues)); Tracker tracker = new Tracker<>(); - underTest = new PullRequestTrackerExecution(baseFactory, targetFactory, tracker, newLinesRepository); + underTest = new PullRequestTrackerExecution(baseFactory, tracker, newLinesRepository); } @Test @@ -84,7 +82,7 @@ public class PullRequestTrackerExecutionTest { when(newLinesRepository.getNewLines(FILE)).thenReturn(Optional.of(new HashSet<>(Arrays.asList(1, 3)))); - Tracking tracking = underTest.track(FILE, createInput(rawIssues)); + Tracking tracking = underTest.track(FILE, createInput(rawIssues), createInput(targetIssues)); assertThat(tracking.getUnmatchedBases()).isEmpty(); assertThat(tracking.getMatchedRaws()).isEmpty(); @@ -120,7 +118,7 @@ public class PullRequestTrackerExecutionTest { when(newLinesRepository.getNewLines(FILE)).thenReturn(Optional.of(new HashSet<>(Arrays.asList(7, 10)))); - Tracking tracking = underTest.track(FILE, createInput(rawIssues)); + Tracking tracking = underTest.track(FILE, createInput(rawIssues), createInput(targetIssues)); assertThat(tracking.getUnmatchedBases()).isEmpty(); assertThat(tracking.getMatchedRaws()).isEmpty(); @@ -135,7 +133,6 @@ public class PullRequestTrackerExecutionTest { rawIssues.add(createIssue(2, RuleTesting.XOO_X2)); rawIssues.add(createIssue(3, RuleTesting.XOO_X3)); - when(targetFactory.hasTargetBranchAnalysis()).thenReturn(true); DefaultIssue resolvedIssue = createIssue(1, RuleTesting.XOO_X1).setStatus(Issue.STATUS_RESOLVED).setResolution(Issue.RESOLUTION_FALSE_POSITIVE); // will cause rawIssue0 to be ignored targetIssues.add(resolvedIssue); @@ -146,7 +143,7 @@ public class PullRequestTrackerExecutionTest { // should be matched baseIssues.add(rawIssues.get(1)); - Tracking tracking = underTest.track(FILE, createInput(rawIssues)); + Tracking tracking = underTest.track(FILE, createInput(rawIssues), createInput(targetIssues)); assertThat(tracking.getMatchedRaws()).isEqualTo(Collections.singletonMap(rawIssues.get(1), rawIssues.get(1))); assertThat(tracking.getUnmatchedRaws()).containsOnly(rawIssues.get(2)); } @@ -160,7 +157,7 @@ public class PullRequestTrackerExecutionTest { rawIssues.add(createIssue(3, RuleTesting.XOO_X3)); baseIssues.add(rawIssues.get(0)); - Tracking tracking = underTest.track(FILE, createInput(rawIssues)); + Tracking tracking = underTest.track(FILE, createInput(rawIssues), createInput(targetIssues)); assertThat(tracking.getMatchedRaws()).isEqualTo(Collections.singletonMap(rawIssues.get(0), rawIssues.get(0))); assertThat(tracking.getUnmatchedRaws()).containsOnly(rawIssues.get(2)); } -- 2.39.5