From 0673317f04ab56280d61ded1e548e27e3eec755e Mon Sep 17 00:00:00 2001 From: Alban Auzeill Date: Fri, 11 Aug 2023 08:19:35 +0200 Subject: [PATCH] Prevent ExternalRuleLoader to manipulate code attribute and impact fields when runtime API < 10.1 --- .../analyzer/commons/ExternalRuleLoader.java | 121 +++++++------ .../commons/ExternalRuleLoaderTest.java | 168 +++++++++++++----- 2 files changed, 196 insertions(+), 93 deletions(-) diff --git a/commons/src/main/java/org/sonarsource/analyzer/commons/ExternalRuleLoader.java b/commons/src/main/java/org/sonarsource/analyzer/commons/ExternalRuleLoader.java index 9422e45c..30f3b969 100644 --- a/commons/src/main/java/org/sonarsource/analyzer/commons/ExternalRuleLoader.java +++ b/commons/src/main/java/org/sonarsource/analyzer/commons/ExternalRuleLoader.java @@ -34,6 +34,7 @@ import org.json.simple.JSONObject; import org.sonar.api.SonarRuntime; import org.sonar.api.batch.rule.Severity; +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; @@ -105,10 +106,7 @@ public void createExternalRuleRepository(org.sonar.api.server.rule.RulesDefiniti newRule.setDebtRemediationFunction(newRule.debtRemediationFunctions().constantPerIssue(rule.constantDebtMinutes + "min")); newRule.setType(rule.type); newRule.setSeverity(rule.severity.name()); - if (rule.codeAttribute != null && rule.codeImpacts != null) { - newRule.setCleanCodeAttribute(rule.codeAttribute); - rule.codeImpacts.forEach(newRule::addDefaultImpact); - } + rule.applyCodeAttributeAndImpact(newRule); if (rule.tags != null) { newRule.setTags(rule.tags); @@ -123,7 +121,7 @@ public Set ruleKeys() { } /** - * If isCleanCodeImpactsAndAttributesSupported() == true then ruleType is deprecated and replaced by codeImpacts + * Deprecated, use {@link #applyTypeAndCleanCodeAttributes(NewExternalIssue, String)} instead. */ @Deprecated(since = "2.6") public RuleType ruleType(String ruleKey) { @@ -136,7 +134,7 @@ public RuleType ruleType(String ruleKey) { } /** - * If isCleanCodeImpactsAndAttributesSupported() == true then ruleSeverity is deprecated and replaced by codeImpacts + * Deprecated, use {@link #applyTypeAndCleanCodeAttributes(NewExternalIssue, String)} instead. */ @Deprecated(since = "2.6") public Severity ruleSeverity(String ruleKey) { @@ -148,24 +146,19 @@ public Severity ruleSeverity(String ruleKey) { } } - @Nullable - public CleanCodeAttribute codeAttribute(String ruleKey) { + public NewExternalIssue applyTypeAndCleanCodeAttributes(NewExternalIssue newExternalIssue, String ruleKey) { ExternalRule externalRule = rulesMap.get(ruleKey); if (externalRule != null) { - return externalRule.codeAttribute; + newExternalIssue + .type(externalRule.type) + .severity(externalRule.severity); + externalRule.applyCodeAttributeAndImpact(newExternalIssue); } else { - return null; - } - } - - @Nullable - public Map codeImpacts(String ruleKey) { - ExternalRule externalRule = rulesMap.get(ruleKey); - if (externalRule != null) { - return externalRule.codeImpacts; - } else { - return null; + newExternalIssue + .type(DEFAULT_ISSUE_TYPE) + .severity(DEFAULT_SEVERITY); } + return newExternalIssue; } public Long ruleConstantDebtMinutes(String ruleKey) { @@ -183,7 +176,8 @@ private void loadMetadataFile(String pathToMetadata) { List> rules = new JsonParser().parseArray(inputStreamReader); for (Map rule : rules) { - ExternalRule externalRule = new ExternalRule(rule, isCleanCodeImpactsAndAttributesSupported); + ExternalRule externalRule = isCleanCodeImpactsAndAttributesSupported ? + new ExternalRuleWithCodeAttribute(rule) : new ExternalRule(rule); rulesMap.put(externalRule.key, externalRule); } @@ -209,13 +203,7 @@ private static class ExternalRule { final Long constantDebtMinutes; - @CheckForNull - final CleanCodeAttribute codeAttribute; - - @CheckForNull - final Map codeImpacts; - - public ExternalRule(Map rule, boolean isCleanCodeImpactsAndAttributesSupported) { + public ExternalRule(Map rule) { this.key = (String) rule.get("key"); this.name = (String) rule.get("name"); this.url = (String) rule.get("url"); @@ -229,13 +217,14 @@ public ExternalRule(Map rule, boolean isCleanCodeImpactsAndAttri } type = getType(rule); severity = getSeverity(rule); - if (isCleanCodeImpactsAndAttributesSupported) { - codeAttribute = getCodeAttribute(rule); - codeImpacts = getCodeImpacts(rule); - } else { - codeAttribute = null; - codeImpacts = null; - } + } + + public void applyCodeAttributeAndImpact(NewRule newRule) { + // only supported by ExternalRuleWithCodeAttribute + } + + public void applyCodeAttributeAndImpact(NewExternalIssue newExternalIssue) { + // only supported by ExternalRuleWithCodeAttribute } private static RuleType getType(Map rule) { @@ -256,6 +245,53 @@ private static Severity getSeverity(Map rule) { } } + String getDescription(String linterKey, String linterName) { + if (description != null && url != null) { + return String.format(DESCRIPTION_WITH_URL, description, url, linterName); + } + + if (description != null) { + return description; + } + + if (url != null) { + return String.format(DESCRIPTION_ONLY_URL, linterName, key, url, linterName); + } + + return String.format(DESCRIPTION_FALLBACK, linterKey, key); + } + } + + private static class ExternalRuleWithCodeAttribute extends ExternalRule { + + @CheckForNull + final CleanCodeAttribute codeAttribute; + + @CheckForNull + final Map codeImpacts; + + public ExternalRuleWithCodeAttribute(Map rule) { + super(rule); + codeAttribute = getCodeAttribute(rule); + codeImpacts = getCodeImpacts(rule); + } + + @Override + public void applyCodeAttributeAndImpact(NewRule newRule) { + if (codeAttribute != null && codeImpacts != null) { + newRule.setCleanCodeAttribute(codeAttribute); + codeImpacts.forEach(newRule::addDefaultImpact); + } + } + + @Override + public void applyCodeAttributeAndImpact(NewExternalIssue newExternalIssue) { + if (codeAttribute != null && codeImpacts != null) { + newExternalIssue.cleanCodeAttribute(codeAttribute); + codeImpacts.forEach(newExternalIssue::addImpact); + } + } + @Nullable private static CleanCodeAttribute getCodeAttribute(Map rule) { JSONObject code = (JSONObject) rule.get("code"); @@ -284,21 +320,6 @@ private static Map getCode return null; } - String getDescription(String linterKey, String linterName) { - if (description != null && url != null) { - return String.format(DESCRIPTION_WITH_URL, description, url, linterName); - } - - if (description != null) { - return description; - } - - if (url != null) { - return String.format(DESCRIPTION_ONLY_URL, linterName, key, url, linterName); - } - - return String.format(DESCRIPTION_FALLBACK, linterKey, key); - } } } diff --git a/commons/src/test/java/org/sonarsource/analyzer/commons/ExternalRuleLoaderTest.java b/commons/src/test/java/org/sonarsource/analyzer/commons/ExternalRuleLoaderTest.java index 66c77197..03de77d9 100644 --- a/commons/src/test/java/org/sonarsource/analyzer/commons/ExternalRuleLoaderTest.java +++ b/commons/src/test/java/org/sonarsource/analyzer/commons/ExternalRuleLoaderTest.java @@ -19,6 +19,7 @@ */ package org.sonarsource.analyzer.commons; +import java.util.LinkedHashMap; import java.util.Map; import java.util.Set; import javax.annotation.Nullable; @@ -27,7 +28,10 @@ import org.sonar.api.SonarEdition; import org.sonar.api.SonarQubeSide; import org.sonar.api.SonarRuntime; +import org.sonar.api.batch.fs.internal.DefaultInputProject; import org.sonar.api.batch.rule.Severity; +import org.sonar.api.batch.sensor.issue.NewExternalIssue; +import org.sonar.api.batch.sensor.issue.internal.DefaultExternalIssue; import org.sonar.api.internal.SonarRuntimeImpl; import org.sonar.api.issue.impact.SoftwareQuality; import org.sonar.api.rules.CleanCodeAttribute; @@ -38,6 +42,7 @@ import org.sonar.api.utils.Version; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; import static org.sonar.api.batch.rule.Severity.BLOCKER; import static org.sonar.api.batch.rule.Severity.INFO; import static org.sonar.api.batch.rule.Severity.MAJOR; @@ -60,51 +65,24 @@ public class ExternalRuleLoaderTest { private static final SonarRuntime RUNTIME_10_1 = SonarRuntimeImpl.forSonarQube(Version.create(10, 1), SonarQubeSide.SERVER, SonarEdition.DEVELOPER); @Test - public void test_getters_runtime_10_0() { - ExternalRuleLoader loader = loadMyLinterJson(RUNTIME_10_0); - assertThat(loader.isCleanCodeImpactsAndAttributesSupported()).isFalse(); - - assertRule(loader, "not-existing-key", CODE_SMELL, MAJOR, 5L, null, null); - assertRule(loader, "bug-rule", BUG, MAJOR, 42L, null, null); - assertRule(loader, "code-smell-rule", CODE_SMELL, MAJOR, 5L, null, null); - assertRule(loader, "vulnerability-rule", VULNERABILITY, INFO, 5L, null, null); - assertRule(loader, "identifiable-low-maintainability-rule", BUG, MINOR, 5L, null, null); - assertRule(loader, "no-type-rule", CODE_SMELL, BLOCKER, 5L, null, null); - } - - @Test - public void test_getters_null_runtime() { + public void test_null_runtime() { ExternalRuleLoader loader = loadMyLinterJson(null); assertThat(loader.isCleanCodeImpactsAndAttributesSupported()).isFalse(); - assertRule(loader, "bug-rule", BUG, MAJOR, 42L, null, null); - assertRule(loader, "identifiable-low-maintainability-rule", BUG, MINOR, 5L, null, null); - } - - @Test - public void test_getters_runtime_10_1() { - ExternalRuleLoader loader = loadMyLinterJson(RUNTIME_10_1); - assertThat(loader.isCleanCodeImpactsAndAttributesSupported()).isTrue(); - - assertRule(loader, - "bug-rule", - BUG, - MAJOR, - 42L, - null, - null); - - assertRule(loader, - "identifiable-low-maintainability-rule", - BUG, - MINOR, - 5L, - IDENTIFIABLE, - Map.of(MAINTAINABILITY, LOW)); + assertRule(loader, "bug-rule", BUG, MAJOR, 42L); + assertRule(loader, "identifiable-low-maintainability-rule", BUG, MINOR, 5L); } @Test public void test_repository_10_0() { ExternalRuleLoader externalRuleLoader = loadMyLinterJson(RUNTIME_10_0); + assertThat(externalRuleLoader.isCleanCodeImpactsAndAttributesSupported()).isFalse(); + assertRule(externalRuleLoader, "not-existing-key", CODE_SMELL, MAJOR, 5L); + assertRule(externalRuleLoader, "bug-rule", BUG, MAJOR, 42L); + assertRule(externalRuleLoader, "code-smell-rule", CODE_SMELL, MAJOR, 5L); + assertRule(externalRuleLoader, "vulnerability-rule", VULNERABILITY, INFO, 5L); + assertRule(externalRuleLoader, "identifiable-low-maintainability-rule", BUG, MINOR, 5L); + assertRule(externalRuleLoader, "no-type-rule", CODE_SMELL, BLOCKER, 5L); + RulesDefinition.Context context = new RulesDefinition.Context(); externalRuleLoader.createExternalRuleRepository(context); @@ -193,6 +171,10 @@ public void test_repository_10_0() { @Test public void test_repository_10_1() { ExternalRuleLoader externalRuleLoader = loadMyLinterJson(RUNTIME_10_1); + assertThat(externalRuleLoader.isCleanCodeImpactsAndAttributesSupported()).isTrue(); + assertRule(externalRuleLoader, "bug-rule", BUG, MAJOR, 42L); + assertRule(externalRuleLoader, "identifiable-low-maintainability-rule", BUG, MINOR, 5L); + RulesDefinition.Context context = new RulesDefinition.Context(); externalRuleLoader.createExternalRuleRepository(context); @@ -262,14 +244,113 @@ public void test_repository_10_1() { ); } + @Test + public void test_code_attributes_on_new_issues_SQ_API_10_0() { + DefaultExternalIssueWithCodeAttribute externalIssue = new DefaultExternalIssueWithCodeAttribute(mock(DefaultInputProject.class)); + ExternalRuleLoader externalRuleLoader = loadMyLinterJson(RUNTIME_10_0); + + String ruleKey = "identifiable-low-maintainability-rule"; + externalRuleLoader.applyTypeAndCleanCodeAttributes(externalIssue, ruleKey); + + SoftAssertions softly = new SoftAssertions(); + softly.assertThat(externalIssue.type()).as("type of " + ruleKey).isEqualTo(BUG); + softly.assertThat(externalIssue.severity()).as("severity of " + ruleKey).isEqualTo(MINOR); + softly.assertThat(externalIssue.cleanCodeAttribute()).as("cleanCodeAttribute of " + ruleKey).isNull(); + softly.assertThat(externalIssue.impacts()).as("impacts of " + ruleKey).isEmpty(); + softly.assertAll(); + } + + @Test + public void test_code_attributes_on_new_issues_SQ_API_10_1() { + ExternalRuleLoader externalRuleLoader = loadMyLinterJson(RUNTIME_10_1); + + String ruleKey = "identifiable-low-maintainability-rule"; + DefaultExternalIssueWithCodeAttribute externalIssue = new DefaultExternalIssueWithCodeAttribute(mock(DefaultInputProject.class)); + externalRuleLoader.applyTypeAndCleanCodeAttributes(externalIssue, ruleKey); + + SoftAssertions softly = new SoftAssertions(); + softly.assertThat(externalIssue.type()).as("type of " + ruleKey).isEqualTo(BUG); + softly.assertThat(externalIssue.severity()).as("severity of " + ruleKey).isEqualTo(MINOR); + softly.assertThat(externalIssue.cleanCodeAttribute()).as("cleanCodeAttribute of " + ruleKey).isEqualTo(IDENTIFIABLE); + softly.assertThat(externalIssue.impacts()).as("impacts of " + ruleKey).isEqualTo(Map.of(MAINTAINABILITY, LOW)); + softly.assertAll(); + + ruleKey = "missing-code-attribute"; + externalIssue = new DefaultExternalIssueWithCodeAttribute(mock(DefaultInputProject.class)); + externalRuleLoader.applyTypeAndCleanCodeAttributes(externalIssue, ruleKey); + + softly = new SoftAssertions(); + softly.assertThat(externalIssue.cleanCodeAttribute()).as("cleanCodeAttribute of " + ruleKey).isNull(); + softly.assertThat(externalIssue.impacts()).as("impacts of " + ruleKey).isEmpty(); + softly.assertAll(); + + ruleKey = "missing-impacts"; + externalIssue = new DefaultExternalIssueWithCodeAttribute(mock(DefaultInputProject.class)); + externalRuleLoader.applyTypeAndCleanCodeAttributes(externalIssue, ruleKey); + + softly = new SoftAssertions(); + softly.assertThat(externalIssue.cleanCodeAttribute()).as("cleanCodeAttribute of " + ruleKey).isNull(); + softly.assertThat(externalIssue.impacts()).as("impacts of " + ruleKey).isEmpty(); + softly.assertAll(); + } + + @Test + public void test_code_attributes_on_unknown_rule() { + DefaultExternalIssueWithCodeAttribute externalIssue = new DefaultExternalIssueWithCodeAttribute(mock(DefaultInputProject.class)); + ExternalRuleLoader externalRuleLoader = loadMyLinterJson(RUNTIME_10_1); + + String ruleKey = "unknown-rule-key"; + externalRuleLoader.applyTypeAndCleanCodeAttributes(externalIssue, ruleKey); + + SoftAssertions softly = new SoftAssertions(); + softly.assertThat(externalIssue.type()).as("type of " + ruleKey).isEqualTo(CODE_SMELL); + softly.assertThat(externalIssue.severity()).as("severity of " + ruleKey).isEqualTo(MAJOR); + softly.assertThat(externalIssue.cleanCodeAttribute()).as("cleanCodeAttribute of " + ruleKey).isNull(); + softly.assertThat(externalIssue.impacts()).as("impacts of " + ruleKey).isEmpty(); + softly.assertAll(); + } + + /** + * This class exists because sonar-plugin-api-impl:10.1.0.73491 does not yet implement all the methods of sonar-plugin-api:10.1.0.809 + * Will be removed and replace with DefaultExternalIssue once sonar-plugin-api-impl has a new version. + */ + class DefaultExternalIssueWithCodeAttribute extends DefaultExternalIssue { + private CleanCodeAttribute attribute = null; + private final Map impacts = new LinkedHashMap<>(); + + public DefaultExternalIssueWithCodeAttribute(DefaultInputProject project) { + super(project); + } + + @Override + public Map impacts() { + return impacts; + } + + @org.jetbrains.annotations.Nullable + @Override + public CleanCodeAttribute cleanCodeAttribute() { + return attribute; + } + + @Override + public NewExternalIssue cleanCodeAttribute(CleanCodeAttribute attribute) { + this.attribute = attribute; + return this; + } + + @Override + public NewExternalIssue addImpact(SoftwareQuality softwareQuality, org.sonar.api.issue.impact.Severity severity) { + impacts.put(softwareQuality, severity); + return this; + } + } + private static void assertRule(ExternalRuleLoader externalRuleLoader, String ruleKey, - RuleType expectedRuleType, Severity expectedRuleSeverity, Long expectedDebtMinutes, - CleanCodeAttribute expectedCodeAttribute, Map expectedCodeImpacts) { + RuleType expectedRuleType, Severity expectedRuleSeverity, Long expectedDebtMinutes) { SoftAssertions softly = new SoftAssertions(); softly.assertThat(externalRuleLoader.ruleType(ruleKey)).as("ruleType of " + ruleKey).isEqualTo(expectedRuleType); softly.assertThat(externalRuleLoader.ruleSeverity(ruleKey)).as("ruleSeverity of " + ruleKey).isEqualTo(expectedRuleSeverity); - softly.assertThat(externalRuleLoader.codeAttribute(ruleKey)).as("codeAttribute of " + ruleKey).isEqualTo(expectedCodeAttribute); - softly.assertThat(externalRuleLoader.codeImpacts(ruleKey)).as("codeImpacts of " + ruleKey).isEqualTo(expectedCodeImpacts); softly.assertThat(externalRuleLoader.ruleConstantDebtMinutes(ruleKey)) .as("ruleConstantDebtMinutes of " + ruleKey).isEqualTo(expectedDebtMinutes); softly.assertAll(); @@ -277,7 +358,8 @@ private static void assertRule(ExternalRuleLoader externalRuleLoader, String rul private static void assertRule(Repository repository, String ruleKey, String expectedName, String expectedDescription, RuleType expectedRuleType, String expectedRuleSeverity, String expectedDebtMinutes, - CleanCodeAttribute expectedCodeAttribute, Map expectedCodeImpacts, + @Nullable CleanCodeAttribute expectedCodeAttribute, + @Nullable Map expectedCodeImpacts, Set expectedTags) { SoftAssertions softly = new SoftAssertions(); Rule rule = repository.rule(ruleKey);