Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixing a couple of edge-cases that cause the ESLint formatter to create invalid SARIF files #2068

Merged
4 commits merged into from
Sep 14, 2020
Merged

Fixing a couple of edge-cases that cause the ESLint formatter to create invalid SARIF files #2068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0f5a635
Only adding shortDescription to a rule if a description exists
Sep 9, 2020
e147c70
Adding test for valid SARIF from a custom rule with no description
Sep 9, 2020
a7aa894
Only adding startLine / startColumn if they are > 0
Sep 9, 2020
e67691a
Adding test for invalid column number
Sep 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions src/ESLint.Formatter/sarif.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,14 +145,16 @@ module.exports = function (results, data) {
// Create a new entry in the rules dictionary.
sarifRules[message.ruleId] = {
id: message.ruleId,
shortDescription: {
text: meta.docs.description
},
helpUri: meta.docs.url,
properties: {
category: meta.docs.category
}
};
if (meta.docs.description) {
sarifRules[message.ruleId].shortDescription = {
text: meta.docs.description
};
}
}
}

Expand All @@ -179,9 +181,12 @@ module.exports = function (results, data) {
}

if (message.line > 0 || message.column > 0) {
sarifRepresentation.locations[0].physicalLocation.region = {
startLine: message.line,
startColumn: message.column
sarifRepresentation.locations[0].physicalLocation.region = {};
if (message.line > 0) {
sarifRepresentation.locations[0].physicalLocation.region.startLine = message.line;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

} [](start = 24, length = 1)

This change is fine. Or, to be precise, it's an improvement over the existing code. Be aware that if a SARIF region is being represented with line/column properties (SARIF can represent regions in other ways, such as offset/length), then the startLine property is required. So if you did have a message with a valid column but without a valid line, then the resulting SARIF file would be invalid. Having said that -- again, you're no worse off than before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense - though it's probably less common that a tool outputs a column without a line. Thanks

if (message.column > 0) {
sarifRepresentation.locations[0].physicalLocation.region.startColumn = message.column;
};
}

Expand Down
73 changes: 73 additions & 0 deletions src/ESLint.Formatter/test/sarif.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,28 @@ describe("formatter:sarif", () => {
});
});

describe("formatter:sarif", () => {
describe("when passed one message with line and invalid column", () => {
const code = [{
filePath: sourceFilePath1,
messages: [{
message: "Unexpected value.",
ruleId: testRuleId,
line: 10,
column: 0
}]
}];

it("should return a log with one result whose location contains a region with line # and no column #", () => {
const log = JSON.parse(formatter(code));

assert.strictEqual(log.runs[0].results[0].locations[0].physicalLocation.region.startLine, code[0].messages[0].line);
assert.isUndefined(log.runs[0].results[0].locations[0].physicalLocation.region.startColumn);
assert.isUndefined(log.runs[0].results[0].locations[0].physicalLocation.region.snippet);
});
});
});

describe("formatter:sarif", () => {
describe("when passed one message with line and column but no source string", () => {
const code = [{
Expand Down Expand Up @@ -473,3 +495,54 @@ describe("formatter:sarif", () => {
});
});
});

describe("formatter:sarif", () => {
describe("when passed a rule with no description", () => {
const ruleid = "custom-rule-no-description";

rules[ruleid] = {
type: "suggestion",
docs: {
category: "Possible Errors"
}
};
const code = [{
filePath: sourceFilePath1,
messages: [{
message: "Custom error.",
ruleId: ruleid,
line: 42
}]
}];
it("should return a log with one file, one rule, and one result", () => {
const log = JSON.parse(formatter(code, { rulesMeta: rules }));
const rule = rules[ruleid];

assert.lengthOf(log.runs[0].artifacts, 1);
assert.lengthOf(log.runs[0].results, 1);

assert.strictEqual(log.runs[0].tool.driver.rules[0].id, ruleid);

assert(log.runs[0].artifacts[0].location.uri.startsWith(uriPrefix));
assert(log.runs[0].artifacts[0].location.uri.endsWith(sourceFilePath1));

assert.strictEqual(log.runs[0].tool.driver.rules[0].id, ruleid);
assert.isUndefined(log.runs[0].tool.driver.rules[0].shortDescription);
assert.strictEqual(log.runs[0].tool.driver.rules[0].helpUri, rule.docs.url);
assert.strictEqual(log.runs[0].tool.driver.rules[0].properties.category, rule.docs.category);

assert.strictEqual(log.runs[0].results[0].ruleId, ruleid);

assert.strictEqual(log.runs[0].results[0].level, "warning");

assert.strictEqual(log.runs[0].results[0].message.text, "Custom error.");

assert(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.uri.startsWith(uriPrefix));
assert(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.uri.endsWith(sourceFilePath1));

assert.strictEqual(log.runs[0].results[0].locations[0].physicalLocation.region.startLine, 42);
assert.isUndefined(log.runs[0].results[0].locations[0].physicalLocation.region.startColumn);
assert.isUndefined(log.runs[0].results[0].locations[0].physicalLocation.region.snippet);
});
});
});