-
Notifications
You must be signed in to change notification settings - Fork 451
Refactor SARIF-related types and functions into a separate module #3528
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.
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
Changes from 7 commits
d7cfd19
2fce45b
40aec38
3b16d31
ae9cb02
9a31859
b43d146
1721ce7
28b449d
6d060bb
2a2f4c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| import * as fs from "fs"; | ||
|
|
||
| import test from "ava"; | ||
|
|
||
| import { setupTests } from "../testing-utils"; | ||
|
|
||
| import { getToolNames, type Log } from "."; | ||
|
|
||
| setupTests(test); | ||
|
|
||
| test("getToolNames", (t) => { | ||
| const input = fs.readFileSync( | ||
| `${__dirname}/../../src/testdata/tool-names.sarif`, | ||
| "utf8", | ||
| ); | ||
| const toolNames = getToolNames(JSON.parse(input) as Log); | ||
| t.deepEqual(toolNames, ["CodeQL command-line toolchain", "ESLint"]); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| import * as fs from "fs"; | ||
|
|
||
| import { Logger } from "../logging"; | ||
|
|
||
| import * as sarif from "sarif"; | ||
|
|
||
| export type * from "sarif"; | ||
|
|
||
| // `automationId` is non-standard. | ||
| export type RunKey = sarif.ToolComponent & { | ||
| automationId: string | undefined; | ||
| }; | ||
|
|
||
| /** | ||
| * An error that occurred due to an invalid SARIF upload request. | ||
| */ | ||
| export class InvalidSarifUploadError extends Error {} | ||
|
|
||
| /** | ||
| * Get the array of all the tool names contained in the given sarif contents. | ||
| * | ||
| * Returns an array of unique string tool names. | ||
| */ | ||
| export function getToolNames(sarifFile: sarif.Log): string[] { | ||
| const toolNames = {}; | ||
|
|
||
| for (const run of sarifFile.runs || []) { | ||
| const tool = run.tool || {}; | ||
| const driver = tool.driver || {}; | ||
| if (typeof driver.name === "string" && driver.name.length > 0) { | ||
| toolNames[driver.name] = true; | ||
| } | ||
| } | ||
|
|
||
| return Object.keys(toolNames); | ||
| } | ||
|
|
||
| export function readSarifFile(sarifFilePath: string): sarif.Log { | ||
| return JSON.parse(fs.readFileSync(sarifFilePath, "utf8")) as sarif.Log; | ||
| } | ||
|
|
||
| // Takes a list of paths to sarif files and combines them together, | ||
| // returning the contents of the combined sarif file. | ||
| export function combineSarifFiles( | ||
| sarifFiles: string[], | ||
| logger: Logger, | ||
| ): sarif.Log { | ||
| logger.info(`Loading SARIF file(s)`); | ||
| const combinedSarif: sarif.Log = { | ||
| version: "2.1.0", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like a potentially breaking change — now we'll only accept SARIF v2.1.0
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not quite. We haven't changed anything about how we parse or validate SARIF files. So although the type from
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose since code scanning only supports v2.1.0 it's not necessarily a bad thing if we fail if users try to upload a SARIF file with a different version number. Though it does seem like a breaking change if the API would previously tolerate different version numbers. Could you explain why this combining logic wouldn't fail with the "Different SARIF versions encountered" error message if a SARIF file contains a different version number or a missing version number?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, sorry. I missed that this (and the other comment) were specifically in the context of I have re-jigged this function slightly now so that the old behaviour is maintained, unless there is no This was supposed to be a fairly small PR to unblock me on something else, so I am not sure I want to deal with a breaking change here. |
||
| runs: [], | ||
| }; | ||
|
|
||
| for (const sarifFile of sarifFiles) { | ||
| logger.debug(`Loading SARIF file: ${sarifFile}`); | ||
| const sarifObject = readSarifFile(sarifFile); | ||
| // Check SARIF version | ||
| if (combinedSarif.version === null) { | ||
|
mbg marked this conversation as resolved.
Outdated
|
||
| combinedSarif.version = sarifObject.version; | ||
| } else if (combinedSarif.version !== sarifObject.version) { | ||
| throw new InvalidSarifUploadError( | ||
| `Different SARIF versions encountered: ${combinedSarif.version} and ${sarifObject.version}`, | ||
| ); | ||
| } | ||
|
|
||
| combinedSarif.runs.push(...sarifObject.runs); | ||
| } | ||
|
|
||
| return combinedSarif; | ||
| } | ||
|
|
||
| /** | ||
| * Checks whether all the runs in the given SARIF files were produced by CodeQL. | ||
| * @param sarifObjects The list of SARIF objects to check. | ||
| */ | ||
| export function areAllRunsProducedByCodeQL(sarifObjects: sarif.Log[]): boolean { | ||
|
mbg marked this conversation as resolved.
Outdated
|
||
| return sarifObjects.every((sarifObject) => { | ||
| return sarifObject.runs?.every( | ||
| (run) => run.tool?.driver?.name === "CodeQL", | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| function createRunKey(run: sarif.Run): RunKey { | ||
| return { | ||
| name: run.tool?.driver?.name, | ||
| fullName: run.tool?.driver?.fullName, | ||
| version: run.tool?.driver?.version, | ||
| semanticVersion: run.tool?.driver?.semanticVersion, | ||
| guid: run.tool?.driver?.guid, | ||
| automationId: run.automationDetails?.id, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Checks whether all runs in the given SARIF files are unique (based on the | ||
| * criteria used by Code Scanning to determine analysis categories). | ||
| * @param sarifObjects The list of SARIF objects to check. | ||
| */ | ||
| export function areAllRunsUnique(sarifObjects: sarif.Log[]): boolean { | ||
| const keys = new Set<string>(); | ||
|
|
||
| for (const sarifObject of sarifObjects) { | ||
| for (const run of sarifObject.runs) { | ||
| const key = JSON.stringify(createRunKey(run)); | ||
|
|
||
| // If the key already exists, the runs are not unique. | ||
| if (keys.has(key)) { | ||
| return false; | ||
| } | ||
|
|
||
| keys.add(key); | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.