From d30ca71585383897343a1767777dd2ed3b676f95 Mon Sep 17 00:00:00 2001 From: Robert Date: Mon, 17 Apr 2023 15:24:56 +0100 Subject: [PATCH 01/28] Add FilterKey type --- .../ql-vscode/src/pure/variant-analysis-filter-sort.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts b/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts index 93fe0504c..35553f2e7 100644 --- a/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts +++ b/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts @@ -4,6 +4,11 @@ import { } from "../variant-analysis/shared/repository"; import { parseDate } from "./date"; +export enum FilterKey { + All = "all", + WithResults = "withResults", +} + export enum SortKey { Name = "name", Stars = "stars", @@ -13,6 +18,7 @@ export enum SortKey { export type RepositoriesFilterSortState = { searchValue: string; + filterKey: FilterKey; sortKey: SortKey; }; @@ -22,6 +28,7 @@ export type RepositoriesFilterSortStateWithIds = RepositoriesFilterSortState & { export const defaultFilterSortState: RepositoriesFilterSortState = { searchValue: "", + filterKey: FilterKey.All, sortKey: SortKey.Name, }; From 563720b1af6cac91e33654ece9cdbd3e68fe8858 Mon Sep 17 00:00:00 2001 From: Robert Date: Mon, 17 Apr 2023 15:23:36 +0100 Subject: [PATCH 02/28] Rename SortableResult => FilterAndSortableResult --- .../src/pure/variant-analysis-filter-sort.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts b/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts index 35553f2e7..ff13107aa 100644 --- a/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts +++ b/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts @@ -78,17 +78,17 @@ export function compareRepository( }; } -type SortableResult = { +type FilterAndSortableResult = { repository: SortableRepository & Pick; resultCount?: number; }; export function compareWithResults( filterSortState: RepositoriesFilterSortState | undefined, -): (left: SortableResult, right: SortableResult) => number { +): (left: FilterAndSortableResult, right: FilterAndSortableResult) => number { const fallbackSort = compareRepository(filterSortState); - return (left: SortableResult, right: SortableResult) => { + return (left: FilterAndSortableResult, right: FilterAndSortableResult) => { // Highest to lowest if (filterSortState?.sortKey === SortKey.ResultsCount) { const resultCount = (right.resultCount ?? 0) - (left.resultCount ?? 0); @@ -102,7 +102,7 @@ export function compareWithResults( } export function filterAndSortRepositoriesWithResultsByName< - T extends SortableResult, + T extends FilterAndSortableResult, >( repositories: T[] | undefined, filterSortState: RepositoriesFilterSortState | undefined, @@ -116,7 +116,9 @@ export function filterAndSortRepositoriesWithResultsByName< .sort(compareWithResults(filterSortState)); } -export function filterAndSortRepositoriesWithResults( +export function filterAndSortRepositoriesWithResults< + T extends FilterAndSortableResult, +>( repositories: T[] | undefined, filterSortState: RepositoriesFilterSortStateWithIds | undefined, ): T[] | undefined { From 18d7c898a6dd638a16c443898ebc9841f5e0c903 Mon Sep 17 00:00:00 2001 From: Robert Date: Mon, 17 Apr 2023 15:44:40 +0100 Subject: [PATCH 03/28] Split out FilterAndSortableResultWithIds to mirror RepositoriesFilterSortStateWithIds --- .../ql-vscode/src/pure/variant-analysis-filter-sort.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts b/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts index ff13107aa..fb1a8437b 100644 --- a/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts +++ b/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts @@ -79,6 +79,11 @@ export function compareRepository( } type FilterAndSortableResult = { + repository: SortableRepository; + resultCount?: number; +}; + +type FilterAndSortableResultWithIds = { repository: SortableRepository & Pick; resultCount?: number; }; @@ -117,7 +122,7 @@ export function filterAndSortRepositoriesWithResultsByName< } export function filterAndSortRepositoriesWithResults< - T extends FilterAndSortableResult, + T extends FilterAndSortableResultWithIds, >( repositories: T[] | undefined, filterSortState: RepositoriesFilterSortStateWithIds | undefined, From 049b4c27cca3370a5f76e6e1456e8105416a9b2c Mon Sep 17 00:00:00 2001 From: Robert Date: Mon, 17 Apr 2023 15:46:53 +0100 Subject: [PATCH 04/28] Change matchesFilter to accept a FilterAndSortableResult --- .../src/pure/variant-analysis-filter-sort.ts | 6 +++--- .../VariantAnalysisSkippedRepositoriesTab.tsx | 4 ++-- .../unit-tests/variant-analysis-filter-sort.test.ts | 11 +++++++---- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts b/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts index fb1a8437b..d9de3fd71 100644 --- a/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts +++ b/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts @@ -33,14 +33,14 @@ export const defaultFilterSortState: RepositoriesFilterSortState = { }; export function matchesFilter( - repo: Pick, + item: FilterAndSortableResult, filterSortState: RepositoriesFilterSortState | undefined, ): boolean { if (!filterSortState) { return true; } - return repo.fullName + return item.repository.fullName .toLowerCase() .includes(filterSortState.searchValue.toLowerCase()); } @@ -117,7 +117,7 @@ export function filterAndSortRepositoriesWithResultsByName< } return repositories - .filter((repo) => matchesFilter(repo.repository, filterSortState)) + .filter((repo) => matchesFilter(repo, filterSortState)) .sort(compareWithResults(filterSortState)); } diff --git a/extensions/ql-vscode/src/view/variant-analysis/VariantAnalysisSkippedRepositoriesTab.tsx b/extensions/ql-vscode/src/view/variant-analysis/VariantAnalysisSkippedRepositoriesTab.tsx index 84d19c4c6..61b07bd3d 100644 --- a/extensions/ql-vscode/src/view/variant-analysis/VariantAnalysisSkippedRepositoriesTab.tsx +++ b/extensions/ql-vscode/src/view/variant-analysis/VariantAnalysisSkippedRepositoriesTab.tsx @@ -56,8 +56,8 @@ export const VariantAnalysisSkippedRepositoriesTab = ({ }: VariantAnalysisSkippedRepositoriesTabProps) => { const repositories = useMemo(() => { return skippedRepositoryGroup.repositories - ?.filter((repo) => { - return matchesFilter(repo, filterSortState); + ?.filter((repository) => { + return matchesFilter({ repository }, filterSortState); }) ?.sort(compareRepository(filterSortState)); }, [filterSortState, skippedRepositoryGroup.repositories]); diff --git a/extensions/ql-vscode/test/unit-tests/variant-analysis-filter-sort.test.ts b/extensions/ql-vscode/test/unit-tests/variant-analysis-filter-sort.test.ts index 601ddaaeb..2492907c6 100644 --- a/extensions/ql-vscode/test/unit-tests/variant-analysis-filter-sort.test.ts +++ b/extensions/ql-vscode/test/unit-tests/variant-analysis-filter-sort.test.ts @@ -32,10 +32,13 @@ describe(matchesFilter.name, () => { "returns $matches if searching for $searchValue", ({ searchValue, matches }) => { expect( - matchesFilter(repository, { - ...defaultFilterSortState, - searchValue, - }), + matchesFilter( + { repository }, + { + ...defaultFilterSortState, + searchValue, + }, + ), ).toBe(matches); }, ); From 72c07a397f48698de060893d2d3961372a7d0915 Mon Sep 17 00:00:00 2001 From: Robert Date: Mon, 17 Apr 2023 15:47:26 +0100 Subject: [PATCH 05/28] Implement support for new filterKey --- .../src/pure/variant-analysis-filter-sort.ts | 29 +++++++++++++++++-- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts b/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts index d9de3fd71..4da5f1819 100644 --- a/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts +++ b/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts @@ -3,6 +3,7 @@ import { RepositoryWithMetadata, } from "../variant-analysis/shared/repository"; import { parseDate } from "./date"; +import { assertNever } from "./helpers-pure"; export enum FilterKey { All = "all", @@ -40,9 +41,31 @@ export function matchesFilter( return true; } - return item.repository.fullName - .toLowerCase() - .includes(filterSortState.searchValue.toLowerCase()); + return ( + matchesSearch(item.repository, filterSortState.searchValue) && + matchesFilterKey(item.resultCount, filterSortState.filterKey) + ); +} + +function matchesSearch( + repository: SortableRepository, + searchValue: string, +): boolean { + return repository.fullName.toLowerCase().includes(searchValue.toLowerCase()); +} + +function matchesFilterKey( + resultCount: number | undefined, + filterKey: FilterKey, +): boolean { + switch (filterKey) { + case FilterKey.All: + return true; + case FilterKey.WithResults: + return resultCount !== undefined && resultCount > 0; + default: + assertNever(filterKey); + } } type SortableRepository = Pick & From 0685218c6a5f0be88bda7009001d6c409f12fbd1 Mon Sep 17 00:00:00 2001 From: Robert Date: Mon, 17 Apr 2023 16:23:37 +0100 Subject: [PATCH 06/28] Clarify some behaviour in filterAndSortRepositoriesWithResults --- extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts b/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts index 4da5f1819..0030fd44b 100644 --- a/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts +++ b/extensions/ql-vscode/src/pure/variant-analysis-filter-sort.ts @@ -154,6 +154,7 @@ export function filterAndSortRepositoriesWithResults< return undefined; } + // If repository IDs are given, then ignore the search value and filter key if ( filterSortState?.repositoryIds && filterSortState.repositoryIds.length > 0 From 2701cd4824b58dcc029fffaea53190234a68890a Mon Sep 17 00:00:00 2001 From: Robert Date: Mon, 17 Apr 2023 16:28:15 +0100 Subject: [PATCH 07/28] Add tests of the new filter key --- .../variant-analysis-filter-sort.test.ts | 126 +++++++++++++++++- 1 file changed, 121 insertions(+), 5 deletions(-) diff --git a/extensions/ql-vscode/test/unit-tests/variant-analysis-filter-sort.test.ts b/extensions/ql-vscode/test/unit-tests/variant-analysis-filter-sort.test.ts index 2492907c6..509ba8260 100644 --- a/extensions/ql-vscode/test/unit-tests/variant-analysis-filter-sort.test.ts +++ b/extensions/ql-vscode/test/unit-tests/variant-analysis-filter-sort.test.ts @@ -4,6 +4,7 @@ import { defaultFilterSortState, filterAndSortRepositoriesWithResults, filterAndSortRepositoriesWithResultsByName, + FilterKey, matchesFilter, SortKey, } from "../../src/pure/variant-analysis-filter-sort"; @@ -42,6 +43,60 @@ describe(matchesFilter.name, () => { ).toBe(matches); }, ); + + it("returns true if filterKey is all and resultCount is positive", () => { + expect( + matchesFilter( + { repository, resultCount: 1 }, + { ...defaultFilterSortState, filterKey: FilterKey.All }, + ), + ).toBe(true); + }); + + it("returns true if filterKey is all and resultCount is zero", () => { + expect( + matchesFilter( + { repository, resultCount: 0 }, + { ...defaultFilterSortState, filterKey: FilterKey.All }, + ), + ).toBe(true); + }); + + it("returns true if filterKey is all and resultCount is undefined", () => { + expect( + matchesFilter( + { repository }, + { ...defaultFilterSortState, filterKey: FilterKey.All }, + ), + ).toBe(true); + }); + + it("returns true if filterKey is withResults and resultCount is positive", () => { + expect( + matchesFilter( + { repository, resultCount: 1 }, + { ...defaultFilterSortState, filterKey: FilterKey.WithResults }, + ), + ).toBe(true); + }); + + it("returns false if filterKey is withResults and resultCount is zero", () => { + expect( + matchesFilter( + { repository, resultCount: 0 }, + { ...defaultFilterSortState, filterKey: FilterKey.WithResults }, + ), + ).toBe(false); + }); + + it("returns false if filterKey is withResults and resultCount is undefined", () => { + expect( + matchesFilter( + { repository }, + { ...defaultFilterSortState, filterKey: FilterKey.WithResults }, + ), + ).toBe(false); + }); }); describe(compareRepository.name, () => { @@ -352,7 +407,7 @@ describe(filterAndSortRepositoriesWithResultsByName.name, () => { }, ]; - describe("when sort key is given without filter", () => { + describe("when sort key is given without search or filter", () => { it("returns the correct results", () => { expect( filterAndSortRepositoriesWithResultsByName(repositories, { @@ -368,7 +423,7 @@ describe(filterAndSortRepositoriesWithResultsByName.name, () => { }); }); - describe("when sort key and search filter are given", () => { + describe("when sort key and search are given without filter", () => { it("returns the correct results", () => { expect( filterAndSortRepositoriesWithResultsByName(repositories, { @@ -379,6 +434,30 @@ describe(filterAndSortRepositoriesWithResultsByName.name, () => { ).toEqual([repositories[2], repositories[0]]); }); }); + + describe("when sort key and filter withResults are given without search", () => { + it("returns the correct results", () => { + expect( + filterAndSortRepositoriesWithResultsByName(repositories, { + ...defaultFilterSortState, + sortKey: SortKey.ResultsCount, + filterKey: FilterKey.WithResults, + }), + ).toEqual([repositories[3], repositories[2], repositories[0]]); + }); + }); + + describe("when sort key and search and filter withResults are given", () => { + it("returns the correct results", () => { + expect( + filterAndSortRepositoriesWithResultsByName(repositories, { + sortKey: SortKey.ResultsCount, + filterKey: FilterKey.WithResults, + searchValue: "r", + }), + ).toEqual([repositories[3]]); + }); + }); }); describe(filterAndSortRepositoriesWithResults.name, () => { @@ -413,7 +492,7 @@ describe(filterAndSortRepositoriesWithResults.name, () => { }, ]; - describe("when sort key is given without filter", () => { + describe("when sort key is given", () => { it("returns the correct results", () => { expect( filterAndSortRepositoriesWithResults(repositories, { @@ -429,7 +508,7 @@ describe(filterAndSortRepositoriesWithResults.name, () => { }); }); - describe("when sort key and search filter are given", () => { + describe("when sort key and search are given", () => { it("returns the correct results", () => { expect( filterAndSortRepositoriesWithResults(repositories, { @@ -441,12 +520,49 @@ describe(filterAndSortRepositoriesWithResults.name, () => { }); }); - describe("when sort key, search filter, and repository ids are given", () => { + describe("when sort key and filter withResults are given", () => { it("returns the correct results", () => { expect( filterAndSortRepositoriesWithResults(repositories, { ...defaultFilterSortState, sortKey: SortKey.ResultsCount, + filterKey: FilterKey.WithResults, + }), + ).toEqual([repositories[3], repositories[2], repositories[0]]); + }); + }); + + describe("when sort key and filter withResults are given", () => { + it("returns the correct results", () => { + expect( + filterAndSortRepositoriesWithResults(repositories, { + ...defaultFilterSortState, + sortKey: SortKey.ResultsCount, + filterKey: FilterKey.WithResults, + }), + ).toEqual([repositories[3], repositories[2], repositories[0]]); + }); + }); + + describe("when sort key and search and filter withResults are given", () => { + it("returns the correct results", () => { + expect( + filterAndSortRepositoriesWithResults(repositories, { + ...defaultFilterSortState, + sortKey: SortKey.ResultsCount, + filterKey: FilterKey.WithResults, + searchValue: "r", + }), + ).toEqual([repositories[3]]); + }); + }); + + describe("when sort key, search, filter withResults, and repository ids are given", () => { + it("returns the correct results", () => { + expect( + filterAndSortRepositoriesWithResults(repositories, { + sortKey: SortKey.ResultsCount, + filterKey: FilterKey.WithResults, searchValue: "la", repositoryIds: [ repositories[1].repository.id, From 7fb9975cfaefcb38c6b0c7dc13ae6a83039aa26b Mon Sep 17 00:00:00 2001 From: Robert Date: Mon, 17 Apr 2023 15:25:28 +0100 Subject: [PATCH 08/28] Add UI for repository filter component --- .../variant-analysis/RepositoriesFilter.tsx | 36 +++++++++++++++++++ .../RepositoriesSearchSortRow.tsx | 20 +++++++++++ 2 files changed, 56 insertions(+) create mode 100644 extensions/ql-vscode/src/view/variant-analysis/RepositoriesFilter.tsx diff --git a/extensions/ql-vscode/src/view/variant-analysis/RepositoriesFilter.tsx b/extensions/ql-vscode/src/view/variant-analysis/RepositoriesFilter.tsx new file mode 100644 index 000000000..b75c5bf30 --- /dev/null +++ b/extensions/ql-vscode/src/view/variant-analysis/RepositoriesFilter.tsx @@ -0,0 +1,36 @@ +import * as React from "react"; +import { useCallback } from "react"; +import styled from "styled-components"; +import { VSCodeDropdown, VSCodeOption } from "@vscode/webview-ui-toolkit/react"; +import { Codicon } from "../common"; +import { FilterKey } from "../../pure/variant-analysis-filter-sort"; + +const Dropdown = styled(VSCodeDropdown)` + width: 100%; +`; + +type Props = { + value: FilterKey; + onChange: (value: FilterKey) => void; + + className?: string; +}; + +export const RepositoriesFilter = ({ value, onChange, className }: Props) => { + const handleInput = useCallback( + (e: InputEvent) => { + const target = e.target as HTMLSelectElement; + + onChange(target.value as FilterKey); + }, + [onChange], + ); + + return ( + + + All + With results + + ); +}; diff --git a/extensions/ql-vscode/src/view/variant-analysis/RepositoriesSearchSortRow.tsx b/extensions/ql-vscode/src/view/variant-analysis/RepositoriesSearchSortRow.tsx index 9d1b1fadf..229b6900d 100644 --- a/extensions/ql-vscode/src/view/variant-analysis/RepositoriesSearchSortRow.tsx +++ b/extensions/ql-vscode/src/view/variant-analysis/RepositoriesSearchSortRow.tsx @@ -2,11 +2,13 @@ import * as React from "react"; import { Dispatch, SetStateAction, useCallback } from "react"; import styled from "styled-components"; import { + FilterKey, RepositoriesFilterSortState, SortKey, } from "../../pure/variant-analysis-filter-sort"; import { RepositoriesSearch } from "./RepositoriesSearch"; import { RepositoriesSort } from "./RepositoriesSort"; +import { RepositoriesFilter } from "./RepositoriesFilter"; type Props = { value: RepositoriesFilterSortState; @@ -25,6 +27,10 @@ const RepositoriesSearchColumn = styled(RepositoriesSearch)` flex: 3; `; +const RepositoriesFilterColumn = styled(RepositoriesFilter)` + flex: 1; +`; + const RepositoriesSortColumn = styled(RepositoriesSort)` flex: 1; `; @@ -40,6 +46,16 @@ export const RepositoriesSearchSortRow = ({ value, onChange }: Props) => { [onChange], ); + const handleFilterKeyChange = useCallback( + (filterKey: FilterKey) => { + onChange((oldValue) => ({ + ...oldValue, + filterKey, + })); + }, + [onChange], + ); + const handleSortKeyChange = useCallback( (sortKey: SortKey) => { onChange((oldValue) => ({ @@ -56,6 +72,10 @@ export const RepositoriesSearchSortRow = ({ value, onChange }: Props) => { value={value.searchValue} onChange={handleSearchValueChange} /> + Date: Mon, 17 Apr 2023 15:20:44 +0100 Subject: [PATCH 09/28] Add storybook for repository filter component --- .../RepositoriesFilter.stories.tsx | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 extensions/ql-vscode/src/stories/variant-analysis/RepositoriesFilter.stories.tsx diff --git a/extensions/ql-vscode/src/stories/variant-analysis/RepositoriesFilter.stories.tsx b/extensions/ql-vscode/src/stories/variant-analysis/RepositoriesFilter.stories.tsx new file mode 100644 index 000000000..8110fba14 --- /dev/null +++ b/extensions/ql-vscode/src/stories/variant-analysis/RepositoriesFilter.stories.tsx @@ -0,0 +1,25 @@ +import * as React from "react"; +import { useState } from "react"; + +import { ComponentMeta } from "@storybook/react"; + +import { RepositoriesFilter as RepositoriesFilterComponent } from "../../view/variant-analysis/RepositoriesFilter"; +import { FilterKey } from "../../pure/variant-analysis-filter-sort"; + +export default { + title: "Variant Analysis/Repositories Filter", + component: RepositoriesFilterComponent, + argTypes: { + value: { + control: { + disable: true, + }, + }, + }, +} as ComponentMeta; + +export const RepositoriesFilter = () => { + const [value, setValue] = useState(FilterKey.All); + + return ; +}; From d3ef29410b7a8714b9c98bdd56d35ebcddfe0ae1 Mon Sep 17 00:00:00 2001 From: Elena Tanasoiu Date: Fri, 14 Apr 2023 12:09:22 +0000 Subject: [PATCH 10/28] Extract finding existing database item into separate method --- .../ql-vscode/src/skeleton-query-wizard.ts | 48 +++++++++---------- 1 file changed, 23 insertions(+), 25 deletions(-) diff --git a/extensions/ql-vscode/src/skeleton-query-wizard.ts b/extensions/ql-vscode/src/skeleton-query-wizard.ts index 0c5ddca9f..2f57d08b6 100644 --- a/extensions/ql-vscode/src/skeleton-query-wizard.ts +++ b/extensions/ql-vscode/src/skeleton-query-wizard.ts @@ -217,40 +217,18 @@ export class SkeletonQueryWizard { } private async selectExistingDatabase() { - if (this.language === undefined) { - throw new Error("Language is undefined"); - } - if (this.qlPackStoragePath === undefined) { throw new Error("QL Pack storage path is undefined"); } - const databaseNwo = QUERY_LANGUAGE_TO_DATABASE_REPO[this.language]; - - const existingDatabaseItem = await this.findDatabaseItemByNwo( - this.language, - databaseNwo, - this.databaseManager.databaseItems, - ); + const existingDatabaseItem = await this.findExistingDatabaseItem(); if (existingDatabaseItem) { // select the found database await this.databaseManager.setCurrentDatabaseItem(existingDatabaseItem); } else { - const sameLanguageDatabaseItem = await this.findDatabaseItemByLanguage( - this.language, - this.databaseManager.databaseItems, - ); - - if (sameLanguageDatabaseItem) { - // select the found database - await this.databaseManager.setCurrentDatabaseItem( - sameLanguageDatabaseItem, - ); - } else { - // download new database and select it - await this.downloadDatabase(); - } + // download new database and select it + await this.downloadDatabase(); } } @@ -286,4 +264,24 @@ export class SkeletonQueryWizard { } return dbs[0]; } + + private async findExistingDatabaseItem() { + if (this.language === undefined) { + throw new Error("Language is undefined"); + } + + const databaseNwo = QUERY_LANGUAGE_TO_DATABASE_REPO[this.language]; + + return ( + (await this.findDatabaseItemByNwo( + this.language, + databaseNwo, + this.databaseManager.databaseItems, + )) || + (await this.findDatabaseItemByLanguage( + this.language, + this.databaseManager.databaseItems, + )) + ); + } } From a5fc96397c7abed64ebc605770ade94789556d0d Mon Sep 17 00:00:00 2001 From: Elena Tanasoiu Date: Fri, 14 Apr 2023 12:14:37 +0000 Subject: [PATCH 11/28] Rename `selectExistingDatabase` -> `selectOrDownloadDatabase` Make it clear that this method can also download a database if it fails to find one. --- extensions/ql-vscode/src/skeleton-query-wizard.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extensions/ql-vscode/src/skeleton-query-wizard.ts b/extensions/ql-vscode/src/skeleton-query-wizard.ts index 2f57d08b6..bc5d6ae39 100644 --- a/extensions/ql-vscode/src/skeleton-query-wizard.ts +++ b/extensions/ql-vscode/src/skeleton-query-wizard.ts @@ -65,7 +65,7 @@ export class SkeletonQueryWizard { // just create a new example query file in skeleton QL pack await this.createExampleFile(); // select existing database for language - await this.selectExistingDatabase(); + await this.selectOrDownloadDatabase(); } else { // generate a new skeleton QL pack with query file await this.createQlPack(); @@ -216,7 +216,7 @@ export class SkeletonQueryWizard { ); } - private async selectExistingDatabase() { + private async selectOrDownloadDatabase() { if (this.qlPackStoragePath === undefined) { throw new Error("QL Pack storage path is undefined"); } From 34d4fd4d41721cc72bd4a6649d7720d4aba4921c Mon Sep 17 00:00:00 2001 From: Elena Tanasoiu Date: Fri, 14 Apr 2023 12:16:11 +0000 Subject: [PATCH 12/28] Always check for an existing database This fixes an issue where we would download a database every time we create a new QL pack. Sometimes, a database is already available so let's always check for it, regardless of the existence of a QL pack. --- extensions/ql-vscode/src/skeleton-query-wizard.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/extensions/ql-vscode/src/skeleton-query-wizard.ts b/extensions/ql-vscode/src/skeleton-query-wizard.ts index bc5d6ae39..72fb3de13 100644 --- a/extensions/ql-vscode/src/skeleton-query-wizard.ts +++ b/extensions/ql-vscode/src/skeleton-query-wizard.ts @@ -64,15 +64,14 @@ export class SkeletonQueryWizard { if (skeletonPackAlreadyExists) { // just create a new example query file in skeleton QL pack await this.createExampleFile(); - // select existing database for language - await this.selectOrDownloadDatabase(); } else { // generate a new skeleton QL pack with query file await this.createQlPack(); - // download database based on language and select it - await this.downloadDatabase(); } + // select existing database for language or download a new one + await this.selectOrDownloadDatabase(); + // open a query file try { From 23abc5de0d32ca9d97cfb6f64b7dbbdb4e3ebb73 Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Tue, 18 Apr 2023 15:47:18 +0200 Subject: [PATCH 13/28] Hide link when model file does not exist This removes the link to the model file when it does not exist. It will still show the filename of the model file. When clicking on "Apply", it will refresh whether the model file exists after writing the file. --- .../data-extensions-editor-view.ts | 20 ++++--- .../extension-pack-picker.ts | 17 +----- .../shared/extension-pack.ts | 15 +++++ .../shared/view-state.ts | 6 ++ .../ql-vscode/src/pure/interface-types.ts | 10 ++-- .../DataExtensionsEditor.stories.tsx | 19 +++++- .../DataExtensionsEditor.tsx | 59 +++++++++++-------- .../extension-pack-picker.test.ts | 6 +- 8 files changed, 93 insertions(+), 59 deletions(-) create mode 100644 extensions/ql-vscode/src/data-extensions-editor/shared/extension-pack.ts create mode 100644 extensions/ql-vscode/src/data-extensions-editor/shared/view-state.ts diff --git a/extensions/ql-vscode/src/data-extensions-editor/data-extensions-editor-view.ts b/extensions/ql-vscode/src/data-extensions-editor/data-extensions-editor-view.ts index 508641c1b..e58d6c21d 100644 --- a/extensions/ql-vscode/src/data-extensions-editor/data-extensions-editor-view.ts +++ b/extensions/ql-vscode/src/data-extensions-editor/data-extensions-editor-view.ts @@ -35,7 +35,7 @@ import { readQueryResults, runQuery } from "./external-api-usage-query"; import { createDataExtensionYaml, loadDataExtensionYaml } from "./yaml"; import { ExternalApiUsage } from "./external-api-usage"; import { ModeledMethod } from "./modeled-method"; -import { ExtensionPackModelFile } from "./extension-pack-picker"; +import { ExtensionPackModelFile } from "./shared/extension-pack"; function getQlSubmoduleFolder(): WorkspaceFolder | undefined { const workspaceFolder = workspace.workspaceFolders?.find( @@ -118,7 +118,7 @@ export class DataExtensionsEditorView extends AbstractWebview< msg.externalApiUsages, msg.modeledMethods, ); - await this.loadExternalApiUsages(); + await Promise.all([this.setViewState(), this.loadExternalApiUsages()]); break; case "generateExternalApi": @@ -134,16 +134,22 @@ export class DataExtensionsEditorView extends AbstractWebview< super.onWebViewLoaded(); await Promise.all([ - this.postMessage({ - t: "setDataExtensionEditorInitialData", - extensionPackName: this.modelFile.extensionPack.name, - modelFilename: this.modelFile.filename, - }), + this.setViewState(), this.loadExternalApiUsages(), this.loadExistingModeledMethods(), ]); } + private async setViewState(): Promise { + await this.postMessage({ + t: "setDataExtensionEditorViewState", + viewState: { + extensionPackModelFile: this.modelFile, + modelFileExists: await pathExists(this.modelFile.filename), + }, + }); + } + protected async jumpToUsage( location: ResolvableLocationValue, ): Promise { diff --git a/extensions/ql-vscode/src/data-extensions-editor/extension-pack-picker.ts b/extensions/ql-vscode/src/data-extensions-editor/extension-pack-picker.ts index b0d102c6e..dc2c4969a 100644 --- a/extensions/ql-vscode/src/data-extensions-editor/extension-pack-picker.ts +++ b/extensions/ql-vscode/src/data-extensions-editor/extension-pack-picker.ts @@ -13,6 +13,7 @@ import { ProgressCallback } from "../progress"; import { DatabaseItem } from "../local-databases"; import { getQlPackPath, QLPACK_FILENAMES } from "../pure/ql"; import { getErrorMessage } from "../pure/helpers-pure"; +import { ExtensionPack, ExtensionPackModelFile } from "./shared/extension-pack"; const maxStep = 3; @@ -22,22 +23,6 @@ const packNameRegex = new RegExp( ); const packNameLength = 128; -export interface ExtensionPack { - path: string; - yamlPath: string; - - name: string; - version: string; - - extensionTargets: Record; - dataExtensions: string[]; -} - -export interface ExtensionPackModelFile { - filename: string; - extensionPack: ExtensionPack; -} - export async function pickExtensionPackModelFile( cliServer: Pick, databaseItem: Pick, diff --git a/extensions/ql-vscode/src/data-extensions-editor/shared/extension-pack.ts b/extensions/ql-vscode/src/data-extensions-editor/shared/extension-pack.ts new file mode 100644 index 000000000..7ac800993 --- /dev/null +++ b/extensions/ql-vscode/src/data-extensions-editor/shared/extension-pack.ts @@ -0,0 +1,15 @@ +export interface ExtensionPack { + path: string; + yamlPath: string; + + name: string; + version: string; + + extensionTargets: Record; + dataExtensions: string[]; +} + +export interface ExtensionPackModelFile { + filename: string; + extensionPack: ExtensionPack; +} diff --git a/extensions/ql-vscode/src/data-extensions-editor/shared/view-state.ts b/extensions/ql-vscode/src/data-extensions-editor/shared/view-state.ts new file mode 100644 index 000000000..0da3f0d25 --- /dev/null +++ b/extensions/ql-vscode/src/data-extensions-editor/shared/view-state.ts @@ -0,0 +1,6 @@ +import { ExtensionPackModelFile } from "./extension-pack"; + +export interface DataExtensionEditorViewState { + extensionPackModelFile: ExtensionPackModelFile; + modelFileExists: boolean; +} diff --git a/extensions/ql-vscode/src/pure/interface-types.ts b/extensions/ql-vscode/src/pure/interface-types.ts index 6dda76926..0dcfc0fd9 100644 --- a/extensions/ql-vscode/src/pure/interface-types.ts +++ b/extensions/ql-vscode/src/pure/interface-types.ts @@ -16,6 +16,7 @@ import { ErrorLike } from "./errors"; import { DataFlowPaths } from "../variant-analysis/shared/data-flow-paths"; import { ExternalApiUsage } from "../data-extensions-editor/external-api-usage"; import { ModeledMethod } from "../data-extensions-editor/modeled-method"; +import { DataExtensionEditorViewState } from "../data-extensions-editor/shared/view-state"; /** * This module contains types and code that are shared between @@ -481,10 +482,9 @@ export type ToDataFlowPathsMessage = SetDataFlowPathsMessage; export type FromDataFlowPathsMessage = CommonFromViewMessages; -export interface SetDataExtensionEditorInitialDataMessage { - t: "setDataExtensionEditorInitialData"; - extensionPackName: string; - modelFilename: string; +export interface SetExtensionPackStateMessage { + t: "setDataExtensionEditorViewState"; + viewState: DataExtensionEditorViewState; } export interface SetExternalApiUsagesMessage { @@ -536,7 +536,7 @@ export interface GenerateExternalApiMessage { } export type ToDataExtensionsEditorMessage = - | SetDataExtensionEditorInitialDataMessage + | SetExtensionPackStateMessage | SetExternalApiUsagesMessage | ShowProgressMessage | AddModeledMethodsMessage; diff --git a/extensions/ql-vscode/src/stories/data-extensions-editor/DataExtensionsEditor.stories.tsx b/extensions/ql-vscode/src/stories/data-extensions-editor/DataExtensionsEditor.stories.tsx index 46b10ff5c..3df104db7 100644 --- a/extensions/ql-vscode/src/stories/data-extensions-editor/DataExtensionsEditor.stories.tsx +++ b/extensions/ql-vscode/src/stories/data-extensions-editor/DataExtensionsEditor.stories.tsx @@ -15,9 +15,22 @@ const Template: ComponentStory = ( export const DataExtensionsEditor = Template.bind({}); DataExtensionsEditor.args = { - initialExtensionPackName: "codeql/sql2o-models", - initialModelFilename: - "/home/user/vscode-codeql-starter/codeql-custom-queries-java/sql2o/models/sql2o.yml", + initialViewState: { + extensionPackModelFile: { + extensionPack: { + path: "/home/user/vscode-codeql-starter/codeql-custom-queries-java/sql2o", + yamlPath: + "/home/user/vscode-codeql-starter/codeql-custom-queries-java/sql2o/codeql-pack.yml", + name: "codeql/sql2o-models", + version: "0.0.0", + extensionTargets: {}, + dataExtensions: [], + }, + filename: + "/home/user/vscode-codeql-starter/codeql-custom-queries-java/sql2o/models/sql2o.yml", + }, + modelFileExists: true, + }, initialExternalApiUsages: [ { signature: "org.sql2o.Connection#createQuery(String)", diff --git a/extensions/ql-vscode/src/view/data-extensions-editor/DataExtensionsEditor.tsx b/extensions/ql-vscode/src/view/data-extensions-editor/DataExtensionsEditor.tsx index 129f69f36..592625e5f 100644 --- a/extensions/ql-vscode/src/view/data-extensions-editor/DataExtensionsEditor.tsx +++ b/extensions/ql-vscode/src/view/data-extensions-editor/DataExtensionsEditor.tsx @@ -20,6 +20,7 @@ import { calculateModeledPercentage } from "./modeled"; import { LinkIconButton } from "../variant-analysis/LinkIconButton"; import { basename } from "../common/path"; import { ViewTitle } from "../common"; +import { DataExtensionEditorViewState } from "../../data-extensions-editor/shared/view-state"; const DataExtensionsEditorContainer = styled.div` margin-top: 1rem; @@ -31,6 +32,12 @@ const DetailsContainer = styled.div` align-items: center; `; +const NonExistingModelFileContainer = styled.div` + display: flex; + gap: 0.2em; + align-items: center; +`; + const EditorContainer = styled.div` margin-top: 1rem; `; @@ -47,24 +54,19 @@ const ProgressBar = styled.div` `; type Props = { - initialExtensionPackName?: string; - initialModelFilename?: string; + initialViewState?: DataExtensionEditorViewState; initialExternalApiUsages?: ExternalApiUsage[]; initialModeledMethods?: Record; }; export function DataExtensionsEditor({ - initialExtensionPackName, - initialModelFilename, + initialViewState, initialExternalApiUsages = [], initialModeledMethods = {}, }: Props): JSX.Element { - const [extensionPackName, setExtensionPackName] = useState< - string | undefined - >(initialExtensionPackName); - const [modelFilename, setModelFilename] = useState( - initialModelFilename, - ); + const [viewState, setViewState] = useState< + DataExtensionEditorViewState | undefined + >(initialViewState); const [externalApiUsages, setExternalApiUsages] = useState< ExternalApiUsage[] @@ -83,9 +85,8 @@ export function DataExtensionsEditor({ if (evt.origin === window.origin) { const msg: ToDataExtensionsEditorMessage = evt.data; switch (msg.t) { - case "setDataExtensionEditorInitialData": - setExtensionPackName(msg.extensionPackName); - setModelFilename(msg.modelFilename); + case "setDataExtensionEditorViewState": + setViewState(msg.viewState); break; case "setExternalApiUsages": setExternalApiUsages(msg.externalApiUsages); @@ -181,17 +182,27 @@ export function DataExtensionsEditor({ <> Data extensions editor - {extensionPackName && ( - - - {extensionPackName} - - )} - {modelFilename && ( - - - {basename(modelFilename)} - + {viewState?.extensionPackModelFile && ( + <> + + + {viewState.extensionPackModelFile.extensionPack.name} + + {viewState.modelFileExists ? ( + + + {basename(viewState.extensionPackModelFile.filename)} + + ) : ( + + + {basename(viewState.extensionPackModelFile.filename)} + + )} + )}
{modeledPercentage.toFixed(2)}% modeled
{unModeledPercentage.toFixed(2)}% unmodeled
diff --git a/extensions/ql-vscode/test/vscode-tests/no-workspace/data-extensions-editor/extension-pack-picker.test.ts b/extensions/ql-vscode/test/vscode-tests/no-workspace/data-extensions-editor/extension-pack-picker.test.ts index 80662b0a3..708b4a4ce 100644 --- a/extensions/ql-vscode/test/vscode-tests/no-workspace/data-extensions-editor/extension-pack-picker.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/no-workspace/data-extensions-editor/extension-pack-picker.test.ts @@ -6,10 +6,8 @@ import { dir } from "tmp-promise"; import { QlpacksInfo, ResolveExtensionsResult } from "../../../../src/cli"; import * as helpers from "../../../../src/helpers"; -import { - ExtensionPack, - pickExtensionPackModelFile, -} from "../../../../src/data-extensions-editor/extension-pack-picker"; +import { pickExtensionPackModelFile } from "../../../../src/data-extensions-editor/extension-pack-picker"; +import { ExtensionPack } from "../../../../src/data-extensions-editor/shared/extension-pack"; describe("pickExtensionPackModelFile", () => { let tmpDir: string; From 6e4124115ff49e47be404f4508ffa740ea23ba47 Mon Sep 17 00:00:00 2001 From: Elena Tanasoiu Date: Tue, 18 Apr 2023 13:12:22 +0000 Subject: [PATCH 14/28] Ensure we're selecting database in single rooted workspace After some testing of the wizard with a single rooted workspace (`github/code-scanning`) we discovered a general VSCode extension bug whereby after we download a database, we don't select it. This isn't an issue in the wizard, but it does affect us as it means we'll generate the QL pack, download the db for you but then you won't know that we haven't selected your database. So let's make sure our flow works for this case by explicitly selecting the database once it's downloaded. We've noticed by testing that we need to set the current database item before we call `addDatabaseSourceArchiveFolder()`. Once that method is called, the call to `setCurrentDatabaseItem()` is ignored. We've had to make some changes to the openDatabase() method to select a database item by default, since most places where we call `openDatabase()` also immediately select the item. There is one exception [1] in the test-runner.ts file, where we set the current database item under special conditions. For this reason, we've made the behaviour configurable and tried to add some descriptive naming to the params so that it's easy to understand what the config is doing. [1]: https://github.com/github/vscode-codeql/blob/4170e7f7a7026b3ebcc75b7509dd55f317293a4d/extensions/ql-vscode/src/test-runner.ts#L120-L124 --- extensions/ql-vscode/src/databaseFetcher.ts | 4 +++- .../ql-vscode/src/local-databases-ui.ts | 14 +++++++---- extensions/ql-vscode/src/local-databases.ts | 6 +++++ .../minimal-workspace/local-databases.test.ts | 24 ++++++++++++++++++- 4 files changed, 42 insertions(+), 6 deletions(-) diff --git a/extensions/ql-vscode/src/databaseFetcher.ts b/extensions/ql-vscode/src/databaseFetcher.ts index fafb22442..247f240f0 100644 --- a/extensions/ql-vscode/src/databaseFetcher.ts +++ b/extensions/ql-vscode/src/databaseFetcher.ts @@ -317,13 +317,15 @@ async function databaseArchiveFetcher( }); await ensureZippedSourceLocation(dbPath); + const makeSelected = true; + const item = await databaseManager.openDatabase( progress, token, Uri.file(dbPath), + makeSelected, nameOverride, ); - await databaseManager.setCurrentDatabaseItem(item); return item; } else { throw new Error("Database not found in archive."); diff --git a/extensions/ql-vscode/src/local-databases-ui.ts b/extensions/ql-vscode/src/local-databases-ui.ts index 526687d2a..798ce4c32 100644 --- a/extensions/ql-vscode/src/local-databases-ui.ts +++ b/extensions/ql-vscode/src/local-databases-ui.ts @@ -307,17 +307,20 @@ export class DatabaseUI extends DisposableObject { ); let databaseItem = this.databaseManager.findDatabaseItem(uri); - const isTutorialDatabase = true; if (databaseItem === undefined) { + const makeSelected = true; + const nameOverride = "CodeQL Tutorial Database"; + const isTutorialDatabase = true; + databaseItem = await this.databaseManager.openDatabase( progress, token, uri, - "CodeQL Tutorial Database", + makeSelected, + nameOverride, isTutorialDatabase, ); } - await this.databaseManager.setCurrentDatabaseItem(databaseItem); await this.handleTourDependencies(); } } catch (e) { @@ -758,14 +761,17 @@ export class DatabaseUI extends DisposableObject { uri: Uri, ): Promise { let databaseItem = this.databaseManager.findDatabaseItem(uri); + + const makeSelected = true; + if (databaseItem === undefined) { databaseItem = await this.databaseManager.openDatabase( progress, token, uri, + makeSelected, ); } - await this.databaseManager.setCurrentDatabaseItem(databaseItem); return databaseItem; } diff --git a/extensions/ql-vscode/src/local-databases.ts b/extensions/ql-vscode/src/local-databases.ts index 080337697..f7fe8bb25 100644 --- a/extensions/ql-vscode/src/local-databases.ts +++ b/extensions/ql-vscode/src/local-databases.ts @@ -621,6 +621,7 @@ export class DatabaseManager extends DisposableObject { progress: ProgressCallback, token: vscode.CancellationToken, uri: vscode.Uri, + makeSelected = false, displayName?: string, isTutorialDatabase?: boolean, ): Promise { @@ -629,6 +630,7 @@ export class DatabaseManager extends DisposableObject { return await this.addExistingDatabaseItem( databaseItem, progress, + makeSelected, token, isTutorialDatabase, ); @@ -643,6 +645,7 @@ export class DatabaseManager extends DisposableObject { public async addExistingDatabaseItem( databaseItem: DatabaseItem, progress: ProgressCallback, + makeSelected = true, token: vscode.CancellationToken, isTutorialDatabase?: boolean, ): Promise { @@ -652,6 +655,9 @@ export class DatabaseManager extends DisposableObject { } await this.addDatabaseItem(progress, token, databaseItem); + if (makeSelected) { + await this.setCurrentDatabaseItem(databaseItem); + } await this.addDatabaseSourceArchiveFolder(databaseItem); if (isCodespacesTemplate() && !isTutorialDatabase) { diff --git a/extensions/ql-vscode/test/vscode-tests/minimal-workspace/local-databases.test.ts b/extensions/ql-vscode/test/vscode-tests/minimal-workspace/local-databases.test.ts index 92d4c0164..40f324850 100644 --- a/extensions/ql-vscode/test/vscode-tests/minimal-workspace/local-databases.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/minimal-workspace/local-databases.test.ts @@ -708,6 +708,7 @@ describe("local databases", () => { describe("openDatabase", () => { let createSkeletonPacksSpy: jest.SpyInstance; let resolveDatabaseContentsSpy: jest.SpyInstance; + let setCurrentDatabaseItemSpy: jest.SpyInstance; let addDatabaseSourceArchiveFolderSpy: jest.SpyInstance; let mockDbItem: DatabaseItemImpl; @@ -722,6 +723,11 @@ describe("local databases", () => { .spyOn(DatabaseResolver, "resolveDatabaseContents") .mockResolvedValue({} as DatabaseContentsWithDbScheme); + setCurrentDatabaseItemSpy = jest.spyOn( + databaseManager, + "setCurrentDatabaseItem", + ); + addDatabaseSourceArchiveFolderSpy = jest.spyOn( databaseManager, "addDatabaseSourceArchiveFolder", @@ -746,6 +752,19 @@ describe("local databases", () => { expect(resolveDatabaseContentsSpy).toBeCalledTimes(1); }); + it("should set the database as the currently selected one", async () => { + const makeSelected = true; + + await databaseManager.openDatabase( + {} as ProgressCallback, + {} as CancellationToken, + mockDbItem.databaseUri, + makeSelected, + ); + + expect(setCurrentDatabaseItemSpy).toBeCalledTimes(1); + }); + it("should add database source archive folder", async () => { await databaseManager.openDatabase( {} as ProgressCallback, @@ -762,12 +781,15 @@ describe("local databases", () => { jest.spyOn(Setting.prototype, "getValue").mockReturnValue(true); const isTutorialDatabase = true; + const makeSelected = true; + const nameOverride = "CodeQL Tutorial Database"; await databaseManager.openDatabase( {} as ProgressCallback, {} as CancellationToken, mockDbItem.databaseUri, - "CodeQL Tutorial Database", + makeSelected, + nameOverride, isTutorialDatabase, ); From 28745da7b8596db5029a34682d907cc227b11aa5 Mon Sep 17 00:00:00 2001 From: Elena Tanasoiu Date: Tue, 18 Apr 2023 15:01:50 +0000 Subject: [PATCH 15/28] Replace `setCurrentDatabaseItem` with `openDatabase` --- .../ql-vscode/src/local-databases-ui.ts | 29 ++++--------------- 1 file changed, 6 insertions(+), 23 deletions(-) diff --git a/extensions/ql-vscode/src/local-databases-ui.ts b/extensions/ql-vscode/src/local-databases-ui.ts index 798ce4c32..7be4c1c7d 100644 --- a/extensions/ql-vscode/src/local-databases-ui.ts +++ b/extensions/ql-vscode/src/local-databases-ui.ts @@ -633,7 +633,7 @@ export class DatabaseUI extends DisposableObject { this.queryServer?.cliServer, ); } else { - await this.setCurrentDatabase(progress, token, uri); + await this.databaseManager.openDatabase(progress, token, uri); } } catch (e) { // rethrow and let this be handled by default error handling. @@ -755,27 +755,6 @@ export class DatabaseUI extends DisposableObject { return this.databaseManager.currentDatabaseItem; } - private async setCurrentDatabase( - progress: ProgressCallback, - token: CancellationToken, - uri: Uri, - ): Promise { - let databaseItem = this.databaseManager.findDatabaseItem(uri); - - const makeSelected = true; - - if (databaseItem === undefined) { - databaseItem = await this.databaseManager.openDatabase( - progress, - token, - uri, - makeSelected, - ); - } - - return databaseItem; - } - /** * Ask the user for a database directory. Returns the chosen database, or `undefined` if the * operation was canceled. @@ -795,7 +774,11 @@ export class DatabaseUI extends DisposableObject { if (byFolder) { const fixedUri = await this.fixDbUri(uri); // we are selecting a database folder - return await this.setCurrentDatabase(progress, token, fixedUri); + return await this.databaseManager.openDatabase( + progress, + token, + fixedUri, + ); } else { // we are selecting a database archive. Must unzip into a workspace-controlled area // before importing. From 30ebe0ab57b085b3caea210bb93144c79de4c091 Mon Sep 17 00:00:00 2001 From: Elena Tanasoiu Date: Tue, 18 Apr 2023 13:38:13 +0000 Subject: [PATCH 16/28] Don't use shorthand boolean for large method calls To make it clear what the search is doing and make the code more readable. Co-authored-by: Robert Brignull --- .../ql-vscode/src/skeleton-query-wizard.ts | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/extensions/ql-vscode/src/skeleton-query-wizard.ts b/extensions/ql-vscode/src/skeleton-query-wizard.ts index 72fb3de13..e3bfde5a5 100644 --- a/extensions/ql-vscode/src/skeleton-query-wizard.ts +++ b/extensions/ql-vscode/src/skeleton-query-wizard.ts @@ -269,18 +269,21 @@ export class SkeletonQueryWizard { throw new Error("Language is undefined"); } - const databaseNwo = QUERY_LANGUAGE_TO_DATABASE_REPO[this.language]; + const defaultDatabaseNwo = QUERY_LANGUAGE_TO_DATABASE_REPO[this.language]; - return ( - (await this.findDatabaseItemByNwo( - this.language, - databaseNwo, - this.databaseManager.databaseItems, - )) || - (await this.findDatabaseItemByLanguage( - this.language, - this.databaseManager.databaseItems, - )) + const defaultDatabaseItem = await this.findDatabaseItemByNwo( + this.language, + defaultDatabaseNwo, + this.databaseManager.databaseItems, + ); + + if (defaultDatabaseItem !== undefined) { + return defaultDatabaseItem; + } + + return await this.findDatabaseItemByLanguage( + this.language, + this.databaseManager.databaseItems, ); } } From 93a2b06b35ad19416336073957d19589dd5010b6 Mon Sep 17 00:00:00 2001 From: Elena Tanasoiu Date: Tue, 18 Apr 2023 14:13:57 +0000 Subject: [PATCH 17/28] Remove unnecessary assignment to variable This was flagged by CodeQL. --- extensions/ql-vscode/src/local-databases-ui.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extensions/ql-vscode/src/local-databases-ui.ts b/extensions/ql-vscode/src/local-databases-ui.ts index 7be4c1c7d..020fe1fab 100644 --- a/extensions/ql-vscode/src/local-databases-ui.ts +++ b/extensions/ql-vscode/src/local-databases-ui.ts @@ -306,13 +306,13 @@ export class DatabaseUI extends DisposableObject { `${workspace.workspaceFolders[0].uri}/.tours/codeql-tutorial-database`, ); - let databaseItem = this.databaseManager.findDatabaseItem(uri); + const databaseItem = this.databaseManager.findDatabaseItem(uri); if (databaseItem === undefined) { const makeSelected = true; const nameOverride = "CodeQL Tutorial Database"; const isTutorialDatabase = true; - databaseItem = await this.databaseManager.openDatabase( + await this.databaseManager.openDatabase( progress, token, uri, From 57d48a7b046b29e8334833e928a547dfb7b1929e Mon Sep 17 00:00:00 2001 From: Elena Tanasoiu Date: Mon, 17 Apr 2023 14:45:29 +0000 Subject: [PATCH 18/28] Prompt non-codespace users for storage path Offer non-codespace users the option to configure their storage folder for skeleton packs. Suggested here: https://github.com/github/vscode-codeql/pull/2310#issuecomment-1507428217 At the moment we're choosing to create our skeleton packs in the first folder in the workspace. This is fine for the codespace template because we can control the folder structure in that repo. For users outside of this we'd like to offer them the option to choose where to save their skeleton packs. --- extensions/ql-vscode/src/config.ts | 16 +++ .../ql-vscode/src/skeleton-query-wizard.ts | 41 +++++- .../skeleton-query-wizard.test.ts | 117 ++++++++++++++++++ 3 files changed, 172 insertions(+), 2 deletions(-) diff --git a/extensions/ql-vscode/src/config.ts b/extensions/ql-vscode/src/config.ts index bd6268a88..adac56c6d 100644 --- a/extensions/ql-vscode/src/config.ts +++ b/extensions/ql-vscode/src/config.ts @@ -619,3 +619,19 @@ export const ALLOW_HTTP_SETTING = new Setting( export function allowHttp(): boolean { return ALLOW_HTTP_SETTING.getValue() || false; } + +/** + * The name of the folder where we want to create skeleton wizard QL packs. + **/ +const SKELETON_WIZARD_FOLDER = new Setting( + "folder", + new Setting("skeletonWizard", ROOT_SETTING), +); + +export function getSkeletonWizardFolder(): string | undefined { + return SKELETON_WIZARD_FOLDER.getValue() || undefined; +} + +export async function setSkeletonWizardFolder(folder: string | undefined) { + await SKELETON_WIZARD_FOLDER.updateValue(folder, ConfigurationTarget.Global); +} diff --git a/extensions/ql-vscode/src/skeleton-query-wizard.ts b/extensions/ql-vscode/src/skeleton-query-wizard.ts index 10f3dbf77..11542f1ff 100644 --- a/extensions/ql-vscode/src/skeleton-query-wizard.ts +++ b/extensions/ql-vscode/src/skeleton-query-wizard.ts @@ -14,7 +14,12 @@ import { QlPackGenerator } from "./qlpack-generator"; import { DatabaseItem, DatabaseManager } from "./local-databases"; import { ProgressCallback, UserCancellationException } from "./progress"; import { askForGitHubRepo, downloadGitHubDatabase } from "./databaseFetcher"; -import { existsSync } from "fs"; +import { + getSkeletonWizardFolder, + isCodespacesTemplate, + setSkeletonWizardFolder, +} from "./config"; +import { existsSync } from "fs-extra"; type QueryLanguagesToDatabaseMap = Record; @@ -55,7 +60,7 @@ export class SkeletonQueryWizard { return; } - this.qlPackStoragePath = getFirstWorkspaceFolder(); + this.qlPackStoragePath = await this.determineStoragePath(); const skeletonPackAlreadyExists = existsSync(join(this.qlPackStoragePath, this.folderName)) || @@ -98,6 +103,38 @@ export class SkeletonQueryWizard { }); } + public async determineStoragePath() { + const firstStorageFolder = getFirstWorkspaceFolder(); + + if (isCodespacesTemplate()) { + return firstStorageFolder; + } + + let storageFolder = getSkeletonWizardFolder(); + + if (storageFolder === undefined || !existsSync(storageFolder)) { + storageFolder = await Window.showInputBox({ + title: + "Please choose a folder in which to create your new query pack. You can change this in the extension settings.", + value: firstStorageFolder, + ignoreFocusOut: true, + }); + } + + if (storageFolder === undefined) { + throw new UserCancellationException("No storage folder entered."); + } + + if (!existsSync(storageFolder)) { + throw new UserCancellationException( + "Invalid folder. Must be a folder that already exists.", + ); + } + + await setSkeletonWizardFolder(storageFolder); + return storageFolder; + } + private async chooseLanguage() { this.progress({ message: "Choose language", diff --git a/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts b/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts index e101cbf17..e82b86c2d 100644 --- a/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts @@ -21,6 +21,7 @@ import { import * as databaseFetcher from "../../../src/databaseFetcher"; import { createMockDB } from "../../factories/databases/databases"; import { asError } from "../../../src/pure/helpers-pure"; +import { Setting } from "../../../src/config"; describe("SkeletonQueryWizard", () => { let mockCli: CodeQLCliServer; @@ -29,6 +30,7 @@ describe("SkeletonQueryWizard", () => { let dir: tmp.DirResult; let storagePath: string; let quickPickSpy: jest.SpiedFunction; + let showInputBoxSpy: jest.SpiedFunction; let generateSpy: jest.SpiedFunction< typeof QlPackGenerator.prototype.generate >; @@ -93,6 +95,9 @@ describe("SkeletonQueryWizard", () => { quickPickSpy = jest .spyOn(window, "showQuickPick") .mockResolvedValueOnce(mockedQuickPickItem(chosenLanguage)); + showInputBoxSpy = jest + .spyOn(window, "showInputBox") + .mockResolvedValue(storagePath); generateSpy = jest .spyOn(QlPackGenerator.prototype, "generate") .mockResolvedValue(undefined); @@ -433,4 +438,116 @@ describe("SkeletonQueryWizard", () => { }); }); }); + + describe("determineStoragePath", () => { + it("should prompt the user to provide a storage path", async () => { + const chosenPath = await wizard.determineStoragePath(); + + expect(showInputBoxSpy).toHaveBeenCalledWith( + expect.objectContaining({ value: storagePath }), + ); + expect(chosenPath).toEqual(storagePath); + }); + + it("should write the chosen folder to settings", async () => { + const updateValueSpy = jest.spyOn(Setting.prototype, "updateValue"); + + await wizard.determineStoragePath(); + + expect(updateValueSpy).toHaveBeenCalledWith(storagePath, 1); + }); + + describe("when the user is using the codespace template", () => { + let originalValue: any; + let storedPath: string; + + beforeEach(async () => { + storedPath = join(dir.name, "pickles-folder"); + ensureDirSync(storedPath); + + originalValue = workspace + .getConfiguration("codeQL.skeletonWizard") + .get("folder"); + + // Set isCodespacesTemplate to true to indicate we are in the codespace template + await workspace + .getConfiguration("codeQL") + .update("codespacesTemplate", true); + }); + + afterEach(async () => { + await workspace + .getConfiguration("codeQL") + .update("codespacesTemplate", originalValue); + }); + + it("should not prompt the user", async () => { + const chosenPath = await wizard.determineStoragePath(); + + expect(showInputBoxSpy).not.toHaveBeenCalled(); + expect(chosenPath).toEqual(storagePath); + }); + }); + + describe("when there is already a saved storage path in settings", () => { + describe("when the saved storage path exists", () => { + let originalValue: any; + let storedPath: string; + + beforeEach(async () => { + storedPath = join(dir.name, "pickles-folder"); + ensureDirSync(storedPath); + + originalValue = workspace + .getConfiguration("codeQL.skeletonWizard") + .get("folder"); + await workspace + .getConfiguration("codeQL.skeletonWizard") + .update("folder", storedPath); + }); + + afterEach(async () => { + await workspace + .getConfiguration("codeQL.skeletonWizard") + .update("folder", originalValue); + }); + + it("should return it and not prompt the user", async () => { + const chosenPath = await wizard.determineStoragePath(); + + expect(showInputBoxSpy).not.toHaveBeenCalled(); + expect(chosenPath).toEqual(storedPath); + }); + }); + + describe("when the saved storage path does not exist", () => { + let originalValue: any; + let storedPath: string; + + beforeEach(async () => { + storedPath = join(dir.name, "this-folder-does-not-exist"); + + originalValue = workspace + .getConfiguration("codeQL.skeletonWizard") + .get("folder"); + await workspace + .getConfiguration("codeQL.skeletonWizard") + .update("folder", storedPath); + }); + + afterEach(async () => { + await workspace + .getConfiguration("codeQL.skeletonWizard") + .update("folder", originalValue); + }); + + it("should prompt the user for to provide a new folder name", async () => { + const chosenPath = await wizard.determineStoragePath(); + + expect(showInputBoxSpy).toHaveBeenCalled(); + expect(chosenPath).toEqual(storagePath); + }); + }); + }); + }); }); From e0bf1ca40fa5b660040946e6beda86b7ee6f35fb Mon Sep 17 00:00:00 2001 From: Elena Tanasoiu Date: Tue, 18 Apr 2023 15:27:58 +0000 Subject: [PATCH 19/28] Rename `skeletonWizard` setting to `createQuery` --- extensions/ql-vscode/src/config.ts | 2 +- .../cli-integration/skeleton-query-wizard.test.ts | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/extensions/ql-vscode/src/config.ts b/extensions/ql-vscode/src/config.ts index adac56c6d..1239ec905 100644 --- a/extensions/ql-vscode/src/config.ts +++ b/extensions/ql-vscode/src/config.ts @@ -625,7 +625,7 @@ export function allowHttp(): boolean { **/ const SKELETON_WIZARD_FOLDER = new Setting( "folder", - new Setting("skeletonWizard", ROOT_SETTING), + new Setting("createQuery", ROOT_SETTING), ); export function getSkeletonWizardFolder(): string | undefined { diff --git a/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts b/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts index e82b86c2d..87fbbc1e5 100644 --- a/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts @@ -466,7 +466,7 @@ describe("SkeletonQueryWizard", () => { ensureDirSync(storedPath); originalValue = workspace - .getConfiguration("codeQL.skeletonWizard") + .getConfiguration("codeQL.createQuery") .get("folder"); // Set isCodespacesTemplate to true to indicate we are in the codespace template @@ -499,16 +499,16 @@ describe("SkeletonQueryWizard", () => { ensureDirSync(storedPath); originalValue = workspace - .getConfiguration("codeQL.skeletonWizard") + .getConfiguration("codeQL.createQuery") .get("folder"); await workspace - .getConfiguration("codeQL.skeletonWizard") + .getConfiguration("codeQL.createQuery") .update("folder", storedPath); }); afterEach(async () => { await workspace - .getConfiguration("codeQL.skeletonWizard") + .getConfiguration("codeQL.createQuery") .update("folder", originalValue); }); @@ -528,16 +528,16 @@ describe("SkeletonQueryWizard", () => { storedPath = join(dir.name, "this-folder-does-not-exist"); originalValue = workspace - .getConfiguration("codeQL.skeletonWizard") + .getConfiguration("codeQL.createQuery") .get("folder"); await workspace - .getConfiguration("codeQL.skeletonWizard") + .getConfiguration("codeQL.createQuery") .update("folder", storedPath); }); afterEach(async () => { await workspace - .getConfiguration("codeQL.skeletonWizard") + .getConfiguration("codeQL.createQuery") .update("folder", originalValue); }); From db35cb294da4fe4a6fab8952ecbccb51c362dc4d Mon Sep 17 00:00:00 2001 From: Elena Tanasoiu Date: Tue, 18 Apr 2023 15:30:21 +0000 Subject: [PATCH 20/28] Add new setting to package.json --- extensions/ql-vscode/package.json | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/extensions/ql-vscode/package.json b/extensions/ql-vscode/package.json index 9905eecbf..d15882443 100644 --- a/extensions/ql-vscode/package.json +++ b/extensions/ql-vscode/package.json @@ -340,6 +340,12 @@ "type": "boolean", "default": false, "description": "Allow database to be downloaded via HTTP. Warning: enabling this option will allow downloading from insecure servers." + }, + "codeQL.createQuery.Folder": { + "type": "string", + "default": "", + "patternErrorMessage": "Please enter a valid folder", + "markdownDescription": "The name of the folder where we want to create queries and query packs via the \"CodeQL: Create Query\" command. The folder should exist in the workspace.`)." } } }, From a8a84a6f8c3ae06388e244f48987e056595e9c5d Mon Sep 17 00:00:00 2001 From: Robert Date: Tue, 18 Apr 2023 16:34:34 +0100 Subject: [PATCH 21/28] Add changelog entry --- extensions/ql-vscode/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/extensions/ql-vscode/CHANGELOG.md b/extensions/ql-vscode/CHANGELOG.md index 78b9281fe..f91a165c7 100644 --- a/extensions/ql-vscode/CHANGELOG.md +++ b/extensions/ql-vscode/CHANGELOG.md @@ -2,6 +2,8 @@ ## [UNRELEASED] +- Added ability to filter repositories for a variant analysis to only those that have results [#2343](https://github.com/github/vscode-codeql/pull/2343) + ## 1.8.2 - 12 April 2023 - Fix bug where users could end up with the managed CodeQL CLI getting uninstalled during upgrades and not reinstalled. [#2294](https://github.com/github/vscode-codeql/pull/2294) From dd630bfca814d7a0c1bbc3be30e3cdd1147fe8d9 Mon Sep 17 00:00:00 2001 From: Robert Date: Tue, 18 Apr 2023 16:38:39 +0100 Subject: [PATCH 22/28] Split tests up into describe blocks --- .../variant-analysis-filter-sort.test.ts | 152 +++++++++--------- 1 file changed, 78 insertions(+), 74 deletions(-) diff --git a/extensions/ql-vscode/test/unit-tests/variant-analysis-filter-sort.test.ts b/extensions/ql-vscode/test/unit-tests/variant-analysis-filter-sort.test.ts index 509ba8260..092849de1 100644 --- a/extensions/ql-vscode/test/unit-tests/variant-analysis-filter-sort.test.ts +++ b/extensions/ql-vscode/test/unit-tests/variant-analysis-filter-sort.test.ts @@ -14,88 +14,92 @@ describe(matchesFilter.name, () => { fullName: "github/codeql", }; - const testCases = [ - { searchValue: "", matches: true }, - { searchValue: "github/codeql", matches: true }, - { searchValue: "github", matches: true }, - { searchValue: "git", matches: true }, - { searchValue: "codeql", matches: true }, - { searchValue: "code", matches: true }, - { searchValue: "ql", matches: true }, - { searchValue: "/", matches: true }, - { searchValue: "gothub/codeql", matches: false }, - { searchValue: "hello", matches: false }, - { searchValue: "cod*ql", matches: false }, - { searchValue: "cod?ql", matches: false }, - ]; + describe("searchValue", () => { + const testCases = [ + { searchValue: "", matches: true }, + { searchValue: "github/codeql", matches: true }, + { searchValue: "github", matches: true }, + { searchValue: "git", matches: true }, + { searchValue: "codeql", matches: true }, + { searchValue: "code", matches: true }, + { searchValue: "ql", matches: true }, + { searchValue: "/", matches: true }, + { searchValue: "gothub/codeql", matches: false }, + { searchValue: "hello", matches: false }, + { searchValue: "cod*ql", matches: false }, + { searchValue: "cod?ql", matches: false }, + ]; - test.each(testCases)( - "returns $matches if searching for $searchValue", - ({ searchValue, matches }) => { + test.each(testCases)( + "returns $matches if searching for $searchValue", + ({ searchValue, matches }) => { + expect( + matchesFilter( + { repository }, + { + ...defaultFilterSortState, + searchValue, + }, + ), + ).toBe(matches); + }, + ); + }); + + describe("filterKey", () => { + it("returns true if filterKey is all and resultCount is positive", () => { + expect( + matchesFilter( + { repository, resultCount: 1 }, + { ...defaultFilterSortState, filterKey: FilterKey.All }, + ), + ).toBe(true); + }); + + it("returns true if filterKey is all and resultCount is zero", () => { + expect( + matchesFilter( + { repository, resultCount: 0 }, + { ...defaultFilterSortState, filterKey: FilterKey.All }, + ), + ).toBe(true); + }); + + it("returns true if filterKey is all and resultCount is undefined", () => { expect( matchesFilter( { repository }, - { - ...defaultFilterSortState, - searchValue, - }, + { ...defaultFilterSortState, filterKey: FilterKey.All }, ), - ).toBe(matches); - }, - ); + ).toBe(true); + }); - it("returns true if filterKey is all and resultCount is positive", () => { - expect( - matchesFilter( - { repository, resultCount: 1 }, - { ...defaultFilterSortState, filterKey: FilterKey.All }, - ), - ).toBe(true); - }); + it("returns true if filterKey is withResults and resultCount is positive", () => { + expect( + matchesFilter( + { repository, resultCount: 1 }, + { ...defaultFilterSortState, filterKey: FilterKey.WithResults }, + ), + ).toBe(true); + }); - it("returns true if filterKey is all and resultCount is zero", () => { - expect( - matchesFilter( - { repository, resultCount: 0 }, - { ...defaultFilterSortState, filterKey: FilterKey.All }, - ), - ).toBe(true); - }); + it("returns false if filterKey is withResults and resultCount is zero", () => { + expect( + matchesFilter( + { repository, resultCount: 0 }, + { ...defaultFilterSortState, filterKey: FilterKey.WithResults }, + ), + ).toBe(false); + }); - it("returns true if filterKey is all and resultCount is undefined", () => { - expect( - matchesFilter( - { repository }, - { ...defaultFilterSortState, filterKey: FilterKey.All }, - ), - ).toBe(true); - }); - - it("returns true if filterKey is withResults and resultCount is positive", () => { - expect( - matchesFilter( - { repository, resultCount: 1 }, - { ...defaultFilterSortState, filterKey: FilterKey.WithResults }, - ), - ).toBe(true); - }); - - it("returns false if filterKey is withResults and resultCount is zero", () => { - expect( - matchesFilter( - { repository, resultCount: 0 }, - { ...defaultFilterSortState, filterKey: FilterKey.WithResults }, - ), - ).toBe(false); - }); - - it("returns false if filterKey is withResults and resultCount is undefined", () => { - expect( - matchesFilter( - { repository }, - { ...defaultFilterSortState, filterKey: FilterKey.WithResults }, - ), - ).toBe(false); + it("returns false if filterKey is withResults and resultCount is undefined", () => { + expect( + matchesFilter( + { repository }, + { ...defaultFilterSortState, filterKey: FilterKey.WithResults }, + ), + ).toBe(false); + }); }); }); From 93acb760993cb4630abf6e59a70711680ec0c87f Mon Sep 17 00:00:00 2001 From: Robert Date: Tue, 18 Apr 2023 16:38:56 +0100 Subject: [PATCH 23/28] Use punctuation to make test names clearer --- .../test/unit-tests/variant-analysis-filter-sort.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extensions/ql-vscode/test/unit-tests/variant-analysis-filter-sort.test.ts b/extensions/ql-vscode/test/unit-tests/variant-analysis-filter-sort.test.ts index 092849de1..90319ee65 100644 --- a/extensions/ql-vscode/test/unit-tests/variant-analysis-filter-sort.test.ts +++ b/extensions/ql-vscode/test/unit-tests/variant-analysis-filter-sort.test.ts @@ -451,7 +451,7 @@ describe(filterAndSortRepositoriesWithResultsByName.name, () => { }); }); - describe("when sort key and search and filter withResults are given", () => { + describe("when sort key, search, and filter withResults are given", () => { it("returns the correct results", () => { expect( filterAndSortRepositoriesWithResultsByName(repositories, { @@ -548,7 +548,7 @@ describe(filterAndSortRepositoriesWithResults.name, () => { }); }); - describe("when sort key and search and filter withResults are given", () => { + describe("when sort key, search, and filter withResults are given", () => { it("returns the correct results", () => { expect( filterAndSortRepositoriesWithResults(repositories, { From 5e76d0b1adc3b3dcb3efada289d8e43d2ea1f204 Mon Sep 17 00:00:00 2001 From: Elena Tanasoiu Date: Tue, 18 Apr 2023 17:27:36 +0100 Subject: [PATCH 24/28] Fix typos Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com> --- extensions/ql-vscode/package.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extensions/ql-vscode/package.json b/extensions/ql-vscode/package.json index d15882443..c5f69ccff 100644 --- a/extensions/ql-vscode/package.json +++ b/extensions/ql-vscode/package.json @@ -341,11 +341,11 @@ "default": false, "description": "Allow database to be downloaded via HTTP. Warning: enabling this option will allow downloading from insecure servers." }, - "codeQL.createQuery.Folder": { + "codeQL.createQuery.folder": { "type": "string", "default": "", "patternErrorMessage": "Please enter a valid folder", - "markdownDescription": "The name of the folder where we want to create queries and query packs via the \"CodeQL: Create Query\" command. The folder should exist in the workspace.`)." + "markdownDescription": "The name of the folder where we want to create queries and query packs via the \"CodeQL: Create Query\" command. The folder should exist in the workspace." } } }, From 154fab845a9515f2ab1a7da237dbf7df577d57fc Mon Sep 17 00:00:00 2001 From: Elena Tanasoiu Date: Wed, 19 Apr 2023 07:56:06 +0000 Subject: [PATCH 25/28] Don't mention workspace --- extensions/ql-vscode/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/ql-vscode/package.json b/extensions/ql-vscode/package.json index c5f69ccff..ad787b172 100644 --- a/extensions/ql-vscode/package.json +++ b/extensions/ql-vscode/package.json @@ -345,7 +345,7 @@ "type": "string", "default": "", "patternErrorMessage": "Please enter a valid folder", - "markdownDescription": "The name of the folder where we want to create queries and query packs via the \"CodeQL: Create Query\" command. The folder should exist in the workspace." + "markdownDescription": "The name of the folder where we want to create queries and query packs via the \"CodeQL: Create Query\" command. The folder should exist." } } }, From 561e07a6890cd05c257d0049226a1ca6321c42a2 Mon Sep 17 00:00:00 2001 From: Elena Tanasoiu Date: Fri, 14 Apr 2023 15:59:21 +0000 Subject: [PATCH 26/28] Sort database items by dateAdded Before selecting an existing database, let's sort them by date. We're opting to exclude databases with errors during the sorting process. --- .../ql-vscode/src/skeleton-query-wizard.ts | 42 ++++--- .../skeleton-query-wizard.test.ts | 118 +++++++++--------- 2 files changed, 88 insertions(+), 72 deletions(-) diff --git a/extensions/ql-vscode/src/skeleton-query-wizard.ts b/extensions/ql-vscode/src/skeleton-query-wizard.ts index baa3178f8..4ae9c973b 100644 --- a/extensions/ql-vscode/src/skeleton-query-wizard.ts +++ b/extensions/ql-vscode/src/skeleton-query-wizard.ts @@ -273,12 +273,8 @@ export class SkeletonQueryWizard { databaseNwo: string, databaseItems: readonly DatabaseItem[], ): Promise { - const dbItems = databaseItems || []; - const dbs = dbItems.filter( - (db) => - db.language === language && - db.name === databaseNwo && - db.error === undefined, + const dbs = databaseItems.filter( + (db) => db.language === language && db.name === databaseNwo, ); if (dbs.length === 0) { @@ -291,10 +287,7 @@ export class SkeletonQueryWizard { language: string, databaseItems: readonly DatabaseItem[], ): Promise { - const dbItems = databaseItems || []; - const dbs = dbItems.filter( - (db) => db.language === language && db.error === undefined, - ); + const dbs = databaseItems.filter((db) => db.language === language); if (dbs.length === 0) { return undefined; } @@ -308,19 +301,38 @@ export class SkeletonQueryWizard { const defaultDatabaseNwo = QUERY_LANGUAGE_TO_DATABASE_REPO[this.language]; + const dbItems = await this.sortDatabaseItemsByDateAdded( + this.databaseManager.databaseItems, + ); + const defaultDatabaseItem = await this.findDatabaseItemByNwo( this.language, defaultDatabaseNwo, - this.databaseManager.databaseItems, + dbItems, ); if (defaultDatabaseItem !== undefined) { return defaultDatabaseItem; } - return await this.findDatabaseItemByLanguage( - this.language, - this.databaseManager.databaseItems, - ); + return await this.findDatabaseItemByLanguage(this.language, dbItems); + } + + public async sortDatabaseItemsByDateAdded( + databaseItems: readonly DatabaseItem[], + ) { + const validDbItems = databaseItems.filter((db) => db.error === undefined); + + return validDbItems.sort((a, b) => { + if (a.dateAdded === undefined) { + return -1; + } + + if (b.dateAdded === undefined) { + return 1; + } + + return a.dateAdded - b.dateAdded; + }); } } diff --git a/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts b/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts index 87fbbc1e5..511bc2e26 100644 --- a/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts @@ -330,37 +330,6 @@ describe("SkeletonQueryWizard", () => { JSON.stringify(mockDbItem), ); }); - - it("should ignore databases with errors", async () => { - const mockDbItem = createMockDB(dir, { - language: "ruby", - dateAdded: 123, - } as FullDatabaseOptions); - const mockDbItem2 = createMockDB(dir, { - language: "javascript", - } as FullDatabaseOptions); - const mockDbItem3 = createMockDB(dir, { - language: "ruby", - dateAdded: 345, - } as FullDatabaseOptions); - - jest.spyOn(mockDbItem, "name", "get").mockReturnValue("mock-name"); - jest.spyOn(mockDbItem3, "name", "get").mockReturnValue(mockDbItem.name); - - jest - .spyOn(mockDbItem, "error", "get") - .mockReturnValue(asError("database go boom!")); - - const databaseItem = await wizard.findDatabaseItemByNwo( - mockDbItem.language, - mockDbItem.name, - [mockDbItem, mockDbItem2, mockDbItem3], - ); - - expect(JSON.stringify(databaseItem)).toEqual( - JSON.stringify(mockDbItem3), - ); - }); }); describe("when the item doesn't exist", () => { @@ -396,32 +365,6 @@ describe("SkeletonQueryWizard", () => { expect(databaseItem).toEqual(mockDbItem); }); - - it("should ignore databases with errors", async () => { - const mockDbItem = createMockDB(dir, { - language: "ruby", - } as FullDatabaseOptions); - const mockDbItem2 = createMockDB(dir, { - language: "javascript", - } as FullDatabaseOptions); - const mockDbItem3 = createMockDB(dir, { - language: "ruby", - } as FullDatabaseOptions); - - jest - .spyOn(mockDbItem, "error", "get") - .mockReturnValue(asError("database go boom!")); - - const databaseItem = await wizard.findDatabaseItemByLanguage("ruby", [ - mockDbItem, - mockDbItem2, - mockDbItem3, - ]); - - expect(JSON.stringify(databaseItem)).toEqual( - JSON.stringify(mockDbItem3), - ); - }); }); describe("when the item doesn't exist", () => { @@ -550,4 +493,65 @@ describe("SkeletonQueryWizard", () => { }); }); }); + + describe("sortDatabaseItemsByDateAdded", () => { + describe("should return a sorted list", () => { + it("should sort the items by dateAdded", async () => { + const mockDbItem = createMockDB(dir, { + dateAdded: 678, + } as FullDatabaseOptions); + const mockDbItem2 = createMockDB(dir, { + dateAdded: 123, + } as FullDatabaseOptions); + const mockDbItem3 = createMockDB(dir, { + dateAdded: undefined, + } as FullDatabaseOptions); + const mockDbItem4 = createMockDB(dir, { + dateAdded: 345, + } as FullDatabaseOptions); + + const sortedList = await wizard.sortDatabaseItemsByDateAdded([ + mockDbItem, + mockDbItem2, + mockDbItem3, + mockDbItem4, + ]); + + expect(sortedList).toEqual([ + mockDbItem3, + mockDbItem2, + mockDbItem4, + mockDbItem, + ]); + }); + + it("should ignore databases with errors", async () => { + const mockDbItem = createMockDB(dir, { + dateAdded: 678, + } as FullDatabaseOptions); + const mockDbItem2 = createMockDB(dir, { + dateAdded: undefined, + } as FullDatabaseOptions); + const mockDbItem3 = createMockDB(dir, { + dateAdded: 345, + } as FullDatabaseOptions); + const mockDbItem4 = createMockDB(dir, { + dateAdded: 123, + } as FullDatabaseOptions); + + jest + .spyOn(mockDbItem, "error", "get") + .mockReturnValue(asError("database go boom!")); + + const sortedList = await wizard.sortDatabaseItemsByDateAdded([ + mockDbItem, + mockDbItem2, + mockDbItem3, + mockDbItem4, + ]); + + expect(sortedList).toEqual([mockDbItem2, mockDbItem4, mockDbItem3]); + }); + }); + }); }); From 02dffe08d5d86eb5e1a8ee24d45dc8377a47efee Mon Sep 17 00:00:00 2001 From: Elena Tanasoiu Date: Fri, 14 Apr 2023 16:51:16 +0000 Subject: [PATCH 27/28] Fetch the latest database from the new sorted list And add tests to check this. I've had to adapt the existing `findExistingDatabaseItem` method so receive params so that I'm better able to send it a language and a list of database items. --- .../ql-vscode/src/skeleton-query-wizard.ts | 39 +++++------ .../skeleton-query-wizard.test.ts | 69 +++++++++++++++++++ 2 files changed, 88 insertions(+), 20 deletions(-) diff --git a/extensions/ql-vscode/src/skeleton-query-wizard.ts b/extensions/ql-vscode/src/skeleton-query-wizard.ts index 4ae9c973b..1fe3042e6 100644 --- a/extensions/ql-vscode/src/skeleton-query-wizard.ts +++ b/extensions/ql-vscode/src/skeleton-query-wizard.ts @@ -253,11 +253,18 @@ export class SkeletonQueryWizard { } private async selectOrDownloadDatabase() { + if (this.language === undefined) { + throw new Error("Language is undefined"); + } + if (this.qlPackStoragePath === undefined) { throw new Error("QL Pack storage path is undefined"); } - const existingDatabaseItem = await this.findExistingDatabaseItem(); + const existingDatabaseItem = await this.findExistingDatabaseItem( + this.language, + this.databaseManager.databaseItems, + ); if (existingDatabaseItem) { // select the found database @@ -277,10 +284,7 @@ export class SkeletonQueryWizard { (db) => db.language === language && db.name === databaseNwo, ); - if (dbs.length === 0) { - return undefined; - } - return dbs[0]; + return dbs.pop(); } public async findDatabaseItemByLanguage( @@ -288,25 +292,20 @@ export class SkeletonQueryWizard { databaseItems: readonly DatabaseItem[], ): Promise { const dbs = databaseItems.filter((db) => db.language === language); - if (dbs.length === 0) { - return undefined; - } - return dbs[0]; + + return dbs.pop(); } - private async findExistingDatabaseItem() { - if (this.language === undefined) { - throw new Error("Language is undefined"); - } + public async findExistingDatabaseItem( + language: string, + databaseItems: readonly DatabaseItem[], + ): Promise { + const defaultDatabaseNwo = QUERY_LANGUAGE_TO_DATABASE_REPO[language]; - const defaultDatabaseNwo = QUERY_LANGUAGE_TO_DATABASE_REPO[this.language]; - - const dbItems = await this.sortDatabaseItemsByDateAdded( - this.databaseManager.databaseItems, - ); + const dbItems = await this.sortDatabaseItemsByDateAdded(databaseItems); const defaultDatabaseItem = await this.findDatabaseItemByNwo( - this.language, + language, defaultDatabaseNwo, dbItems, ); @@ -315,7 +314,7 @@ export class SkeletonQueryWizard { return defaultDatabaseItem; } - return await this.findDatabaseItemByLanguage(this.language, dbItems); + return await this.findDatabaseItemByLanguage(language, dbItems); } public async sortDatabaseItemsByDateAdded( diff --git a/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts b/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts index 511bc2e26..de8686871 100644 --- a/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts @@ -554,4 +554,73 @@ describe("SkeletonQueryWizard", () => { }); }); }); + + describe("findExistingDatabaseItem", () => { + describe("when there are multiple items with the same name", () => { + it("should choose the latest one", async () => { + const mockDbItem = createMockDB(dir, { + language: "javascript", + dateAdded: 456, + } as FullDatabaseOptions); + const mockDbItem2 = createMockDB(dir, { + language: "ruby", + dateAdded: 789, + } as FullDatabaseOptions); + const mockDbItem3 = createMockDB(dir, { + language: "javascript", + dateAdded: 123, + } as FullDatabaseOptions); + const mockDbItem4 = createMockDB(dir, { + language: "javascript", + dateAdded: undefined, + } as FullDatabaseOptions); + + jest + .spyOn(mockDbItem, "name", "get") + .mockReturnValue(QUERY_LANGUAGE_TO_DATABASE_REPO["javascript"]); + jest + .spyOn(mockDbItem2, "name", "get") + .mockReturnValue(QUERY_LANGUAGE_TO_DATABASE_REPO["javascript"]); + + const databaseItem = await wizard.findExistingDatabaseItem( + "javascript", + [mockDbItem, mockDbItem2, mockDbItem3, mockDbItem4], + ); + + expect(JSON.stringify(databaseItem)).toEqual( + JSON.stringify(mockDbItem), + ); + }); + }); + + describe("when there are multiple items with the same language", () => { + it("should choose the latest one", async () => { + const mockDbItem = createMockDB(dir, { + language: "ruby", + dateAdded: 789, + } as FullDatabaseOptions); + const mockDbItem2 = createMockDB(dir, { + language: "javascript", + dateAdded: 456, + } as FullDatabaseOptions); + const mockDbItem3 = createMockDB(dir, { + language: "ruby", + dateAdded: 123, + } as FullDatabaseOptions); + const mockDbItem4 = createMockDB(dir, { + language: "javascript", + dateAdded: undefined, + } as FullDatabaseOptions); + + const databaseItem = await wizard.findExistingDatabaseItem( + "javascript", + [mockDbItem, mockDbItem2, mockDbItem3, mockDbItem4], + ); + + expect(JSON.stringify(databaseItem)).toEqual( + JSON.stringify(mockDbItem2), + ); + }); + }); + }); }); From 02e17516d9331c9b8c7d56b792a56c02d1fba978 Mon Sep 17 00:00:00 2001 From: Elena Tanasoiu Date: Tue, 18 Apr 2023 14:10:24 +0000 Subject: [PATCH 28/28] Convert db item methods to be static So that we make it clear we should be passing state as params instead of reading it off `this`. --- .../ql-vscode/src/skeleton-query-wizard.ts | 28 ++++++---- .../skeleton-query-wizard.test.ts | 52 ++++++++++--------- 2 files changed, 45 insertions(+), 35 deletions(-) diff --git a/extensions/ql-vscode/src/skeleton-query-wizard.ts b/extensions/ql-vscode/src/skeleton-query-wizard.ts index 1fe3042e6..f6c8a38ad 100644 --- a/extensions/ql-vscode/src/skeleton-query-wizard.ts +++ b/extensions/ql-vscode/src/skeleton-query-wizard.ts @@ -261,10 +261,11 @@ export class SkeletonQueryWizard { throw new Error("QL Pack storage path is undefined"); } - const existingDatabaseItem = await this.findExistingDatabaseItem( - this.language, - this.databaseManager.databaseItems, - ); + const existingDatabaseItem = + await SkeletonQueryWizard.findExistingDatabaseItem( + this.language, + this.databaseManager.databaseItems, + ); if (existingDatabaseItem) { // select the found database @@ -275,7 +276,7 @@ export class SkeletonQueryWizard { } } - public async findDatabaseItemByNwo( + public static async findDatabaseItemByNwo( language: string, databaseNwo: string, databaseItems: readonly DatabaseItem[], @@ -287,7 +288,7 @@ export class SkeletonQueryWizard { return dbs.pop(); } - public async findDatabaseItemByLanguage( + public static async findDatabaseItemByLanguage( language: string, databaseItems: readonly DatabaseItem[], ): Promise { @@ -296,15 +297,17 @@ export class SkeletonQueryWizard { return dbs.pop(); } - public async findExistingDatabaseItem( + public static async findExistingDatabaseItem( language: string, databaseItems: readonly DatabaseItem[], ): Promise { const defaultDatabaseNwo = QUERY_LANGUAGE_TO_DATABASE_REPO[language]; - const dbItems = await this.sortDatabaseItemsByDateAdded(databaseItems); + const dbItems = await SkeletonQueryWizard.sortDatabaseItemsByDateAdded( + databaseItems, + ); - const defaultDatabaseItem = await this.findDatabaseItemByNwo( + const defaultDatabaseItem = await SkeletonQueryWizard.findDatabaseItemByNwo( language, defaultDatabaseNwo, dbItems, @@ -314,10 +317,13 @@ export class SkeletonQueryWizard { return defaultDatabaseItem; } - return await this.findDatabaseItemByLanguage(language, dbItems); + return await SkeletonQueryWizard.findDatabaseItemByLanguage( + language, + dbItems, + ); } - public async sortDatabaseItemsByDateAdded( + public static async sortDatabaseItemsByDateAdded( databaseItems: readonly DatabaseItem[], ) { const validDbItems = databaseItems.filter((db) => db.error === undefined); diff --git a/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts b/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts index de8686871..f083ed83d 100644 --- a/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts @@ -320,7 +320,7 @@ describe("SkeletonQueryWizard", () => { jest.spyOn(mockDbItem, "name", "get").mockReturnValue("mock-name"); - const databaseItem = await wizard.findDatabaseItemByNwo( + const databaseItem = await SkeletonQueryWizard.findDatabaseItemByNwo( mockDbItem.language, mockDbItem.name, [mockDbItem, mockDbItem2], @@ -337,7 +337,7 @@ describe("SkeletonQueryWizard", () => { const mockDbItem = createMockDB(dir); const mockDbItem2 = createMockDB(dir); - const databaseItem = await wizard.findDatabaseItemByNwo( + const databaseItem = await SkeletonQueryWizard.findDatabaseItemByNwo( "ruby", "mock-nwo", [mockDbItem, mockDbItem2], @@ -358,10 +358,11 @@ describe("SkeletonQueryWizard", () => { language: "javascript", } as FullDatabaseOptions); - const databaseItem = await wizard.findDatabaseItemByLanguage("ruby", [ - mockDbItem, - mockDbItem2, - ]); + const databaseItem = + await SkeletonQueryWizard.findDatabaseItemByLanguage("ruby", [ + mockDbItem, + mockDbItem2, + ]); expect(databaseItem).toEqual(mockDbItem); }); @@ -372,10 +373,11 @@ describe("SkeletonQueryWizard", () => { const mockDbItem = createMockDB(dir); const mockDbItem2 = createMockDB(dir); - const databaseItem = await wizard.findDatabaseItemByLanguage("ruby", [ - mockDbItem, - mockDbItem2, - ]); + const databaseItem = + await SkeletonQueryWizard.findDatabaseItemByLanguage("ruby", [ + mockDbItem, + mockDbItem2, + ]); expect(databaseItem).toBeUndefined(); }); @@ -510,12 +512,13 @@ describe("SkeletonQueryWizard", () => { dateAdded: 345, } as FullDatabaseOptions); - const sortedList = await wizard.sortDatabaseItemsByDateAdded([ - mockDbItem, - mockDbItem2, - mockDbItem3, - mockDbItem4, - ]); + const sortedList = + await SkeletonQueryWizard.sortDatabaseItemsByDateAdded([ + mockDbItem, + mockDbItem2, + mockDbItem3, + mockDbItem4, + ]); expect(sortedList).toEqual([ mockDbItem3, @@ -543,12 +546,13 @@ describe("SkeletonQueryWizard", () => { .spyOn(mockDbItem, "error", "get") .mockReturnValue(asError("database go boom!")); - const sortedList = await wizard.sortDatabaseItemsByDateAdded([ - mockDbItem, - mockDbItem2, - mockDbItem3, - mockDbItem4, - ]); + const sortedList = + await SkeletonQueryWizard.sortDatabaseItemsByDateAdded([ + mockDbItem, + mockDbItem2, + mockDbItem3, + mockDbItem4, + ]); expect(sortedList).toEqual([mockDbItem2, mockDbItem4, mockDbItem3]); }); @@ -582,7 +586,7 @@ describe("SkeletonQueryWizard", () => { .spyOn(mockDbItem2, "name", "get") .mockReturnValue(QUERY_LANGUAGE_TO_DATABASE_REPO["javascript"]); - const databaseItem = await wizard.findExistingDatabaseItem( + const databaseItem = await SkeletonQueryWizard.findExistingDatabaseItem( "javascript", [mockDbItem, mockDbItem2, mockDbItem3, mockDbItem4], ); @@ -612,7 +616,7 @@ describe("SkeletonQueryWizard", () => { dateAdded: undefined, } as FullDatabaseOptions); - const databaseItem = await wizard.findExistingDatabaseItem( + const databaseItem = await SkeletonQueryWizard.findExistingDatabaseItem( "javascript", [mockDbItem, mockDbItem2, mockDbItem3, mockDbItem4], );