From 072791ac2b31eb8f01b686b754c930a1ff4a600e Mon Sep 17 00:00:00 2001 From: =?utf8?q?L=C3=A9o=20Geoffroy?= Date: Fri, 18 Aug 2023 14:03:55 +0200 Subject: [PATCH] SONAR-20021 Fix Quality gate failure and issue with adhoc rule definition --- .../rule/OneExternalIssuePerLineSensor.java | 7 +++ .../rule/internal/DefaultAdHocRule.java | 17 ++++--- .../rule/internal/DefaultAdHocRuleTest.java | 27 +++++++++-- .../scanner/sensor/DefaultSensorStorage.java | 31 ++++++++++++- .../sensor/DefaultSensorStorageTest.java | 46 +++++++++++++++++++ 5 files changed, 116 insertions(+), 12 deletions(-) diff --git a/plugins/sonar-xoo-plugin/src/main/java/org/sonar/xoo/rule/OneExternalIssuePerLineSensor.java b/plugins/sonar-xoo-plugin/src/main/java/org/sonar/xoo/rule/OneExternalIssuePerLineSensor.java index 03a18f350b2..84a11798a15 100644 --- a/plugins/sonar-xoo-plugin/src/main/java/org/sonar/xoo/rule/OneExternalIssuePerLineSensor.java +++ b/plugins/sonar-xoo-plugin/src/main/java/org/sonar/xoo/rule/OneExternalIssuePerLineSensor.java @@ -27,6 +27,8 @@ import org.sonar.api.batch.sensor.Sensor; import org.sonar.api.batch.sensor.SensorContext; import org.sonar.api.batch.sensor.SensorDescriptor; import org.sonar.api.batch.sensor.issue.NewExternalIssue; +import org.sonar.api.issue.impact.SoftwareQuality; +import org.sonar.api.rules.CleanCodeAttribute; import org.sonar.api.rules.RuleType; import org.sonar.xoo.Xoo; @@ -63,6 +65,9 @@ public class OneExternalIssuePerLineSensor implements Sensor { .description("blah blah") .severity(Severity.BLOCKER) .type(RuleType.BUG) + .cleanCodeAttribute(CleanCodeAttribute.CLEAR) + .addDefaultImpact(SoftwareQuality.MAINTAINABILITY, org.sonar.api.issue.impact.Severity.MEDIUM) + .addDefaultImpact(SoftwareQuality.RELIABILITY, org.sonar.api.issue.impact.Severity.LOW) .save(); } } @@ -78,6 +83,8 @@ public class OneExternalIssuePerLineSensor implements Sensor { .at(file.selectLine(line)) .message("This issue is generated on each line")) .severity(Severity.valueOf(SEVERITY)) + //Overrides default impact from the adhoc rule + .addImpact(SoftwareQuality.MAINTAINABILITY, org.sonar.api.issue.impact.Severity.HIGH) .remediationEffortMinutes(EFFORT) .type(TYPE) .save(); diff --git a/sonar-plugin-api-impl/src/main/java/org/sonar/api/batch/sensor/rule/internal/DefaultAdHocRule.java b/sonar-plugin-api-impl/src/main/java/org/sonar/api/batch/sensor/rule/internal/DefaultAdHocRule.java index 2edc54cf8a3..389a81361f1 100644 --- a/sonar-plugin-api-impl/src/main/java/org/sonar/api/batch/sensor/rule/internal/DefaultAdHocRule.java +++ b/sonar-plugin-api-impl/src/main/java/org/sonar/api/batch/sensor/rule/internal/DefaultAdHocRule.java @@ -19,7 +19,7 @@ */ package org.sonar.api.batch.sensor.rule.internal; -import java.util.Collections; +import java.util.EnumMap; import java.util.Map; import javax.annotation.CheckForNull; import javax.annotation.Nullable; @@ -43,6 +43,10 @@ public class DefaultAdHocRule extends DefaultStorable implements AdHocRule, NewA private String engineId; private String ruleId; + private CleanCodeAttribute cleanCodeAttribute; + + private Map impacts = new EnumMap<>(SoftwareQuality.class); + public DefaultAdHocRule() { super(null); } @@ -59,6 +63,7 @@ public class DefaultAdHocRule extends DefaultStorable implements AdHocRule, NewA @Override public DefaultAdHocRule addDefaultImpact(SoftwareQuality softwareQuality, org.sonar.api.issue.impact.Severity severity) { + impacts.put(softwareQuality, severity); return this; } @@ -93,8 +98,7 @@ public class DefaultAdHocRule extends DefaultStorable implements AdHocRule, NewA checkState(isNotBlank(engineId), "Engine id is mandatory on ad hoc rule"); checkState(isNotBlank(ruleId), "Rule id is mandatory on ad hoc rule"); checkState(isNotBlank(name), "Name is mandatory on every ad hoc rule"); - checkState(severity != null, "Severity is mandatory on every ad hoc rule"); - checkState(type != null, "Type is mandatory on every ad hoc rule"); + checkState(!impacts.isEmpty() || (severity != null && type != null), "Impact should be provided, or Severity and Type instead"); storage.store(this); } @@ -105,13 +109,13 @@ public class DefaultAdHocRule extends DefaultStorable implements AdHocRule, NewA @Override public Map defaultImpacts() { - return Collections.emptyMap(); + return impacts; } @CheckForNull @Override public CleanCodeAttribute cleanCodeAttribute() { - return null; + return cleanCodeAttribute; } @Override @@ -145,7 +149,8 @@ public class DefaultAdHocRule extends DefaultStorable implements AdHocRule, NewA } @Override - public DefaultAdHocRule cleanCodeAttribute(CleanCodeAttribute attribute) { + public DefaultAdHocRule cleanCodeAttribute(CleanCodeAttribute cleanCodeAttribute) { + this.cleanCodeAttribute = cleanCodeAttribute; return this; } diff --git a/sonar-plugin-api-impl/src/test/java/org/sonar/api/batch/sensor/rule/internal/DefaultAdHocRuleTest.java b/sonar-plugin-api-impl/src/test/java/org/sonar/api/batch/sensor/rule/internal/DefaultAdHocRuleTest.java index 354a729c0bb..8cd63fe79c4 100644 --- a/sonar-plugin-api-impl/src/test/java/org/sonar/api/batch/sensor/rule/internal/DefaultAdHocRuleTest.java +++ b/sonar-plugin-api-impl/src/test/java/org/sonar/api/batch/sensor/rule/internal/DefaultAdHocRuleTest.java @@ -23,6 +23,8 @@ import org.junit.Test; import org.sonar.api.batch.rule.Severity; import org.sonar.api.batch.sensor.internal.SensorStorage; import org.sonar.api.batch.sensor.rule.NewAdHocRule; +import org.sonar.api.issue.impact.SoftwareQuality; +import org.sonar.api.rules.CleanCodeAttribute; import org.sonar.api.rules.RuleType; import static org.assertj.core.api.Assertions.assertThat; @@ -42,7 +44,9 @@ public class DefaultAdHocRuleTest { .name("name") .description("desc") .severity(Severity.BLOCKER) - .type(RuleType.CODE_SMELL); + .type(RuleType.CODE_SMELL) + .addDefaultImpact(SoftwareQuality.MAINTAINABILITY, org.sonar.api.issue.impact.Severity.HIGH) + .cleanCodeAttribute(CleanCodeAttribute.CONVENTIONAL); rule.save(); assertThat(rule.engineId()).isEqualTo("engine"); @@ -51,7 +55,8 @@ public class DefaultAdHocRuleTest { assertThat(rule.description()).isEqualTo("desc"); assertThat(rule.severity()).isEqualTo(Severity.BLOCKER); assertThat(rule.type()).isEqualTo(RuleType.CODE_SMELL); - + assertThat(rule.defaultImpacts()).containsEntry(SoftwareQuality.MAINTAINABILITY, org.sonar.api.issue.impact.Severity.HIGH); + assertThat(rule.cleanCodeAttribute()).isEqualTo(CleanCodeAttribute.CONVENTIONAL); verify(storage).store(any(DefaultAdHocRule.class)); } @@ -69,6 +74,20 @@ public class DefaultAdHocRuleTest { verify(storage).store(any(DefaultAdHocRule.class)); } + @Test + public void type_and_severity_are_optional() { + SensorStorage storage = mock(SensorStorage.class); + new DefaultAdHocRule(storage) + .engineId("engine") + .ruleId("ruleId") + .name("name") + .addDefaultImpact(SoftwareQuality.MAINTAINABILITY, org.sonar.api.issue.impact.Severity.HIGH) + .save(); + + verify(storage).store(any(DefaultAdHocRule.class)); + } + + @Test public void fail_to_store_if_no_engine_id() { SensorStorage storage = mock(SensorStorage.class); @@ -129,7 +148,7 @@ public class DefaultAdHocRuleTest { assertThatThrownBy(() -> rule.save()) .isInstanceOf(IllegalStateException.class) - .hasMessageContaining("Severity is mandatory"); + .hasMessageContaining("Impact should be provided, or Severity and Type instead"); } @Test @@ -144,6 +163,6 @@ public class DefaultAdHocRuleTest { assertThatThrownBy(() -> rule.save()) .isInstanceOf(IllegalStateException.class) - .hasMessageContaining("Type is mandatory"); + .hasMessageContaining("Impact should be provided, or Severity and Type instead"); } } diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/sensor/DefaultSensorStorage.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/sensor/DefaultSensorStorage.java index cdd771266b5..e679d683df9 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/sensor/DefaultSensorStorage.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/sensor/DefaultSensorStorage.java @@ -57,7 +57,11 @@ import org.sonar.api.batch.sensor.rule.AdHocRule; import org.sonar.api.batch.sensor.symbol.NewSymbolTable; import org.sonar.api.batch.sensor.symbol.internal.DefaultSymbolTable; import org.sonar.api.config.Configuration; +import org.sonar.api.issue.impact.Severity; +import org.sonar.api.issue.impact.SoftwareQuality; import org.sonar.api.measures.CoreMetrics; +import org.sonar.api.rules.CleanCodeAttribute; +import org.sonar.api.rules.RuleType; import org.sonar.api.utils.KeyValueFormat; import org.sonar.core.metric.ScannerMetrics; import org.sonar.core.util.CloseableIterator; @@ -257,11 +261,34 @@ public class DefaultSensorStorage implements SensorStorage { if (description != null) { builder.setDescription(description); } - builder.setSeverity(Constants.Severity.valueOf(adHocRule.severity().name())); - builder.setType(ScannerReport.IssueType.valueOf(adHocRule.type().name())); + + + org.sonar.api.batch.rule.Severity severity = adHocRule.severity(); + if (severity != null) { + builder.setSeverity(Constants.Severity.valueOf(severity.name())); + } + + RuleType type = adHocRule.type(); + if (type != null) { + builder.setType(ScannerReport.IssueType.valueOf(type.name())); + } + builder.addAllDefaultImpacts(mapImpacts(adHocRule.defaultImpacts())); + + CleanCodeAttribute cleanCodeAttribute = adHocRule.cleanCodeAttribute(); + if (cleanCodeAttribute != null) { + builder.setCleanCodeAttribute(cleanCodeAttribute.name()); + } writer.appendAdHocRule(builder.build()); } + private static List mapImpacts(Map impactsMap) { + return impactsMap.entrySet().stream() + .map(e -> ScannerReport.Impact.newBuilder() + .setSoftwareQuality(e.getKey().name()) + .setSeverity(e.getValue().name()).build()) + .collect(toList()); + } + @Override public void store(NewHighlighting newHighlighting) { DefaultHighlighting highlighting = (DefaultHighlighting) newHighlighting; diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/sensor/DefaultSensorStorageTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/sensor/DefaultSensorStorageTest.java index 10f0b9662e8..46cc94f03cf 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/sensor/DefaultSensorStorageTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/sensor/DefaultSensorStorageTest.java @@ -23,6 +23,7 @@ import java.io.File; import java.io.IOException; import java.util.ArrayList; import java.util.List; +import org.assertj.core.groups.Tuple; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -46,12 +47,19 @@ import org.sonar.api.batch.sensor.issue.internal.DefaultExternalIssue; import org.sonar.api.batch.sensor.issue.internal.DefaultIssue; import org.sonar.api.batch.sensor.issue.internal.DefaultIssueLocation; import org.sonar.api.batch.sensor.measure.internal.DefaultMeasure; +import org.sonar.api.batch.sensor.rule.internal.DefaultAdHocRule; import org.sonar.api.batch.sensor.symbol.internal.DefaultSymbolTable; import org.sonar.api.config.internal.MapSettings; +import org.sonar.api.issue.impact.Severity; +import org.sonar.api.issue.impact.SoftwareQuality; import org.sonar.api.measures.CoreMetrics; +import org.sonar.api.rules.CleanCodeAttribute; +import org.sonar.api.rules.RuleType; import org.sonar.core.metric.ScannerMetrics; +import org.sonar.core.util.CloseableIterator; import org.sonar.scanner.cpd.index.SonarCpdBlockIndex; import org.sonar.scanner.issue.IssuePublisher; +import org.sonar.scanner.protocol.Constants; import org.sonar.scanner.protocol.output.FileStructure; import org.sonar.scanner.protocol.output.ScannerReport; import org.sonar.scanner.protocol.output.ScannerReportReader; @@ -347,4 +355,42 @@ public class DefaultSensorStorageTest { assertThat(contextPropertiesCache.getAll()).containsOnly(entry("foo", "bar")); } + @Test + public void store_whenAdhocRuleIsSpecified_shouldWriteAdhocRuleToReport() throws IOException { + + underTest.store(new DefaultAdHocRule().ruleId("ruleId").engineId("engineId") + .name("name") + .addDefaultImpact(SoftwareQuality.MAINTAINABILITY, Severity.HIGH) + .addDefaultImpact(SoftwareQuality.RELIABILITY, Severity.MEDIUM) + .cleanCodeAttribute(CleanCodeAttribute.CLEAR) + .severity(org.sonar.api.batch.rule.Severity.MAJOR) + .type(RuleType.CODE_SMELL) + .description("description")); + + try (CloseableIterator adhocRuleIt = reportReader.readAdHocRules()) { + ScannerReport.AdHocRule adhocRule = adhocRuleIt.next(); + assertThat(adhocRule).extracting(r -> r.getRuleId(), r -> r.getName(), r -> r.getSeverity(), r -> r.getType(), r -> r.getDescription()) + .containsExactlyInAnyOrder("ruleId", "name", Constants.Severity.MAJOR, ScannerReport.IssueType.CODE_SMELL, "description"); + assertThat(adhocRule.getDefaultImpactsList()).hasSize(2).extracting(i -> i.getSoftwareQuality(), i -> i.getSeverity()) + .containsExactlyInAnyOrder( + Tuple.tuple(SoftwareQuality.MAINTAINABILITY.name(), Severity.HIGH.name()), + Tuple.tuple(SoftwareQuality.RELIABILITY.name(), Severity.MEDIUM.name())); + assertThat(adhocRule.getCleanCodeAttribute()) + .isEqualTo(CleanCodeAttribute.CLEAR.name()); + } + } + + @Test + public void store_whenAdhocRuleIsSpecifiedWithOptionalFieldEmpty_shouldWriteAdhocRuleToReport() throws IOException { + underTest.store(new DefaultAdHocRule().ruleId("ruleId").engineId("engineId") + .name("name") + .description("description")); + try (CloseableIterator adhocRuleIt = reportReader.readAdHocRules()) { + ScannerReport.AdHocRule adhocRule = adhocRuleIt.next(); + assertThat(adhocRule).extracting(r -> r.getSeverity(), r -> r.getType()) + .containsExactlyInAnyOrder(Constants.Severity.UNSET_SEVERITY, ScannerReport.IssueType.UNSET); + assertThat(adhocRule.getDefaultImpactsList()).isEmpty(); + assertThat(adhocRule.getCleanCodeAttribute()).isNullOrEmpty(); + } + } } -- 2.39.5