Skip to content

Commit

Permalink
SONAR-20659 Add logs when importing SARIF report if the referenced fi…
Browse files Browse the repository at this point in the history 
…les location cannot be resolved

Co-authored-by: antoine.vinot <antoine.vinot@sonarsource.com>
  • Loading branch information
@javier-garcia-sonarsource @antoine-vinot-sonarsource
2 people authored and sonartech committed Dec 20, 2024
1 parent 611b4ce commit 3ce1c4c
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 22 deletions.
12 changes: 11 additions & 1 deletion sonar-scanner-engine/src/main/java/org/sonar/scanner/externalissue/sarif/LocationMapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,13 @@
import javax.annotation.Nullable;
import org.apache.commons.lang.StringUtils;
import org.jetbrains.annotations.NotNull;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.sonar.api.batch.fs.InputFile;
import org.sonar.api.batch.fs.internal.predicates.AbstractFilePredicate;
import org.sonar.api.batch.sensor.SensorContext;
import org.sonar.api.batch.sensor.issue.NewIssueLocation;
import org.sonar.api.notifications.AnalysisWarnings;
import org.sonar.api.scanner.ScannerSide;
import org.sonar.sarif.pojo.ArtifactLocation;
import org.sonar.sarif.pojo.Location;
Expand All @@ -46,21 +49,25 @@

@ScannerSide
public class LocationMapper {
private static final Logger LOG = LoggerFactory.getLogger(LocationMapper.class);

private static final int CACHE_SIZE = 500;
private static final int CACHE_EXPIRY = 60;

private final SensorContext sensorContext;
private final RegionMapper regionMapper;
private final AnalysisWarnings analysisWarnings;

LoadingCache<String, Optional<InputFile>> inputFileCache = CacheBuilder.newBuilder()
.maximumSize(CACHE_SIZE)
.expireAfterAccess(CACHE_EXPIRY, TimeUnit.SECONDS)
.concurrencyLevel(Runtime.getRuntime().availableProcessors())
.build(getCacheLoader());

LocationMapper(SensorContext sensorContext, RegionMapper regionMapper) {
LocationMapper(SensorContext sensorContext, RegionMapper regionMapper, AnalysisWarnings analysisWarnings) {
this.sensorContext = sensorContext;
this.regionMapper = regionMapper;
this.analysisWarnings = analysisWarnings;
}

void fillIssueInProjectLocation(NewIssueLocation newIssueLocation) {
Expand All @@ -74,6 +81,9 @@ boolean fillIssueInFileLocation(NewIssueLocation newIssueLocation, Location loca
String fileUri = getFileUriOrThrow(location);
Optional<InputFile> file = findFile(fileUri);
if (file.isEmpty()) {
String unresolvedLocationWarning = String.format("Unable to resolve Issue location from SARIF physical location %s. Falling back to the project location.", fileUri);
analysisWarnings.addUnique(unresolvedLocationWarning);
LOG.warn(unresolvedLocationWarning);
return false;
}
InputFile inputFile = file.get();
Expand Down
45 changes: 24 additions & 21 deletions ...canner-engine/src/test/java/org/sonar/scanner/externalissue/sarif/LocationMapperTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,13 @@
*/
package org.sonar.scanner.externalissue.sarif;

import com.tngtech.java.junit.dataprovider.DataProvider;
import com.tngtech.java.junit.dataprovider.DataProviderRunner;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Optional;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import org.mockito.Answers;
import org.mockito.InjectMocks;
import org.mockito.Mock;
Expand All @@ -36,6 +35,7 @@
import org.sonar.api.batch.fs.TextRange;
import org.sonar.api.batch.sensor.SensorContext;
import org.sonar.api.batch.sensor.issue.NewIssueLocation;
import org.sonar.api.notifications.AnalysisWarnings;
import org.sonar.api.scanner.fs.InputProject;
import org.sonar.sarif.pojo.Location;

Expand All @@ -48,8 +48,7 @@
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;

@RunWith(DataProviderRunner.class)
public class LocationMapperTest {
class LocationMapperTest {

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.";
Expand All @@ -60,6 +59,9 @@ public class LocationMapperTest {
@Mock
private RegionMapper regionMapper;

@Mock
private AnalysisWarnings analysisWarnings;

@InjectMocks
private LocationMapper locationMapper;

Expand All @@ -72,8 +74,8 @@ public class LocationMapperTest {
@Mock
private InputFile inputFile;

@Before
public void setup() {
@BeforeEach
void setup() {
MockitoAnnotations.openMocks(this);
when(newIssueLocation.on(any())).thenReturn(newIssueLocation);
when(newIssueLocation.at(any())).thenReturn(newIssueLocation);
Expand All @@ -84,32 +86,33 @@ public void setup() {
}

@Test
public void isPredicate_whenDifferentFile_returnsFalse() {
void isPredicate_whenDifferentFile_returnsFalse() {
Path path = Paths.get("file");
when(inputFile.path()).thenReturn(Paths.get("file2"));
LocationMapper.IsPredicate isPredicate = new LocationMapper.IsPredicate(path);
assertThat(isPredicate.apply(inputFile)).isFalse();
}

@Test
public void isPredicate_whenSameFile_returnsTrue() {
void isPredicate_whenSameFile_returnsTrue() {
Path path = Paths.get("file");
when(inputFile.path()).thenReturn(path);
LocationMapper.IsPredicate isPredicate = new LocationMapper.IsPredicate(path);
assertThat(isPredicate.apply(inputFile)).isTrue();
}

@Test
public void fillIssueInFileLocation_whenFileNotFound_returnsFalse() {
void fillIssueInFileLocation_whenFileNotFound_returnsFalseAndAddsWarningMessage() {
when(sensorContext.fileSystem().inputFile(any())).thenReturn(null);

boolean success = locationMapper.fillIssueInFileLocation(newIssueLocation, location);

assertThat(success).isFalse();
verify(analysisWarnings).addUnique(String.format("Unable to resolve Issue location from SARIF physical location %s. Falling back to the project location.", URI_TEST));
}

@Test
public void fillIssueInFileLocation_whenMapRegionReturnsNull_onlyFillsSimpleFields() {
void fillIssueInFileLocation_whenMapRegionReturnsNull_onlyFillsSimpleFields() {
when(regionMapper.mapRegion(location.getPhysicalLocation().getRegion(), inputFile))
.thenReturn(Optional.empty());

Expand All @@ -121,7 +124,7 @@ public void fillIssueInFileLocation_whenMapRegionReturnsNull_onlyFillsSimpleFiel
}

@Test
public void fillIssueInFileLocation_whenMapRegionReturnsRegion_callsAt() {
void fillIssueInFileLocation_whenMapRegionReturnsRegion_callsAt() {
TextRange textRange = mock(TextRange.class);
when(regionMapper.mapRegion(location.getPhysicalLocation().getRegion(), inputFile))
.thenReturn(Optional.of(textRange));
Expand All @@ -135,17 +138,17 @@ public void fillIssueInFileLocation_whenMapRegionReturnsRegion_callsAt() {
}

@Test
public void fillIssueInFileLocation_ifNullUri_throws() {
void fillIssueInFileLocation_ifNullUri_throws() {
when(location.getPhysicalLocation().getArtifactLocation().getUri()).thenReturn(null);

assertThatIllegalArgumentException()
.isThrownBy(() -> locationMapper.fillIssueInFileLocation(newIssueLocation, location))
.withMessage(EXPECTED_MESSAGE_URI_MISSING);
}

@Test
@DataProvider({"file:///", "file:///path/", "file://host/", "file://host/path/", "file:////server/"})
public void fillIssueInFileLocation_ifCorrectUriWithFileScheme_returnsTrue(String uriPrefix) {
@ParameterizedTest
@ValueSource(strings = {"file:///", "file:///path/", "file://host/", "file://host/path/", "file:////server/"})
void fillIssueInFileLocation_ifCorrectUriWithFileScheme_returnsTrue(String uriPrefix) {
when(location.getPhysicalLocation().getArtifactLocation().getUri()).thenReturn(uriPrefix + URI_TEST);

boolean success = locationMapper.fillIssueInFileLocation(newIssueLocation, location);
Expand All @@ -156,15 +159,15 @@ public void fillIssueInFileLocation_ifCorrectUriWithFileScheme_returnsTrue(Strin
}

@Test
public void fillIssueInFileLocation_ifIncorrectUriWithFileScheme_throws() {
void fillIssueInFileLocation_ifIncorrectUriWithFileScheme_throws() {
when(location.getPhysicalLocation().getArtifactLocation().getUri()).thenReturn("file://" + URI_TEST);

assertThatThrownBy(() -> locationMapper.fillIssueInFileLocation(newIssueLocation, location))
.hasRootCause(new IllegalArgumentException("Invalid file scheme URI: file://" + URI_TEST));
}

@Test
public void fillIssueInFileLocation_ifNullArtifactLocation_throws() {
void fillIssueInFileLocation_ifNullArtifactLocation_throws() {
when(location.getPhysicalLocation().getArtifactLocation()).thenReturn(null);

assertThatIllegalArgumentException()
Expand All @@ -173,7 +176,7 @@ public void fillIssueInFileLocation_ifNullArtifactLocation_throws() {
}

@Test
public void fillIssueInFileLocation_ifNullPhysicalLocation_throws() {
void fillIssueInFileLocation_ifNullPhysicalLocation_throws() {
when(location.getPhysicalLocation()).thenReturn(null);

assertThatIllegalArgumentException()
Expand Down

0 comments on commit 3ce1c4c

Please sign in to comment.