Add validation for duplicate names in the db config (#1867)

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<void>;
@@ -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<DbConfig, string> {
+  public getConfig(): ValueResult<DbConfig, DbConfigValidationError> {
     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) {

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 ${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;
   }
 }

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<void>;
@@ -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<DbItem[], string> {
+  public getDbItems(): ValueResult<DbItem[], DbConfigValidationError> {
     const configResult = this.dbConfigStore.getConfig();
     if (configResult.isFailure) {
       return ValueResult.fail(configResult.errors);

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

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

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

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<void> {
     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 &&

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

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 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 dbList1 list: db1",
+    });
   });
 });

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