Browse Source

Revert "SONAR-18877 Compute primary location hash for all issues"

This reverts commit 8f608206ce.
tags/10.1.0.73491
Antoine Vigneau 11 months ago
parent
commit
8acf02b481

+ 42
- 58
server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/ComputeLocationHashesVisitor.java View File

@@ -35,17 +35,16 @@ import org.sonar.db.protobuf.DbIssues;
import org.sonar.server.issue.TaintChecker;

import static org.apache.commons.lang.StringUtils.defaultIfEmpty;
import static org.sonar.api.rules.RuleType.SECURITY_HOTSPOT;

/**
* This visitor will update the locations field of issues, by filling hashes for their locations:
* - Primary location hash: for all issues, when needed (ie. is missing or the issue is new/updated)
* - Secondary location hash: only for taint vulnerabilities and security hotspots, when needed (the issue is new/updated)
* This visitor will update the locations field of issues, by filling the hashes for all locations.
* It only applies to issues that are taint vulnerabilities or security hotspots, and that are new or were changed.
* For performance reasons, it will read each source code file once and feed the lines to all locations in that file.
*/
public class ComputeLocationHashesVisitor extends IssueVisitor {
private static final Pattern MATCH_ALL_WHITESPACES = Pattern.compile("\\s");
private final List<DefaultIssue> issuesForAllLocations = new LinkedList<>();
private final List<DefaultIssue> issuesForPrimaryLocation = new LinkedList<>();
private final List<DefaultIssue> issues = new LinkedList<>();
private final SourceLinesRepository sourceLinesRepository;
private final TreeRootHolder treeRootHolder;
private final TaintChecker taintChecker;
@@ -58,41 +57,20 @@ public class ComputeLocationHashesVisitor extends IssueVisitor {

@Override
public void beforeComponent(Component component) {
issuesForAllLocations.clear();
issuesForPrimaryLocation.clear();
issues.clear();
}

@Override
public void onIssue(Component component, DefaultIssue issue) {
if (issueNeedsLocationHashes(issue)) {
if (shouldComputeAllLocationHashes(issue)) {
issuesForAllLocations.add(issue);
} else if (shouldComputePrimaryLocationHash(issue)) {
// Issues in this situation are not necessarily marked as changed, so we do it to ensure persistence
issue.setChanged(true);
issuesForPrimaryLocation.add(issue);
}
if (shouldComputeLocation(issue)) {
issues.add(issue);
}
}

private static boolean issueNeedsLocationHashes(DefaultIssue issue) {
DbIssues.Locations locations = issue.getLocations();
return locations != null && !issue.isFromExternalRuleEngine();
}

private boolean shouldComputeAllLocationHashes(DefaultIssue issue) {
return taintChecker.isTaintVulnerability(issue)
&& isIssueUpdated(issue);
}

private static boolean shouldComputePrimaryLocationHash(DefaultIssue issue) {
DbIssues.Locations locations = issue.getLocations();
return (locations.hasTextRange() && !locations.hasChecksum())
|| isIssueUpdated(issue);
}

private static boolean isIssueUpdated(DefaultIssue issue) {
return issue.isNew() || issue.locationsChanged();
private boolean shouldComputeLocation(DefaultIssue issue) {
return (taintChecker.isTaintVulnerability(issue) || SECURITY_HOTSPOT.equals(issue.type()))
&& !issue.isFromExternalRuleEngine()
&& (issue.isNew() || issue.locationsChanged());
}

@Override
@@ -100,10 +78,20 @@ public class ComputeLocationHashesVisitor extends IssueVisitor {
Map<Component, List<Location>> locationsByComponent = new HashMap<>();
List<LocationToSet> locationsToSet = new LinkedList<>();

// Issues that needs both primary and secondary locations hashes
extractForAllLocations(component, locationsByComponent, locationsToSet);
// Then issues that needs only primary locations
extractForPrimaryLocation(component, locationsByComponent, locationsToSet);
for (DefaultIssue issue : issues) {
DbIssues.Locations locations = issue.getLocations();
if (locations == null) {
continue;
}

DbIssues.Locations.Builder primaryLocationBuilder = locations.toBuilder();
boolean hasTextRange = addLocations(component, issue, locationsByComponent, primaryLocationBuilder);

// If any location was added (because it had a text range), we'll need to update the issue at the end with the new object containing the hashes
if (hasTextRange) {
locationsToSet.add(new LocationToSet(issue, primaryLocationBuilder));
}
}

// Feed lines to locations, component by component
locationsByComponent.forEach(this::updateLocationsInComponent);
@@ -114,41 +102,37 @@ public class ComputeLocationHashesVisitor extends IssueVisitor {
// set new locations to issues
locationsToSet.forEach(LocationToSet::set);

issuesForAllLocations.clear();
issuesForPrimaryLocation.clear();
issues.clear();
}

private void extractForAllLocations(Component component, Map<Component, List<Location>> locationsByComponent, List<LocationToSet> locationsToSet) {
for (DefaultIssue issue : issuesForAllLocations) {
DbIssues.Locations.Builder locationsBuilder = ((DbIssues.Locations) issue.getLocations()).toBuilder();
addPrimaryLocation(component, locationsByComponent, locationsBuilder);
addSecondaryLocations(issue, locationsByComponent, locationsBuilder);
locationsToSet.add(new LocationToSet(issue, locationsBuilder));
}
private boolean addLocations(Component component, DefaultIssue issue, Map<Component, List<Location>> locationsByComponent, DbIssues.Locations.Builder primaryLocationBuilder) {
boolean hasPrimaryLocation = addPrimaryLocation(component, locationsByComponent, primaryLocationBuilder);
boolean hasSecondaryLocations = addSecondaryLocations(issue, locationsByComponent, primaryLocationBuilder);
return hasPrimaryLocation || hasSecondaryLocations;
}

private void extractForPrimaryLocation(Component component, Map<Component, List<Location>> locationsByComponent, List<LocationToSet> locationsToSet) {
for (DefaultIssue issue : issuesForPrimaryLocation) {
DbIssues.Locations.Builder locationsBuilder = ((DbIssues.Locations) issue.getLocations()).toBuilder();
addPrimaryLocation(component, locationsByComponent, locationsBuilder);
locationsToSet.add(new LocationToSet(issue, locationsBuilder));
private static boolean addPrimaryLocation(Component component, Map<Component, List<Location>> locationsByComponent, DbIssues.Locations.Builder primaryLocationBuilder) {
if (!primaryLocationBuilder.hasTextRange()) {
return false;
}
PrimaryLocation primaryLocation = new PrimaryLocation(primaryLocationBuilder);
locationsByComponent.computeIfAbsent(component, c -> new LinkedList<>()).add(primaryLocation);
return true;
}

private static void addPrimaryLocation(Component component, Map<Component, List<Location>> locationsByComponent, DbIssues.Locations.Builder locationsBuilder) {
if (locationsBuilder.hasTextRange()) {
PrimaryLocation primaryLocation = new PrimaryLocation(locationsBuilder);
locationsByComponent.computeIfAbsent(component, c -> new LinkedList<>()).add(primaryLocation);
private boolean addSecondaryLocations(DefaultIssue issue, Map<Component, List<Location>> locationsByComponent, DbIssues.Locations.Builder primaryLocationBuilder) {
if (SECURITY_HOTSPOT.equals(issue.type())) {
return false;
}
}

private void addSecondaryLocations(DefaultIssue issue, Map<Component, List<Location>> locationsByComponent, DbIssues.Locations.Builder locationsBuilder) {
List<DbIssues.Location.Builder> locationBuilders = locationsBuilder.getFlowBuilderList().stream()
List<DbIssues.Location.Builder> locationBuilders = primaryLocationBuilder.getFlowBuilderList().stream()
.flatMap(flowBuilder -> flowBuilder.getLocationBuilderList().stream())
.filter(DbIssues.Location.Builder::hasTextRange)
.toList();

locationBuilders.forEach(locationBuilder -> addSecondaryLocation(locationBuilder, issue, locationsByComponent));

return !locationBuilders.isEmpty();
}

private void addSecondaryLocation(DbIssues.Location.Builder locationBuilder, DefaultIssue issue, Map<Component, List<Location>> locationsByComponent) {

+ 81
- 106
server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/ComputeLocationHashesVisitorTest.java View File

@@ -51,17 +51,11 @@ import static org.mockito.Mockito.when;

public class ComputeLocationHashesVisitorTest {
private static final String EXAMPLE_LINE_OF_CODE_FORMAT = "int example = line + of + code + %d; ";
private static final DbCommons.TextRange EXAMPLE_TEXT_RANGE = DbCommons.TextRange.newBuilder()
.setStartLine(1).setStartOffset(0)
.setEndLine(3).setEndOffset(EXAMPLE_LINE_OF_CODE_FORMAT.length() - 1)
.build();
private static final String LINE_IN_THE_MAIN_FILE = "String string = 'line-in-the-main-file';";
private static final String ANOTHER_LINE_IN_THE_MAIN_FILE = "String string = 'another-line-in-the-main-file';";
private static final String LINE_IN_ANOTHER_FILE = "String string = 'line-in-the-another-file';";
private static final String CHECKSUM = "CHECKSUM";

private static final RuleKey TAINTED_RULE_KEY = RuleKey.of("javasecurity", "key");
private static final RuleKey NOT_TAINTED_RULE_KEY = RuleKey.of("java", "key");
private static final RuleKey RULE_KEY = RuleKey.of("javasecurity", "S001");
private static final Component FILE_1 = ReportComponent.builder(Component.Type.FILE, 2).build();
private static final Component FILE_2 = ReportComponent.builder(Component.Type.FILE, 3).build();
private static final Component ROOT = ReportComponent.builder(Component.Type.PROJECT, 1)
@@ -86,112 +80,89 @@ public class ComputeLocationHashesVisitorTest {
}

@Test
public void beforeCaching_whenIssueUnchangedAndHasChecksum_shouldDoNothing() {
DefaultIssue notTaintedIssue = createNotTaintedIssue()
public void do_nothing_if_issue_is_unchanged() {
DefaultIssue issue = createIssue()
.setLocationsChanged(false)
.setNew(false)
.setLocations(DbIssues.Locations.newBuilder()
.setChecksum(CHECKSUM)
.setTextRange(EXAMPLE_TEXT_RANGE)
.build());
.setLocations(DbIssues.Locations.newBuilder().setTextRange(createRange(1, 0, 3, EXAMPLE_LINE_OF_CODE_FORMAT.length() - 1)).build());

executeBeforeCaching(FILE_1, notTaintedIssue);
underTest.beforeComponent(FILE_1);
underTest.onIssue(FILE_1, issue);
underTest.beforeCaching(FILE_1);

DbIssues.Locations locations = notTaintedIssue.getLocations();
assertThat(locations.getChecksum()).isEqualTo(CHECKSUM);
DbIssues.Locations locations = issue.getLocations();
assertThat(locations.getChecksum()).isEmpty();
verifyNoInteractions(sourceLinesRepository);
}

@Test
public void beforeCaching_whenIssueHasNoLocation_shouldDoNothing() {
DefaultIssue notTaintedIssue = createNotTaintedIssue();
executeBeforeCaching(FILE_1, notTaintedIssue);
public void do_nothing_if_issue_is_not_taint_vulnerability() {
DefaultIssue issue = createIssue()
.setRuleKey(RuleKey.of("repo", "rule"))
.setLocations(DbIssues.Locations.newBuilder().setTextRange(createRange(1, 0, 3, EXAMPLE_LINE_OF_CODE_FORMAT.length() - 1)).build());

DbIssues.Locations locations = notTaintedIssue.getLocations();
assertThat(locations).isNull();
verifyNoInteractions(sourceLinesRepository);
}

@Test
public void beforeCaching_whenIssueIsExternal_shouldDoNothing() {
DefaultIssue notTaintedIssue = createNotTaintedIssue()
.setIsFromExternalRuleEngine(true)
.setLocations(DbIssues.Locations.newBuilder()
.setChecksum(CHECKSUM)
.setTextRange(EXAMPLE_TEXT_RANGE)
.build());;
underTest.onIssue(FILE_1, issue);
underTest.beforeCaching(FILE_1);

executeBeforeCaching(FILE_1, notTaintedIssue);

DbIssues.Locations locations = notTaintedIssue.getLocations();
assertThat(locations.getChecksum()).isEqualTo(CHECKSUM);
DbIssues.Locations locations = issue.getLocations();
assertThat(locations.getChecksum()).isEmpty();
verifyNoInteractions(sourceLinesRepository);
}

@Test
public void beforeCaching_whenIssueHasNoChecksum_shouldComputeChecksum() {
DefaultIssue notTaintedIssue = createNotTaintedIssue()
.setLocationsChanged(false)
.setNew(false)
.setLocations(DbIssues.Locations.newBuilder()
.setTextRange(EXAMPLE_TEXT_RANGE)
.build());

executeBeforeCaching(FILE_1, notTaintedIssue);

assertLocationHashIsMadeOf(notTaintedIssue, "intexample=line+of+code+1;intexample=line+of+code+2;intexample=line+of+code+3;");
verify(sourceLinesRepository).readLines(FILE_1);
}

@Test
public void beforeCaching_whenMultipleLinesTaintedIssue_shouldComputeChecksum() {
DefaultIssue taintedIssue = createTaintedIssue()
.setLocations(DbIssues.Locations.newBuilder().setTextRange(EXAMPLE_TEXT_RANGE).build());
public void calculates_hash_for_multiple_lines() {
DefaultIssue issue = createIssue()
.setLocations(DbIssues.Locations.newBuilder().setTextRange(createRange(1, 0, 3, EXAMPLE_LINE_OF_CODE_FORMAT.length() - 1)).build());

executeBeforeCaching(FILE_1, taintedIssue);
underTest.onIssue(FILE_1, issue);
underTest.beforeCaching(FILE_1);

assertLocationHashIsMadeOf(taintedIssue, "intexample=line+of+code+1;intexample=line+of+code+2;intexample=line+of+code+3;");
assertLocationHashIsMadeOf(issue, "intexample=line+of+code+1;intexample=line+of+code+2;intexample=line+of+code+3;");
}

@Test
public void beforeCaching_whenMultipleTaintedIssuesAndMultipleComponents_shouldComputeAllChecksums() {
DefaultIssue taintedIssue1 = createTaintedIssue()
.setLocations(DbIssues.Locations.newBuilder().setTextRange(EXAMPLE_TEXT_RANGE).build());
DefaultIssue taintedIssue2 = createTaintedIssue()
public void calculates_hash_for_multiple_files() {
DefaultIssue issue1 = createIssue()
.setLocations(DbIssues.Locations.newBuilder().setTextRange(createRange(1, 0, 3, EXAMPLE_LINE_OF_CODE_FORMAT.length() - 1)).build());
DefaultIssue issue2 = createIssue()
.setLocations(DbIssues.Locations.newBuilder().setTextRange(createRange(1, 0, 1, LINE_IN_ANOTHER_FILE.length())).build());

executeBeforeCaching(FILE_1, taintedIssue1);
executeBeforeCaching(FILE_2, taintedIssue2);
underTest.onIssue(FILE_1, issue1);
underTest.beforeCaching(FILE_1);

assertLocationHashIsMadeOf(taintedIssue1, "intexample=line+of+code+1;intexample=line+of+code+2;intexample=line+of+code+3;");
assertLocationHashIsMadeOf(taintedIssue2, "Stringstring='line-in-the-another-file';");
underTest.onIssue(FILE_2, issue2);
underTest.beforeCaching(FILE_2);

assertLocationHashIsMadeOf(issue1, "intexample=line+of+code+1;intexample=line+of+code+2;intexample=line+of+code+3;");
assertLocationHashIsMadeOf(issue2, "Stringstring='line-in-the-another-file';");
}

@Test
public void beforeCaching_whenPartialLineTaintedIssue_shouldComputeChecksum() {
DefaultIssue taintedIssue = createTaintedIssue()
public void calculates_hash_for_partial_line() {
DefaultIssue issue = createIssue()
.setLocations(DbIssues.Locations.newBuilder().setTextRange(createRange(1, 13, 1, EXAMPLE_LINE_OF_CODE_FORMAT.length() - 1)).build());

executeBeforeCaching(FILE_1, taintedIssue);
underTest.onIssue(FILE_1, issue);
underTest.beforeCaching(FILE_1);

assertLocationHashIsMadeOf(taintedIssue, "line+of+code+1;");
assertLocationHashIsMadeOf(issue, "line+of+code+1;");
}

@Test
public void beforeCaching_whenPartialMultipleLinesTaintedIssue_shouldComputeChecksum() {
DefaultIssue taintedIssue = createTaintedIssue()
public void calculates_hash_for_partial_multiple_lines() {
DefaultIssue issue = createIssue()
.setLocations(DbIssues.Locations.newBuilder().setTextRange(createRange(1, 13, 3, 11)).build());

executeBeforeCaching(FILE_1, taintedIssue);
underTest.onIssue(FILE_1, issue);
underTest.beforeCaching(FILE_1);

assertLocationHashIsMadeOf(taintedIssue, "line+of+code+1;intexample=line+of+code+2;intexample");
assertLocationHashIsMadeOf(issue, "line+of+code+1;intexample=line+of+code+2;intexample");
}

@Test
public void beforeCaching_whenNoTextRange_shouldNotComputeChecksum() {
public void dont_calculate_hash_if_no_textRange() {
// primary location and one of the secondary locations have no text range
DefaultIssue taintedIssue = createTaintedIssue()
DefaultIssue issue = createIssue()
.setLocations(DbIssues.Locations.newBuilder()
.addFlow(DbIssues.Flow.newBuilder()
.addLocation(DbIssues.Location.newBuilder()
@@ -206,18 +177,19 @@ public class ComputeLocationHashesVisitorTest {

when(sourceLinesRepository.readLines(FILE_1)).thenReturn(newOneLineIterator(LINE_IN_THE_MAIN_FILE));

executeBeforeCaching(FILE_1, taintedIssue);
underTest.onIssue(FILE_1, issue);
underTest.beforeCaching(FILE_1);

verify(sourceLinesRepository).readLines(FILE_1);
verifyNoMoreInteractions(sourceLinesRepository);
DbIssues.Locations locations = taintedIssue.getLocations();
DbIssues.Locations locations = issue.getLocations();
assertThat(locations.getFlow(0).getLocation(0).getChecksum()).isEqualTo(DigestUtils.md5Hex("Stringstring='line-in-the-main-file';"));
assertThat(locations.getFlow(0).getLocation(1).getChecksum()).isEmpty();
}

@Test
public void beforeCaching_whenMultipleLocationsInMultipleFiles_shouldComputeAllChecksums() {
DefaultIssue taintedIssue = createTaintedIssue()
public void calculates_hash_for_multiple_locations() {
DefaultIssue issue = createIssue()
.setLocations(DbIssues.Locations.newBuilder()
.setTextRange(createRange(1, 0, 1, LINE_IN_THE_MAIN_FILE.length()))
.addFlow(DbIssues.Flow.newBuilder()
@@ -235,17 +207,18 @@ public class ComputeLocationHashesVisitorTest {
when(sourceLinesRepository.readLines(FILE_1)).thenReturn(newOneLineIterator(LINE_IN_THE_MAIN_FILE));
when(sourceLinesRepository.readLines(FILE_2)).thenReturn(newOneLineIterator(LINE_IN_ANOTHER_FILE));

executeBeforeCaching(FILE_1, taintedIssue);
underTest.onIssue(FILE_1, issue);
underTest.beforeCaching(FILE_1);

DbIssues.Locations locations = taintedIssue.getLocations();
DbIssues.Locations locations = issue.getLocations();

assertThat(locations.getFlow(0).getLocation(0).getChecksum()).isEqualTo(DigestUtils.md5Hex("Stringstring='line-in-the-main-file';"));
assertThat(locations.getFlow(0).getLocation(1).getChecksum()).isEqualTo(DigestUtils.md5Hex("Stringstring='line-in-the-another-file';"));
}

@Test
public void beforeCaching_whenMultipleLocationsInSameFile_shouldComputeAllChecksums() {
DefaultIssue taintedIssue = createTaintedIssue()
public void calculates_hash_for_multiple_locations_in_same_file() {
DefaultIssue issue = createIssue()
.setComponentUuid(FILE_1.getUuid())
.setLocations(DbIssues.Locations.newBuilder()
.setTextRange(createRange(1, 0, 1, LINE_IN_THE_MAIN_FILE.length()))
@@ -263,17 +236,32 @@ public class ComputeLocationHashesVisitorTest {

when(sourceLinesRepository.readLines(FILE_1)).thenReturn(manyLinesIterator(LINE_IN_THE_MAIN_FILE, ANOTHER_LINE_IN_THE_MAIN_FILE));

executeBeforeCaching(FILE_1, taintedIssue);
underTest.onIssue(FILE_1, issue);
underTest.beforeCaching(FILE_1);

DbIssues.Locations locations = taintedIssue.getLocations();
DbIssues.Locations locations = issue.getLocations();

assertThat(locations.getFlow(0).getLocation(0).getChecksum()).isEqualTo(DigestUtils.md5Hex("Stringstring='line-in-the-main-file';"));
assertThat(locations.getFlow(0).getLocation(1).getChecksum()).isEqualTo(DigestUtils.md5Hex("Stringstring='another-line-in-the-main-file';"));
}

@Test
public void beforeCaching_whenNotTaintedIssue_shouldNotComputeChecksumForSecondaryLocations() {
DefaultIssue notTaintedIssue = createNotTaintedIssue()
public void beforeCaching_whenSecurityHotspots_shouldCalculateHashForPrimaryLocation() {
DefaultIssue issue = createIssue()
.setRuleKey(RuleKey.of("repo", "rule"))
.setType(RuleType.SECURITY_HOTSPOT)
.setLocations(DbIssues.Locations.newBuilder().setTextRange(createRange(1, 0, 3, EXAMPLE_LINE_OF_CODE_FORMAT.length() - 1)).build());
underTest.onIssue(FILE_1, issue);

underTest.beforeCaching(FILE_1);

assertLocationHashIsMadeOf(issue, "intexample=line+of+code+1;intexample=line+of+code+2;intexample=line+of+code+3;");
}

@Test
public void beforeCaching_whenSecurityHotspots_shouldNotCalculateHashForSecondaryLocations() {
DefaultIssue issue = createIssue()
.setType(RuleType.SECURITY_HOTSPOT)
.setLocations(DbIssues.Locations.newBuilder()
.setTextRange(createRange(1, 0, 1, LINE_IN_THE_MAIN_FILE.length()))
.addFlow(DbIssues.Flow.newBuilder()
@@ -289,21 +277,16 @@ public class ComputeLocationHashesVisitorTest {
.build());
when(sourceLinesRepository.readLines(FILE_1)).thenReturn(newOneLineIterator(LINE_IN_THE_MAIN_FILE));
when(sourceLinesRepository.readLines(FILE_2)).thenReturn(newOneLineIterator(LINE_IN_ANOTHER_FILE));
underTest.onIssue(FILE_1, issue);

executeBeforeCaching(FILE_1, notTaintedIssue);
underTest.beforeCaching(FILE_1);

DbIssues.Locations locations = notTaintedIssue.getLocations();
assertLocationHashIsMadeOf(notTaintedIssue, "Stringstring='line-in-the-main-file';");
DbIssues.Locations locations = issue.getLocations();
assertLocationHashIsMadeOf(issue, "Stringstring='line-in-the-main-file';");
assertThat(locations.getFlow(0).getLocation(0).getChecksum()).isEmpty();
assertThat(locations.getFlow(0).getLocation(1).getChecksum()).isEmpty();
}

private void executeBeforeCaching(Component component, DefaultIssue issue) {
underTest.beforeComponent(component);
underTest.onIssue(component, issue);
underTest.beforeCaching(component);
}

private DbCommons.TextRange createRange(int startLine, int startOffset, int endLine, int endOffset) {
return DbCommons.TextRange.newBuilder()
.setStartLine(startLine).setStartOffset(startOffset)
@@ -311,18 +294,10 @@ public class ComputeLocationHashesVisitorTest {
.build();
}

private DefaultIssue createTaintedIssue() {
return createIssue(TAINTED_RULE_KEY);
}

private DefaultIssue createNotTaintedIssue() {
return createIssue(NOT_TAINTED_RULE_KEY);
}

private DefaultIssue createIssue(RuleKey ruleKey) {
private DefaultIssue createIssue() {
return new DefaultIssue()
.setLocationsChanged(true)
.setRuleKey(ruleKey)
.setRuleKey(RULE_KEY)
.setIsFromExternalRuleEngine(false)
.setType(RuleType.CODE_SMELL);
}

Loading…
Cancel
Save