Merge pull request #3317 from github/koesie10/sarif-comparison

Fix bug in SARIF comparison

extensions/ql-vscode/src/compare/sarif-diff.ts

@@ -1,5 +1,40 @@
 import type { Result } from "sarif";
 
+function toCanonicalResult(result: Result): Result {
+  const canonicalResult = {
+    ...result,
+  };
+
+  if (canonicalResult.locations) {
+    canonicalResult.locations = canonicalResult.locations.map((location) => {
+      if (location.physicalLocation?.artifactLocation?.index !== undefined) {
+        const canonicalLocation = {
+          ...location,
+        };
+
+        canonicalLocation.physicalLocation = {
+          ...canonicalLocation.physicalLocation,
+        };
+
+        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;
+      }
+
+      // Don't create a new object if we don't need to
+      return location;
+    });
+  }
+
+  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 +60,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 (

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);
+  });
+});