Merge pull request #2450 from github/koesie10/move-database-item-refresh

Extract `DatabaseItem.refresh` method to `DatabaseManager`

Author: koesie10

extensions/ql-vscode/src/databases/local-databases/database-item-impl.ts

@@ -3,7 +3,6 @@ import * as cli from "../../codeql-cli/cli";
 import vscode from "vscode";
 import { FullDatabaseOptions } from "./database-options";
 import { basename, dirname, join, relative } from "path";
-import { asError } from "../../pure/helpers-pure";
 import {
   decodeSourceArchiveUri,
   encodeArchiveBasePath,
@@ -15,29 +14,27 @@ import { isLikelyDatabaseRoot } from "../../helpers";
 import { stat } from "fs-extra";
 import { pathsEqual } from "../../pure/files";
 import { DatabaseContents } from "./database-contents";
-import { DatabaseResolver } from "./database-resolver";
-import { DatabaseChangedEvent, DatabaseEventKind } from "./database-events";
 
 export class DatabaseItemImpl implements DatabaseItem {
-  private _error: Error | undefined = undefined;
-  private _contents: DatabaseContents | undefined;
+  // These are only public in the implementation, they are readonly in the interface
+  public error: Error | undefined = undefined;
+  public contents: DatabaseContents | undefined;
   /** A cache of database info */
   private _dbinfo: cli.DbInfo | undefined;
 
   public constructor(
     public readonly databaseUri: vscode.Uri,
     contents: DatabaseContents | undefined,
     private options: FullDatabaseOptions,
-    private readonly onChanged: (event: DatabaseChangedEvent) => void,
   ) {
-    this._contents = contents;
+    this.contents = contents;
   }
 
   public get name(): string {
     if (this.options.displayName) {
       return this.options.displayName;
-    } else if (this._contents) {
-      return this._contents.name;
+    } else if (this.contents) {
+      return this.contents.name;
     } else {
       return basename(this.databaseUri.fsPath);
     }
@@ -48,45 +45,17 @@ export class DatabaseItemImpl implements DatabaseItem {
   }
 
   public get sourceArchive(): vscode.Uri | undefined {
-    if (this.options.ignoreSourceArchive || this._contents === undefined) {
+    if (this.options.ignoreSourceArchive || this.contents === undefined) {
       return undefined;
     } else {
-      return this._contents.sourceArchiveUri;
+      return this.contents.sourceArchiveUri;
     }
   }
 
-  public get contents(): DatabaseContents | undefined {
-    return this._contents;
-  }
-
   public get dateAdded(): number | undefined {
     return this.options.dateAdded;
   }
 
-  public get error(): Error | undefined {
-    return this._error;
-  }
-
-  public async refresh(): Promise<void> {
-    try {
-      try {
-        this._contents = await DatabaseResolver.resolveDatabaseContents(
-          this.databaseUri,
-        );
-        this._error = undefined;
-      } catch (e) {
-        this._contents = undefined;
-        this._error = asError(e);
-        throw e;
-      }
-    } finally {
-      this.onChanged({
-        kind: DatabaseEventKind.Refresh,
-        item: this,
-      });
-    }
-  }
-
   public resolveSourceFile(uriStr: string | undefined): vscode.Uri {
     const sourceArchive = this.sourceArchive;
     const uri = uriStr ? vscode.Uri.parse(uriStr, true) : undefined;

extensions/ql-vscode/src/databases/local-databases/database-item.ts

@@ -27,15 +27,7 @@ export interface DatabaseItem {
 
   /** If the database is invalid, describes why. */
   readonly error: Error | undefined;
-  /**
-   * Resolves the contents of the database.
-   *
-   * @remarks
-   * The contents include the database directory, source archive, and metadata about the database.
-   * If the database is invalid, `this.error` is updated with the error object that describes why
-   * the database is invalid. This error is also thrown.
-   */
-  refresh(): Promise<void>;
+
   /**
    * Resolves a filename to its URI in the source archive.
    *

extensions/ql-vscode/src/databases/local-databases/database-manager.ts

@@ -83,7 +83,7 @@ export class DatabaseManager extends DisposableObject {
   readonly onDidChangeCurrentDatabaseItem =
     this._onDidChangeCurrentDatabaseItem.event;
 
-  private readonly _databaseItems: DatabaseItem[] = [];
+  private readonly _databaseItems: DatabaseItemImpl[] = [];
   private _currentDatabaseItem: DatabaseItem | undefined = undefined;
 
   constructor(
@@ -127,8 +127,8 @@ export class DatabaseManager extends DisposableObject {
    *
    * Typically, the item will have been created by {@link createOrOpenDatabaseItem} or {@link openDatabase}.
    */
-  public async addExistingDatabaseItem(
-    databaseItem: DatabaseItem,
+  private async addExistingDatabaseItem(
+    databaseItem: DatabaseItemImpl,
     progress: ProgressCallback,
     makeSelected: boolean,
     token: vscode.CancellationToken,
@@ -162,7 +162,7 @@ export class DatabaseManager extends DisposableObject {
   private async createDatabaseItem(
     uri: vscode.Uri,
     displayName: string | undefined,
-  ): Promise<DatabaseItem> {
+  ): Promise<DatabaseItemImpl> {
     const contents = await DatabaseResolver.resolveDatabaseContents(uri);
     // Ignore the source archive for QLTest databases by default.
     const isQLTestDatabase = extname(uri.fsPath) === ".testproj";
@@ -173,14 +173,7 @@ export class DatabaseManager extends DisposableObject {
       dateAdded: Date.now(),
       language: await this.getPrimaryLanguage(uri.fsPath),
     };
-    const databaseItem = new DatabaseItemImpl(
-      uri,
-      contents,
-      fullOptions,
-      (event) => {
-        this._onDidChangeDatabaseItem.fire(event);
-      },
-    );
+    const databaseItem = new DatabaseItemImpl(uri, contents, fullOptions);
 
     return databaseItem;
   }
@@ -329,7 +322,7 @@ export class DatabaseManager extends DisposableObject {
     progress: ProgressCallback,
     token: vscode.CancellationToken,
     state: PersistedDatabaseItem,
-  ): Promise<DatabaseItem> {
+  ): Promise<DatabaseItemImpl> {
     let displayName: string | undefined = undefined;
     let ignoreSourceArchive = false;
     let dateAdded = undefined;
@@ -359,14 +352,7 @@ export class DatabaseManager extends DisposableObject {
       dateAdded,
       language,
     };
-    const item = new DatabaseItemImpl(
-      dbBaseUri,
-      undefined,
-      fullOptions,
-      (event) => {
-        this._onDidChangeDatabaseItem.fire(event);
-      },
-    );
+    const item = new DatabaseItemImpl(dbBaseUri, undefined, fullOptions);
 
     // Avoid persisting the database state after adding since that should happen only after
     // all databases have been added.
@@ -407,7 +393,7 @@ export class DatabaseManager extends DisposableObject {
             database,
           );
           try {
-            await databaseItem.refresh();
+            await this.refreshDatabase(databaseItem);
             await this.registerDatabase(progress, token, databaseItem);
             if (currentDatabaseUri === database.uri) {
               await this.setCurrentDatabaseItem(databaseItem, true);
@@ -449,8 +435,12 @@ export class DatabaseManager extends DisposableObject {
     item: DatabaseItem | undefined,
     skipRefresh = false,
   ): Promise<void> {
-    if (!skipRefresh && item !== undefined) {
-      await item.refresh(); // Will throw on invalid database.
+    if (
+      !skipRefresh &&
+      item !== undefined &&
+      item instanceof DatabaseItemImpl
+    ) {
+      await this.refreshDatabase(item); // Will throw on invalid database.
     }
     if (this._currentDatabaseItem !== item) {
       this._currentDatabaseItem = item;
@@ -499,7 +489,7 @@ export class DatabaseManager extends DisposableObject {
   private async addDatabaseItem(
     progress: ProgressCallback,
     token: vscode.CancellationToken,
-    item: DatabaseItem,
+    item: DatabaseItemImpl,
     updatePersistedState = true,
   ) {
     this._databaseItems.push(item);
@@ -616,6 +606,34 @@ export class DatabaseManager extends DisposableObject {
     await this.qs.registerDatabase(progress, token, dbItem);
   }
 
+  /**
+   * Resolves the contents of the database.
+   *
+   * @remarks
+   * The contents include the database directory, source archive, and metadata about the database.
+   * If the database is invalid, `databaseItem.error` is updated with the error object that describes why
+   * the database is invalid. This error is also thrown.
+   */
+  private async refreshDatabase(databaseItem: DatabaseItemImpl) {
+    try {
+      try {
+        databaseItem.contents = await DatabaseResolver.resolveDatabaseContents(
+          databaseItem.databaseUri,
+        );
+        databaseItem.error = undefined;
+      } catch (e) {
+        databaseItem.contents = undefined;
+        databaseItem.error = asError(e);
+        throw e;
+      }
+    } finally {
+      this._onDidChangeDatabaseItem.fire({
+        kind: DatabaseEventKind.Refresh,
+        item: databaseItem,
+      });
+    }
+  }
+
   private updatePersistedCurrentDatabaseItem(): void {
     void this.ctx.workspaceState.update(
       CURRENT_DB,

extensions/ql-vscode/test/factories/databases/databases.ts

@@ -33,7 +33,6 @@ export function createMockDB(
       datasetUri: databaseUri,
     } as DatabaseContents,
     dbOptions,
-    () => void 0,
   );
 }
 

extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts

@@ -546,9 +546,7 @@ describe("SkeletonQueryWizard", () => {
           dateAdded: 123,
         } as FullDatabaseOptions);
 
-        jest
-          .spyOn(mockDbItem, "error", "get")
-          .mockReturnValue(asError("database go boom!"));
+        mockDbItem.error = asError("database go boom!");
 
         const sortedList =
           await SkeletonQueryWizard.sortDatabaseItemsByDateAdded([

extensions/ql-vscode/test/vscode-tests/minimal-workspace/local-databases.test.ts

@@ -327,7 +327,7 @@ describe("local databases", () => {
         mockDbOptions(),
         Uri.parse("file:/sourceArchive-uri/"),
       );
-      (db as any)._contents.sourceArchiveUri = undefined;
+      (db as any).contents.sourceArchiveUri = undefined;
       expect(() => db.resolveSourceFile("abc")).toThrowError(
         "Scheme is missing",
       );
@@ -339,7 +339,7 @@ describe("local databases", () => {
         mockDbOptions(),
         Uri.parse("file:/sourceArchive-uri/"),
       );
-      (db as any)._contents.sourceArchiveUri = undefined;
+      (db as any).contents.sourceArchiveUri = undefined;
       expect(() => db.resolveSourceFile("http://abc")).toThrowError(
         "Invalid uri scheme",
       );
@@ -352,7 +352,7 @@ describe("local databases", () => {
           mockDbOptions(),
           Uri.parse("file:/sourceArchive-uri/"),
         );
-        (db as any)._contents.sourceArchiveUri = undefined;
+        (db as any).contents.sourceArchiveUri = undefined;
         const resolved = db.resolveSourceFile(undefined);
         expect(resolved.toString(true)).toBe(dbLocationUri(dir).toString(true));
       });
@@ -363,7 +363,7 @@ describe("local databases", () => {
           mockDbOptions(),
           Uri.parse("file:/sourceArchive-uri/"),
         );
-        (db as any)._contents.sourceArchiveUri = undefined;
+        (db as any).contents.sourceArchiveUri = undefined;
         const resolved = db.resolveSourceFile("file:");
         expect(resolved.toString()).toBe("file:///");
       });

extensions/ql-vscode/test/vscode-tests/no-workspace/query-testing/test-runner.test.ts

@@ -40,17 +40,11 @@ describe("test-runner", () => {
     Uri.file("/path/to/test/dir/dir.testproj"),
     undefined,
     mockedObject<FullDatabaseOptions>({ displayName: "custom display name" }),
-    (_) => {
-      /* no change event listener */
-    },
   );
   const postTestDatabaseItem = new DatabaseItemImpl(
     Uri.file("/path/to/test/dir/dir.testproj"),
     undefined,
     mockedObject<FullDatabaseOptions>({ displayName: "default name" }),
-    (_) => {
-      /* no change event listener */
-    },
   );
 
   beforeEach(() => {