Merge pull request #3616 from github/aeisenberg/flush-cache-on-db-remove

Fix bug with reimporting test cases

Author: aeisenberg

extensions/ql-vscode/CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## [UNRELEASED]
 
+- Fix a bug when re-importing test databases that erroneously showed old source code. [#3616](https://github.com/github/vscode-codeql/pull/3616)
+
 ## 1.13.0 - 1 May 2024
 
 - Add Ruby support to the CodeQL Model Editor. [#3584](https://github.com/github/vscode-codeql/pull/3584)

extensions/ql-vscode/src/common/vscode/archive-filesystem-provider.ts

@@ -26,6 +26,8 @@ import {
 // All path operations in this file must be on paths *within* the zip
 // archive.
 import { posix } from "path";
+import { DatabaseEventKind } from "../../databases/local-databases/database-events";
+import type { DatabaseManager } from "../../databases/local-databases/database-manager";
 
 const path = posix;
 
@@ -242,15 +244,8 @@ export class ArchiveFileSystemProvider implements FileSystemProvider {
 
   root = new Directory("");
 
-  constructor() {
-    // When a file system archive is removed from the workspace, we should
-    // also remove it from our cache.
-    workspace.onDidChangeWorkspaceFolders((event) => {
-      for (const removed of event.removed) {
-        const zipPath = removed.uri.fsPath;
-        this.archives.delete(zipPath);
-      }
-    });
+  flushCache(zipPath: string) {
+    this.archives.delete(zipPath);
   }
 
   // metadata
@@ -366,15 +361,35 @@ export class ArchiveFileSystemProvider implements FileSystemProvider {
  */
 export const zipArchiveScheme = "codeql-zip-archive";
 
-export function activate(ctx: ExtensionContext) {
+export function activate(ctx: ExtensionContext, dbm?: DatabaseManager) {
+  const afsp = new ArchiveFileSystemProvider();
+  if (dbm) {
+    ctx.subscriptions.push(
+      dbm.onDidChangeDatabaseItem(async ({ kind, item: db }) => {
+        if (kind === DatabaseEventKind.Remove) {
+          if (db?.sourceArchive) {
+            afsp.flushCache(db.sourceArchive.fsPath);
+          }
+        }
+      }),
+    );
+  }
+
   ctx.subscriptions.push(
-    workspace.registerFileSystemProvider(
-      zipArchiveScheme,
-      new ArchiveFileSystemProvider(),
-      {
-        isCaseSensitive: true,
-        isReadonly: true,
-      },
-    ),
+    // When a file system archive is removed from the workspace, we should
+    // also remove it from our cache.
+    workspace.onDidChangeWorkspaceFolders((event) => {
+      for (const removed of event.removed) {
+        const zipPath = removed.uri.fsPath;
+        afsp.flushCache(zipPath);
+      }
+    }),
+  );
+
+  ctx.subscriptions.push(
+    workspace.registerFileSystemProvider(zipArchiveScheme, afsp, {
+      isCaseSensitive: true,
+      isReadonly: true,
+    }),
   );
 }

extensions/ql-vscode/src/databases/local-databases-ui.ts

@@ -109,9 +109,8 @@ class DatabaseTreeDataProvider
     // Note that events from the database manager are instances of DatabaseChangedEvent
     // and events fired by the UI are instances of DatabaseItem
 
-    // When event.item is undefined, then the entire tree is refreshed.
-    // When event.item is a db item, then only that item is refreshed.
-    this._onDidChangeTreeData.fire(event.item);
+    // When a full refresh has occurred, then all items are refreshed by passing undefined.
+    this._onDidChangeTreeData.fire(event.fullRefresh ? undefined : event.item);
   }
 
   private handleDidChangeCurrentDatabaseItem(

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

@@ -16,4 +16,8 @@ export enum DatabaseEventKind {
 export interface DatabaseChangedEvent {
   kind: DatabaseEventKind;
   item: DatabaseItem | undefined;
+  // If true, event handlers should consider the database manager
+  // to have been fully refreshed. Any state managed by the
+  // event handler should be fully refreshed as well.
+  fullRefresh: boolean;
 }

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

@@ -613,6 +613,7 @@ export class DatabaseManager extends DisposableObject {
       this._onDidChangeCurrentDatabaseItem.fire({
         item,
         kind: DatabaseEventKind.Change,
+        fullRefresh: false,
       });
     }
   }
@@ -662,18 +663,19 @@ export class DatabaseManager extends DisposableObject {
     }
     // note that we use undefined as the item in order to reset the entire tree
     this._onDidChangeDatabaseItem.fire({
-      item: undefined,
+      item,
       kind: DatabaseEventKind.Add,
+      fullRefresh: true,
     });
   }
 
   public async renameDatabaseItem(item: DatabaseItem, newName: string) {
     item.name = newName;
     await this.updatePersistedDatabaseList();
     this._onDidChangeDatabaseItem.fire({
-      // pass undefined so that the entire tree is rebuilt in order to re-sort
-      item: undefined,
+      item,
       kind: DatabaseEventKind.Rename,
+      fullRefresh: true,
     });
   }
 
@@ -720,10 +722,10 @@ export class DatabaseManager extends DisposableObject {
       );
     }
 
-    // note that we use undefined as the item in order to reset the entire tree
     this._onDidChangeDatabaseItem.fire({
-      item: undefined,
+      item,
       kind: DatabaseEventKind.Remove,
+      fullRefresh: true,
     });
   }
 
@@ -776,6 +778,7 @@ export class DatabaseManager extends DisposableObject {
       this._onDidChangeDatabaseItem.fire({
         kind: DatabaseEventKind.Refresh,
         item: databaseItem,
+        fullRefresh: false,
       });
     }
   }

extensions/ql-vscode/src/extension.ts

@@ -947,7 +947,7 @@ async function activateWithInstalledDistribution(
   ctx.subscriptions.push(compareView);
 
   void extLogger.log("Initializing source archive filesystem provider.");
-  archiveFilesystemProvider_activate(ctx);
+  archiveFilesystemProvider_activate(ctx, dbm);
 
   const qhelpTmpDir = dirSync({
     prefix: "qhelp_",

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

@@ -140,7 +140,8 @@ describe("local databases", () => {
       },
     ]);
     expect(onDidChangeDatabaseItem).toHaveBeenCalledWith({
-      item: undefined,
+      fullRefresh: true,
+      item: mockDbItem,
       kind: DatabaseEventKind.Add,
     });
 
@@ -152,7 +153,8 @@ describe("local databases", () => {
     expect((databaseManager as any)._databaseItems).toEqual([]);
     expect(updateSpy).toHaveBeenCalledWith("databaseList", []);
     expect(onDidChangeDatabaseItem).toHaveBeenCalledWith({
-      item: undefined,
+      fullRefresh: true,
+      item: mockDbItem,
       kind: DatabaseEventKind.Remove,
     });
   });
@@ -175,7 +177,8 @@ describe("local databases", () => {
       ]);
 
       expect(onDidChangeDatabaseItem).toHaveBeenCalledWith({
-        item: undefined,
+        fullRefresh: true,
+        item: mockDbItem,
         kind: DatabaseEventKind.Rename,
       });
     });
@@ -198,7 +201,8 @@ describe("local databases", () => {
       ]);
 
       const mockEvent = {
-        item: undefined,
+        fullRefresh: true,
+        item: mockDbItem,
         kind: DatabaseEventKind.Add,
       };
       expect(onDidChangeDatabaseItem).toHaveBeenCalledWith(mockEvent);