Handle db validation errors gracefully (#1895)

Author: charisk

extensions/ql-vscode/src/databases/config/db-config-store.ts

@@ -99,6 +99,10 @@ export class DbConfigStore extends DisposableObject {
       throw Error("Cannot add remote repo if config is not loaded");
     }
 
+    if (repoNwo === "") {
+      throw Error("Repository name cannot be empty");
+    }
+
     if (this.doesRemoteDbExist(repoNwo)) {
       throw Error(
         `A remote repository with the name '${repoNwo}' already exists`,
@@ -116,6 +120,10 @@ export class DbConfigStore extends DisposableObject {
       throw Error("Cannot add remote owner if config is not loaded");
     }
 
+    if (owner === "") {
+      throw Error("Owner name cannot be empty");
+    }
+
     if (this.doesRemoteOwnerExist(owner)) {
       throw Error(`A remote owner with the name '${owner}' already exists`);
     }
@@ -131,6 +139,10 @@ export class DbConfigStore extends DisposableObject {
       throw Error("Cannot add remote list if config is not loaded");
     }
 
+    if (listName === "") {
+      throw Error("List name cannot be empty");
+    }
+
     if (this.doesRemoteListExist(listName)) {
       throw Error(`A remote list with the name '${listName}' already exists`);
     }
@@ -154,6 +166,14 @@ export class DbConfigStore extends DisposableObject {
     );
   }
 
+  public doesLocalListExist(listName: string): boolean {
+    if (!this.config) {
+      throw Error("Cannot check local list existence if config is not loaded");
+    }
+
+    return this.config.databases.local.lists.some((l) => l.name === listName);
+  }
+
   public doesRemoteDbExist(dbName: string, listName?: string): boolean {
     if (!this.config) {
       throw Error(

extensions/ql-vscode/src/databases/db-item.ts

@@ -25,6 +25,11 @@ export const localDbKinds = [
   DbItemKind.LocalDatabase,
 ];
 
+export enum DbListKind {
+  Local = "Local",
+  Remote = "Remote",
+}
+
 export interface RootLocalDbItem {
   kind: DbItemKind.RootLocal;
   expanded: boolean;

extensions/ql-vscode/src/databases/db-manager.ts

@@ -2,7 +2,7 @@ import { App } from "../common/app";
 import { AppEvent, AppEventEmitter } from "../common/events";
 import { ValueResult } from "../common/value-result";
 import { DbConfigStore } from "./config/db-config-store";
-import { DbItem, DbItemKind, remoteDbKinds } from "./db-item";
+import { DbItem, DbListKind } from "./db-item";
 import { calculateNewExpandedState } from "./db-item-expansion";
 import {
   getSelectedDbItem,
@@ -76,39 +76,45 @@ export class DbManager {
   }
 
   public async addNewRemoteRepo(nwo: string): Promise<void> {
-    if (nwo === "") {
-      throw new Error("Repository name cannot be empty");
-    }
-    if (this.dbConfigStore.doesRemoteDbExist(nwo)) {
-      throw new Error(`The repository '${nwo}' already exists`);
-    }
-
     await this.dbConfigStore.addRemoteRepo(nwo);
   }
 
   public async addNewRemoteOwner(owner: string): Promise<void> {
-    if (owner === "") {
-      throw Error("Owner name cannot be empty");
+    await this.dbConfigStore.addRemoteOwner(owner);
+  }
+
+  public async addNewList(
+    listKind: DbListKind,
+    listName: string,
+  ): Promise<void> {
+    switch (listKind) {
+      case DbListKind.Local:
+        // Adding a local list is not supported yet.
+        throw Error("Cannot add a local list");
+      case DbListKind.Remote:
+        await this.dbConfigStore.addRemoteList(listName);
+        break;
+      default:
+        throw Error(`Unknown list kind '${listKind}'`);
     }
-    if (this.dbConfigStore.doesRemoteOwnerExist(owner)) {
-      throw Error(`The owner '${owner}' already exists`);
+  }
+
+  public doesListExist(listKind: DbListKind, listName: string): boolean {
+    switch (listKind) {
+      case DbListKind.Local:
+        return this.dbConfigStore.doesLocalListExist(listName);
+      case DbListKind.Remote:
+        return this.dbConfigStore.doesRemoteListExist(listName);
+      default:
+        throw Error(`Unknown list kind '${listKind}'`);
     }
+  }
 
-    await this.dbConfigStore.addRemoteOwner(owner);
+  public doesRemoteOwnerExist(owner: string): boolean {
+    return this.dbConfigStore.doesRemoteOwnerExist(owner);
   }
 
-  public async addNewList(kind: DbItemKind, listName: string): Promise<void> {
-    if (remoteDbKinds.includes(kind)) {
-      if (listName === "") {
-        throw Error("List name cannot be empty");
-      }
-      if (this.dbConfigStore.doesRemoteListExist(listName)) {
-        throw Error(`A list with the name '${listName}' already exists`);
-      }
-
-      await this.dbConfigStore.addRemoteList(listName);
-    } else {
-      throw Error("Cannot add a local list");
-    }
+  public doesRemoteRepoExist(nwo: string, listName?: string): boolean {
+    return this.dbConfigStore.doesRemoteDbExist(nwo, listName);
   }
 }

extensions/ql-vscode/src/databases/ui/db-panel.ts

@@ -12,8 +12,9 @@ import {
   getOwnerFromGitHubUrl,
   isValidGitHubOwner,
 } from "../../common/github-url-identifier-helper";
+import { showAndLogErrorMessage } from "../../helpers";
 import { DisposableObject } from "../../pure/disposable-object";
-import { DbItem, DbItemKind } from "../db-item";
+import { DbItem, DbItemKind, DbListKind, remoteDbKinds } from "../db-item";
 import { DbManager } from "../db-manager";
 import { DbTreeDataProvider } from "./db-tree-data-provider";
 import { DbTreeViewItem } from "./db-tree-view-item";
@@ -126,7 +127,13 @@ export class DbPanel extends DisposableObject {
 
     const nwo = getNwoFromGitHubUrl(repoName) || repoName;
     if (!isValidGitHubNwo(nwo)) {
-      throw new Error(`Invalid GitHub repository: ${repoName}`);
+      void showAndLogErrorMessage(`Invalid GitHub repository: ${repoName}`);
+      return;
+    }
+
+    if (this.dbManager.doesRemoteRepoExist(nwo)) {
+      void showAndLogErrorMessage(`The repository '${nwo}' already exists`);
+      return;
     }
 
     await this.dbManager.addNewRemoteRepo(nwo);
@@ -145,7 +152,13 @@ export class DbPanel extends DisposableObject {
 
     const owner = getOwnerFromGitHubUrl(ownerName) || ownerName;
     if (!isValidGitHubOwner(owner)) {
-      throw new Error(`Invalid user or organization: ${owner}`);
+      void showAndLogErrorMessage(`Invalid user or organization: ${owner}`);
+      return;
+    }
+
+    if (this.dbManager.doesRemoteOwnerExist(owner)) {
+      void showAndLogErrorMessage(`The owner '${owner}' already exists`);
+      return;
     }
 
     await this.dbManager.addNewRemoteOwner(owner);
@@ -156,7 +169,7 @@ export class DbPanel extends DisposableObject {
       prompt: "Enter a name for the new list",
       placeHolder: "example-list",
     });
-    if (listName === undefined) {
+    if (listName === undefined || listName === "") {
       return;
     }
 
@@ -166,7 +179,15 @@ export class DbPanel extends DisposableObject {
     // we default to the "RootRemote" kind.
     // In future: if the highlighted item is undefined, we'll show a quick pick where
     // a user can select whether to add a remote or local list.
-    const listKind = highlightedItem?.kind || DbItemKind.RootRemote;
+    const highlightedItemKind = highlightedItem?.kind || DbItemKind.RootRemote;
+    const listKind = remoteDbKinds.includes(highlightedItemKind)
+      ? DbListKind.Remote
+      : DbListKind.Local;
+
+    if (this.dbManager.doesListExist(listKind, listName)) {
+      void showAndLogErrorMessage(`The list '${listName}' already exists`);
+      return;
+    }
 
     await this.dbManager.addNewList(listKind, listName);
   }

extensions/ql-vscode/src/vscode-tests/minimal-workspace/databases/db-panel.test.ts

@@ -8,7 +8,11 @@ import {
 import { DbManager } from "../../../databases/db-manager";
 import { DbConfigStore } from "../../../databases/config/db-config-store";
 import { DbTreeDataProvider } from "../../../databases/ui/db-tree-data-provider";
-import { DbItemKind, LocalDatabaseDbItem } from "../../../databases/db-item";
+import {
+  DbItemKind,
+  DbListKind,
+  LocalDatabaseDbItem,
+} from "../../../databases/db-item";
 import { DbTreeViewItem } from "../../../databases/ui/db-tree-view-item";
 import { ExtensionApp } from "../../../common/vscode/vscode-app";
 import { createMockExtensionContext } from "../../factories/extension-context";
@@ -478,7 +482,7 @@ describe("db panel", () => {
       expect(remoteUserDefinedLists.length).toBe(1);
       expect(remoteUserDefinedLists[0]).toBe(list1);
 
-      await dbManager.addNewList(DbItemKind.RootRemote, "my-list-2");
+      await dbManager.addNewList(DbListKind.Remote, "my-list-2");
 
       // Read the workspace databases JSON file directly to check that the new list has been added.
       // We can't use the dbConfigStore's `read` function here because it depends on the file watcher
@@ -497,9 +501,9 @@ describe("db panel", () => {
       const dbConfig: DbConfig = createDbConfig();
       await saveDbConfig(dbConfig);
 
-      await expect(
-        dbManager.addNewList(DbItemKind.RootLocal, ""),
-      ).rejects.toThrow(new Error("Cannot add a local list"));
+      await expect(dbManager.addNewList(DbListKind.Local, "")).rejects.toThrow(
+        new Error("Cannot add a local list"),
+      );
     });
   });
 
@@ -566,9 +570,9 @@ describe("db panel", () => {
 
       await saveDbConfig(dbConfig);
 
-      await expect(
-        dbManager.addNewList(DbItemKind.RootRemote, ""),
-      ).rejects.toThrow(new Error("List name cannot be empty"));
+      await expect(dbManager.addNewList(DbListKind.Remote, "")).rejects.toThrow(
+        new Error("List name cannot be empty"),
+      );
     });
 
     it("should not allow adding a list with duplicate name", async () => {
@@ -584,9 +588,9 @@ describe("db panel", () => {
       await saveDbConfig(dbConfig);
 
       await expect(
-        dbManager.addNewList(DbItemKind.RootRemote, "my-list-1"),
+        dbManager.addNewList(DbListKind.Remote, "my-list-1"),
       ).rejects.toThrow(
-        new Error("A list with the name 'my-list-1' already exists"),
+        new Error("A remote list with the name 'my-list-1' already exists"),
       );
     });
 
@@ -608,7 +612,9 @@ describe("db panel", () => {
       await saveDbConfig(dbConfig);
 
       await expect(dbManager.addNewRemoteRepo("owner1/repo1")).rejects.toThrow(
-        new Error("The repository 'owner1/repo1' already exists"),
+        new Error(
+          "A remote repository with the name 'owner1/repo1' already exists",
+        ),
       );
     });
 
@@ -630,7 +636,7 @@ describe("db panel", () => {
       await saveDbConfig(dbConfig);
 
       await expect(dbManager.addNewRemoteOwner("owner1")).rejects.toThrow(
-        new Error("The owner 'owner1' already exists"),
+        new Error("A remote owner with the name 'owner1' already exists"),
       );
     });
   });