From 541868edf65f6cadc030730c1e3788ff23d18d2e Mon Sep 17 00:00:00 2001 From: Alain Kermis Date: Thu, 19 Oct 2023 11:35:23 +0200 Subject: [PATCH] SONAR-20552 Refactor external issue format to comply with CCT --- .../externalissue/ExternalIssueImporter.java | 28 ++- .../externalissue/ExternalIssueReport.java | 3 +- .../ExternalIssueReportValidator.java | 73 ++++--- .../ExternalIssueImporterTest.java | 17 +- .../ExternalIssueReportParserTest.java | 13 +- .../ExternalIssueReportValidatorTest.java | 131 +++++++----- .../scanner/externalissue/cct/report.json | 194 +++++++++--------- .../{cct => }/invalid_report.json | 0 8 files changed, 258 insertions(+), 201 deletions(-) rename sonar-scanner-engine/src/test/resources/org/sonar/scanner/externalissue/{cct => }/invalid_report.json (100%) diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/ExternalIssueImporter.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/ExternalIssueImporter.java index c629025b1de..1b08ed881ef 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/ExternalIssueImporter.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/ExternalIssueImporter.java @@ -19,7 +19,9 @@ */ package org.sonar.scanner.externalissue; +import java.util.HashMap; import java.util.LinkedHashSet; +import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.stream.Collectors; @@ -58,7 +60,7 @@ public class ExternalIssueImporter { public void execute() { if (report.rules != null) { - importRules(); + importNewFormat(); } else { importDeprecatedFormat(); } @@ -74,23 +76,27 @@ public class ExternalIssueImporter { logStatistics(issueCount, StringUtils.EMPTY); } - private void importRules() { + private void importNewFormat() { + Map rulesMap = new HashMap<>(); + for (Rule rule : report.rules) { + rulesMap.put(rule.id, rule); NewAdHocRule adHocRule = createAdHocRule(rule); - int issueCount = 0; - for (Issue issue : rule.issues) { - if (importIssue(issue, rule)) { - issueCount++; - } - } - logStatistics(issueCount, String.format(" for ruleId '%s'", rule.ruleId)); adHocRule.save(); } + + int issueCount = 0; + for (Issue issue : report.issues) { + if (importIssue(issue, rulesMap.get(issue.ruleId))) { + issueCount++; + } + } + logStatistics(issueCount, StringUtils.EMPTY); } private NewAdHocRule createAdHocRule(Rule rule) { NewAdHocRule adHocRule = context.newAdHocRule(); - adHocRule.ruleId(rule.ruleId); + adHocRule.ruleId(rule.id); adHocRule.name(rule.name); adHocRule.description(rule.description); adHocRule.engineId(rule.engineId); @@ -151,7 +157,7 @@ public class ExternalIssueImporter { private boolean importIssue(Issue issue, ExternalIssueReport.Rule rule) { NewExternalIssue externalIssue = context.newExternalIssue() .engineId(rule.engineId) - .ruleId(rule.ruleId) + .ruleId(rule.id) .severity(backmapSeverityFromImpact(rule)) .type(backmapTypeFromImpact(rule)); diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/ExternalIssueReport.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/ExternalIssueReport.java index fcf19637e37..e4a52caa55d 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/ExternalIssueReport.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/ExternalIssueReport.java @@ -42,14 +42,13 @@ public class ExternalIssueReport { } static class Rule { - String ruleId; + String id; String engineId; String name; @Nullable String description; String cleanCodeAttribute; Impact[] impacts; - Issue[] issues; } static class Impact { diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/ExternalIssueReportValidator.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/ExternalIssueReportValidator.java index d3b81b93218..82541cd2767 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/ExternalIssueReportValidator.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/ExternalIssueReportValidator.java @@ -20,8 +20,9 @@ package org.sonar.scanner.externalissue; import java.nio.file.Path; +import java.util.HashSet; +import java.util.Set; import javax.annotation.Nullable; -import org.apache.commons.lang.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.sonar.api.scanner.ScannerSide; @@ -30,7 +31,7 @@ import org.sonar.core.documentation.DocumentationLinkGenerator; @ScannerSide public class ExternalIssueReportValidator { private static final Logger LOGGER = LoggerFactory.getLogger(ExternalIssueReportValidator.class); - private static final String RULE_ID = "ruleId"; + private static final String ISSUE_RULE_ID = "ruleId"; private static final String SEVERITY = "severity"; private static final String TYPE = "type"; private static final String DOCUMENTATION_SUFFIX = "/analyzing-source-code/importing-external-issues/generic-issue-import-format/"; @@ -40,23 +41,42 @@ public class ExternalIssueReportValidator { this.documentationLinkGenerator = documentationLinkGenerator; } + /** + *

Since we are supporting deprecated format, we decide which format it is in order by: + *

+ *

+ */ public void validate(ExternalIssueReport report, Path reportPath) { - if (report.rules != null) { - checkNoField(report.issues, "issues", reportPath); - validateRules(report.rules, reportPath); - } else if (report.issues != null) { + if (report.rules != null && report.issues != null) { + Set ruleIds = validateRules(report.rules, reportPath); + validateIssuesCctFormat(report.issues, ruleIds, reportPath); + } else if (report.rules == null && report.issues != null) { String documentationLink = documentationLinkGenerator.getDocumentationLink(DOCUMENTATION_SUFFIX); LOGGER.warn("External issues were imported with a deprecated format which will be removed soon. " + "Please switch to the newest format to fully benefit from Clean Code: {}", documentationLink); - validateIssues(report.issues, reportPath); + validateIssuesDeprecatedFormat(report.issues, reportPath); } else { - throw new IllegalStateException(String.format("Failed to parse report '%s': missing mandatory field 'rules'", reportPath)); + throw new IllegalStateException(String.format("Failed to parse report '%s': invalid report detected.", reportPath)); } } - private static void validateIssues(ExternalIssueReport.Issue[] issues, Path reportPath) { + private static void validateIssuesCctFormat(ExternalIssueReport.Issue[] issues, Set ruleIds, Path reportPath) { for (ExternalIssueReport.Issue issue : issues) { - mandatoryField(issue.ruleId, RULE_ID, reportPath); + mandatoryField(issue.ruleId, ISSUE_RULE_ID, reportPath); + checkRuleExistsInReport(ruleIds, issue, reportPath); + checkNoField(issue.severity, SEVERITY, reportPath); + checkNoField(issue.type, TYPE, reportPath); + validateAlwaysRequiredIssueFields(issue, reportPath); + } + } + + private static void validateIssuesDeprecatedFormat(ExternalIssueReport.Issue[] issues, Path reportPath) { + for (ExternalIssueReport.Issue issue : issues) { + mandatoryField(issue.ruleId, ISSUE_RULE_ID, reportPath); mandatoryField(issue.severity, SEVERITY, reportPath); mandatoryField(issue.type, TYPE, reportPath); mandatoryField(issue.engineId, "engineId", reportPath); @@ -64,24 +84,21 @@ public class ExternalIssueReportValidator { } } - private static void validateRules(ExternalIssueReport.Rule[] rules, Path reportPath) { + private static Set validateRules(ExternalIssueReport.Rule[] rules, Path reportPath) { + Set ruleIds = new HashSet<>(); for (ExternalIssueReport.Rule rule : rules) { - mandatoryField(rule.ruleId, RULE_ID, reportPath); + mandatoryField(rule.id, "id", reportPath); mandatoryField(rule.name, "name", reportPath); mandatoryField(rule.engineId, "engineId", reportPath); mandatoryField(rule.cleanCodeAttribute, "cleanCodeAttribute", reportPath); - mandatoryArray(rule.impacts, "impacts", reportPath); - validateIssuesAsPartOfARule(rule.issues, reportPath); - } - } + checkImpactsArray(rule.impacts, reportPath); - private static void validateIssuesAsPartOfARule(ExternalIssueReport.Issue[] issues, Path reportPath) { - for (ExternalIssueReport.Issue issue : issues) { - validateAlwaysRequiredIssueFields(issue, reportPath); - checkNoField(issue.severity, SEVERITY, reportPath); - checkNoField(issue.type, TYPE, reportPath); - checkNoField(issue.ruleId, RULE_ID, reportPath); + if (!ruleIds.add(rule.id)) { + throw new IllegalStateException(String.format("Failed to parse report '%s': found duplicate rule ID '%s'.", reportPath, rule.id)); + } } + + return ruleIds; } private static void checkNoField(@Nullable Object value, String fieldName, Path reportPath) { @@ -128,17 +145,17 @@ public class ExternalIssueReportValidator { } } - private static void mandatoryArray(@Nullable Object[] value, String fieldName, Path reportPath) { - mandatoryField(value, fieldName, reportPath); + private static void checkImpactsArray(@Nullable Object[] value, Path reportPath) { + mandatoryField(value, "impacts", reportPath); if (value.length == 0) { throw new IllegalStateException(String.format("Failed to parse report '%s': mandatory array '%s' not populated.", reportPath, - fieldName)); + "impacts")); } } - private static void mandatoryField(@Nullable String value, String fieldName, Path reportPath) { - if (StringUtils.isBlank(value)) { - throw new IllegalStateException(String.format("Failed to parse report '%s': missing mandatory field '%s'.", reportPath, fieldName)); + private static void checkRuleExistsInReport(Set ruleIds, ExternalIssueReport.Issue issue, Path reportPath) { + if (!ruleIds.contains(issue.ruleId)) { + throw new IllegalStateException(String.format("Failed to parse report '%s': rule with '%s' not present.", reportPath, issue.ruleId)); } } } diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/ExternalIssueImporterTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/ExternalIssueImporterTest.java index 7b29c33aa0b..4641efa95d1 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/ExternalIssueImporterTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/ExternalIssueImporterTest.java @@ -82,7 +82,7 @@ public class ExternalIssueImporterTest { public void execute_whenNewFormatWithZeroIssues() { ExternalIssueReport report = new ExternalIssueReport(); ExternalIssueReport.Rule rule = createRule(); - rule.issues = new ExternalIssueReport.Issue[0]; + report.issues = new ExternalIssueReport.Issue[0]; report.rules = new ExternalIssueReport.Rule[]{rule}; ExternalIssueImporter underTest = new ExternalIssueImporter(this.context, report); @@ -90,7 +90,7 @@ public class ExternalIssueImporterTest { assertThat(context.allExternalIssues()).isEmpty(); assertThat(context.allIssues()).isEmpty(); - assertThat(logs.logs(Level.INFO)).contains("Imported 0 issues in 0 files for ruleId 'some_rule_id'"); + assertThat(logs.logs(Level.INFO)).contains("Imported 0 issues in 0 files"); } @Test @@ -109,7 +109,7 @@ public class ExternalIssueImporterTest { assertThat(output.severity()).isEqualTo(Severity.CRITICAL); //backmapped assertThat(output.type()).isEqualTo(RuleType.VULNERABILITY); //backmapped assertThat(output.remediationEffort()).isNull(); - assertThat(logs.logs(Level.INFO)).contains("Imported 1 issue in 1 file for ruleId 'some_rule_id'"); + assertThat(logs.logs(Level.INFO)).contains("Imported 1 issue in 1 file"); assertThat(context.allAdHocRules()).hasSize(1); AdHocRule output1 = context.allAdHocRules().iterator().next(); @@ -177,7 +177,7 @@ public class ExternalIssueImporterTest { public void execute_whenNewFormatContainsNonExistentCleanCodeAttribute_shouldThrowException() { ExternalIssueReport report = new ExternalIssueReport(); ExternalIssueReport.Rule rule = createRule("not_existent_attribute", MAINTAINABILITY.name(), HIGH.name()); - rule.issues = new ExternalIssueReport.Issue[]{}; + report.issues = new ExternalIssueReport.Issue[]{}; report.rules = new ExternalIssueReport.Rule[]{rule}; ExternalIssueImporter underTest = new ExternalIssueImporter(this.context, report); @@ -191,7 +191,7 @@ public class ExternalIssueImporterTest { public void execute_whenNewFormatContainsNonExistentSoftwareQuality_shouldThrowException() { ExternalIssueReport report = new ExternalIssueReport(); ExternalIssueReport.Rule rule = createRule(CleanCodeAttribute.CONVENTIONAL.name(), "not_existent_software_quality", HIGH.name()); - rule.issues = new ExternalIssueReport.Issue[]{}; + report.issues = new ExternalIssueReport.Issue[]{}; report.rules = new ExternalIssueReport.Rule[]{rule}; ExternalIssueImporter underTest = new ExternalIssueImporter(this.context, report); @@ -206,7 +206,7 @@ public class ExternalIssueImporterTest { ExternalIssueReport report = new ExternalIssueReport(); ExternalIssueReport.Rule rule = createRule(CleanCodeAttribute.CONVENTIONAL.name(), SoftwareQuality.RELIABILITY.name(), "not_existent_impact_severity"); - rule.issues = new ExternalIssueReport.Issue[]{}; + report.issues = new ExternalIssueReport.Issue[]{}; report.rules = new ExternalIssueReport.Rule[]{rule}; ExternalIssueImporter underTest = new ExternalIssueImporter(this.context, report); @@ -310,7 +310,7 @@ public class ExternalIssueImporterTest { private static ExternalIssueReport.Rule createRule(String cleanCodeAttribute, String softwareQuality, String impactSeverity) { ExternalIssueReport.Rule rule = new ExternalIssueReport.Rule(); - rule.ruleId = RULE_ID; + rule.id = RULE_ID; rule.name = RULE_NAME; rule.engineId = RULE_ENGINE_ID; rule.cleanCodeAttribute = cleanCodeAttribute; @@ -342,7 +342,8 @@ public class ExternalIssueImporterTest { private void runOn(ExternalIssueReport.Issue input) { ExternalIssueReport report = new ExternalIssueReport(); ExternalIssueReport.Rule rule = createRule(); - rule.issues = new ExternalIssueReport.Issue[]{input}; + input.ruleId = rule.id; + report.issues = new ExternalIssueReport.Issue[]{input}; report.rules = new ExternalIssueReport.Rule[]{rule}; ExternalIssueImporter underTest = new ExternalIssueImporter(this.context, report); diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/ExternalIssueReportParserTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/ExternalIssueReportParserTest.java index 645ecf5d5b3..067dd24304e 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/ExternalIssueReportParserTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/ExternalIssueReportParserTest.java @@ -121,11 +121,10 @@ public class ExternalIssueReportParserTest { } private static void assertCctReport(ExternalIssueReport report) { - assertThat(report.issues).isNull(); assertThat(report.rules).hasSize(2); ExternalIssueReport.Rule rule = report.rules[0]; - assertThat(rule.ruleId).isEqualTo("rule1"); + assertThat(rule.id).isEqualTo("rule1"); assertThat(rule.engineId).isEqualTo("test"); assertThat(rule.name).isEqualTo("just_some_rule_name"); assertThat(rule.description).isEqualTo("just_some_description"); @@ -136,11 +135,11 @@ public class ExternalIssueReportParserTest { assertThat(rule.impacts[1].severity).isEqualTo("LOW"); assertThat(rule.impacts[1].softwareQuality).isEqualTo("SECURITY"); - assertThat(rule.issues).hasSize(4); + assertThat(report.issues).hasSize(8); - ExternalIssueReport.Issue issue1 = rule.issues[0]; + ExternalIssueReport.Issue issue1 = report.issues[0]; assertThat(issue1.engineId).isNull(); - assertThat(issue1.ruleId).isNull(); + assertThat(issue1.ruleId).isEqualTo("rule1"); assertThat(issue1.severity).isNull(); assertThat(issue1.effortMinutes).isEqualTo(40); assertThat(issue1.type).isNull(); @@ -152,9 +151,9 @@ public class ExternalIssueReportParserTest { assertThat(issue1.primaryLocation.textRange.endLine).isEqualTo(3); assertThat(issue1.secondaryLocations).isNull(); - ExternalIssueReport.Issue issue2 = rule.issues[3]; + ExternalIssueReport.Issue issue2 = report.issues[7]; assertThat(issue2.engineId).isNull(); - assertThat(issue2.ruleId).isNull(); + assertThat(issue2.ruleId).isEqualTo("rule2"); assertThat(issue2.severity).isNull(); assertThat(issue2.effortMinutes).isNull(); assertThat(issue2.type).isNull(); diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/ExternalIssueReportValidatorTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/ExternalIssueReportValidatorTest.java index 8f7bd5ac173..c466357417a 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/ExternalIssueReportValidatorTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/ExternalIssueReportValidatorTest.java @@ -48,7 +48,7 @@ public class ExternalIssueReportValidatorTest { private static final String TEST_URL = "test-url"; private final Gson gson = new Gson(); - private final Path reportPath = Paths.get("some-folder"); + private final Path reportPath = Paths.get("report-path"); private final DocumentationLinkGenerator documentationLinkGenerator = mock(DocumentationLinkGenerator.class); private final ExternalIssueReportValidator validator = new ExternalIssueReportValidator(documentationLinkGenerator); @@ -60,12 +60,30 @@ public class ExternalIssueReportValidatorTest { when(documentationLinkGenerator.getDocumentationLink(URL)).thenReturn(TEST_URL); } + @Test + public void validate_whenInvalidReport_shouldThrowException() throws IOException { + ExternalIssueReport report = readInvalidReport(DEPRECATED_REPORTS_LOCATION); + assertThatThrownBy(() -> validator.validate(report, reportPath)) + .isInstanceOf(IllegalStateException.class) + .hasMessage("Failed to parse report 'report-path': invalid report detected."); + } + @Test public void validate_whenCorrect_shouldNotThrowException() throws IOException { ExternalIssueReport report = read(REPORTS_LOCATION); assertThatCode(() -> validator.validate(report, reportPath)).doesNotThrowAnyException(); } + @Test + public void validate_whenDuplicateRuleIdFound_shouldThrowException() throws IOException { + ExternalIssueReport report = read(REPORTS_LOCATION); + report.rules[0].id = "rule2"; + + assertThatThrownBy(() -> validator.validate(report, reportPath)) + .isInstanceOf(IllegalStateException.class) + .hasMessage("Failed to parse report 'report-path': found duplicate rule ID 'rule2'."); + } + @Test public void validate_whenMissingMandatoryCleanCodeAttributeField_shouldThrowException() throws IOException { ExternalIssueReport report = read(REPORTS_LOCATION); @@ -73,7 +91,7 @@ public class ExternalIssueReportValidatorTest { assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'cleanCodeAttribute'."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'cleanCodeAttribute'."); } @Test @@ -83,17 +101,17 @@ public class ExternalIssueReportValidatorTest { assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'engineId'."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'engineId'."); } @Test public void validate_whenMissingFilepathFieldForPrimaryLocation_shouldThrowException() throws IOException { ExternalIssueReport report = read(REPORTS_LOCATION); - report.rules[0].issues[0].primaryLocation.filePath = null; + report.issues[0].primaryLocation.filePath = null; assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'filePath' in the primary location of the issue."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'filePath' in the primary location of the issue."); } @Test @@ -103,57 +121,57 @@ public class ExternalIssueReportValidatorTest { assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'impacts'."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'impacts'."); } @Test public void validate_whenMissingMessageFieldForPrimaryLocation_shouldThrowException() throws IOException { ExternalIssueReport report = read(REPORTS_LOCATION); - report.rules[0].issues[0].primaryLocation.message = null; + report.issues[0].primaryLocation.message = null; assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'message' in the primary location of the issue."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'message' in the primary location of the issue."); } @Test public void validate_whenMissingStartLineFieldForPrimaryLocation_shouldThrowException() throws IOException { ExternalIssueReport report = read(REPORTS_LOCATION); - report.rules[0].issues[0].primaryLocation.textRange.startLine = null; + report.issues[0].primaryLocation.textRange.startLine = null; assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'startLine of the text range' in the primary location of the issue."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'startLine of the text range' in the primary location of the issue."); } @Test public void validate_whenReportMissingFilePathForSecondaryLocation_shouldThrowException() throws IOException { ExternalIssueReport report = read(REPORTS_LOCATION); - report.rules[0].issues[3].secondaryLocations[0].filePath = null; + report.issues[3].secondaryLocations[0].filePath = null; assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'filePath' in a secondary location of the issue."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'filePath' in a secondary location of the issue."); } @Test public void validate_whenReportMissingTextRangeForSecondaryLocation_shouldThrowException() throws IOException { ExternalIssueReport report = read(REPORTS_LOCATION); - report.rules[0].issues[3].secondaryLocations[0].textRange = null; + report.issues[3].secondaryLocations[0].textRange = null; assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'textRange' in a secondary location of the issue."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'textRange' in a secondary location of the issue."); } @Test public void validate_whenReportMissingTextRangeStartLineForSecondaryLocation_shouldThrowException() throws IOException { ExternalIssueReport report = read(REPORTS_LOCATION); - report.rules[0].issues[3].secondaryLocations[0].textRange.startLine = null; + report.issues[3].secondaryLocations[0].textRange.startLine = null; assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'startLine of the text range' in a secondary location of the issue."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'startLine of the text range' in a secondary location of the issue."); } @Test @@ -163,57 +181,73 @@ public class ExternalIssueReportValidatorTest { assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'name'."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'name'."); } @Test public void validate_whenMissingPrimaryLocationField_shouldThrowException() throws IOException { ExternalIssueReport report = read(REPORTS_LOCATION); - report.rules[0].issues[0].primaryLocation = null; + report.issues[0].primaryLocation = null; + + assertThatThrownBy(() -> validator.validate(report, reportPath)) + .isInstanceOf(IllegalStateException.class) + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'primaryLocation'."); + } + + @Test + public void validate_whenMissingOrEmptyRuleIdField_shouldThrowException() throws IOException { + String errorMessage = "Failed to parse report 'report-path': missing mandatory field 'id'."; + + ExternalIssueReport report = read(REPORTS_LOCATION); + report.rules[0].id = null; + assertThatThrownBy(() -> validator.validate(report, reportPath)) + .isInstanceOf(IllegalStateException.class) + .hasMessage(errorMessage); + report.rules[0].id = ""; assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'primaryLocation'."); + .hasMessage(errorMessage); } @Test - public void validate_whenMissingRuleIdField_shouldThrowException() throws IOException { + public void validate_whenIssueContainsRuleIdNotPresentInReport_shouldThrowException() throws IOException { ExternalIssueReport report = read(REPORTS_LOCATION); - report.rules[0].ruleId = null; + report.issues[0].ruleId = "rule-id-not-present"; assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'ruleId'."); + .hasMessage("Failed to parse report 'report-path': rule with 'rule-id-not-present' not present."); } @Test - public void validate_whenContainsDeprecatedIssuesEntry_shouldThrowException() throws IOException { + public void validate_whenIssueRuleIdNotPresentInReport_shouldThrowException() throws IOException { ExternalIssueReport report = read(REPORTS_LOCATION); - report.issues = new ExternalIssueReport.Issue[] { new ExternalIssueReport.Issue() }; + report.issues[0].ruleId = null; assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Deprecated 'issues' field found in the following report: 'some-folder'."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'ruleId'."); } @Test public void validate_whenContainsDeprecatedSeverityEntry_shouldThrowException() throws IOException { ExternalIssueReport report = read(REPORTS_LOCATION); - report.rules[0].issues[0].severity = "MAJOR"; + report.issues[0].severity = "MAJOR"; assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Deprecated 'severity' field found in the following report: 'some-folder'."); + .hasMessage("Deprecated 'severity' field found in the following report: 'report-path'."); } @Test public void validate_whenContainsDeprecatedTypeEntry_shouldThrowException() throws IOException { ExternalIssueReport report = read(REPORTS_LOCATION); - report.rules[0].issues[0].type = "BUG"; + report.issues[0].type = "BUG"; assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Deprecated 'type' field found in the following report: 'some-folder'."); + .hasMessage("Deprecated 'type' field found in the following report: 'report-path'."); } @Test @@ -223,7 +257,7 @@ public class ExternalIssueReportValidatorTest { assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': mandatory array 'impacts' not populated."); + .hasMessage("Failed to parse report 'report-path': mandatory array 'impacts' not populated."); } @Test @@ -240,7 +274,7 @@ public class ExternalIssueReportValidatorTest { assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'engineId'."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'engineId'."); assertWarningLog(); } @@ -251,7 +285,7 @@ public class ExternalIssueReportValidatorTest { assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'filePath' in the primary location of the issue."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'filePath' in the primary location of the issue."); assertWarningLog(); } @@ -262,7 +296,7 @@ public class ExternalIssueReportValidatorTest { assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'message' in the primary location of the issue."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'message' in the primary location of the issue."); assertWarningLog(); } @@ -273,7 +307,7 @@ public class ExternalIssueReportValidatorTest { assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'primaryLocation'."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'primaryLocation'."); assertWarningLog(); } @@ -284,7 +318,7 @@ public class ExternalIssueReportValidatorTest { assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'startLine of the text range' in the primary location of the issue."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'startLine of the text range' in the primary location of the issue."); assertWarningLog(); } @@ -295,7 +329,7 @@ public class ExternalIssueReportValidatorTest { assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'filePath' in a secondary location of the issue."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'filePath' in a secondary location of the issue."); assertWarningLog(); } @@ -306,7 +340,7 @@ public class ExternalIssueReportValidatorTest { assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'textRange' in a secondary location of the issue."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'textRange' in a secondary location of the issue."); assertWarningLog(); } @@ -317,7 +351,7 @@ public class ExternalIssueReportValidatorTest { assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'startLine of the text range' in a secondary location of the issue."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'startLine of the text range' in a secondary location of the issue."); assertWarningLog(); } @@ -328,7 +362,7 @@ public class ExternalIssueReportValidatorTest { assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'ruleId'."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'ruleId'."); assertWarningLog(); } @@ -339,7 +373,7 @@ public class ExternalIssueReportValidatorTest { assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'severity'."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'severity'."); assertWarningLog(); } @@ -350,20 +384,10 @@ public class ExternalIssueReportValidatorTest { assertThatThrownBy(() -> validator.validate(report, reportPath)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'type'."); + .hasMessage("Failed to parse report 'report-path': missing mandatory field 'type'."); assertWarningLog(); } - @Test - public void validate_whenEmptyStringForMandatoryField_shouldStillThrowException() throws IOException { - ExternalIssueReport report = read(REPORTS_LOCATION); - report.rules[0].ruleId = ""; - - assertThatThrownBy(() -> validator.validate(report, reportPath)) - .isInstanceOf(IllegalStateException.class) - .hasMessage("Failed to parse report 'some-folder': missing mandatory field 'ruleId'."); - } - private void assertWarningLog() { assertThat(logTester.getLogs(Level.WARN)) .extracting(LogAndArguments::getFormattedMsg) @@ -376,4 +400,9 @@ public class ExternalIssueReportValidatorTest { return gson.fromJson(reader, ExternalIssueReport.class); } + private ExternalIssueReport readInvalidReport(String location) throws IOException { + Reader reader = Files.newBufferedReader(Paths.get(location + "invalid_report.json"), StandardCharsets.UTF_8); + return gson.fromJson(reader, ExternalIssueReport.class); + } + } diff --git a/sonar-scanner-engine/src/test/resources/org/sonar/scanner/externalissue/cct/report.json b/sonar-scanner-engine/src/test/resources/org/sonar/scanner/externalissue/cct/report.json index 7f797d663b6..878432e24b4 100644 --- a/sonar-scanner-engine/src/test/resources/org/sonar/scanner/externalissue/cct/report.json +++ b/sonar-scanner-engine/src/test/resources/org/sonar/scanner/externalissue/cct/report.json @@ -1,7 +1,7 @@ { "rules": [ { - "ruleId": "rule1", + "id": "rule1", "name": "just_some_rule_name", "description": "just_some_description", "engineId": "test", @@ -15,61 +15,10 @@ "softwareQuality": "SECURITY", "severity": "LOW" } - ], - "issues": [ - { - "effortMinutes": 40, - "primaryLocation": { - "message": "fix the issue here", - "filePath": "file1.js", - "textRange": { - "startLine": 1, - "startColumn": 2, - "endLine": 3, - "endColumn": 4 - } - } - }, - { - "primaryLocation": { - "message": "fix the bug here", - "filePath": "file2.js", - "textRange": { - "startLine": 3 - } - } - }, - { - "primaryLocation": { - "message": "fix the bug here", - "filePath": "file3.js" - } - }, - { - "primaryLocation": { - "message": "fix the bug here", - "filePath": "file3.js" - }, - "secondaryLocations": [ - { - "message": "fix the bug here", - "filePath": "file1.js", - "textRange": { - "startLine": 1 - } - }, - { - "filePath": "file2.js", - "textRange": { - "startLine": 2 - } - } - ] - } ] }, { - "ruleId": "rule2", + "id": "rule2", "name": "just_some_other_rule_name", "description": "just_some_description", "engineId": "test2", @@ -79,56 +28,113 @@ "softwareQuality": "RELIABILITY", "severity": "LOW" } - ], - "issues": [ + ] + } + ], + "issues": [ + { + "ruleId": "rule1", + "effortMinutes": 40, + "primaryLocation": { + "message": "fix the issue here", + "filePath": "file1.js", + "textRange": { + "startLine": 1, + "startColumn": 2, + "endLine": 3, + "endColumn": 4 + } + } + }, + { + "ruleId": "rule1", + "primaryLocation": { + "message": "fix the bug here", + "filePath": "file2.js", + "textRange": { + "startLine": 3 + } + } + }, + { + "ruleId": "rule1", + "primaryLocation": { + "message": "fix the bug here", + "filePath": "file3.js" + } + }, + { + "ruleId": "rule1", + "primaryLocation": { + "message": "fix the bug here", + "filePath": "file3.js" + }, + "secondaryLocations": [ { - "effortMinutes": 40, - "primaryLocation": { - "message": "fix the issue here", - "filePath": "file1.js", - "textRange": { - "startLine": 1, - "startColumn": 2, - "endLine": 3, - "endColumn": 4 - } + "message": "fix the bug here", + "filePath": "file1.js", + "textRange": { + "startLine": 1 } }, { - "primaryLocation": { - "message": "fix the bug here", - "filePath": "file2.js", - "textRange": { - "startLine": 3 - } + "filePath": "file2.js", + "textRange": { + "startLine": 2 } - }, + } + ] + }, + { + "ruleId": "rule2", + "effortMinutes": 40, + "primaryLocation": { + "message": "fix the issue here", + "filePath": "file1.js", + "textRange": { + "startLine": 1, + "startColumn": 2, + "endLine": 3, + "endColumn": 4 + } + } + }, + { + "ruleId": "rule2", + "primaryLocation": { + "message": "fix the bug here", + "filePath": "file2.js", + "textRange": { + "startLine": 3 + } + } + }, + { + "ruleId": "rule2", + "primaryLocation": { + "message": "fix the bug here", + "filePath": "file3.js" + } + }, + { + "ruleId": "rule2", + "primaryLocation": { + "message": "fix the bug here", + "filePath": "file3.js" + }, + "secondaryLocations": [ { - "primaryLocation": { - "message": "fix the bug here", - "filePath": "file3.js" + "message": "fix the bug here", + "filePath": "file1.js", + "textRange": { + "startLine": 1 } }, { - "primaryLocation": { - "message": "fix the bug here", - "filePath": "file3.js" - }, - "secondaryLocations": [ - { - "message": "fix the bug here", - "filePath": "file1.js", - "textRange": { - "startLine": 1 - } - }, - { - "filePath": "file2.js", - "textRange": { - "startLine": 2 - } - } - ] + "filePath": "file2.js", + "textRange": { + "startLine": 2 + } } ] } diff --git a/sonar-scanner-engine/src/test/resources/org/sonar/scanner/externalissue/cct/invalid_report.json b/sonar-scanner-engine/src/test/resources/org/sonar/scanner/externalissue/invalid_report.json similarity index 100% rename from sonar-scanner-engine/src/test/resources/org/sonar/scanner/externalissue/cct/invalid_report.json rename to sonar-scanner-engine/src/test/resources/org/sonar/scanner/externalissue/invalid_report.json -- 2.39.5