From: Julien HENRY Date: Wed, 20 Jun 2018 16:28:39 +0000 (+0200) Subject: SONAR-10868 Fix from_hotspot for external issues X-Git-Tag: 7.5~882 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=bcb9937aa3d92eb934c089d59b0ddcb6ca28f864;p=sonarqube.git SONAR-10868 Fix from_hotspot for external issues --- diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/container/ProjectAnalysisTaskContainerPopulator.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/container/ProjectAnalysisTaskContainerPopulator.java index 124589cce06..72a0233c818 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/container/ProjectAnalysisTaskContainerPopulator.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/container/ProjectAnalysisTaskContainerPopulator.java @@ -71,7 +71,6 @@ import org.sonar.ce.task.projectanalysis.issue.NewEffortAggregator; import org.sonar.ce.task.projectanalysis.issue.RemoveProcessedComponentsVisitor; import org.sonar.ce.task.projectanalysis.issue.RuleRepositoryImpl; import org.sonar.ce.task.projectanalysis.issue.RuleTagsCopier; -import org.sonar.ce.task.projectanalysis.issue.RuleTypeCopier; import org.sonar.ce.task.projectanalysis.issue.ScmAccountToUser; import org.sonar.ce.task.projectanalysis.issue.ScmAccountToUserLoader; import org.sonar.ce.task.projectanalysis.issue.ShortBranchIssueMerger; @@ -230,7 +229,6 @@ public final class ProjectAnalysisTaskContainerPopulator implements ContainerPop // order is important: RuleTypeCopier must be the first one. And DebtAggregator must be before NewDebtAggregator (new debt requires // debt) - RuleTypeCopier.class, RuleTagsCopier.class, IssueCreationDateCalculator.class, DebtCalculator.class, diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueLifecycle.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueLifecycle.java index 52855f4234b..97ab018769f 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueLifecycle.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueLifecycle.java @@ -20,9 +20,11 @@ package org.sonar.ce.task.projectanalysis.issue; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Preconditions; import java.util.Date; import java.util.Optional; import org.sonar.api.issue.Issue; +import org.sonar.api.rules.RuleType; import org.sonar.core.issue.DefaultIssue; import org.sonar.core.issue.DefaultIssueComment; import org.sonar.core.issue.FieldDiffs; @@ -44,30 +46,43 @@ public class IssueLifecycle { private final IssueWorkflow workflow; private final IssueChangeContext changeContext; + private final RuleRepository ruleRepository; private final IssueFieldsSetter updater; private final DebtCalculator debtCalculator; private final AnalysisMetadataHolder analysisMetadataHolder; - public IssueLifecycle(AnalysisMetadataHolder analysisMetadataHolder, IssueWorkflow workflow, IssueFieldsSetter updater, DebtCalculator debtCalculator) { - this(analysisMetadataHolder, IssueChangeContext.createScan(new Date(analysisMetadataHolder.getAnalysisDate())), workflow, updater, debtCalculator); + public IssueLifecycle(AnalysisMetadataHolder analysisMetadataHolder, IssueWorkflow workflow, IssueFieldsSetter updater, DebtCalculator debtCalculator, + RuleRepository ruleRepository) { + this(analysisMetadataHolder, IssueChangeContext.createScan(new Date(analysisMetadataHolder.getAnalysisDate())), workflow, updater, debtCalculator, ruleRepository); } @VisibleForTesting IssueLifecycle(AnalysisMetadataHolder analysisMetadataHolder, IssueChangeContext changeContext, IssueWorkflow workflow, IssueFieldsSetter updater, - DebtCalculator debtCalculator) { + DebtCalculator debtCalculator, RuleRepository ruleRepository) { this.analysisMetadataHolder = analysisMetadataHolder; this.workflow = workflow; this.updater = updater; this.debtCalculator = debtCalculator; this.changeContext = changeContext; + this.ruleRepository = ruleRepository; } public void initNewOpenIssue(DefaultIssue issue) { + Preconditions.checkArgument(issue.isFromExternalRuleEngine() != (issue.type() == null), "At this stage issue type should be set for and only for external issues"); issue.setKey(Uuids.create()); issue.setCreationDate(changeContext.date()); issue.setUpdateDate(changeContext.date()); issue.setStatus(Issue.STATUS_OPEN); issue.setEffort(debtCalculator.calculate(issue)); + setType(issue); + } + + private void setType(DefaultIssue issue) { + if (!issue.isFromExternalRuleEngine()) { + Rule rule = ruleRepository.getByKey(issue.ruleKey()); + issue.setType(rule.getType()); + } + issue.setIsFromHotspot(issue.type() == RuleType.SECURITY_HOTSPOT); } public void copyExistingOpenIssueFromLongLivingBranch(DefaultIssue raw, DefaultIssue base, String fromLongBranchName) { @@ -129,8 +144,14 @@ public class IssueLifecycle { } public void mergeExistingOpenIssue(DefaultIssue raw, DefaultIssue base) { + Preconditions.checkArgument(raw.isFromExternalRuleEngine() != (raw.type() == null), "At this stage issue type should be set for and only for external issues"); raw.setKey(base.key()); raw.setNew(false); + setType(raw); + if (raw.isFromHotspot() != base.isFromHotspot()) { + // This is to force DB update of the issue + raw.setChanged(true); + } copyFields(raw, base); if (base.manualSeverity()) { diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/RuleTypeCopier.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/RuleTypeCopier.java deleted file mode 100644 index cecfee91350..00000000000 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/RuleTypeCopier.java +++ /dev/null @@ -1,45 +0,0 @@ -/* - * SonarQube - * Copyright (C) 2009-2018 SonarSource SA - * mailto:info AT sonarsource DOT com - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 3 of the License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public License - * along with this program; if not, write to the Free Software Foundation, - * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. - */ -package org.sonar.ce.task.projectanalysis.issue; - -import org.sonar.api.rules.RuleType; -import org.sonar.ce.task.projectanalysis.component.Component; -import org.sonar.core.issue.DefaultIssue; - -public class RuleTypeCopier extends IssueVisitor { - - private final RuleRepository ruleRepository; - - public RuleTypeCopier(RuleRepository ruleRepository) { - this.ruleRepository = ruleRepository; - } - - @Override - public void onIssue(Component component, DefaultIssue issue) { - Rule rule = ruleRepository.getByKey(issue.ruleKey()); - if (issue.type() == null) { - if (!rule.isExternal()) { - // rule type should never be null for rules created by plugins (non-external rules) - issue.setType(rule.getType()); - } - } - issue.setIsFromHotspot(rule.getType() == RuleType.SECURITY_HOTSPOT); - } -} diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/DumbRule.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/DumbRule.java index 95a6b75c6cb..058cbf3ae66 100644 --- a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/DumbRule.java +++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/DumbRule.java @@ -125,7 +125,7 @@ public class DumbRule implements Rule { return this; } - public DumbRule setIsExtenral(boolean isExternal) { + public DumbRule setIsExternal(boolean isExternal) { this.isExternal = isExternal; return this; } diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/IssueLifecycleTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/IssueLifecycleTest.java index e76865e0c92..b7d57d42914 100644 --- a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/IssueLifecycleTest.java +++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/IssueLifecycleTest.java @@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableMap; import java.util.Date; import org.junit.Rule; import org.junit.Test; +import org.sonar.api.rules.RuleType; import org.sonar.api.utils.Duration; import org.sonar.ce.task.projectanalysis.analysis.AnalysisMetadataHolderRule; import org.sonar.ce.task.projectanalysis.analysis.Branch; @@ -48,6 +49,7 @@ import static org.sonar.api.issue.Issue.STATUS_CLOSED; import static org.sonar.api.issue.Issue.STATUS_OPEN; import static org.sonar.api.rule.Severity.BLOCKER; import static org.sonar.api.utils.DateUtils.parseDate; +import static org.sonar.db.rule.RuleTesting.XOO_X1; public class IssueLifecycleTest { @@ -55,6 +57,11 @@ public class IssueLifecycleTest { private static final Duration DEFAULT_DURATION = Duration.create(10); + DumbRule rule = new DumbRule(XOO_X1); + + @org.junit.Rule + public RuleRepositoryRule ruleRepository = new RuleRepositoryRule().add(rule); + @Rule public AnalysisMetadataHolderRule analysisMetadataHolder = new AnalysisMetadataHolderRule(); @@ -66,11 +73,31 @@ public class IssueLifecycleTest { private DebtCalculator debtCalculator = mock(DebtCalculator.class); - private IssueLifecycle underTest = new IssueLifecycle(analysisMetadataHolder, issueChangeContext, workflow, updater, debtCalculator); + private IssueLifecycle underTest = new IssueLifecycle(analysisMetadataHolder, issueChangeContext, workflow, updater, debtCalculator, ruleRepository); @Test public void initNewOpenIssue() { - DefaultIssue issue = new DefaultIssue(); + DefaultIssue issue = new DefaultIssue() + .setRuleKey(XOO_X1); + when(debtCalculator.calculate(issue)).thenReturn(DEFAULT_DURATION); + + underTest.initNewOpenIssue(issue); + + assertThat(issue.key()).isNotNull(); + assertThat(issue.creationDate()).isNotNull(); + assertThat(issue.updateDate()).isNotNull(); + assertThat(issue.status()).isEqualTo(STATUS_OPEN); + assertThat(issue.debt()).isEqualTo(DEFAULT_DURATION); + assertThat(issue.isNew()).isTrue(); + assertThat(issue.isCopied()).isFalse(); + assertThat(issue.isFromHotspot()).isFalse(); + } + + @Test + public void initNewOpenHotspot() { + rule.setType(RuleType.SECURITY_HOTSPOT); + DefaultIssue issue = new DefaultIssue() + .setRuleKey(XOO_X1); when(debtCalculator.calculate(issue)).thenReturn(DEFAULT_DURATION); underTest.initNewOpenIssue(issue); @@ -82,6 +109,7 @@ public class IssueLifecycleTest { assertThat(issue.effort()).isEqualTo(DEFAULT_DURATION); assertThat(issue.isNew()).isTrue(); assertThat(issue.isCopied()).isFalse(); + assertThat(issue.isFromHotspot()).isTrue(); } @Test @@ -215,6 +243,70 @@ public class IssueLifecycleTest { DefaultIssue raw = new DefaultIssue() .setNew(true) .setKey("RAW_KEY") + .setRuleKey(XOO_X1) + .setCreationDate(parseDate("2015-10-01")) + .setUpdateDate(parseDate("2015-10-02")) + .setCloseDate(parseDate("2015-10-03")); + + DbIssues.Locations issueLocations = DbIssues.Locations.newBuilder() + .setTextRange(DbCommons.TextRange.newBuilder() + .setStartLine(10) + .setEndLine(12) + .build()) + .build(); + DefaultIssue base = new DefaultIssue() + .setKey("BASE_KEY") + .setCreationDate(parseDate("2015-01-01")) + .setUpdateDate(parseDate("2015-01-02")) + .setCloseDate(parseDate("2015-01-03")) + .setResolution(RESOLUTION_FIXED) + .setStatus(STATUS_CLOSED) + .setSeverity(BLOCKER) + .setAssigneeUuid("base assignee uuid") + .setAuthorLogin("base author") + .setTags(newArrayList("base tag")) + .setOnDisabledRule(true) + .setSelectedAt(1000L) + .setLine(10) + .setMessage("message") + .setGap(15d) + .setEffort(Duration.create(15L)) + .setManualSeverity(false) + .setLocations(issueLocations); + + when(debtCalculator.calculate(raw)).thenReturn(DEFAULT_DURATION); + + underTest.mergeExistingOpenIssue(raw, base); + + assertThat(raw.isNew()).isFalse(); + assertThat(raw.key()).isEqualTo("BASE_KEY"); + assertThat(raw.creationDate()).isEqualTo(base.creationDate()); + assertThat(raw.updateDate()).isEqualTo(base.updateDate()); + assertThat(raw.closeDate()).isEqualTo(base.closeDate()); + assertThat(raw.resolution()).isEqualTo(RESOLUTION_FIXED); + assertThat(raw.status()).isEqualTo(STATUS_CLOSED); + assertThat(raw.assignee()).isEqualTo("base assignee uuid"); + assertThat(raw.authorLogin()).isEqualTo("base author"); + assertThat(raw.tags()).containsOnly("base tag"); + assertThat(raw.debt()).isEqualTo(DEFAULT_DURATION); + assertThat(raw.isOnDisabledRule()).isTrue(); + assertThat(raw.selectedAt()).isEqualTo(1000L); + assertThat(raw.isChanged()).isFalse(); + + verify(updater).setPastSeverity(raw, BLOCKER, issueChangeContext); + verify(updater).setPastLine(raw, 10); + verify(updater).setPastMessage(raw, "message", issueChangeContext); + verify(updater).setPastEffort(raw, Duration.create(15L), issueChangeContext); + verify(updater).setPastLocations(raw, issueLocations); + } + + @Test + public void mergeExistingOpenIssue_vulnerability_changed_to_hotspot() { + rule.setType(RuleType.SECURITY_HOTSPOT); + DefaultIssue raw = new DefaultIssue() + .setNew(true) + .setKey("RAW_KEY") + .setRuleKey(XOO_X1) .setCreationDate(parseDate("2015-10-01")) .setUpdateDate(parseDate("2015-10-02")) .setCloseDate(parseDate("2015-10-03")); @@ -227,6 +319,9 @@ public class IssueLifecycleTest { .build(); DefaultIssue base = new DefaultIssue() .setKey("BASE_KEY") + .setType(RuleType.VULNERABILITY) + // First analysis before rule was changed to hotspot + .setIsFromHotspot(false) .setCreationDate(parseDate("2015-01-01")) .setUpdateDate(parseDate("2015-01-02")) .setCloseDate(parseDate("2015-01-03")) @@ -262,6 +357,8 @@ public class IssueLifecycleTest { assertThat(raw.effort()).isEqualTo(DEFAULT_DURATION); assertThat(raw.isOnDisabledRule()).isTrue(); assertThat(raw.selectedAt()).isEqualTo(1000L); + assertThat(raw.isFromHotspot()).isTrue(); + assertThat(raw.isChanged()).isTrue(); verify(updater).setPastSeverity(raw, BLOCKER, issueChangeContext); verify(updater).setPastLine(raw, 10); @@ -274,7 +371,8 @@ public class IssueLifecycleTest { public void mergeExistingOpenIssue_with_manual_severity() { DefaultIssue raw = new DefaultIssue() .setNew(true) - .setKey("RAW_KEY"); + .setKey("RAW_KEY") + .setRuleKey(XOO_X1); DefaultIssue base = new DefaultIssue() .setKey("BASE_KEY") .setResolution(RESOLUTION_FIXED) @@ -294,7 +392,8 @@ public class IssueLifecycleTest { public void mergeExistingOpenIssue_with_attributes() { DefaultIssue raw = new DefaultIssue() .setNew(true) - .setKey("RAW_KEY"); + .setKey("RAW_KEY") + .setRuleKey(XOO_X1); DefaultIssue base = new DefaultIssue() .setKey("BASE_KEY") .setResolution(RESOLUTION_FIXED) diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/RuleTypeCopierTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/RuleTypeCopierTest.java deleted file mode 100644 index f29f69f3755..00000000000 --- a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/RuleTypeCopierTest.java +++ /dev/null @@ -1,71 +0,0 @@ -/* - * SonarQube - * Copyright (C) 2009-2018 SonarSource SA - * mailto:info AT sonarsource DOT com - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 3 of the License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public License - * along with this program; if not, write to the Free Software Foundation, - * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. - */ -package org.sonar.ce.task.projectanalysis.issue; - -import org.junit.Test; -import org.sonar.api.rules.RuleType; -import org.sonar.ce.task.projectanalysis.component.Component; -import org.sonar.core.issue.DefaultIssue; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.mock; -import static org.sonar.db.rule.RuleTesting.XOO_X1; - -public class RuleTypeCopierTest { - - DumbRule rule = new DumbRule(XOO_X1); - - @org.junit.Rule - public RuleRepositoryRule ruleRepository = new RuleRepositoryRule().add(rule); - - DefaultIssue issue = new DefaultIssue().setRuleKey(rule.getKey()); - RuleTypeCopier underTest = new RuleTypeCopier(ruleRepository); - - @Test - public void copy_rule_type_if_missing() { - rule.setType(RuleType.BUG); - - underTest.onIssue(mock(Component.class), issue); - - assertThat(issue.type()).isEqualTo(RuleType.BUG); - assertThat(issue.isFromHotspot()).isEqualTo(false); - } - - @Test - public void do_not_copy_type_if_present() { - rule.setType(RuleType.BUG); - issue.setType(RuleType.VULNERABILITY); - - underTest.onIssue(mock(Component.class), issue); - - assertThat(issue.type()).isEqualTo(RuleType.VULNERABILITY); - assertThat(issue.isFromHotspot()).isEqualTo(false); - } - - @Test - public void set_from_hotspot_flag_for_hotspot() { - rule.setType(RuleType.SECURITY_HOTSPOT); - - underTest.onIssue(mock(Component.class), issue); - - assertThat(issue.type()).isEqualTo(RuleType.SECURITY_HOTSPOT); - assertThat(issue.isFromHotspot()).isEqualTo(true); - } -} diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/TrackerRawInputFactoryTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/TrackerRawInputFactoryTest.java index e98c64d0125..c977257a543 100644 --- a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/TrackerRawInputFactoryTest.java +++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/TrackerRawInputFactoryTest.java @@ -27,6 +27,7 @@ import org.junit.Test; import org.sonar.api.issue.Issue; import org.sonar.api.rule.RuleKey; import org.sonar.api.rule.Severity; +import org.sonar.api.rules.RuleType; import org.sonar.api.utils.Duration; import org.sonar.ce.task.projectanalysis.batch.BatchReportReaderRule; import org.sonar.ce.task.projectanalysis.component.Component; @@ -194,6 +195,7 @@ public class TrackerRawInputFactoryTest { .setRuleKey("S001") .setSeverity(Constants.Severity.BLOCKER) .setEffort(20l) + .setType(ScannerReport.IssueType.SECURITY_HOTSPOT) .build(); reportReader.putExternalIssues(FILE.getReportAttributes().getRef(), asList(reportIssue)); Input input = underTest.create(FILE); @@ -208,6 +210,7 @@ public class TrackerRawInputFactoryTest { assertThat(issue.line()).isEqualTo(2); assertThat(issue.effort()).isEqualTo(Duration.create(20l)); assertThat(issue.message()).isEqualTo("the message"); + assertThat(issue.type()).isEqualTo(RuleType.SECURITY_HOTSPOT); // fields set by compute engine assertThat(issue.checksum()).isEqualTo(input.getLineHashSequence().getHashForLine(2)); @@ -224,6 +227,7 @@ public class TrackerRawInputFactoryTest { .setRuleRepository("eslint") .setRuleKey("S001") .setSeverity(Constants.Severity.BLOCKER) + .setType(ScannerReport.IssueType.BUG) .build(); reportReader.putExternalIssues(FILE.getReportAttributes().getRef(), asList(reportIssue)); Input input = underTest.create(FILE); diff --git a/sonar-scanner-protocol/src/main/protobuf/scanner_report.proto b/sonar-scanner-protocol/src/main/protobuf/scanner_report.proto index 821b35e4d9a..8a1e78dde6a 100644 --- a/sonar-scanner-protocol/src/main/protobuf/scanner_report.proto +++ b/sonar-scanner-protocol/src/main/protobuf/scanner_report.proto @@ -201,10 +201,11 @@ message ExternalIssue { } enum IssueType { - CODE_SMELL = 0; - BUG = 1; - VULNERABILITY = 2; - SECURITY_HOTSPOT = 3; + UNSET = 0; + CODE_SMELL = 1; + BUG = 2; + VULNERABILITY = 3; + SECURITY_HOTSPOT = 4; } message IssueLocation {