diff options
4 files changed, 187 insertions, 70 deletions
diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/sarif/LocationMapper.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/sarif/LocationMapper.java index df816a466d7..4b668c90d56 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/sarif/LocationMapper.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/sarif/LocationMapper.java @@ -30,7 +30,6 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.Optional; import java.util.concurrent.TimeUnit; -import javax.annotation.CheckForNull; import javax.annotation.Nullable; import org.jetbrains.annotations.NotNull; import org.sonar.api.batch.fs.InputFile; @@ -41,9 +40,7 @@ import org.sonar.api.scanner.ScannerSide; import org.sonar.sarif.pojo.ArtifactLocation; import org.sonar.sarif.pojo.Location; import org.sonar.sarif.pojo.PhysicalLocation; -import org.sonar.sarif.pojo.Result; -import static java.util.Objects.requireNonNull; import static org.sonar.api.utils.Preconditions.checkArgument; @ScannerSide @@ -65,31 +62,23 @@ public class LocationMapper { this.regionMapper = regionMapper; } - NewIssueLocation fillIssueInProjectLocation(Result result, NewIssueLocation newIssueLocation) { - return newIssueLocation - .message(getResultMessageOrThrow(result)) + void fillIssueInProjectLocation(NewIssueLocation newIssueLocation) { + newIssueLocation .on(sensorContext.project()); } - @CheckForNull - NewIssueLocation fillIssueInFileLocation(Result result, NewIssueLocation newIssueLocation, Location location) { - newIssueLocation.message(getResultMessageOrThrow(result)); + boolean fillIssueInFileLocation(NewIssueLocation newIssueLocation, Location location) { PhysicalLocation physicalLocation = location.getPhysicalLocation(); String fileUri = getFileUriOrThrow(location); Optional<InputFile> file = findFile(fileUri); if (file.isEmpty()) { - return null; + return false; } InputFile inputFile = file.get(); newIssueLocation.on(inputFile); regionMapper.mapRegion(physicalLocation.getRegion(), inputFile).ifPresent(newIssueLocation::at); - return newIssueLocation; - } - - private static String getResultMessageOrThrow(Result result) { - requireNonNull(result.getMessage(), "No messages found for issue thrown by rule " + result.getRuleId()); - return requireNonNull(result.getMessage().getText(), "No text found for messages in issue thrown by rule " + result.getRuleId()); + return true; } private static String getFileUriOrThrow(Location location) { diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/sarif/ResultMapper.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/sarif/ResultMapper.java index 21e85505172..05f8fa09b6e 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/sarif/ResultMapper.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/sarif/ResultMapper.java @@ -20,10 +20,13 @@ package org.sonar.scanner.externalissue.sarif; import com.google.common.collect.ImmutableMap; +import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Set; +import javax.annotation.CheckForNull; import javax.annotation.Nullable; import org.sonar.api.batch.rule.Severity; import org.sonar.api.batch.sensor.SensorContext; @@ -34,7 +37,9 @@ import org.sonar.api.rules.CleanCodeAttribute; import org.sonar.api.rules.RuleType; import org.sonar.api.scanner.ScannerSide; import org.sonar.sarif.pojo.Location; +import org.sonar.sarif.pojo.Message; import org.sonar.sarif.pojo.Result; +import org.sonar.sarif.pojo.Stack; import static java.util.Objects.requireNonNull; import static org.sonar.api.issue.impact.Severity.HIGH; @@ -96,29 +101,89 @@ public class ResultMapper { } private void mapLocations(Result result, NewExternalIssue newExternalIssue) { - NewIssueLocation newIssueLocation = newExternalIssue.newLocation(); - List<Location> locations = result.getLocations(); - if (locations == null || locations.isEmpty()) { - newExternalIssue.at(locationMapper.fillIssueInProjectLocation(result, newIssueLocation)); - } else { - Location firstLocation = locations.iterator().next(); - NewIssueLocation primaryLocation = fillFileOrProjectLocation(result, newIssueLocation, firstLocation); - newExternalIssue.at(primaryLocation); + createPrimaryLocation(newExternalIssue, result); + createSecondaryLocations(result, newExternalIssue); + createFlows(result, newExternalIssue); + } + + private void createFlows(Result result, NewExternalIssue newExternalIssue) { + Set<Stack> stacks = Optional.ofNullable(result.getStacks()).orElse(Set.of()); + if (!stacks.isEmpty()) { + stacks.forEach(stack -> { + var frames = Optional.ofNullable(stack.getFrames()).orElse(List.of()); + if (!frames.isEmpty()) { + List<NewIssueLocation> flow = new ArrayList<>(); + frames.forEach(frame -> { + var frameLocation = frame.getLocation(); + if (frameLocation != null) { + var newFrameLocation = createIssueLocation(newExternalIssue, frameLocation); + flow.add(newFrameLocation); + } + }); + newExternalIssue.addFlow(flow); + } + }); } + } - Set<Location> relatedLocations = result.getRelatedLocations(); - if (relatedLocations != null && !relatedLocations.isEmpty()) { + private void createSecondaryLocations(Result result, NewExternalIssue newExternalIssue) { + Set<Location> relatedLocations = Optional.ofNullable(result.getRelatedLocations()).orElse(Set.of()); + if (!relatedLocations.isEmpty()) { relatedLocations.forEach(relatedLocation -> { - NewIssueLocation newRelatedLocation = newExternalIssue.newLocation(); - fillFileOrProjectLocation(result, newRelatedLocation, relatedLocation); + var newRelatedLocation = createIssueLocation(newExternalIssue, relatedLocation); newExternalIssue.addLocation(newRelatedLocation); }); } } - private NewIssueLocation fillFileOrProjectLocation(Result result, NewIssueLocation newIssueLocation, Location firstLocation) { - return Optional.ofNullable(locationMapper.fillIssueInFileLocation(result, newIssueLocation, firstLocation)) - .orElseGet(() -> locationMapper.fillIssueInProjectLocation(result, newIssueLocation)); + private NewIssueLocation createIssueLocation(NewExternalIssue newExternalIssue, Location sarifLocation) { + NewIssueLocation newRelatedLocation = newExternalIssue.newLocation(); + var locationMessageText = getTextMessageOrNull(sarifLocation.getMessage()); + if (locationMessageText != null) { + newRelatedLocation.message(locationMessageText); + } + fillFileOrProjectLocation(newRelatedLocation, sarifLocation); + return newRelatedLocation; + } + + private void createPrimaryLocation(NewExternalIssue newExternalIssue, Result result) { + NewIssueLocation sonarLocation = newExternalIssue.newLocation(); + List<Location> sarifLocations = Optional.ofNullable(result.getLocations()).orElse(List.of()); + var primaryMessage = computePrimaryMessage(result, sarifLocations); + sonarLocation.message(primaryMessage); + if (sarifLocations.isEmpty()) { + locationMapper.fillIssueInProjectLocation(sonarLocation); + } else { + Location firstSarifLocation = sarifLocations.get(0); + fillFileOrProjectLocation(sonarLocation, firstSarifLocation); + } + newExternalIssue.at(sonarLocation); + } + + private static String computePrimaryMessage(Result result, List<Location> locations) { + String resultMessage = Objects.requireNonNull(result.getMessage().getText(), "Message text is required on result"); + if (!locations.isEmpty()) { + Location firstLocation = locations.get(0); + var locationMessageText = getTextMessageOrNull(firstLocation.getMessage()); + if (locationMessageText != null) { + return resultMessage + " - " + locationMessageText; + } + } + return resultMessage; + } + + @CheckForNull + private static String getTextMessageOrNull(@Nullable Message message) { + if (message == null) { + return null; + } + return message.getText(); + } + + private void fillFileOrProjectLocation(NewIssueLocation newIssueLocation, Location firstLocation) { + if (!locationMapper.fillIssueInFileLocation(newIssueLocation, firstLocation)) { + locationMapper.fillIssueInProjectLocation(newIssueLocation); + } } } diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/sarif/LocationMapperTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/sarif/LocationMapperTest.java index 18ddca2a866..c05e35429a3 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/sarif/LocationMapperTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/sarif/LocationMapperTest.java @@ -36,7 +36,6 @@ import org.sonar.api.batch.sensor.SensorContext; import org.sonar.api.batch.sensor.issue.NewIssueLocation; import org.sonar.api.scanner.fs.InputProject; import org.sonar.sarif.pojo.Location; -import org.sonar.sarif.pojo.Result; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; @@ -49,7 +48,6 @@ import static org.mockito.Mockito.when; @RunWith(MockitoJUnitRunner.class) public class LocationMapperTest { - private static final String TEST_MESSAGE = "test message"; private static final String URI_TEST = "URI_TEST"; private static final String EXPECTED_MESSAGE_URI_MISSING = "The field location.physicalLocation.artifactLocation.uri is not set."; @@ -66,8 +64,6 @@ public class LocationMapperTest { private NewIssueLocation newIssueLocation; @Mock(answer = Answers.RETURNS_DEEP_STUBS) - private Result result; - @Mock(answer = Answers.RETURNS_DEEP_STUBS) private Location location; @Mock @@ -75,13 +71,10 @@ public class LocationMapperTest { @Before public void setup() { - when(newIssueLocation.message(any())).thenReturn(newIssueLocation); when(newIssueLocation.on(any())).thenReturn(newIssueLocation); when(newIssueLocation.at(any())).thenReturn(newIssueLocation); when(sensorContext.project()).thenReturn(mock(InputProject.class)); - when(result.getMessage().getText()).thenReturn(TEST_MESSAGE); - when(location.getPhysicalLocation().getArtifactLocation().getUri()).thenReturn(URI_TEST); when(sensorContext.fileSystem().inputFile(any(FilePredicate.class))).thenReturn(inputFile); } @@ -105,12 +98,12 @@ public class LocationMapperTest { } @Test - public void fillIssueInFileLocation_whenFileNotFound_returnsNull() { + public void fillIssueInFileLocation_whenFileNotFound_returnsFalse() { when(sensorContext.fileSystem().inputFile(any())).thenReturn(null); - NewIssueLocation actualIssueLocation = locationMapper.fillIssueInFileLocation(result, newIssueLocation, location); + boolean success = locationMapper.fillIssueInFileLocation(newIssueLocation, location); - assertThat(actualIssueLocation).isNull(); + assertThat(success).isFalse(); } @Test @@ -118,10 +111,9 @@ public class LocationMapperTest { when(regionMapper.mapRegion(location.getPhysicalLocation().getRegion(), inputFile)) .thenReturn(Optional.empty()); - NewIssueLocation actualIssueLocation = locationMapper.fillIssueInFileLocation(result, newIssueLocation, location); + boolean success = locationMapper.fillIssueInFileLocation(newIssueLocation, location); - assertThat(actualIssueLocation).isSameAs(newIssueLocation); - verify(newIssueLocation).message(TEST_MESSAGE); + assertThat(success).isTrue(); verify(newIssueLocation).on(inputFile); verifyNoMoreInteractions(newIssueLocation); } @@ -132,10 +124,9 @@ public class LocationMapperTest { when(regionMapper.mapRegion(location.getPhysicalLocation().getRegion(), inputFile)) .thenReturn(Optional.of(textRange)); - NewIssueLocation actualIssueLocation = locationMapper.fillIssueInFileLocation(result, newIssueLocation, location); + boolean success = locationMapper.fillIssueInFileLocation(newIssueLocation, location); - assertThat(actualIssueLocation).isSameAs(newIssueLocation); - verify(newIssueLocation).message(TEST_MESSAGE); + assertThat(success).isTrue(); verify(newIssueLocation).on(inputFile); verify(newIssueLocation).at(textRange); verifyNoMoreInteractions(newIssueLocation); @@ -146,7 +137,7 @@ public class LocationMapperTest { when(location.getPhysicalLocation().getArtifactLocation().getUri()).thenReturn(null); assertThatIllegalArgumentException() - .isThrownBy(() -> locationMapper.fillIssueInFileLocation(result, newIssueLocation, location)) + .isThrownBy(() -> locationMapper.fillIssueInFileLocation(newIssueLocation, location)) .withMessage(EXPECTED_MESSAGE_URI_MISSING); } @@ -155,7 +146,7 @@ public class LocationMapperTest { when(location.getPhysicalLocation().getArtifactLocation()).thenReturn(null); assertThatIllegalArgumentException() - .isThrownBy(() -> locationMapper.fillIssueInFileLocation(result, newIssueLocation, location)) + .isThrownBy(() -> locationMapper.fillIssueInFileLocation(newIssueLocation, location)) .withMessage(EXPECTED_MESSAGE_URI_MISSING); } @@ -164,7 +155,7 @@ public class LocationMapperTest { when(location.getPhysicalLocation().getArtifactLocation()).thenReturn(null); assertThatIllegalArgumentException() - .isThrownBy(() -> locationMapper.fillIssueInFileLocation(result, newIssueLocation, location)) + .isThrownBy(() -> locationMapper.fillIssueInFileLocation(newIssueLocation, location)) .withMessage(EXPECTED_MESSAGE_URI_MISSING); } diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/sarif/ResultMapperTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/sarif/ResultMapperTest.java index 80e75793223..3efc8c51228 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/sarif/ResultMapperTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/externalissue/sarif/ResultMapperTest.java @@ -36,10 +36,14 @@ import org.sonar.api.batch.sensor.issue.NewExternalIssue; import org.sonar.api.batch.sensor.issue.NewIssueLocation; import org.sonar.api.rules.RuleType; import org.sonar.sarif.pojo.Location; +import org.sonar.sarif.pojo.Message; import org.sonar.sarif.pojo.Result; +import org.sonar.sarif.pojo.Stack; +import org.sonar.sarif.pojo.StackFrame; import static org.assertj.core.api.Assertions.assertThatNullPointerException; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; @@ -68,7 +72,6 @@ public class ResultMapperTest { @Mock private NewIssueLocation newExternalIssueLocation; - @Mock private Result result; @InjectMocks @@ -77,10 +80,10 @@ public class ResultMapperTest { @Before public void setUp() { MockitoAnnotations.openMocks(this); - when(result.getRuleId()).thenReturn(RULE_ID); + result = new Result().withMessage(new Message().withText("Result message")); + result.withRuleId(RULE_ID); when(sensorContext.newExternalIssue()).thenReturn(mockExternalIssue); - when(locationMapper.fillIssueInFileLocation(any(), any(), any())).thenReturn(newExternalIssueLocation); - when(locationMapper.fillIssueInProjectLocation(any(), any())).thenReturn(newExternalIssueLocation); + when(locationMapper.fillIssueInFileLocation(any(), any())).thenReturn(true); when(mockExternalIssue.newLocation()).thenReturn(newExternalIssueLocation); } @@ -96,7 +99,7 @@ public class ResultMapperTest { @Test public void mapResult_ifRuleIdMissing_fails() { - when(result.getRuleId()).thenReturn(null); + result.withRuleId(null); assertThatNullPointerException() .isThrownBy(() -> resultMapper.mapResult(DRIVER_NAME, WARNING, WARNING, result)) .withMessage("No ruleId found for issue thrown by driver driverName"); @@ -104,12 +107,12 @@ public class ResultMapperTest { @Test public void mapResult_whenLocationExists_createsFileLocation() { - Location location = mock(Location.class); - when(result.getLocations()).thenReturn(List.of(location)); + Location location = new Location(); + result.withLocations(List.of(location)); NewExternalIssue newExternalIssue = resultMapper.mapResult(DRIVER_NAME, WARNING, WARNING, result); - verify(locationMapper).fillIssueInFileLocation(result, newExternalIssueLocation, location); + verify(locationMapper).fillIssueInFileLocation(newExternalIssueLocation, location); verifyNoMoreInteractions(locationMapper); verify(newExternalIssue).at(newExternalIssueLocation); verify(newExternalIssue, never()).addLocation(any()); @@ -117,31 +120,100 @@ public class ResultMapperTest { } @Test + public void mapResult_useResultMessageForIssue() { + Location location = new Location(); + result.withLocations(List.of(location)); + + resultMapper.mapResult(DRIVER_NAME, WARNING, WARNING, result); + + verify(newExternalIssueLocation).message("Result message"); + } + + @Test + public void mapResult_concatResultMessageAndLocationMessageForIssue() { + Location location = new Location().withMessage(new Message().withText("Location message")); + result.withLocations(List.of(location)); + + resultMapper.mapResult(DRIVER_NAME, WARNING, WARNING, result); + + verify(newExternalIssueLocation).message("Result message - Location message"); + } + + @Test public void mapResult_whenRelatedLocationExists_createsSecondaryFileLocation() { - Location location = mock(Location.class); - when(result.getRelatedLocations()).thenReturn(Set.of(location)); + Location relatedLocation = new Location().withMessage(new Message().withText("Related location message")); + result.withRelatedLocations(Set.of(relatedLocation)); var newIssueLocationCall2 = mock(NewIssueLocation.class); when(mockExternalIssue.newLocation()).thenReturn(newExternalIssueLocation, newIssueLocationCall2); NewExternalIssue newExternalIssue = resultMapper.mapResult(DRIVER_NAME, WARNING, WARNING, result); - verify(locationMapper).fillIssueInProjectLocation(result, newExternalIssueLocation); - verify(locationMapper).fillIssueInFileLocation(result, newIssueLocationCall2, location); + verify(locationMapper).fillIssueInProjectLocation(newExternalIssueLocation); + verify(locationMapper).fillIssueInFileLocation(newIssueLocationCall2, relatedLocation); verifyNoMoreInteractions(locationMapper); verify(newExternalIssue).at(newExternalIssueLocation); verify(newExternalIssue).addLocation(newIssueLocationCall2); verify(newExternalIssue, never()).addFlow(any()); + verify(newIssueLocationCall2).message("Related location message"); + } + + @Test + public void mapResult_whenRelatedLocationExists_createsSecondaryFileLocation_no_messages() { + Location relatedLocationWithoutMessage = new Location(); + result.withRelatedLocations(Set.of(relatedLocationWithoutMessage)); + var newIssueLocationCall2 = mock(NewIssueLocation.class); + when(mockExternalIssue.newLocation()).thenReturn(newExternalIssueLocation, newIssueLocationCall2); + + NewExternalIssue newExternalIssue = resultMapper.mapResult(DRIVER_NAME, WARNING, WARNING, result); + + verify(newExternalIssue).addLocation(newIssueLocationCall2); + verify(newIssueLocationCall2, never()).message(anyString()); + } + + @Test + public void mapResult_whenStacksLocationExists_createsCodeFlowFileLocation() { + Location stackFrameLocation = new Location().withMessage(new Message().withText("Stack frame location message")); + var stackWithFrame = new Stack().withFrames(List.of(new StackFrame().withLocation(stackFrameLocation))); + var stackWithFrameNoLocation = new Stack().withFrames(List.of(new StackFrame())); + var stackWithoutFrame = new Stack().withFrames(List.of()); + var stackWithoutFrame2 = new Stack(); + result.withStacks(Set.of(stackWithFrame, stackWithFrameNoLocation, stackWithoutFrame, stackWithoutFrame2)); + var newIssueLocationCall2 = mock(NewIssueLocation.class); + when(mockExternalIssue.newLocation()).thenReturn(newExternalIssueLocation, newIssueLocationCall2); + + NewExternalIssue newExternalIssue = resultMapper.mapResult(DRIVER_NAME, WARNING, WARNING, result); + + verify(locationMapper).fillIssueInProjectLocation(newExternalIssueLocation); + verify(locationMapper).fillIssueInFileLocation(newIssueLocationCall2, stackFrameLocation); + verifyNoMoreInteractions(locationMapper); + verify(newExternalIssue).at(newExternalIssueLocation); + verify(newExternalIssue).addFlow(List.of(newIssueLocationCall2)); + verify(newExternalIssue, never()).addLocation(any()); + verify(newIssueLocationCall2).message("Stack frame location message"); + } + + @Test + public void mapResult_whenStacksLocationExists_createsCodeFlowFileLocation_no_text_messages() { + Location stackFrameLocationWithoutMessage = new Location().withMessage(new Message().withId("1")); + result.withStacks(Set.of(new Stack().withFrames(List.of(new StackFrame().withLocation(stackFrameLocationWithoutMessage))))); + var newIssueLocationCall2 = mock(NewIssueLocation.class); + when(mockExternalIssue.newLocation()).thenReturn(newExternalIssueLocation, newIssueLocationCall2); + + NewExternalIssue newExternalIssue = resultMapper.mapResult(DRIVER_NAME, WARNING, WARNING, result); + + verify(newExternalIssue).addFlow(List.of(newIssueLocationCall2)); + verify(newIssueLocationCall2, never()).message(anyString()); } @Test - public void mapResult_whenLocationExistsButLocationMapperReturnsNull_createsProjectLocation() { - Location location = mock(Location.class); - when(result.getLocations()).thenReturn(List.of(location)); - when(locationMapper.fillIssueInFileLocation(any(), any(), any())).thenReturn(null); + public void mapResult_whenLocationExistsButLocationMapperReturnsFalse_createsProjectLocation() { + Location location = new Location(); + result.withLocations(List.of(location)); + when(locationMapper.fillIssueInFileLocation(any(), any())).thenReturn(false); NewExternalIssue newExternalIssue = resultMapper.mapResult(DRIVER_NAME, WARNING, WARNING, result); - verify(locationMapper).fillIssueInProjectLocation(result, newExternalIssueLocation); + verify(locationMapper).fillIssueInProjectLocation(newExternalIssueLocation); verify(newExternalIssue).at(newExternalIssueLocation); verify(newExternalIssue, never()).addLocation(any()); verify(newExternalIssue, never()).addFlow(any()); @@ -150,8 +222,9 @@ public class ResultMapperTest { @Test public void mapResult_whenLocationsIsEmpty_createsProjectLocation() { NewExternalIssue newExternalIssue = resultMapper.mapResult(DRIVER_NAME, WARNING, WARNING, result); + result.withLocations(List.of()); - verify(locationMapper).fillIssueInProjectLocation(result, newExternalIssueLocation); + verify(locationMapper).fillIssueInProjectLocation(newExternalIssueLocation); verifyNoMoreInteractions(locationMapper); verify(newExternalIssue).at(newExternalIssueLocation); verify(newExternalIssue, never()).addLocation(any()); @@ -160,10 +233,9 @@ public class ResultMapperTest { @Test public void mapResult_whenLocationsIsNull_createsProjectLocation() { - when(result.getLocations()).thenReturn(null); NewExternalIssue newExternalIssue = resultMapper.mapResult(DRIVER_NAME, WARNING, WARNING, result); - verify(locationMapper).fillIssueInProjectLocation(result, newExternalIssueLocation); + verify(locationMapper).fillIssueInProjectLocation(newExternalIssueLocation); verifyNoMoreInteractions(locationMapper); verify(newExternalIssue).at(newExternalIssueLocation); verify(newExternalIssue, never()).addLocation(any()); |