From 51be7023b973899d09c30752d793758013e5e00c Mon Sep 17 00:00:00 2001 From: Charis Kyriakou Date: Tue, 13 Dec 2022 14:44:44 +0000 Subject: [PATCH 1/2] Add validation for duplicate names in the db config --- .../src/databases/config/db-config-store.ts | 24 ++- .../databases/config/db-config-validator.ts | 117 +++++++++++++- .../ql-vscode/src/databases/db-manager.ts | 9 +- .../src/databases/db-validation-errors.ts | 10 ++ .../src/databases/ui/db-tree-data-provider.ts | 28 +++- extensions/ql-vscode/src/text-utils.ts | 8 + .../databases/db-panel.test.ts | 84 +++++++++- .../test/factories/db-config-factories.ts | 20 +++ .../config/db-config-validator.test.ts | 148 ++++++++++++++++-- .../test/pure-tests/text-utils.test.ts | 15 ++ 10 files changed, 437 insertions(+), 26 deletions(-) create mode 100644 extensions/ql-vscode/src/databases/db-validation-errors.ts create mode 100644 extensions/ql-vscode/test/pure-tests/text-utils.test.ts diff --git a/extensions/ql-vscode/src/databases/config/db-config-store.ts b/extensions/ql-vscode/src/databases/config/db-config-store.ts index fe71a0b4f04..3c2a2c5a910 100644 --- a/extensions/ql-vscode/src/databases/config/db-config-store.ts +++ b/extensions/ql-vscode/src/databases/config/db-config-store.ts @@ -9,9 +9,13 @@ import { import * as chokidar from "chokidar"; import { DisposableObject, DisposeHandler } from "../../pure/disposable-object"; import { DbConfigValidator } from "./db-config-validator"; -import { ValueResult } from "../../common/value-result"; import { App } from "../../common/app"; import { AppEvent, AppEventEmitter } from "../../common/events"; +import { + DbConfigValidationError, + DbConfigValidationErrorKind, +} from "../db-validation-errors"; +import { ValueResult } from "../../common/value-result"; export class DbConfigStore extends DisposableObject { public readonly onDidChangeConfig: AppEvent; @@ -21,7 +25,7 @@ export class DbConfigStore extends DisposableObject { private readonly configValidator: DbConfigValidator; private config: DbConfig | undefined; - private configErrors: string[]; + private configErrors: DbConfigValidationError[]; private configWatcher: chokidar.FSWatcher | undefined; public constructor(app: App) { @@ -48,7 +52,7 @@ export class DbConfigStore extends DisposableObject { this.configWatcher?.unwatch(this.configPath); } - public getConfig(): ValueResult { + public getConfig(): ValueResult { if (this.config) { // Clone the config so that it's not modified outside of this class. return ValueResult.ok(cloneDbConfig(this.config)); @@ -124,7 +128,12 @@ export class DbConfigStore extends DisposableObject { try { newConfig = await readJSON(this.configPath); } catch (e) { - this.configErrors = [`Failed to read config file: ${this.configPath}`]; + this.configErrors = [ + { + kind: DbConfigValidationErrorKind.InvalidJson, + message: `Failed to read config file: ${this.configPath}`, + }, + ]; } if (newConfig) { @@ -139,7 +148,12 @@ export class DbConfigStore extends DisposableObject { try { newConfig = readJSONSync(this.configPath); } catch (e) { - this.configErrors = [`Failed to read config file: ${this.configPath}`]; + this.configErrors = [ + { + kind: DbConfigValidationErrorKind.InvalidJson, + message: `Failed to read config file: ${this.configPath}`, + }, + ]; } if (newConfig) { diff --git a/extensions/ql-vscode/src/databases/config/db-config-validator.ts b/extensions/ql-vscode/src/databases/config/db-config-validator.ts index 211a815f545..0e14981ff09 100644 --- a/extensions/ql-vscode/src/databases/config/db-config-validator.ts +++ b/extensions/ql-vscode/src/databases/config/db-config-validator.ts @@ -2,6 +2,11 @@ import { readJsonSync } from "fs-extra"; import { resolve } from "path"; import Ajv from "ajv"; import { DbConfig } from "./db-config"; +import { findDuplicateStrings } from "../../text-utils"; +import { + DbConfigValidationError, + DbConfigValidationErrorKind, +} from "../db-validation-errors"; export class DbConfigValidator { private readonly schema: any; @@ -14,16 +19,118 @@ export class DbConfigValidator { this.schema = readJsonSync(schemaPath); } - public validate(dbConfig: DbConfig): string[] { + public validate(dbConfig: DbConfig): DbConfigValidationError[] { const ajv = new Ajv({ allErrors: true }); ajv.validate(this.schema, dbConfig); if (ajv.errors) { - return ajv.errors.map( - (error) => `${error.instancePath} ${error.message}`, - ); + return ajv.errors.map((error) => ({ + kind: DbConfigValidationErrorKind.InvalidConfig, + message: `${error.instancePath} ${error.message}`, + })); } - return []; + return [ + ...this.validateDbListNames(dbConfig), + ...this.validateDbNames(dbConfig), + ...this.validateDbNamesInLists(dbConfig), + ...this.validateOwners(dbConfig), + ]; + } + + private validateDbListNames(dbConfig: DbConfig): DbConfigValidationError[] { + const errors: DbConfigValidationError[] = []; + + const buildError = (dups: string[]) => ({ + kind: DbConfigValidationErrorKind.DuplicateNames, + message: `There are database lists with the same name: ${dups.join( + ", ", + )}`, + }); + + const duplicateLocalDbLists = findDuplicateStrings( + dbConfig.databases.local.lists.map((n) => n.name), + ); + + if (duplicateLocalDbLists.length > 0) { + errors.push(buildError(duplicateLocalDbLists)); + } + + const duplicateRemoteDbLists = findDuplicateStrings( + dbConfig.databases.remote.repositoryLists.map((n) => n.name), + ); + if (duplicateRemoteDbLists.length > 0) { + errors.push(buildError(duplicateRemoteDbLists)); + } + + return errors; + } + + private validateDbNames(dbConfig: DbConfig): DbConfigValidationError[] { + const errors: DbConfigValidationError[] = []; + + const buildError = (dups: string[]) => ({ + kind: DbConfigValidationErrorKind.DuplicateNames, + message: `There are databases with the same name: ${dups.join(", ")}`, + }); + + const duplicateLocalDbs = findDuplicateStrings( + dbConfig.databases.local.databases.map((d) => d.name), + ); + + if (duplicateLocalDbs.length > 0) { + errors.push(buildError(duplicateLocalDbs)); + } + + const duplicateRemoteDbs = findDuplicateStrings( + dbConfig.databases.remote.repositories, + ); + if (duplicateRemoteDbs.length > 0) { + errors.push(buildError(duplicateRemoteDbs)); + } + + return errors; + } + + private validateDbNamesInLists( + dbConfig: DbConfig, + ): DbConfigValidationError[] { + const errors: DbConfigValidationError[] = []; + + const buildError = (listName: string, dups: string[]) => ({ + kind: DbConfigValidationErrorKind.DuplicateNames, + message: `There are databases with the same name in the the ${listName} list: ${dups.join( + ", ", + )}`, + }); + + for (const list of dbConfig.databases.local.lists) { + const dups = findDuplicateStrings(list.databases.map((d) => d.name)); + if (dups.length > 0) { + errors.push(buildError(list.name, dups)); + } + } + + for (const list of dbConfig.databases.remote.repositoryLists) { + const dups = findDuplicateStrings(list.repositories); + if (dups.length > 0) { + errors.push(buildError(list.name, dups)); + } + } + + return errors; + } + + private validateOwners(dbConfig: DbConfig): DbConfigValidationError[] { + const errors: DbConfigValidationError[] = []; + + const dups = findDuplicateStrings(dbConfig.databases.remote.owners); + if (dups.length > 0) { + errors.push({ + kind: DbConfigValidationErrorKind.DuplicateNames, + message: `There are owners with the same name: ${dups.join(", ")}`, + }); + } + return errors; } } diff --git a/extensions/ql-vscode/src/databases/db-manager.ts b/extensions/ql-vscode/src/databases/db-manager.ts index 3e9d0a0dbb2..c9378615d3c 100644 --- a/extensions/ql-vscode/src/databases/db-manager.ts +++ b/extensions/ql-vscode/src/databases/db-manager.ts @@ -9,6 +9,7 @@ import { mapDbItemToSelectedDbItem, } from "./db-item-selection"; import { createLocalTree, createRemoteTree } from "./db-tree-creator"; +import { DbConfigValidationError } from "./db-validation-errors"; export class DbManager { public readonly onDbItemsChanged: AppEvent; @@ -24,16 +25,16 @@ export class DbManager { } public getSelectedDbItem(): DbItem | undefined { - const dbItems = this.getDbItems(); + const dbItemsResult = this.getDbItems(); - if (dbItems.isFailure) { + if (dbItemsResult.errors.length > 0) { return undefined; } - return getSelectedDbItem(dbItems.value); + return getSelectedDbItem(dbItemsResult.value); } - public getDbItems(): ValueResult { + public getDbItems(): ValueResult { const configResult = this.dbConfigStore.getConfig(); if (configResult.isFailure) { return ValueResult.fail(configResult.errors); diff --git a/extensions/ql-vscode/src/databases/db-validation-errors.ts b/extensions/ql-vscode/src/databases/db-validation-errors.ts new file mode 100644 index 00000000000..cc614694cbf --- /dev/null +++ b/extensions/ql-vscode/src/databases/db-validation-errors.ts @@ -0,0 +1,10 @@ +export enum DbConfigValidationErrorKind { + InvalidJson = "InvalidJson", + InvalidConfig = "InvalidConfig", + DuplicateNames = "DuplicateNames", +} + +export interface DbConfigValidationError { + kind: DbConfigValidationErrorKind; + message: string; +} diff --git a/extensions/ql-vscode/src/databases/ui/db-tree-data-provider.ts b/extensions/ql-vscode/src/databases/ui/db-tree-data-provider.ts index 1040b8f6054..ee569eb73c3 100644 --- a/extensions/ql-vscode/src/databases/ui/db-tree-data-provider.ts +++ b/extensions/ql-vscode/src/databases/ui/db-tree-data-provider.ts @@ -9,6 +9,10 @@ import { createDbTreeViewItemError, DbTreeViewItem } from "./db-tree-view-item"; import { DbManager } from "../db-manager"; import { mapDbItemToTreeViewItem } from "./db-item-mapper"; import { DisposableObject } from "../../pure/disposable-object"; +import { + DbConfigValidationError, + DbConfigValidationErrorKind, +} from "../db-validation-errors"; export class DbTreeDataProvider extends DisposableObject @@ -61,14 +65,34 @@ export class DbTreeDataProvider const dbItemsResult = this.dbManager.getDbItems(); if (dbItemsResult.isFailure) { + return this.createErrorItems(dbItemsResult.errors); + } + + return dbItemsResult.value.map(mapDbItemToTreeViewItem); + } + + private createErrorItems( + errors: DbConfigValidationError[], + ): DbTreeViewItem[] { + if ( + errors.some( + (e) => + e.kind === DbConfigValidationErrorKind.InvalidJson || + e.kind === DbConfigValidationErrorKind.InvalidConfig, + ) + ) { const errorTreeViewItem = createDbTreeViewItemError( "Error when reading databases config", "Please open your databases config and address errors", ); return [errorTreeViewItem]; + } else { + return errors + .filter((e) => e.kind === DbConfigValidationErrorKind.DuplicateNames) + .map((e) => + createDbTreeViewItemError(e.message, "Please remove duplicates"), + ); } - - return dbItemsResult.value.map(mapDbItemToTreeViewItem); } } diff --git a/extensions/ql-vscode/src/text-utils.ts b/extensions/ql-vscode/src/text-utils.ts index 7a0d1560a3a..5e5e2b08382 100644 --- a/extensions/ql-vscode/src/text-utils.ts +++ b/extensions/ql-vscode/src/text-utils.ts @@ -31,3 +31,11 @@ export function convertNonPrintableChars(label: string | undefined) { return convertedLabelArray.join(""); } } + +export function findDuplicateStrings(strings: string[]): string[] { + const dups = strings.filter( + (string, index, strings) => strings.indexOf(string) !== index, + ); + + return [...new Set(dups)]; +} diff --git a/extensions/ql-vscode/src/vscode-tests/minimal-workspace/databases/db-panel.test.ts b/extensions/ql-vscode/src/vscode-tests/minimal-workspace/databases/db-panel.test.ts index 22390bc18cd..bf6f3377802 100644 --- a/extensions/ql-vscode/src/vscode-tests/minimal-workspace/databases/db-panel.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/minimal-workspace/databases/db-panel.test.ts @@ -1,4 +1,4 @@ -import { TreeItemCollapsibleState, ThemeIcon } from "vscode"; +import { TreeItemCollapsibleState, ThemeIcon, ThemeColor } from "vscode"; import { join } from "path"; import { ensureDir, readJSON, remove, writeJson } from "fs-extra"; import { @@ -591,6 +591,73 @@ describe("db panel", () => { }); }); + it("should show error for invalid config", async () => { + // We're intentionally bypassing the type check because we'd + // like to make sure validation errors are highlighted. + const dbConfig = { + databases: {}, + } as any as DbConfig; + + await saveDbConfig(dbConfig); + + const dbTreeItems = await dbTreeDataProvider.getChildren(); + + expect(dbTreeItems).toBeTruthy(); + const items = dbTreeItems!; + expect(items.length).toBe(1); + + checkErrorItem( + items[0], + "Error when reading databases config", + "Please open your databases config and address errors", + ); + }); + + it("should show errors for duplicate names", async () => { + const dbConfig: DbConfig = { + databases: { + remote: { + repositoryLists: [ + { + name: "my-list-1", + repositories: ["owner1/repo1", "owner1/repo2"], + }, + { + name: "my-list-1", + repositories: ["owner1/repo1", "owner2/repo2"], + }, + ], + owners: [], + repositories: ["owner1/repo1", "owner1/repo1"], + }, + local: { + lists: [], + databases: [], + }, + }, + expanded: [], + }; + + await saveDbConfig(dbConfig); + + const dbTreeItems = await dbTreeDataProvider.getChildren(); + + expect(dbTreeItems).toBeTruthy(); + const items = dbTreeItems!; + expect(items.length).toBe(2); + + checkErrorItem( + items[0], + "There are database lists with the same name: my-list-1", + "Please remove duplicates", + ); + checkErrorItem( + items[1], + "There are databases with the same name: owner1/repo1", + "Please remove duplicates", + ); + }); + async function saveDbConfig(dbConfig: DbConfig): Promise { await writeJson(dbConfigFilePath, dbConfig); @@ -672,6 +739,21 @@ describe("db panel", () => { expect(item.collapsibleState).toBe(TreeItemCollapsibleState.None); } + function checkErrorItem( + item: DbTreeViewItem, + label: string, + tooltip: string, + ): void { + expect(item.dbItem).toBe(undefined); + expect(item.iconPath).toEqual( + new ThemeIcon("error", new ThemeColor("problemsErrorIcon.foreground")), + ); + expect(item.label).toBe(label); + expect(item.tooltip).toBe(tooltip); + expect(item.collapsibleState).toBe(TreeItemCollapsibleState.None); + expect(item.children.length).toBe(0); + } + function isTreeViewItemSelectable(treeViewItem: DbTreeViewItem) { return ( treeViewItem.resourceUri === undefined && diff --git a/extensions/ql-vscode/test/factories/db-config-factories.ts b/extensions/ql-vscode/test/factories/db-config-factories.ts index cf171e02057..9e1db9aede8 100644 --- a/extensions/ql-vscode/test/factories/db-config-factories.ts +++ b/extensions/ql-vscode/test/factories/db-config-factories.ts @@ -1,3 +1,4 @@ +import { faker } from "@faker-js/faker"; import { DbConfig, ExpandedDbItem, @@ -40,3 +41,22 @@ export function createDbConfig({ selected, }; } + +export function createLocalDbConfigItem({ + name = `database${faker.datatype.number()}`, + dateAdded = faker.date.past().getTime(), + language = `language${faker.datatype.number()}`, + storagePath = `storagePath${faker.datatype.number()}`, +}: { + name?: string; + dateAdded?: number; + language?: string; + storagePath?: string; +} = {}): LocalDatabase { + return { + name, + dateAdded, + language, + storagePath, + }; +} diff --git a/extensions/ql-vscode/test/pure-tests/databases/config/db-config-validator.test.ts b/extensions/ql-vscode/test/pure-tests/databases/config/db-config-validator.test.ts index 565d76735f5..386b203ec92 100644 --- a/extensions/ql-vscode/test/pure-tests/databases/config/db-config-validator.test.ts +++ b/extensions/ql-vscode/test/pure-tests/databases/config/db-config-validator.test.ts @@ -1,6 +1,11 @@ import { join } from "path"; import { DbConfig } from "../../../../src/databases/config/db-config"; import { DbConfigValidator } from "../../../../src/databases/config/db-config-validator"; +import { DbConfigValidationErrorKind } from "../../../../src/databases/db-validation-errors"; +import { + createDbConfig, + createLocalDbConfigItem, +} from "../../../factories/db-config-factories"; describe("db config validation", () => { const extensionPath = join(__dirname, "../../../.."); @@ -29,14 +34,139 @@ describe("db config validation", () => { expect(validationOutput).toHaveLength(3); - expect(validationOutput[0]).toEqual( - "/databases must have required property 'local'", - ); - expect(validationOutput[1]).toEqual( - "/databases/remote must have required property 'owners'", - ); - expect(validationOutput[2]).toEqual( - "/databases/remote must NOT have additional properties", - ); + expect(validationOutput[0]).toEqual({ + kind: DbConfigValidationErrorKind.InvalidConfig, + message: "/databases must have required property 'local'", + }); + expect(validationOutput[1]).toEqual({ + kind: DbConfigValidationErrorKind.InvalidConfig, + message: "/databases/remote must have required property 'owners'", + }); + expect(validationOutput[2]).toEqual({ + kind: DbConfigValidationErrorKind.InvalidConfig, + message: "/databases/remote must NOT have additional properties", + }); + }); + + it("should return error when there are multiple remote db lists with the same name", async () => { + const dbConfig = createDbConfig({ + remoteLists: [ + { + name: "repoList1", + repositories: ["owner1/repo1", "owner1/repo2"], + }, + { + name: "repoList1", + repositories: ["owner2/repo1", "owner2/repo2"], + }, + ], + }); + + const validationOutput = configValidator.validate(dbConfig); + + expect(validationOutput).toHaveLength(1); + expect(validationOutput[0]).toEqual({ + kind: DbConfigValidationErrorKind.DuplicateNames, + message: "There are database lists with the same name: repoList1", + }); + }); + + it("should return error when there are multiple remote dbs with the same name", async () => { + const dbConfig = createDbConfig({ + remoteRepos: ["owner1/repo1", "owner1/repo2", "owner1/repo2"], + }); + + const validationOutput = configValidator.validate(dbConfig); + + expect(validationOutput).toHaveLength(1); + expect(validationOutput[0]).toEqual({ + kind: DbConfigValidationErrorKind.DuplicateNames, + message: "There are databases with the same name: owner1/repo2", + }); + }); + + it("should return error when there are multiple remote dbs with the same name in the same list", async () => { + const dbConfig = createDbConfig({ + remoteLists: [ + { + name: "repoList1", + repositories: ["owner1/repo1", "owner1/repo2", "owner1/repo2"], + }, + ], + }); + + const validationOutput = configValidator.validate(dbConfig); + + expect(validationOutput).toHaveLength(1); + expect(validationOutput[0]).toEqual({ + kind: DbConfigValidationErrorKind.DuplicateNames, + message: + "There are databases with the same name in the the repoList1 list: owner1/repo2", + }); + }); + + it("should return error when there are multiple local db lists with the same name", async () => { + const dbConfig = createDbConfig({ + localLists: [ + { + name: "dbList1", + databases: [createLocalDbConfigItem()], + }, + { + name: "dbList1", + databases: [createLocalDbConfigItem()], + }, + ], + }); + + const validationOutput = configValidator.validate(dbConfig); + + expect(validationOutput).toHaveLength(1); + expect(validationOutput[0]).toEqual({ + kind: DbConfigValidationErrorKind.DuplicateNames, + message: "There are database lists with the same name: dbList1", + }); + }); + + it("should return error when there are multiple local dbs with the same name", async () => { + const dbConfig = createDbConfig({ + localDbs: [ + createLocalDbConfigItem({ name: "db1" }), + createLocalDbConfigItem({ name: "db2" }), + createLocalDbConfigItem({ name: "db1" }), + ], + }); + + const validationOutput = configValidator.validate(dbConfig); + + expect(validationOutput).toHaveLength(1); + expect(validationOutput[0]).toEqual({ + kind: DbConfigValidationErrorKind.DuplicateNames, + message: "There are databases with the same name: db1", + }); + }); + + it("should return error when there are multiple local dbs with the same name in the same list", async () => { + const dbConfig = createDbConfig({ + localLists: [ + { + name: "dbList1", + databases: [ + createLocalDbConfigItem({ name: "db1" }), + createLocalDbConfigItem({ name: "db2" }), + createLocalDbConfigItem({ name: "db1" }), + ], + }, + ], + }); + + const validationOutput = configValidator.validate(dbConfig); + + expect(validationOutput).toHaveLength(1); + expect(validationOutput[0]).toEqual({ + kind: DbConfigValidationErrorKind.DuplicateNames, + message: + "There are databases with the same name in the the dbList1 list: db1", + }); }); }); diff --git a/extensions/ql-vscode/test/pure-tests/text-utils.test.ts b/extensions/ql-vscode/test/pure-tests/text-utils.test.ts new file mode 100644 index 00000000000..bc2499d4203 --- /dev/null +++ b/extensions/ql-vscode/test/pure-tests/text-utils.test.ts @@ -0,0 +1,15 @@ +import { findDuplicateStrings } from "../../src/text-utils"; + +describe("findDuplicateStrings", () => { + it("should find duplicates strings in an array of strings", () => { + const strings = ["a", "b", "c", "a", "aa", "bb"]; + const duplicates = findDuplicateStrings(strings); + expect(duplicates).toEqual(["a"]); + }); + + it("should not find duplicates strings if there aren't any", () => { + const strings = ["a", "b", "c", "aa", "bb"]; + const duplicates = findDuplicateStrings(strings); + expect(duplicates).toEqual([]); + }); +}); From 5a40cdd0fe57475f1069834db0187ff9fecf6fe8 Mon Sep 17 00:00:00 2001 From: Charis Kyriakou Date: Wed, 14 Dec 2022 08:33:01 +0000 Subject: [PATCH 2/2] Apply suggestions from code review Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com> --- .../ql-vscode/src/databases/config/db-config-validator.ts | 2 +- .../pure-tests/databases/config/db-config-validator.test.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/extensions/ql-vscode/src/databases/config/db-config-validator.ts b/extensions/ql-vscode/src/databases/config/db-config-validator.ts index 0e14981ff09..ee820181be2 100644 --- a/extensions/ql-vscode/src/databases/config/db-config-validator.ts +++ b/extensions/ql-vscode/src/databases/config/db-config-validator.ts @@ -99,7 +99,7 @@ export class DbConfigValidator { const buildError = (listName: string, dups: string[]) => ({ kind: DbConfigValidationErrorKind.DuplicateNames, - message: `There are databases with the same name in the the ${listName} list: ${dups.join( + message: `There are databases with the same name in the ${listName} list: ${dups.join( ", ", )}`, }); diff --git a/extensions/ql-vscode/test/pure-tests/databases/config/db-config-validator.test.ts b/extensions/ql-vscode/test/pure-tests/databases/config/db-config-validator.test.ts index 386b203ec92..35e06cccd2b 100644 --- a/extensions/ql-vscode/test/pure-tests/databases/config/db-config-validator.test.ts +++ b/extensions/ql-vscode/test/pure-tests/databases/config/db-config-validator.test.ts @@ -101,7 +101,7 @@ describe("db config validation", () => { expect(validationOutput[0]).toEqual({ kind: DbConfigValidationErrorKind.DuplicateNames, message: - "There are databases with the same name in the the repoList1 list: owner1/repo2", + "There are databases with the same name in the repoList1 list: owner1/repo2", }); }); @@ -166,7 +166,7 @@ describe("db config validation", () => { expect(validationOutput[0]).toEqual({ kind: DbConfigValidationErrorKind.DuplicateNames, message: - "There are databases with the same name in the the dbList1 list: db1", + "There are databases with the same name in the dbList1 list: db1", }); }); });