From 73b6cc475c1669ac87e55050cf61c4b94bcb05d0 Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Tue, 6 Feb 2024 13:40:09 +0100 Subject: [PATCH 1/4] 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/4] 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); + }); +}); From 0477a9dee3b12bfb810cbe9581391cff9e8f7277 Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Tue, 6 Feb 2024 14:06:57 +0100 Subject: [PATCH 3/4] Make query constraints for access paths query configurable --- .../model-editor/languages/models-as-data.ts | 1 + .../src/model-editor/languages/ruby/index.ts | 5 ++++ .../src/model-editor/model-editor-view.ts | 1 + .../src/model-editor/suggestion-queries.ts | 16 +++++------ .../model-editor/suggestion-queries.test.ts | 28 +++++++++++++++++-- 5 files changed, 39 insertions(+), 12 deletions(-) diff --git a/extensions/ql-vscode/src/model-editor/languages/models-as-data.ts b/extensions/ql-vscode/src/model-editor/languages/models-as-data.ts index 25ebd21d2..4fad24268 100644 --- a/extensions/ql-vscode/src/model-editor/languages/models-as-data.ts +++ b/extensions/ql-vscode/src/model-editor/languages/models-as-data.ts @@ -44,6 +44,7 @@ type ModelsAsDataLanguageModelGeneration = { }; type ModelsAsDataLanguageAccessPathSuggestions = { + queryConstraints: (mode: Mode) => QueryConstraints; parseResults: ( // The results of a single predicate of the query. bqrs: DecodedBqrsChunk, diff --git a/extensions/ql-vscode/src/model-editor/languages/ruby/index.ts b/extensions/ql-vscode/src/model-editor/languages/ruby/index.ts index e1b9d9a5b..faa1c6fc7 100644 --- a/extensions/ql-vscode/src/model-editor/languages/ruby/index.ts +++ b/extensions/ql-vscode/src/model-editor/languages/ruby/index.ts @@ -12,6 +12,7 @@ import { rubyPath, } from "./access-paths"; import { parseAccessPathSuggestionsResults } from "./suggestions"; +import { modeTag } from "../../mode-tag"; export const ruby: ModelsAsDataLanguage = { availableModes: [Mode.Framework], @@ -170,6 +171,10 @@ export const ruby: ModelsAsDataLanguage = { parseResults: parseGenerateModelResults, }, accessPathSuggestions: { + queryConstraints: (mode) => ({ + kind: "table", + "tags contain all": ["modeleditor", "access-paths", modeTag(mode)], + }), parseResults: parseAccessPathSuggestionsResults, }, getArgumentOptions: (method) => { diff --git a/extensions/ql-vscode/src/model-editor/model-editor-view.ts b/extensions/ql-vscode/src/model-editor/model-editor-view.ts index 0680f8f09..7bb5114d7 100644 --- a/extensions/ql-vscode/src/model-editor/model-editor-view.ts +++ b/extensions/ql-vscode/src/model-editor/model-editor-view.ts @@ -529,6 +529,7 @@ export class ModelEditorView extends AbstractWebview< modelsAsDataLanguage, this.app.logger, ), + queryConstraints: accessPathSuggestions.queryConstraints(mode), cliServer: this.cliServer, queryRunner: this.queryRunner, queryStorageDir: this.queryStorageDir, diff --git a/extensions/ql-vscode/src/model-editor/suggestion-queries.ts b/extensions/ql-vscode/src/model-editor/suggestion-queries.ts index ef96da2d3..37ce78fd3 100644 --- a/extensions/ql-vscode/src/model-editor/suggestion-queries.ts +++ b/extensions/ql-vscode/src/model-editor/suggestion-queries.ts @@ -1,7 +1,7 @@ import type { CodeQLCliServer } from "../codeql-cli/cli"; import type { Mode } from "./shared/mode"; +import type { QueryConstraints } from "../local-queries"; import { resolveQueriesFromPacks } from "../local-queries"; -import { modeTag } from "./mode-tag"; import { getOnDiskWorkspaceFolders } from "../common/vscode/workspace-folders"; import type { NotificationLogger } from "../common/logging"; import { showAndLogExceptionWithTelemetry } from "../common/logging"; @@ -22,6 +22,7 @@ type RunQueryOptions = { parseResults: ( results: DecodedBqrsChunk, ) => AccessPathSuggestionRow[] | Promise; + queryConstraints: QueryConstraints; cliServer: CodeQLCliServer; queryRunner: QueryRunner; @@ -39,6 +40,7 @@ export async function runSuggestionsQuery( mode: Mode, { parseResults, + queryConstraints, cliServer, queryRunner, logger, @@ -68,6 +70,7 @@ export async function runSuggestionsQuery( cliServer, databaseItem.language, mode, + queryConstraints, ); if (!queryPath) { void showAndLogExceptionWithTelemetry( @@ -141,6 +144,7 @@ export async function runSuggestionsQuery( * @param cliServer The CodeQL CLI server to use. * @param language The language of the query pack to use. * @param mode The mode to resolve the query for. + * @param queryConstraints Constraints to apply to the query. * @param additionalPackNames Additional pack names to search. * @param additionalPackPaths Additional pack paths to search. */ @@ -148,6 +152,7 @@ async function resolveSuggestionsQuery( cliServer: CodeQLCliServer, language: string, mode: Mode, + queryConstraints: QueryConstraints, additionalPackNames: string[] = [], additionalPackPaths: string[] = [], ): Promise { @@ -156,14 +161,7 @@ async function resolveSuggestionsQuery( const queries = await resolveQueriesFromPacks( cliServer, packsToSearch, - { - kind: "table", - "tags contain all": [ - "modeleditor", - "access-path-suggestions", - modeTag(mode), - ], - }, + queryConstraints, additionalPackPaths, ); if (queries.length > 1) { diff --git a/extensions/ql-vscode/test/vscode-tests/no-workspace/model-editor/suggestion-queries.test.ts b/extensions/ql-vscode/test/vscode-tests/no-workspace/model-editor/suggestion-queries.test.ts index 336acb2fd..0290717eb 100644 --- a/extensions/ql-vscode/test/vscode-tests/no-workspace/model-editor/suggestion-queries.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/no-workspace/model-editor/suggestion-queries.test.ts @@ -1,3 +1,5 @@ +import { load } from "js-yaml"; +import { readFile } from "fs-extra"; import { createMockLogger } from "../../../__mocks__/loggerMock"; import type { DatabaseItem } from "../../../../src/databases/local-databases"; import { DatabaseKind } from "../../../../src/databases/local-databases"; @@ -138,15 +140,21 @@ describe("runSuggestionsQuery", () => { .mockResolvedValueOnce(mockInputSuggestions) .mockResolvedValueOnce(mockOutputSuggestions); + const resolveQueriesInSuite = jest + .fn() + .mockResolvedValue(["/a/b/c/FrameworkModeAccessPathSuggestions.ql"]); + const options = { parseResults, + queryConstraints: { + kind: "table", + "tags all": ["modeleditor", "access-paths", "ruby", "foo"], + }, cliServer: mockedObject({ resolveQlpacks: jest.fn().mockResolvedValue({ "my/extensions": "/a/b/c/", }), - resolveQueriesInSuite: jest - .fn() - .mockResolvedValue(["/a/b/c/FrameworkModeAccessPathSuggestions.ql"]), + resolveQueriesInSuite, packPacklist: jest .fn() .mockResolvedValue([ @@ -205,6 +213,20 @@ describe("runSuggestionsQuery", () => { undefined, undefined, ); + expect(options.cliServer.resolveQueriesInSuite).toHaveBeenCalledTimes(1); + + expect( + load(await readFile(resolveQueriesInSuite.mock.calls[0][0], "utf-8")), + ).toEqual([ + { + from: "codeql/ruby-queries", + include: { + kind: "table", + "tags all": ["modeleditor", "access-paths", "ruby", "foo"], + }, + queries: ".", + }, + ]); expect(options.parseResults).toHaveBeenCalledTimes(2); From 19d85a73e15b57ee759336329e17e06d46219aee Mon Sep 17 00:00:00 2001 From: Shati Patel <42641846+shati-patel@users.noreply.github.com> Date: Tue, 6 Feb 2024 15:11:56 +0000 Subject: [PATCH 4/4] CodeQL model editor: Use suggest box for type path suggestions (#3316) --- .../src/view/model-editor/MethodRow.tsx | 1 + .../model-editor/ModelInputSuggestBox.tsx | 12 +++- .../model-editor/ModelOutputSuggestBox.tsx | 8 ++- .../model-editor/ModelTypePathSuggestBox.tsx | 68 +++++++++++++++++++ 4 files changed, 84 insertions(+), 5 deletions(-) create mode 100644 extensions/ql-vscode/src/view/model-editor/ModelTypePathSuggestBox.tsx diff --git a/extensions/ql-vscode/src/view/model-editor/MethodRow.tsx b/extensions/ql-vscode/src/view/model-editor/MethodRow.tsx index e54e52d48..4b3b9e0df 100644 --- a/extensions/ql-vscode/src/view/model-editor/MethodRow.tsx +++ b/extensions/ql-vscode/src/view/model-editor/MethodRow.tsx @@ -278,6 +278,7 @@ const ModelableMethodRow = forwardRef( )} diff --git a/extensions/ql-vscode/src/view/model-editor/ModelInputSuggestBox.tsx b/extensions/ql-vscode/src/view/model-editor/ModelInputSuggestBox.tsx index 16ef1a330..b290dff43 100644 --- a/extensions/ql-vscode/src/view/model-editor/ModelInputSuggestBox.tsx +++ b/extensions/ql-vscode/src/view/model-editor/ModelInputSuggestBox.tsx @@ -4,7 +4,6 @@ import { calculateNewProvenance, modeledMethodSupportsInput, } from "../../model-editor/modeled-method"; -import { ReadonlyDropdown } from "../common/ReadonlyDropdown"; import type { AccessPathOption } from "../../model-editor/suggestions"; import { SuggestBox } from "../common/SuggestBox"; import { useDebounceCallback } from "../common/useDebounceCallback"; @@ -14,10 +13,12 @@ import { validateAccessPath, } from "../../model-editor/shared/access-paths"; import { ModelSuggestionIcon } from "./ModelSuggestionIcon"; +import { ModelTypePathSuggestBox } from "./ModelTypePathSuggestBox"; type Props = { modeledMethod: ModeledMethod | undefined; suggestions: AccessPathOption[]; + typePathSuggestions: AccessPathOption[]; onChange: (modeledMethod: ModeledMethod) => void; }; @@ -33,6 +34,7 @@ const getDetails = (option: AccessPathOption) => option.details; export const ModelInputSuggestBox = ({ modeledMethod, suggestions, + typePathSuggestions, onChange, }: Props) => { const [value, setValue] = useState( @@ -75,7 +77,13 @@ export const ModelInputSuggestBox = ({ ); if (modeledMethod?.type === "type") { - return ; + return ( + + ); } return ( diff --git a/extensions/ql-vscode/src/view/model-editor/ModelOutputSuggestBox.tsx b/extensions/ql-vscode/src/view/model-editor/ModelOutputSuggestBox.tsx index d7ebfc4dd..0aeb3d6de 100644 --- a/extensions/ql-vscode/src/view/model-editor/ModelOutputSuggestBox.tsx +++ b/extensions/ql-vscode/src/view/model-editor/ModelOutputSuggestBox.tsx @@ -4,7 +4,6 @@ import { calculateNewProvenance, modeledMethodSupportsOutput, } from "../../model-editor/modeled-method"; -import { ReadonlyDropdown } from "../common/ReadonlyDropdown"; import type { AccessPathOption } from "../../model-editor/suggestions"; import { SuggestBox } from "../common/SuggestBox"; import { useDebounceCallback } from "../common/useDebounceCallback"; @@ -14,6 +13,7 @@ import { validateAccessPath, } from "../../model-editor/shared/access-paths"; import { ModelSuggestionIcon } from "./ModelSuggestionIcon"; +import { ModelTypeTextbox } from "./ModelTypeTextbox"; type Props = { modeledMethod: ModeledMethod | undefined; @@ -76,8 +76,10 @@ export const ModelOutputSuggestBox = ({ if (modeledMethod?.type === "type") { return ( - ); diff --git a/extensions/ql-vscode/src/view/model-editor/ModelTypePathSuggestBox.tsx b/extensions/ql-vscode/src/view/model-editor/ModelTypePathSuggestBox.tsx new file mode 100644 index 000000000..044c65766 --- /dev/null +++ b/extensions/ql-vscode/src/view/model-editor/ModelTypePathSuggestBox.tsx @@ -0,0 +1,68 @@ +import { useEffect, useState } from "react"; +import type { TypeModeledMethod } from "../../model-editor/modeled-method"; +import type { AccessPathOption } from "../../model-editor/suggestions"; +import { SuggestBox } from "../common/SuggestBox"; +import { useDebounceCallback } from "../common/useDebounceCallback"; +import type { AccessPathDiagnostic } from "../../model-editor/shared/access-paths"; +import { + parseAccessPathTokens, + validateAccessPath, +} from "../../model-editor/shared/access-paths"; +import { ModelSuggestionIcon } from "./ModelSuggestionIcon"; + +type Props = { + modeledMethod: TypeModeledMethod; + suggestions: AccessPathOption[]; + onChange: (modeledMethod: TypeModeledMethod) => void; +}; + +const parseValueToTokens = (value: string) => + parseAccessPathTokens(value).map((t) => t.text); + +const getIcon = (option: AccessPathOption) => ( + +); + +const getDetails = (option: AccessPathOption) => option.details; + +export const ModelTypePathSuggestBox = ({ + modeledMethod, + suggestions, + onChange, +}: Props) => { + const [value, setValue] = useState(modeledMethod.path); + + useEffect(() => { + setValue(modeledMethod.path); + }, [modeledMethod]); + + // Debounce the callback to avoid updating the model too often. + // Not doing this results in a lot of lag when typing. + useDebounceCallback( + value, + (path: string | undefined) => { + if (path === undefined) { + return; + } + + onChange({ + ...modeledMethod, + path, + }); + }, + 500, + ); + + return ( + + value={value} + options={suggestions} + onChange={setValue} + parseValueToTokens={parseValueToTokens} + validateValue={validateAccessPath} + getIcon={getIcon} + getDetails={getDetails} + aria-label="Path" + /> + ); +};