Skip to content

NEW @W-17159591@ SARIF output now passes official validation #144

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

Merged
jfeingold35 merged 1 commit into from
Dec 3, 2024
Merged

NEW @W-17159591@ SARIF output now passes official validation #144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
42 changes: 26 additions & 16 deletions packages/code-analyzer-core/src/output-formats/sarif-output-format.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import path from 'node:path';
import {CodeLocation, EngineRunResults, RunResults, Violation} from "../results";
import * as sarif from "sarif";
import {Rule, SeverityLevel} from "../rules";
Expand Down Expand Up @@ -38,7 +39,7 @@ function toSarifRun(engineRunResults: EngineRunResults, runDir: string): sarif.R
rules: rules.map(toSarifReportingDescriptor),
}
},
results: violations.map(v => toSarifResult(v, ruleNames.indexOf(v.getRule().getName()))),
results: violations.map(v => toSarifResult(v, runDir, ruleNames.indexOf(v.getRule().getName()))),
invocations: [
{
executionSuccessful: true,
Expand All @@ -50,7 +51,7 @@ function toSarifRun(engineRunResults: EngineRunResults, runDir: string): sarif.R
};
}

function toSarifResult(violation: Violation, ruleIndex: number) : sarif.Result {
function toSarifResult(violation: Violation, runDir: string, ruleIndex: number) : sarif.Result {
const primaryCodeLocation = violation.getCodeLocations()[violation.getPrimaryLocationIndex()];
return {
ruleId: violation.getRule().getName(),
Expand All @@ -60,26 +61,35 @@ function toSarifResult(violation: Violation, ruleIndex: number) : sarif.Result {

// Note that sarif format has a limit of 10 elements in the locations array, so we only store
// the primary location (which is what most utilities expect) here
locations: [toSarifLocation(primaryCodeLocation)],
locations: [toSarifLocation(primaryCodeLocation, runDir)],

// And then we store the full locations array in the relatedLocations field if users want to see all of them
relatedLocations: violation.getCodeLocations().map(toSarifLocation)
relatedLocations: violation.getCodeLocations().map(codeLoc => toSarifLocation(codeLoc, runDir))
};
}

function toSarifLocation(codeLocation: CodeLocation): sarif.Location {
return {
physicalLocation: {
artifactLocation: {
uri: codeLocation.getFile(),
},
region: {
startLine: codeLocation.getStartLine(),
startColumn: codeLocation.getStartColumn(),
endLine: codeLocation.getEndLine(),
endColumn: codeLocation.getEndColumn()
} as sarif.Region
function toSarifLocation(codeLocation: CodeLocation, runDir: string): sarif.Location {
if (codeLocation.getFile()) {
return {
physicalLocation: {
artifactLocation: {
uri: path.relative(runDir, codeLocation.getFile()!),
uriBaseId: runDir
},
region: {
startLine: codeLocation.getStartLine(),
startColumn: codeLocation.getStartColumn(),
endLine: codeLocation.getEndLine(),
endColumn: codeLocation.getEndColumn()
} as sarif.Region
}
}
} else {
return {
Copy link
Contributor Author

@jfeingold35 jfeingold35 Dec 3, 2024

Choose a reason for hiding this comment

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

The validator didn't like having an empty region object, so I'm just using an empty artifactLocation object outright when there's no code location.
Technically I suppose it's possible we could see a case where there's a code location with no start line or anything else, and the output of that would be invalid SARIF. I can account for that scenario if consensus is that it's worth the trouble.

physicalLocation: {
artifactLocation: {}
}
};
}
}

Expand Down
42 changes: 28 additions & 14 deletions ...s/code-analyzer-core/test/test-data/expectedOutputFiles/multipleViolations.goldfile.sarif
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@
{
"physicalLocation": {
"artifactLocation": {
"uri": "{{RUNDIR}}test{{PATHSEP}}config.test.ts"
"uri": "test{{PATHSEP}}config.test.ts",
"uriBaseId": "{{RUNDIR}}"
},
"region": {
"startLine": 3,
Expand All @@ -72,7 +73,8 @@
{
"physicalLocation": {
"artifactLocation": {
"uri": "{{RUNDIR}}test{{PATHSEP}}config.test.ts"
"uri": "test{{PATHSEP}}config.test.ts",
"uriBaseId": "{{RUNDIR}}"
},
"region": {
"startLine": 3,
Expand All @@ -95,7 +97,8 @@
{
"physicalLocation": {
"artifactLocation": {
"uri": "{{RUNDIR}}test{{PATHSEP}}run.test.ts"
"uri": "test{{PATHSEP}}run.test.ts",
"uriBaseId": "{{RUNDIR}}"
},
"region": {
"startLine": 21,
Expand All @@ -110,7 +113,8 @@
{
"physicalLocation": {
"artifactLocation": {
"uri": "{{RUNDIR}}test{{PATHSEP}}run.test.ts"
"uri": "test{{PATHSEP}}run.test.ts",
"uriBaseId": "{{RUNDIR}}"
},
"region": {
"startLine": 21,
Expand All @@ -133,7 +137,8 @@
{
"physicalLocation": {
"artifactLocation": {
"uri": "{{RUNDIR}}test{{PATHSEP}}run.test.ts"
"uri": "test{{PATHSEP}}run.test.ts",
"uriBaseId": "{{RUNDIR}}"
},
"region": {
"startLine": 56,
Expand All @@ -146,7 +151,8 @@
{
"physicalLocation": {
"artifactLocation": {
"uri": "{{RUNDIR}}test{{PATHSEP}}run.test.ts"
"uri": "test{{PATHSEP}}run.test.ts",
"uriBaseId": "{{RUNDIR}}"
},
"region": {
"startLine": 56,
Expand Down Expand Up @@ -198,7 +204,8 @@
{
"physicalLocation": {
"artifactLocation": {
"uri": "{{RUNDIR}}test{{PATHSEP}}stubs.ts"
"uri": "test{{PATHSEP}}stubs.ts",
"uriBaseId": "{{RUNDIR}}"
},
"region": {
"startLine": 76,
Expand All @@ -211,7 +218,8 @@
{
"physicalLocation": {
"artifactLocation": {
"uri": "{{RUNDIR}}test{{PATHSEP}}stubs.ts"
"uri": "test{{PATHSEP}}stubs.ts",
"uriBaseId": "{{RUNDIR}}"
},
"region": {
"startLine": 4,
Expand All @@ -222,7 +230,8 @@
{
"physicalLocation": {
"artifactLocation": {
"uri": "{{RUNDIR}}test{{PATHSEP}}test-helpers.ts"
"uri": "test{{PATHSEP}}test-helpers.ts",
"uriBaseId": "{{RUNDIR}}"
},
"region": {
"startLine": 9,
Expand All @@ -233,7 +242,8 @@
{
"physicalLocation": {
"artifactLocation": {
"uri": "{{RUNDIR}}test{{PATHSEP}}stubs.ts"
"uri": "test{{PATHSEP}}stubs.ts",
"uriBaseId": "{{RUNDIR}}"
},
"region": {
"startLine": 76,
Expand Down Expand Up @@ -285,7 +295,8 @@
{
"physicalLocation": {
"artifactLocation": {
"uri": "{{RUNDIR}}test{{PATHSEP}}stubs.ts"
"uri": "test{{PATHSEP}}stubs.ts",
"uriBaseId": "{{RUNDIR}}"
},
"region": {
"startLine": 90,
Expand All @@ -300,7 +311,8 @@
{
"physicalLocation": {
"artifactLocation": {
"uri": "{{RUNDIR}}test{{PATHSEP}}stubs.ts"
"uri": "test{{PATHSEP}}stubs.ts",
"uriBaseId": "{{RUNDIR}}"
},
"region": {
"startLine": 20,
Expand All @@ -313,7 +325,8 @@
{
"physicalLocation": {
"artifactLocation": {
"uri": "{{RUNDIR}}test{{PATHSEP}}test-helpers.ts"
"uri": "test{{PATHSEP}}test-helpers.ts",
"uriBaseId": "{{RUNDIR}}"
},
"region": {
"startLine": 5,
Expand All @@ -324,7 +337,8 @@
{
"physicalLocation": {
"artifactLocation": {
"uri": "{{RUNDIR}}test{{PATHSEP}}stubs.ts"
"uri": "test{{PATHSEP}}stubs.ts",
"uriBaseId": "{{RUNDIR}}"
},
"region": {
"startLine": 90,
Expand Down
6 changes: 2 additions & 4 deletions ...zer-core/test/test-data/expectedOutputFiles/unexpectedEngineErrorViolation.goldfile.sarif
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,14 @@
"locations": [
{
"physicalLocation": {
"artifactLocation": {},
"region": {}
"artifactLocation": {}
}
}
],
"relatedLocations": [
{
"physicalLocation": {
"artifactLocation": {},
"region": {}
"artifactLocation": {}
}
}
]
Expand Down
Loading