Skip to content

Commit

Permalink
SONARPY-1533: Ruff report is not using the correct column and row loc…
Browse files Browse the repository at this point in the history 
…ation (#1621)
  • Loading branch information
@joke1196
joke1196 authored Oct 27, 2023
1 parent bbb023e commit b9f1e08
Show file tree
Hide file tree
Showing 5 changed files with 109 additions and 48 deletions.
12 changes: 6 additions & 6 deletions sonar-python-plugin/src/main/java/org/sonar/plugins/python/ruff/RuffJsonReportReader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,22 +81,22 @@ private void onResult(JSONObject result) {
issue.startLocationCol = toInteger(location.get("column"));
issue.startLocationRow = toInteger(location.get("row"));
JSONObject endLocation = (JSONObject) result.get("end_location");
issue.endLocationCol = correctEndLocationCol(endLocation.get("column"), issue.startLocationCol);
issue.endLocationRow = toInteger(endLocation.get("row"));
issue.endLocationCol = correctEndLocationCol(endLocation.get("column"), issue.startLocationCol, issue.startLocationRow, issue.endLocationRow);
consumer.accept(issue);
}

/*
Ruff returns the col number of the last char + 1.
In order to properly read the col number we need to return the number of the last char.
In order to properly read the col number we need to return the col number of the last char.
*/
private static Integer correctEndLocationCol(Object value, int startLocationCol) {
private static Integer correctEndLocationCol(Object value, int startLocationCol, int startLocationRow, int endLocationRow) {
Integer endLocationCol = toInteger(value);
if (endLocationCol != null) {
if (endLocationCol > startLocationCol) {
return endLocationCol - 1;
} else {
if (endLocationRow == startLocationRow && endLocationCol <= startLocationCol) {
return startLocationCol;
} else {
return endLocationCol - 1;
}
}
return null;
Expand Down
28 changes: 15 additions & 13 deletions sonar-python-plugin/src/main/java/org/sonar/plugins/python/ruff/RuffSensor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,17 +70,17 @@ protected Logger logger() {

@Override
protected void importReport(File reportPath, SensorContext context, Set<String> unresolvedInputFiles)
throws IOException, ParseException {
throws IOException, ParseException {
InputStream in = new FileInputStream(reportPath);
LOG.info("Importing {}", reportPath);
RuffJsonReportReader.read(in, issue -> saveIssue(context, issue, unresolvedInputFiles));
}

private static void saveIssue(SensorContext context, RuffJsonReportReader.Issue issue,
Set<String> unresolvedInputFiles) {
Set<String> unresolvedInputFiles) {
if (isEmpty(issue.ruleKey) || isEmpty(issue.filePath) || isEmpty(issue.message)) {
LOG.debug("Missing information for ruleKey:'{}', filePath:'{}', message:'{}'", issue.ruleKey, issue.filePath,
issue.message);
issue.message);
return;
}

Expand All @@ -92,18 +92,18 @@ private static void saveIssue(SensorContext context, RuffJsonReportReader.Issue

NewExternalIssue newExternalIssue = context.newExternalIssue();
newExternalIssue
.type(RuleType.CODE_SMELL)
.severity(Severity.MAJOR)
.remediationEffortMinutes(DEFAULT_CONSTANT_DEBT_MINUTES);
.type(RuleType.CODE_SMELL)
.severity(Severity.MAJOR)
.remediationEffortMinutes(DEFAULT_CONSTANT_DEBT_MINUTES);

NewIssueLocation primaryLocation = newExternalIssue.newLocation()
.message(issue.message)
.on(inputFile);
.message(issue.message)
.on(inputFile);

if (issue.startLocationRow != null) {
if (isValidEndLocation(issue, inputFile)) {
primaryLocation.at(inputFile.newRange(issue.startLocationRow, issue.startLocationCol, issue.endLocationRow,
issue.endLocationCol));
issue.endLocationCol));
} else {
primaryLocation.at(inputFile.selectLine(issue.startLocationRow));
}
Expand All @@ -119,10 +119,12 @@ private static void saveIssue(SensorContext context, RuffJsonReportReader.Issue
*/
private static boolean isValidEndLocation(RuffJsonReportReader.Issue issue, InputFile inputFile) {
return issue.startLocationCol != null &&
issue.endLocationRow != null &&
issue.endLocationCol != null &&
isColInBounds(issue.startLocationRow, issue.startLocationCol, inputFile) &&
(!issue.endLocationRow.equals(issue.startLocationRow) || !issue.endLocationCol.equals(issue.startLocationCol));
issue.endLocationRow != null &&
issue.endLocationCol != null &&
isColInBounds(issue.startLocationRow, issue.startLocationCol, inputFile) &&
((issue.endLocationRow.equals(issue.startLocationRow) && issue.endLocationCol > issue.startLocationCol) ||
!issue.endLocationRow.equals(issue.startLocationRow));

}

private static boolean isColInBounds(int lineNumber, int columnNumber, InputFile inputFile) {
Expand Down
95 changes: 66 additions & 29 deletions sonar-python-plugin/src/test/java/org/sonar/plugins/python/ruff/RuffSensorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,19 @@
*/
package org.sonar.plugins.python.ruff;

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.assertj.core.api.Assertions.assertThat;

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.List;
import java.util.stream.Stream;

import javax.annotation.Nullable;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
import org.slf4j.event.Level;
Expand All @@ -46,9 +51,6 @@
import org.sonar.api.utils.Version;
import org.sonar.api.utils.log.LoggerLevel;

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.assertj.core.api.Assertions.assertThat;

class RuffSensorTest {

private static final String RUFF_FILE = "python-project:ruff/file1.py";
Expand All @@ -57,7 +59,7 @@ class RuffSensorTest {
private static final String RUFF_REPORT_UNKNOWN_FILES = "unknown-file-path.json";

private static final Path PROJECT_DIR = Paths.get("src", "test", "resources", "org", "sonar", "plugins", "python",
"ruff");
"ruff");

private static RuffSensor ruffSensor = new RuffSensor();

Expand Down Expand Up @@ -87,7 +89,7 @@ void test_descriptor() {
@Test
void issues_with_json_format() throws IOException {
List<ExternalIssue> externalIssues = executeSensorImporting(7, 9, RUFF_JSON_REPORT);
assertThat(externalIssues).hasSize(9);
assertThat(externalIssues).hasSize(10);

ExternalIssue first = externalIssues.get(0);
assertThat(first.ruleKey()).hasToString("external_ruff:S107");
Expand All @@ -96,7 +98,7 @@ void issues_with_json_format() throws IOException {
IssueLocation firstPrimaryLoc = first.primaryLocation();
assertThat(firstPrimaryLoc.inputComponent().key()).isEqualTo(RUFF_FILE);
assertThat(firstPrimaryLoc.message())
.isEqualTo("Possible hardcoded password assigned to function default: \"secret\"");
.isEqualTo("Possible hardcoded password assigned to function default: \"secret\"");
TextRange firstTextRange = firstPrimaryLoc.textRange();
assertThat(firstTextRange).isNotNull();
assertThat(firstTextRange.start().line()).isEqualTo(5);
Expand All @@ -118,6 +120,16 @@ void issues_with_json_format() throws IOException {
assertThat(secondTextRange.end().line()).isEqualTo(6);
assertThat(secondTextRange.end().lineOffset()).isEqualTo(42);

assertNoErrorWarnLogs(logTester);
assertThat(logTester.logs(LoggerLevel.DEBUG)).isEmpty();

}

@Test
void issues_primary_location_check() throws IOException {
List<ExternalIssue> externalIssues = executeSensorImporting(7, 9, RUFF_JSON_REPORT);
assertThat(externalIssues).hasSize(10);

ExternalIssue fourth = externalIssues.get(3);
assertThat(fourth.ruleKey()).hasToString("external_ruff:F821");
assertThat(fourth.type()).isEqualTo(RuleType.CODE_SMELL);
Expand All @@ -126,13 +138,38 @@ void issues_with_json_format() throws IOException {
assertThat(fourthPrimaryLoc.inputComponent().key()).isEqualTo(RUFF_FILE);
assertThat(fourthPrimaryLoc.message()).isEqualTo("Undefined name `random`");

ExternalIssue last = externalIssues.get(8);
assertThat(last.ruleKey()).hasToString("external_ruff:S110");
ExternalIssue secondToLast = externalIssues.get(8);
assertThat(secondToLast.ruleKey()).hasToString("external_ruff:S110");
assertThat(secondToLast.type()).isEqualTo(RuleType.CODE_SMELL);
assertThat(secondToLast.severity()).isEqualTo(Severity.MAJOR);
IssueLocation secondToLastPrimaryLoc = secondToLast.primaryLocation();
assertThat(secondToLastPrimaryLoc.inputComponent().key()).isEqualTo(RUFF_FILE);
assertThat(secondToLastPrimaryLoc.message()).isEqualTo("`try`-`except`-`pass` detected, consider logging the exception");

assertNoErrorWarnLogs(logTester);
assertThat(logTester.logs(LoggerLevel.DEBUG)).isEmpty();

}

@Test
void issues_multiline_check() throws IOException {
List<ExternalIssue> externalIssues = executeSensorImporting(7, 9, RUFF_JSON_REPORT);
assertThat(externalIssues).hasSize(10);

ExternalIssue last = externalIssues.get(9);
assertThat(last.ruleKey()).hasToString("external_ruff:C417");
assertThat(last.type()).isEqualTo(RuleType.CODE_SMELL);
assertThat(last.severity()).isEqualTo(Severity.MAJOR);
IssueLocation lastPrimaryLoc = last.primaryLocation();
assertThat(lastPrimaryLoc.inputComponent().key()).isEqualTo(RUFF_FILE);
assertThat(lastPrimaryLoc.message()).isEqualTo("`try`-`except`-`pass` detected, consider logging the exception");
assertThat(lastPrimaryLoc.message()).isEqualTo("Unnecessary `map` usage (rewrite using a `list` comprehension)");

TextRange lastTextRange = lastPrimaryLoc.textRange();
assertThat(lastTextRange).isNotNull();
assertThat(lastTextRange.start().line()).isEqualTo(25);
assertThat(lastTextRange.start().lineOffset()).isEqualTo(33);
assertThat(lastTextRange.end().line()).isEqualTo(27);
assertThat(lastTextRange.end().lineOffset()).isEqualTo(5);

assertNoErrorWarnLogs(logTester);
assertThat(logTester.logs(LoggerLevel.DEBUG)).isEmpty();
Expand All @@ -151,7 +188,7 @@ void report_on_first_line() throws IOException {
IssueLocation firstPrimaryLoc = first.primaryLocation();
assertThat(firstPrimaryLoc.inputComponent().key()).isEqualTo("python-project:ruff/file1.py");
assertThat(firstPrimaryLoc.message())
.isEqualTo("Missing docstring in public module");
.isEqualTo("Missing docstring in public module");
TextRange firstTextRange = firstPrimaryLoc.textRange();
assertThat(firstTextRange).isNotNull();
assertThat(firstTextRange.start().line()).isEqualTo(1);
Expand All @@ -172,7 +209,7 @@ void report_on_empty_file() throws IOException {
IssueLocation firstPrimaryLoc = first.primaryLocation();
assertThat(firstPrimaryLoc.inputComponent().key()).isEqualTo("python-project:ruff/__init__.py");
assertThat(firstPrimaryLoc.message())
.isEqualTo("Missing docstring in public package");
.isEqualTo("Missing docstring in public package");
TextRange firstTextRange = firstPrimaryLoc.textRange();
assertThat(firstTextRange).isNotNull();
assertThat(firstTextRange.start().line()).isEqualTo(1);
Expand All @@ -187,8 +224,8 @@ void unknown_json_file_path() throws IOException {
assertThat(externalIssues).hasSize(1);

assertThat(onlyOneLogElement(logTester.logs(LoggerLevel.WARN)))
.isEqualTo(
"Failed to resolve 1 file path(s) in Ruff report. No issues imported related to file(s): unknown/file.py");
.isEqualTo(
"Failed to resolve 1 file path(s) in Ruff report. No issues imported related to file(s): unknown/file.py");
}

@Test
Expand All @@ -205,11 +242,11 @@ void missing_rule_key_file_name_or_message() throws IOException {

assertThat(logTester.logs(LoggerLevel.DEBUG)).hasSize(3);
assertThat(logTester.logs(LoggerLevel.DEBUG).get(0))
.startsWith("Missing information for ruleKey:'null',");
.startsWith("Missing information for ruleKey:'null',");
assertThat(logTester.logs(LoggerLevel.DEBUG).get(1))
.contains("filePath:'null'");
.contains("filePath:'null'");
assertThat(logTester.logs(LoggerLevel.DEBUG).get(2))
.contains("message:'null'");
.contains("message:'null'");
}

@Test
Expand All @@ -218,8 +255,8 @@ void no_issues_with_invalid_report_path() throws IOException {
assertThat(externalIssues).isEmpty();

assertThat(onlyOneLogElement(logTester.logs(LoggerLevel.ERROR)))
.startsWith("No issues information will be saved as the report file '")
.contains("invalid-path.json' can't be read.");
.startsWith("No issues information will be saved as the report file '")
.contains("invalid-path.json' can't be read.");
}

@Test
Expand All @@ -231,8 +268,8 @@ void no_issues_with_empty_or_invalid_ruff_file() throws IOException {
externalIssues = executeSensorImporting(7, 9, "ruff-invalid-file.json");
assertThat(externalIssues).isEmpty();
assertThat(onlyOneLogElement(logTester.logs(LoggerLevel.ERROR)))
.startsWith("No issues information will be saved as the report file '")
.contains("ruff-invalid-file.json' can't be read.");
.startsWith("No issues information will be saved as the report file '")
.contains("ruff-invalid-file.json' can't be read.");
}

@Test
Expand All @@ -244,7 +281,7 @@ void unknown_rule() throws IOException {
assertThat(first.ruleKey()).hasToString("external_ruff:ZZZ999");
assertThat(first.type()).isEqualTo(RuleType.CODE_SMELL);
assertThat(first.severity()).isEqualTo(Severity.MAJOR);

assertNoErrorWarnLogs(logTester);

}
Expand All @@ -262,7 +299,7 @@ void incorrect_end_location() throws IOException {
IssueLocation firstPrimaryLoc = first.primaryLocation();
assertThat(firstPrimaryLoc.inputComponent().key()).isEqualTo(RUFF_FILE);
assertThat(firstPrimaryLoc.message())
.isEqualTo("Possible hardcoded password assigned to function default: \"secret\"");
.isEqualTo("Possible hardcoded password assigned to function default: \"secret\"");
TextRange firstTextRange = firstPrimaryLoc.textRange();
assertThat(firstTextRange).isNotNull();
assertThat(firstTextRange.start().line()).isEqualTo(5);
Expand All @@ -273,13 +310,13 @@ void incorrect_end_location() throws IOException {
}

private static List<ExternalIssue> executeSensorImporting(int majorVersion, int minorVersion,
@Nullable String fileName) throws IOException {
@Nullable String fileName) throws IOException {
Path baseDir = PROJECT_DIR.getParent();
SensorContextTester context = SensorContextTester.create(baseDir);
try (Stream<Path> fileStream = Files.list(PROJECT_DIR)) {
fileStream.forEach(file -> addFileToContext(context, baseDir, file));
context.setRuntime(SonarRuntimeImpl.forSonarQube(Version.create(majorVersion, minorVersion), SonarQubeSide.SERVER,
SonarEdition.DEVELOPER));
SonarEdition.DEVELOPER));
if (fileName != null) {
String path = PROJECT_DIR.resolve(fileName).toAbsolutePath().toString();
context.settings().setProperty("sonar.python.ruff.reportPaths", path);
Expand All @@ -293,11 +330,11 @@ private static void addFileToContext(SensorContextTester context, Path projectDi
try {
String projectId = projectDir.getFileName().toString() + "-project";
context.fileSystem().add(TestInputFileBuilder.create(projectId, projectDir.toFile(), file.toFile())
.setCharset(UTF_8)
.setLanguage(language(file))
.setContents(new String(Files.readAllBytes(file), UTF_8))
.setType(InputFile.Type.MAIN)
.build());
.setCharset(UTF_8)
.setLanguage(language(file))
.setContents(new String(Files.readAllBytes(file), UTF_8))
.setType(InputFile.Type.MAIN)
.build());
} catch (IOException e) {
throw new IllegalStateException(e);
}
Expand Down
6 changes: 6 additions & 0 deletions sonar-python-plugin/src/test/resources/org/sonar/plugins/python/ruff/file1.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,9 @@ def baz():
print("42")
except:
pass

def process(data):
engaged_ratio_values_list = list(
map(lambda data_point: data_point["engaged_ratio"], data)
)
return engaged_ratio_values_list
16 changes: 16 additions & 0 deletions sonar-python-plugin/src/test/resources/org/sonar/plugins/python/ruff/ruff-json-format.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,5 +158,21 @@
"message": "`try`-`except`-`pass` detected, consider logging the exception",
"noqa_row": 21,
"url": "https://beta.ruff.rs/docs/rules/try-except-pass"
},
{
"code": "C417",
"end_location": {
"column": 6,
"row": 27
},
"filename": "ruff/file1.py",
"fix": null,
"location": {
"column": 33,
"row": 25
},
"message": "Unnecessary `map` usage (rewrite using a `list` comprehension)",
"noqa_row": 5,
"url": "https://docs.astral.sh/ruff/rules/unnecessary-map"
}
]

0 comments on commit b9f1e08

Please sign in to comment.