From a86f840b160eeeca5c712689321cf4b205da8256 Mon Sep 17 00:00:00 2001 From: Wojtek Wajerowicz <115081248+wojciech-wajerowicz-sonarsource@users.noreply.github.com> Date: Mon, 12 Jun 2023 14:39:49 +0200 Subject: [PATCH] SONAR-19309 Improve SARIF logs in case of failure. (#8503) --- .../sarif/SarifIssuesImportSensor.java | 19 ++- .../sarif/SarifIssuesImportSensorTest.java | 136 ++++++++++++++---- 2 files changed, 122 insertions(+), 33 deletions(-) diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/sarif/SarifIssuesImportSensor.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/sarif/SarifIssuesImportSensor.java index 7bb05650617..346cb727f7d 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/sarif/SarifIssuesImportSensor.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/sarif/SarifIssuesImportSensor.java @@ -27,6 +27,8 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.stream.Collectors; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.sonar.api.CoreProperties; import org.sonar.api.batch.sensor.SensorContext; import org.sonar.api.batch.sensor.SensorDescriptor; @@ -35,15 +37,13 @@ import org.sonar.api.config.PropertyDefinition; import org.sonar.api.resources.Qualifiers; import org.sonar.api.scanner.ScannerSide; import org.sonar.api.scanner.sensor.ProjectSensor; -import org.sonar.api.utils.log.Logger; -import org.sonar.api.utils.log.Loggers; import org.sonar.core.sarif.Sarif210; import org.sonar.core.sarif.SarifSerializer; @ScannerSide public class SarifIssuesImportSensor implements ProjectSensor { - private static final Logger LOG = Loggers.get(SarifIssuesImportSensor.class); + private static final Logger LOG = LoggerFactory.getLogger(SarifIssuesImportSensor.class); static final String SARIF_REPORT_PATHS_PROPERTY_KEY = "sonar.sarifReportPaths"; private final SarifSerializer sarifSerializer; @@ -100,7 +100,16 @@ public class SarifIssuesImportSensor implements ProjectSensor { } private static void displayResults(String filePath, SarifImportResults sarifImportResults) { - LOG.info("File {}: successfully imported {} vulnerabilities spread in {} runs. {} failed run(s).", - filePath, sarifImportResults.getSuccessFullyImportedIssues(), sarifImportResults.getSuccessFullyImportedRuns(), sarifImportResults.getFailedRuns()); + if (sarifImportResults.getFailedRuns() > 0 && sarifImportResults.getSuccessFullyImportedRuns() > 0) { + LOG.warn("File {}: {} run(s) could not be imported (see warning above) and {} run(s) successfully imported ({} vulnerabilities in total).", + filePath, sarifImportResults.getFailedRuns(), sarifImportResults.getSuccessFullyImportedRuns(), sarifImportResults.getSuccessFullyImportedIssues()); + + } else if (sarifImportResults.getFailedRuns() > 0) { + LOG.warn("File {}: {} run(s) could not be imported (see warning above).", + filePath, sarifImportResults.getFailedRuns()); + } else { + LOG.info("File {}: {} run(s) successfully imported ({} vulnerabilities in total).", + filePath, sarifImportResults.getSuccessFullyImportedRuns(), sarifImportResults.getSuccessFullyImportedIssues()); + } } } diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/sarif/SarifIssuesImportSensorTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/sarif/SarifIssuesImportSensorTest.java index 533e05d9711..6a85b604a86 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/sarif/SarifIssuesImportSensorTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/sarif/SarifIssuesImportSensorTest.java @@ -22,7 +22,6 @@ package org.sonar.scanner.externalissue.sarif; import com.google.common.collect.MoreCollectors; import java.nio.file.Path; import java.util.Optional; -import org.apache.commons.lang.math.RandomUtils; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -69,10 +68,10 @@ public class SarifIssuesImportSensorTest { private final SensorContextTester sensorContext = SensorContextTester.create(Path.of(".")); @Test - public void execute_single_files() { + public void execute_whenSingleFileIsSpecified_shouldImportResults() { sensorSettings.setProperty("sonar.sarifReportPaths", FILE_1); - ReportAndResults reportAndResults = mockReportAndResults(FILE_1); + ReportAndResults reportAndResults = mockSuccessfulReportAndResults(FILE_1); SarifIssuesImportSensor sensor = new SarifIssuesImportSensor(sarifSerializer, sarifImporter, sensorSettings.asConfig()); sensor.execute(sensorContext); @@ -80,16 +79,14 @@ public class SarifIssuesImportSensorTest { verify(sarifImporter).importSarif(reportAndResults.getSarifReport()); assertThat(logTester.logs(Level.INFO)).hasSize(1); - assertSummaryIsCorrectlyDisplayed(FILE_1, reportAndResults.getSarifImportResults()); + assertSummaryIsCorrectlyDisplayedForSuccessfulFile(FILE_1, reportAndResults.getSarifImportResults()); } @Test - public void execute_multiple_files() { - + public void execute_whenMultipleFilesAreSpecified_shouldImportResults() { sensorSettings.setProperty("sonar.sarifReportPaths", SARIF_REPORT_PATHS_PARAM); - - ReportAndResults reportAndResults1 = mockReportAndResults(FILE_1); - ReportAndResults reportAndResults2 = mockReportAndResults(FILE_2); + ReportAndResults reportAndResults1 = mockSuccessfulReportAndResults(FILE_1); + ReportAndResults reportAndResults2 = mockSuccessfulReportAndResults(FILE_2); SarifIssuesImportSensor sensor = new SarifIssuesImportSensor(sarifSerializer, sarifImporter, sensorSettings.asConfig()); sensor.execute(sensorContext); @@ -97,16 +94,54 @@ public class SarifIssuesImportSensorTest { verify(sarifImporter).importSarif(reportAndResults1.getSarifReport()); verify(sarifImporter).importSarif(reportAndResults2.getSarifReport()); - assertSummaryIsCorrectlyDisplayed(FILE_1, reportAndResults1.getSarifImportResults()); - assertSummaryIsCorrectlyDisplayed(FILE_2, reportAndResults2.getSarifImportResults()); + assertSummaryIsCorrectlyDisplayedForSuccessfulFile(FILE_1, reportAndResults1.getSarifImportResults()); + assertSummaryIsCorrectlyDisplayedForSuccessfulFile(FILE_2, reportAndResults2.getSarifImportResults()); + } + + @Test + public void execute_whenFileContainsOnlySuccessfulRuns_shouldLogCorrectMessage() { + sensorSettings.setProperty("sonar.sarifReportPaths", FILE_1); + ReportAndResults reportAndResults = mockSuccessfulReportAndResults(FILE_1); + + SarifIssuesImportSensor sensor = new SarifIssuesImportSensor(sarifSerializer, sarifImporter, sensorSettings.asConfig()); + sensor.execute(sensorContext); + + assertSummaryIsCorrectlyDisplayedForSuccessfulFile(FILE_1, reportAndResults.getSarifImportResults()); } @Test - public void skip_report_when_import_fails() { + public void execute_whenFileContainsOnlyFailedRuns_shouldLogCorrectMessage() { + + sensorSettings.setProperty("sonar.sarifReportPaths", FILE_1); + ReportAndResults reportAndResults = mockFailedReportAndResults(FILE_1); + + SarifIssuesImportSensor sensor = new SarifIssuesImportSensor(sarifSerializer, sarifImporter, sensorSettings.asConfig()); + sensor.execute(sensorContext); + + assertSummaryIsCorrectlyDisplayedForFailedFile(FILE_1, reportAndResults.getSarifImportResults()); + } + + @Test + public void execute_whenFileContainsFailedAndSuccessfulRuns_shouldLogCorrectMessage() { + + sensorSettings.setProperty("sonar.sarifReportPaths", FILE_1); + + ReportAndResults reportAndResults = mockMixedReportAndResults(FILE_1); + + SarifIssuesImportSensor sensor = new SarifIssuesImportSensor(sarifSerializer, sarifImporter, sensorSettings.asConfig()); + sensor.execute(sensorContext); + + verify(sarifImporter).importSarif(reportAndResults.getSarifReport()); + + assertSummaryIsCorrectlyDisplayedForMixedFile(FILE_1, reportAndResults.getSarifImportResults()); + } + + @Test + public void execute_whenImportFails_shouldSkipReport() { sensorSettings.setProperty("sonar.sarifReportPaths", SARIF_REPORT_PATHS_PARAM); - ReportAndResults reportAndResults1 = mockReportAndResults(FILE_1); - ReportAndResults reportAndResults2 = mockReportAndResults(FILE_2); + ReportAndResults reportAndResults1 = mockFailedReportAndResults(FILE_1); + ReportAndResults reportAndResults2 = mockSuccessfulReportAndResults(FILE_2); doThrow(new NullPointerException("import failed")).when(sarifImporter).importSarif(reportAndResults1.getSarifReport()); @@ -115,22 +150,22 @@ public class SarifIssuesImportSensorTest { verify(sarifImporter).importSarif(reportAndResults2.getSarifReport()); assertThat(logTester.logs(Level.WARN)).contains("Failed to process SARIF report from file 'path/to/sarif/file.sarif', error: 'import failed'"); - assertSummaryIsCorrectlyDisplayed(FILE_2, reportAndResults2.getSarifImportResults()); + assertSummaryIsCorrectlyDisplayedForSuccessfulFile(FILE_2, reportAndResults2.getSarifImportResults()); } @Test - public void skip_report_when_deserialization_fails() { + public void execute_whenDeserializationFails_shouldSkipReport() { sensorSettings.setProperty("sonar.sarifReportPaths", SARIF_REPORT_PATHS_PARAM); failDeserializingReport(FILE_1); - ReportAndResults reportAndResults2 = mockReportAndResults(FILE_2); + ReportAndResults reportAndResults2 = mockSuccessfulReportAndResults(FILE_2); SarifIssuesImportSensor sensor = new SarifIssuesImportSensor(sarifSerializer, sarifImporter, sensorSettings.asConfig()); sensor.execute(sensorContext); verify(sarifImporter).importSarif(reportAndResults2.getSarifReport()); assertThat(logTester.logs(Level.WARN)).contains("Failed to process SARIF report from file 'path/to/sarif/file.sarif', error: 'deserialization failed'"); - assertSummaryIsCorrectlyDisplayed(FILE_2, reportAndResults2.getSarifImportResults()); + assertSummaryIsCorrectlyDisplayedForSuccessfulFile(FILE_2, reportAndResults2.getSarifImportResults()); } private void failDeserializingReport(String path) { @@ -138,29 +173,74 @@ public class SarifIssuesImportSensorTest { when(sarifSerializer.deserialize(reportFilePath)).thenThrow(new NullPointerException("deserialization failed")); } - private ReportAndResults mockReportAndResults(String path) { + private ReportAndResults mockSuccessfulReportAndResults(String path) { + Sarif210 report = mockSarifReport(path); + + SarifImportResults sarifImportResults = mock(SarifImportResults.class); + when(sarifImportResults.getSuccessFullyImportedIssues()).thenReturn(10); + when(sarifImportResults.getSuccessFullyImportedRuns()).thenReturn(3); + when(sarifImportResults.getFailedRuns()).thenReturn(0); + + when(sarifImporter.importSarif(report)).thenReturn(sarifImportResults); + return new ReportAndResults(report, sarifImportResults); + } + + private Sarif210 mockSarifReport(String path) { Sarif210 report = mock(Sarif210.class); Path reportFilePath = sensorContext.fileSystem().resolvePath(path).toPath(); when(sarifSerializer.deserialize(reportFilePath)).thenReturn(report); + return report; + } + + private ReportAndResults mockFailedReportAndResults(String path) { + Sarif210 report = mockSarifReport(path); SarifImportResults sarifImportResults = mock(SarifImportResults.class); - when(sarifImportResults.getSuccessFullyImportedIssues()).thenReturn(RandomUtils.nextInt()); - when(sarifImportResults.getSuccessFullyImportedRuns()).thenReturn(RandomUtils.nextInt()); - when(sarifImportResults.getFailedRuns()).thenReturn(RandomUtils.nextInt()); + when(sarifImportResults.getSuccessFullyImportedRuns()).thenReturn(0); + when(sarifImportResults.getFailedRuns()).thenReturn(1); when(sarifImporter.importSarif(report)).thenReturn(sarifImportResults); return new ReportAndResults(report, sarifImportResults); } - private void assertSummaryIsCorrectlyDisplayed(String filePath, SarifImportResults sarifImportResults) { - LogAndArguments logAndArguments = findLogEntry(filePath); - assertThat(logAndArguments.getRawMsg()).isEqualTo("File {}: successfully imported {} vulnerabilities spread in {} runs. {} failed run(s)."); + private ReportAndResults mockMixedReportAndResults(String path) { + Sarif210 report = mockSarifReport(path); + + SarifImportResults sarifImportResults = mock(SarifImportResults.class); + when(sarifImportResults.getSuccessFullyImportedIssues()).thenReturn(10); + when(sarifImportResults.getSuccessFullyImportedRuns()).thenReturn(3); + when(sarifImportResults.getFailedRuns()).thenReturn(1); + + when(sarifImporter.importSarif(report)).thenReturn(sarifImportResults); + return new ReportAndResults(report, sarifImportResults); + } + + private void assertSummaryIsCorrectlyDisplayedForSuccessfulFile(String filePath, SarifImportResults sarifImportResults) { + verifyLogContainsLine(LoggerLevel.INFO, filePath, "File {}: {} run(s) successfully imported ({} vulnerabilities in total).", + filePath, sarifImportResults.getSuccessFullyImportedRuns(), sarifImportResults.getSuccessFullyImportedIssues()); + } + + private void assertSummaryIsCorrectlyDisplayedForFailedFile(String filePath, SarifImportResults sarifImportResults) { + verifyLogContainsLine(LoggerLevel.WARN, filePath, "File {}: {} run(s) could not be imported (see warning above).", + filePath, sarifImportResults.getFailedRuns()); + } + + private void assertSummaryIsCorrectlyDisplayedForMixedFile(String filePath, SarifImportResults sarifImportResults) { + verifyLogContainsLine(LoggerLevel.WARN, filePath, + "File {}: {} run(s) could not be imported (see warning above) and {} run(s) successfully imported ({} vulnerabilities in total).", + filePath, sarifImportResults.getFailedRuns(), sarifImportResults.getSuccessFullyImportedRuns(), sarifImportResults.getSuccessFullyImportedIssues()); + } + + private void verifyLogContainsLine(LoggerLevel level, String filePath, String rawMsg, Object... arguments) { + LogAndArguments logAndArguments = findLogEntry(level, filePath); + assertThat(logAndArguments.getRawMsg()) + .isEqualTo(rawMsg); assertThat(logAndArguments.getArgs()).isPresent() - .contains(new Object[] {filePath, sarifImportResults.getSuccessFullyImportedIssues(), sarifImportResults.getSuccessFullyImportedRuns(), sarifImportResults.getFailedRuns()}); + .contains(arguments); } - private LogAndArguments findLogEntry(String filePath) { - Optional optLogAndArguments = logTester.getLogs(LoggerLevel.INFO).stream() + private LogAndArguments findLogEntry(LoggerLevel level, String filePath) { + Optional optLogAndArguments = logTester.getLogs(level).stream() .filter(log -> log.getFormattedMsg().contains(filePath)) .collect(MoreCollectors.toOptional()); assertThat(optLogAndArguments).as("Log entry missing for file %s", filePath).isPresent(); -- 2.39.5