From 73b6cc475c1669ac87e55050cf61c4b94bcb05d0 Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Tue, 6 Feb 2024 13:40:09 +0100 Subject: [PATCH 1/2] Fix bug in SARIF comparison The SARIF comparison code was comparing the index of the artifact location, which is not useful for comparison and may differ between runs of very similar queries. This adds a function to convert a SARIF result to a canonical form, which removes the index from the artifact location. --- .../ql-vscode/src/compare/sarif-diff.ts | 34 +++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/extensions/ql-vscode/src/compare/sarif-diff.ts b/extensions/ql-vscode/src/compare/sarif-diff.ts index 9f8089526..22fe81da8 100644 --- a/extensions/ql-vscode/src/compare/sarif-diff.ts +++ b/extensions/ql-vscode/src/compare/sarif-diff.ts @@ -1,5 +1,32 @@ import type { Result } from "sarif"; +function toCanonicalResult(result: Result): Result { + const canonicalResult = { + ...result, + }; + + if (canonicalResult.locations) { + canonicalResult.locations = canonicalResult.locations.map((location) => { + const canonicalLocation = { + ...location, + }; + + if (canonicalLocation.physicalLocation?.artifactLocation) { + canonicalLocation.physicalLocation.artifactLocation = { + ...canonicalLocation.physicalLocation.artifactLocation, + }; + // The index is dependent on the result of the SARIF file and usually doesn't really tell + // us anything useful, so we remove it from the comparison. + delete canonicalLocation.physicalLocation.artifactLocation.index; + } + + return canonicalLocation; + }); + } + + return canonicalResult; +} + /** * Compare the alerts of two queries. Use deep equality to determine if * results have been added or removed across two invocations of a query. @@ -25,9 +52,12 @@ export function sarifDiff(fromResults: Result[], toResults: Result[]) { throw new Error("CodeQL Compare: Target query has no results."); } + const canonicalFromResults = fromResults.map(toCanonicalResult); + const canonicalToResults = toResults.map(toCanonicalResult); + const results = { - from: arrayDiff(fromResults, toResults), - to: arrayDiff(toResults, fromResults), + from: arrayDiff(canonicalFromResults, canonicalToResults), + to: arrayDiff(canonicalToResults, canonicalFromResults), }; if ( From c5db59767659f8eb20d4301a9ef6b844b352b167 Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Tue, 6 Feb 2024 13:48:37 +0100 Subject: [PATCH 2/2] Add tests for SARIF diff --- .../ql-vscode/src/compare/sarif-diff.ts | 18 +- .../unit-tests/compare/sarif-diff.test.ts | 183 ++++++++++++++++++ 2 files changed, 196 insertions(+), 5 deletions(-) create mode 100644 extensions/ql-vscode/test/unit-tests/compare/sarif-diff.test.ts diff --git a/extensions/ql-vscode/src/compare/sarif-diff.ts b/extensions/ql-vscode/src/compare/sarif-diff.ts index 22fe81da8..8f8dafd3a 100644 --- a/extensions/ql-vscode/src/compare/sarif-diff.ts +++ b/extensions/ql-vscode/src/compare/sarif-diff.ts @@ -7,20 +7,28 @@ function toCanonicalResult(result: Result): Result { if (canonicalResult.locations) { canonicalResult.locations = canonicalResult.locations.map((location) => { - const canonicalLocation = { - ...location, - }; + if (location.physicalLocation?.artifactLocation?.index !== undefined) { + const canonicalLocation = { + ...location, + }; + + canonicalLocation.physicalLocation = { + ...canonicalLocation.physicalLocation, + }; - if (canonicalLocation.physicalLocation?.artifactLocation) { canonicalLocation.physicalLocation.artifactLocation = { ...canonicalLocation.physicalLocation.artifactLocation, }; + // The index is dependent on the result of the SARIF file and usually doesn't really tell // us anything useful, so we remove it from the comparison. delete canonicalLocation.physicalLocation.artifactLocation.index; + + return canonicalLocation; } - return canonicalLocation; + // Don't create a new object if we don't need to + return location; }); } diff --git a/extensions/ql-vscode/test/unit-tests/compare/sarif-diff.test.ts b/extensions/ql-vscode/test/unit-tests/compare/sarif-diff.test.ts new file mode 100644 index 000000000..ac6190dcf --- /dev/null +++ b/extensions/ql-vscode/test/unit-tests/compare/sarif-diff.test.ts @@ -0,0 +1,183 @@ +import type { Result } from "sarif"; +import { sarifDiff } from "../../../src/compare/sarif-diff"; + +describe("sarifDiff", () => { + const result1: Result = { + message: { + text: "result1", + }, + }; + const result2: Result = { + message: { + text: "result2", + }, + }; + const result3: Result = { + message: { + text: "result3", + }, + }; + + it("throws an error when the source query has no results", () => { + expect(() => sarifDiff([], [result1, result2])).toThrow( + "CodeQL Compare: Source query has no results.", + ); + }); + + it("throws an error when the target query has no results", () => { + expect(() => sarifDiff([result1, result2], [])).toThrow( + "CodeQL Compare: Target query has no results.", + ); + }); + + it("throws an error when there is no overlap between the source and target queries", () => { + expect(() => sarifDiff([result1], [result2])).toThrow( + "CodeQL Compare: No overlap between the selected queries.", + ); + }); + + it("return an empty comparison when the results are the same", () => { + expect(sarifDiff([result1, result2], [result1, result2])).toEqual({ + from: [], + to: [], + }); + }); + + it("returns the added and removed results", () => { + expect(sarifDiff([result1, result3], [result1, result2])).toEqual({ + from: [result3], + to: [result2], + }); + }); + + it("does not use reference equality to compare results", () => { + const result = { + message: { + text: "result1", + }, + }; + expect(sarifDiff([result], [result1])).toEqual({ + from: [], + to: [], + }); + }); + + it("does not take into account the index of the artifact location", () => { + const result1: Result = { + message: { + text: "result1", + }, + locations: [ + { + physicalLocation: { + artifactLocation: { + uri: "file:///path/to/file1", + uriBaseId: "%SRCROOT%", + index: 1, + }, + }, + }, + ], + }; + const result2: Result = { + message: { + text: "result1", + }, + locations: [ + { + physicalLocation: { + artifactLocation: { + uri: "file:///path/to/file1", + uriBaseId: "%SRCROOT%", + index: 2, + }, + }, + }, + ], + }; + expect(sarifDiff([result1], [result2])).toEqual({ + from: [], + to: [], + }); + }); + + it("takes into account the other properties of the artifact location", () => { + const result1: Result = { + message: { + text: "result1", + }, + locations: [ + { + physicalLocation: { + artifactLocation: { + uri: "file:///path/to/file1", + uriBaseId: "%SRCROOT%", + }, + }, + }, + ], + }; + const result2: Result = { + message: { + text: "result1", + }, + locations: [ + { + physicalLocation: { + artifactLocation: { + uri: "file:///path/to/file2", + uriBaseId: "%SRCROOT%", + }, + }, + }, + ], + }; + expect(sarifDiff([result1], [result1, result2])).toEqual({ + from: [], + to: [result2], + }); + }); + + it("does not modify the input", () => { + const result1: Result = { + message: { + text: "result1", + }, + locations: [ + { + physicalLocation: { + artifactLocation: { + uri: "file:///path/to/file1", + uriBaseId: "%SRCROOT%", + index: 1, + }, + }, + }, + ], + }; + const result1Copy = JSON.parse(JSON.stringify(result1)); + const result2: Result = { + message: { + text: "result1", + }, + locations: [ + { + physicalLocation: { + artifactLocation: { + uri: "file:///path/to/file1", + uriBaseId: "%SRCROOT%", + index: 2, + }, + }, + }, + ], + }; + const result2Copy = JSON.parse(JSON.stringify(result2)); + expect(sarifDiff([result1], [result2])).toEqual({ + from: [], + to: [], + }); + expect(result1).toEqual(result1Copy); + expect(result2).toEqual(result2Copy); + }); +});