From: Antoine Vigneau Date: Tue, 6 Jun 2023 13:29:02 +0000 (+0200) Subject: SONAR-18877 Compute primary location hash for all issues X-Git-Tag: 10.4.0.87286~549 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=ce4b7fee29e8f923c014bd2a2e6e3b841cdf8d3d;p=sonarqube.git SONAR-18877 Compute primary location hash for all issues --- diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/ComputeLocationHashesVisitor.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/ComputeLocationHashesVisitor.java index 6697699a44d..4f5ecfb7882 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/ComputeLocationHashesVisitor.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/ComputeLocationHashesVisitor.java @@ -25,6 +25,8 @@ import java.util.List; import java.util.Map; import java.util.regex.Pattern; import org.apache.commons.codec.digest.DigestUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.sonar.ce.task.projectanalysis.component.Component; import org.sonar.ce.task.projectanalysis.component.TreeRootHolder; import org.sonar.ce.task.projectanalysis.source.SourceLinesRepository; @@ -35,16 +37,19 @@ 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 the hashes for all locations. - * It only applies to issues that are taint vulnerabilities or security hotspots, and that are new or were changed. + * 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) * 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 Logger LOGGER = LoggerFactory.getLogger(ComputeLocationHashesVisitor.class); + private static final Pattern MATCH_ALL_WHITESPACES = Pattern.compile("\\s"); - private final List issues = new LinkedList<>(); + private final List issuesForAllLocations = new LinkedList<>(); + private final List issuesForPrimaryLocation = new LinkedList<>(); private final SourceLinesRepository sourceLinesRepository; private final TreeRootHolder treeRootHolder; private final TaintChecker taintChecker; @@ -57,20 +62,43 @@ public class ComputeLocationHashesVisitor extends IssueVisitor { @Override public void beforeComponent(Component component) { - issues.clear(); + issuesForAllLocations.clear(); + issuesForPrimaryLocation.clear(); } @Override public void onIssue(Component component, DefaultIssue issue) { - if (shouldComputeLocation(issue)) { - issues.add(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); + } } } - private boolean shouldComputeLocation(DefaultIssue issue) { - return (taintChecker.isTaintVulnerability(issue) || SECURITY_HOTSPOT.equals(issue.type())) - && !issue.isFromExternalRuleEngine() - && (issue.isNew() || issue.locationsChanged()); + private static boolean issueNeedsLocationHashes(DefaultIssue issue) { + DbIssues.Locations locations = issue.getLocations(); + return !issue.isFromExternalRuleEngine() + && !issue.isBeingClosed() + && locations != null; + } + + 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(); } @Override @@ -78,20 +106,10 @@ public class ComputeLocationHashesVisitor extends IssueVisitor { Map> locationsByComponent = new HashMap<>(); List locationsToSet = new LinkedList<>(); - 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)); - } - } + // Issues that needs both primary and secondary locations hashes + extractForAllLocations(component, locationsByComponent, locationsToSet); + // Then issues that needs only primary locations + extractForPrimaryLocation(component, locationsByComponent, locationsToSet); // Feed lines to locations, component by component locationsByComponent.forEach(this::updateLocationsInComponent); @@ -102,37 +120,41 @@ public class ComputeLocationHashesVisitor extends IssueVisitor { // set new locations to issues locationsToSet.forEach(LocationToSet::set); - issues.clear(); + issuesForAllLocations.clear(); + issuesForPrimaryLocation.clear(); } - private boolean addLocations(Component component, DefaultIssue issue, Map> locationsByComponent, DbIssues.Locations.Builder primaryLocationBuilder) { - boolean hasPrimaryLocation = addPrimaryLocation(component, locationsByComponent, primaryLocationBuilder); - boolean hasSecondaryLocations = addSecondaryLocations(issue, locationsByComponent, primaryLocationBuilder); - return hasPrimaryLocation || hasSecondaryLocations; + private void extractForAllLocations(Component component, Map> locationsByComponent, List 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 static boolean addPrimaryLocation(Component component, Map> locationsByComponent, DbIssues.Locations.Builder primaryLocationBuilder) { - if (!primaryLocationBuilder.hasTextRange()) { - return false; + private void extractForPrimaryLocation(Component component, Map> locationsByComponent, List locationsToSet) { + for (DefaultIssue issue : issuesForPrimaryLocation) { + DbIssues.Locations.Builder locationsBuilder = ((DbIssues.Locations) issue.getLocations()).toBuilder(); + addPrimaryLocation(component, locationsByComponent, locationsBuilder); + locationsToSet.add(new LocationToSet(issue, locationsBuilder)); } - PrimaryLocation primaryLocation = new PrimaryLocation(primaryLocationBuilder); - locationsByComponent.computeIfAbsent(component, c -> new LinkedList<>()).add(primaryLocation); - return true; } - private boolean addSecondaryLocations(DefaultIssue issue, Map> locationsByComponent, DbIssues.Locations.Builder primaryLocationBuilder) { - if (SECURITY_HOTSPOT.equals(issue.type())) { - return false; + private static void addPrimaryLocation(Component component, Map> locationsByComponent, DbIssues.Locations.Builder locationsBuilder) { + if (locationsBuilder.hasTextRange()) { + PrimaryLocation primaryLocation = new PrimaryLocation(locationsBuilder); + locationsByComponent.computeIfAbsent(component, c -> new LinkedList<>()).add(primaryLocation); } + } - List locationBuilders = primaryLocationBuilder.getFlowBuilderList().stream() + private void addSecondaryLocations(DefaultIssue issue, Map> locationsByComponent, DbIssues.Locations.Builder locationsBuilder) { + List locationBuilders = locationsBuilder.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> locationsByComponent) { @@ -216,15 +238,19 @@ public class ComputeLocationHashesVisitor extends IssueVisitor { if (lineNumber > textRange.getEndLine() || lineNumber < textRange.getStartLine()) { return; } - - if (lineNumber == textRange.getStartLine() && lineNumber == textRange.getEndLine()) { - hashBuilder.append(line, textRange.getStartOffset(), textRange.getEndOffset()); - } else if (lineNumber == textRange.getStartLine()) { - hashBuilder.append(line, textRange.getStartOffset(), line.length()); - } else if (lineNumber < textRange.getEndLine()) { - hashBuilder.append(line); - } else { - hashBuilder.append(line, 0, textRange.getEndOffset()); + try { + if (lineNumber == textRange.getStartLine() && lineNumber == textRange.getEndLine()) { + hashBuilder.append(line, textRange.getStartOffset(), textRange.getEndOffset()); + } else if (lineNumber == textRange.getStartLine()) { + hashBuilder.append(line, textRange.getStartOffset(), line.length()); + } else if (lineNumber < textRange.getEndLine()) { + hashBuilder.append(line); + } else { + hashBuilder.append(line, 0, textRange.getEndOffset()); + } + } catch (IndexOutOfBoundsException e) { + LOGGER.debug("Try to compute issue location hash from {} to {} on line ({} chars): {}", + textRange.getStartOffset(), textRange.getEndOffset(), line.length(), line); } } diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/ComputeLocationHashesVisitorTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/ComputeLocationHashesVisitorTest.java index e764a9d9b4e..9f54f823aca 100644 --- a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/ComputeLocationHashesVisitorTest.java +++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/ComputeLocationHashesVisitorTest.java @@ -29,9 +29,11 @@ import org.apache.commons.codec.digest.DigestUtils; import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.slf4j.event.Level; import org.sonar.api.config.Configuration; import org.sonar.api.rule.RuleKey; import org.sonar.api.rules.RuleType; +import org.sonar.api.testfixtures.log.LogTester; import org.sonar.ce.task.projectanalysis.component.Component; import org.sonar.ce.task.projectanalysis.component.ReportComponent; import org.sonar.ce.task.projectanalysis.component.TreeRootHolderRule; @@ -51,11 +53,17 @@ 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 RULE_KEY = RuleKey.of("javasecurity", "S001"); + 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 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) @@ -67,6 +75,8 @@ public class ComputeLocationHashesVisitorTest { private final TaintChecker taintChecker = new TaintChecker(configuration); @Rule public TreeRootHolderRule treeRootHolder = new TreeRootHolderRule(); + @Rule + public LogTester logTester = new LogTester(); private final ComputeLocationHashesVisitor underTest = new ComputeLocationHashesVisitor(taintChecker, sourceLinesRepository, treeRootHolder); @Before @@ -77,92 +87,148 @@ public class ComputeLocationHashesVisitorTest { when(sourceLinesRepository.readLines(FILE_1)).thenReturn(CloseableIterator.from(stringIterator)); when(sourceLinesRepository.readLines(FILE_2)).thenReturn(newOneLineIterator(LINE_IN_ANOTHER_FILE)); treeRootHolder.setRoot(ROOT); + logTester.setLevel(Level.DEBUG); } @Test - public void do_nothing_if_issue_is_unchanged() { - DefaultIssue issue = createIssue() + public void beforeCaching_whenIssueUnchangedAndHasChecksum_shouldDoNothing() { + DefaultIssue notTaintedIssue = createNotTaintedIssue() .setLocationsChanged(false) .setNew(false) - .setLocations(DbIssues.Locations.newBuilder().setTextRange(createRange(1, 0, 3, EXAMPLE_LINE_OF_CODE_FORMAT.length() - 1)).build()); + .setLocations(DbIssues.Locations.newBuilder() + .setChecksum(CHECKSUM) + .setTextRange(EXAMPLE_TEXT_RANGE) + .build()); - underTest.beforeComponent(FILE_1); - underTest.onIssue(FILE_1, issue); - underTest.beforeCaching(FILE_1); + executeBeforeCaching(FILE_1, notTaintedIssue); - DbIssues.Locations locations = issue.getLocations(); - assertThat(locations.getChecksum()).isEmpty(); + DbIssues.Locations locations = notTaintedIssue.getLocations(); + assertThat(locations.getChecksum()).isEqualTo(CHECKSUM); verifyNoInteractions(sourceLinesRepository); } @Test - 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()); + public void beforeCaching_whenIssueHasNoLocation_shouldDoNothing() { + DefaultIssue notTaintedIssue = createNotTaintedIssue(); - underTest.onIssue(FILE_1, issue); - underTest.beforeCaching(FILE_1); + executeBeforeCaching(FILE_1, notTaintedIssue); - DbIssues.Locations locations = issue.getLocations(); - assertThat(locations.getChecksum()).isEmpty(); + DbIssues.Locations locations = notTaintedIssue.getLocations(); + assertThat(locations).isNull(); verifyNoInteractions(sourceLinesRepository); } @Test - 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()); + 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); - assertLocationHashIsMadeOf(issue, "intexample=line+of+code+1;intexample=line+of+code+2;intexample=line+of+code+3;"); + DbIssues.Locations locations = notTaintedIssue.getLocations(); + assertThat(locations.getChecksum()).isEqualTo(CHECKSUM); + verifyNoInteractions(sourceLinesRepository); } @Test - 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()); + public void beforeCaching_whenIssueNoLongerExists_shouldDoNothing() { + DefaultIssue notTaintedIssue = createNotTaintedIssue() + .setBeingClosed(true) + .setLocations(DbIssues.Locations.newBuilder() + .setChecksum(CHECKSUM) + .setTextRange(EXAMPLE_TEXT_RANGE) + .build());; + + executeBeforeCaching(FILE_1, notTaintedIssue); + + DbIssues.Locations locations = notTaintedIssue.getLocations(); + assertThat(locations.getChecksum()).isEqualTo(CHECKSUM); + verifyNoInteractions(sourceLinesRepository); + } + + @Test + public void beforeCaching_whenIssueLocationIsOutOfBound_shouldLog() { + DefaultIssue notTaintedIssue = createNotTaintedIssue() + .setChecksum(CHECKSUM) + .setLocations(DbIssues.Locations.newBuilder() + .setTextRange(DbCommons.TextRange.newBuilder() + .setStartLine(1).setStartOffset(0) + .setEndLine(1).setEndOffset(EXAMPLE_LINE_OF_CODE_FORMAT.length() + 1) + .build()) + .build()); + + executeBeforeCaching(FILE_1, notTaintedIssue); + + assertThat(logTester.logs(Level.DEBUG)).contains("Try to compute issue location hash from 0 to 38 on line (36 chars): " + String.format(EXAMPLE_LINE_OF_CODE_FORMAT, 1)); + } + + @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()); - underTest.onIssue(FILE_1, issue1); - underTest.beforeCaching(FILE_1); + executeBeforeCaching(FILE_1, taintedIssue); - underTest.onIssue(FILE_2, issue2); - underTest.beforeCaching(FILE_2); + assertLocationHashIsMadeOf(taintedIssue, "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() + .setLocations(DbIssues.Locations.newBuilder().setTextRange(createRange(1, 0, 1, LINE_IN_ANOTHER_FILE.length())).build()); - 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';"); + executeBeforeCaching(FILE_1, taintedIssue1); + executeBeforeCaching(FILE_2, taintedIssue2); + + 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';"); } @Test - public void calculates_hash_for_partial_line() { - DefaultIssue issue = createIssue() + public void beforeCaching_whenPartialLineTaintedIssue_shouldComputeChecksum() { + DefaultIssue taintedIssue = createTaintedIssue() .setLocations(DbIssues.Locations.newBuilder().setTextRange(createRange(1, 13, 1, EXAMPLE_LINE_OF_CODE_FORMAT.length() - 1)).build()); - underTest.onIssue(FILE_1, issue); - underTest.beforeCaching(FILE_1); + executeBeforeCaching(FILE_1, taintedIssue); - assertLocationHashIsMadeOf(issue, "line+of+code+1;"); + assertLocationHashIsMadeOf(taintedIssue, "line+of+code+1;"); } @Test - public void calculates_hash_for_partial_multiple_lines() { - DefaultIssue issue = createIssue() + public void beforeCaching_whenPartialMultipleLinesTaintedIssue_shouldComputeChecksum() { + DefaultIssue taintedIssue = createTaintedIssue() .setLocations(DbIssues.Locations.newBuilder().setTextRange(createRange(1, 13, 3, 11)).build()); - underTest.onIssue(FILE_1, issue); - underTest.beforeCaching(FILE_1); + executeBeforeCaching(FILE_1, taintedIssue); - assertLocationHashIsMadeOf(issue, "line+of+code+1;intexample=line+of+code+2;intexample"); + assertLocationHashIsMadeOf(taintedIssue, "line+of+code+1;intexample=line+of+code+2;intexample"); } @Test - public void dont_calculate_hash_if_no_textRange() { + public void beforeCaching_whenNoTextRange_shouldNotComputeChecksum() { // primary location and one of the secondary locations have no text range - DefaultIssue issue = createIssue() + DefaultIssue taintedIssue = createTaintedIssue() .setLocations(DbIssues.Locations.newBuilder() .addFlow(DbIssues.Flow.newBuilder() .addLocation(DbIssues.Location.newBuilder() @@ -177,19 +243,18 @@ public class ComputeLocationHashesVisitorTest { when(sourceLinesRepository.readLines(FILE_1)).thenReturn(newOneLineIterator(LINE_IN_THE_MAIN_FILE)); - underTest.onIssue(FILE_1, issue); - underTest.beforeCaching(FILE_1); + executeBeforeCaching(FILE_1, taintedIssue); verify(sourceLinesRepository).readLines(FILE_1); verifyNoMoreInteractions(sourceLinesRepository); - DbIssues.Locations locations = issue.getLocations(); + DbIssues.Locations locations = taintedIssue.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 calculates_hash_for_multiple_locations() { - DefaultIssue issue = createIssue() + public void beforeCaching_whenMultipleLocationsInMultipleFiles_shouldComputeAllChecksums() { + DefaultIssue taintedIssue = createTaintedIssue() .setLocations(DbIssues.Locations.newBuilder() .setTextRange(createRange(1, 0, 1, LINE_IN_THE_MAIN_FILE.length())) .addFlow(DbIssues.Flow.newBuilder() @@ -207,18 +272,17 @@ 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)); - underTest.onIssue(FILE_1, issue); - underTest.beforeCaching(FILE_1); + executeBeforeCaching(FILE_1, taintedIssue); - DbIssues.Locations locations = issue.getLocations(); + DbIssues.Locations locations = taintedIssue.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 calculates_hash_for_multiple_locations_in_same_file() { - DefaultIssue issue = createIssue() + public void beforeCaching_whenMultipleLocationsInSameFile_shouldComputeAllChecksums() { + DefaultIssue taintedIssue = createTaintedIssue() .setComponentUuid(FILE_1.getUuid()) .setLocations(DbIssues.Locations.newBuilder() .setTextRange(createRange(1, 0, 1, LINE_IN_THE_MAIN_FILE.length())) @@ -236,32 +300,17 @@ public class ComputeLocationHashesVisitorTest { when(sourceLinesRepository.readLines(FILE_1)).thenReturn(manyLinesIterator(LINE_IN_THE_MAIN_FILE, ANOTHER_LINE_IN_THE_MAIN_FILE)); - underTest.onIssue(FILE_1, issue); - underTest.beforeCaching(FILE_1); + executeBeforeCaching(FILE_1, taintedIssue); - DbIssues.Locations locations = issue.getLocations(); + DbIssues.Locations locations = taintedIssue.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_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) + public void beforeCaching_whenNotTaintedIssue_shouldNotComputeChecksumForSecondaryLocations() { + DefaultIssue notTaintedIssue = createNotTaintedIssue() .setLocations(DbIssues.Locations.newBuilder() .setTextRange(createRange(1, 0, 1, LINE_IN_THE_MAIN_FILE.length())) .addFlow(DbIssues.Flow.newBuilder() @@ -277,16 +326,21 @@ 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); - underTest.beforeCaching(FILE_1); + executeBeforeCaching(FILE_1, notTaintedIssue); - DbIssues.Locations locations = issue.getLocations(); - assertLocationHashIsMadeOf(issue, "Stringstring='line-in-the-main-file';"); + DbIssues.Locations locations = notTaintedIssue.getLocations(); + assertLocationHashIsMadeOf(notTaintedIssue, "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) @@ -294,10 +348,18 @@ public class ComputeLocationHashesVisitorTest { .build(); } - private DefaultIssue createIssue() { + private DefaultIssue createTaintedIssue() { + return createIssue(TAINTED_RULE_KEY); + } + + private DefaultIssue createNotTaintedIssue() { + return createIssue(NOT_TAINTED_RULE_KEY); + } + + private DefaultIssue createIssue(RuleKey ruleKey) { return new DefaultIssue() .setLocationsChanged(true) - .setRuleKey(RULE_KEY) + .setRuleKey(ruleKey) .setIsFromExternalRuleEngine(false) .setType(RuleType.CODE_SMELL); }