]> source.dussan.org Git - sonarqube.git/commitdiff
SONAR-10868 Fix from_hotspot for external issues
authorJulien HENRY <julien.henry@sonarsource.com>
Wed, 20 Jun 2018 16:28:39 +0000 (18:28 +0200)
committersonartech <sonartech@sonarsource.com>
Wed, 4 Jul 2018 07:31:04 +0000 (09:31 +0200)
server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/container/ProjectAnalysisTaskContainerPopulator.java
server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueLifecycle.java
server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/RuleTypeCopier.java [deleted file]
server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/DumbRule.java
server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/IssueLifecycleTest.java
server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/RuleTypeCopierTest.java [deleted file]
server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/TrackerRawInputFactoryTest.java
sonar-scanner-protocol/src/main/protobuf/scanner_report.proto

index 124589cce06b94d8e1520a62fbb2c8c4b5e42500..72a0233c818e1d38f85c00783cb13d8fc8ffd49a 100644 (file)
@@ -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,
index 52855f4234bab0fbdb5a4d529b9e8b75b6870763..97ab018769f42e8b339d162e8d1a5f0f0ba26ec8 100644 (file)
 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 (file)
index cecfee9..0000000
+++ /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);
-  }
-}
index 95a6b75c6cb8d7bcd325f713ab6468ba9c92cc0a..058cbf3ae6656e4ab78305a93b22a9b1282cd16c 100644 (file)
@@ -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;
   }
index e76865e0c92fddf0ed671010bf9700b50e9f36d0..b7d57d429146d768fa3d754287daad7f13de25eb 100644 (file)
@@ -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 (file)
index f29f69f..0000000
+++ /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);
-  }
-}
index e98c64d01250df3fed9e2022acf1300f818fd350..c977257a5435dbb0cae00a4137aec4c1815245b2 100644 (file)
@@ -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<DefaultIssue> 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<DefaultIssue> input = underTest.create(FILE);
index 821b35e4d9a9f64595b0869348c6e9870014a226..8a1e78dde6ac2b9b3e364385b14899bd4bf7814d 100644 (file)
@@ -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 {