From 376d6b75d6f00cda6d26d0fddf134f0aadc1e23f Mon Sep 17 00:00:00 2001 From: Andrew Eisenberg Date: Fri, 17 May 2024 23:10:56 +0000 Subject: [PATCH 1/3] Fix bug with reimporting test cases When re-importing a test database, if the source archive for that database is not in the workspace, then old source code will be seen when inspected. To reproduce: 1. Run a ql test file with a failure (I'm using a javascript DB). 2. Right click and import on the db that sticks around. 3. Change the JS source file for the test. 4. Re-run and still have a failure. 5. Re-import the database 6. Run the query under test 7. Click on a result item 8. **BUG:** The source code is old The problem is that the source archive cache is not being flushed in this case. I added a case to flush the source archive when the archive was imported into the workspace as a folder, but not when the archive is external. The fix is to listen for database remove events in the archive filesystem provider and flush the associated database source archive. There is a complication: The database remove event didn't emit the removed database. This is because the only place that consumed the event didn't need it. To get around this, I changed the structure of the events. I added a new `fullRefresh` boolean. If true, then the original database change handler will ensure the entire tree is refreshed. If false, only the selected database. --- .../vscode/archive-filesystem-provider.ts | 51 ++++++++++++------- .../src/databases/local-databases-ui.ts | 5 +- .../local-databases/database-events.ts | 4 ++ .../local-databases/database-manager.ts | 12 +++-- extensions/ql-vscode/src/extension.ts | 2 +- 5 files changed, 48 insertions(+), 26 deletions(-) diff --git a/extensions/ql-vscode/src/common/vscode/archive-filesystem-provider.ts b/extensions/ql-vscode/src/common/vscode/archive-filesystem-provider.ts index 3ccdc3668..483dafabf 100644 --- a/extensions/ql-vscode/src/common/vscode/archive-filesystem-provider.ts +++ b/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,10 @@ 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); - } - }); + constructor() {} + + flushCache(zipPath: string) { + this.archives.delete(zipPath); } // metadata @@ -366,15 +363,33 @@ 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(); ctx.subscriptions.push( - workspace.registerFileSystemProvider( - zipArchiveScheme, - new ArchiveFileSystemProvider(), - { - isCaseSensitive: true, - isReadonly: true, - }, - ), + dbm.onDidChangeDatabaseItem(async ({ kind, item: db }) => { + if (kind === DatabaseEventKind.Remove) { + if (db?.sourceArchive) { + afsp.flushCache(db.sourceArchive.fsPath); + } + } + }), + ); + + ctx.subscriptions.push( + // 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, + }), ); } diff --git a/extensions/ql-vscode/src/databases/local-databases-ui.ts b/extensions/ql-vscode/src/databases/local-databases-ui.ts index 91e0994cb..8a1298395 100644 --- a/extensions/ql-vscode/src/databases/local-databases-ui.ts +++ b/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( diff --git a/extensions/ql-vscode/src/databases/local-databases/database-events.ts b/extensions/ql-vscode/src/databases/local-databases/database-events.ts index 41c2179a1..9cf7c4865 100644 --- a/extensions/ql-vscode/src/databases/local-databases/database-events.ts +++ b/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 shouuld be fully refreshed as well. + fullRefresh: boolean; } diff --git a/extensions/ql-vscode/src/databases/local-databases/database-manager.ts b/extensions/ql-vscode/src/databases/local-databases/database-manager.ts index b766fae7d..b4b75fd7e 100644 --- a/extensions/ql-vscode/src/databases/local-databases/database-manager.ts +++ b/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,8 +663,9 @@ 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, }); } @@ -671,9 +673,9 @@ export class DatabaseManager extends DisposableObject { 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, }); } @@ -722,8 +724,9 @@ 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 +779,7 @@ export class DatabaseManager extends DisposableObject { this._onDidChangeDatabaseItem.fire({ kind: DatabaseEventKind.Refresh, item: databaseItem, + fullRefresh: false, }); } } diff --git a/extensions/ql-vscode/src/extension.ts b/extensions/ql-vscode/src/extension.ts index 59a0c4977..2b67b374d 100644 --- a/extensions/ql-vscode/src/extension.ts +++ b/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_", From 088d2fa91e76cd44aa2d51daa5e5a58b044bf69f Mon Sep 17 00:00:00 2001 From: Andrew Eisenberg Date: Wed, 22 May 2024 22:55:59 +0000 Subject: [PATCH 2/3] Fix failing tests Also: - Address comments in PR - Add changelog note --- extensions/ql-vscode/CHANGELOG.md | 2 ++ .../vscode/archive-filesystem-provider.ts | 22 +++++++++---------- .../local-databases/database-events.ts | 2 +- .../local-databases/database-manager.ts | 1 - .../local-queries/local-databases.test.ts | 12 ++++++---- 5 files changed, 22 insertions(+), 17 deletions(-) diff --git a/extensions/ql-vscode/CHANGELOG.md b/extensions/ql-vscode/CHANGELOG.md index 4ff96f90f..bb3311a05 100644 --- a/extensions/ql-vscode/CHANGELOG.md +++ b/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) diff --git a/extensions/ql-vscode/src/common/vscode/archive-filesystem-provider.ts b/extensions/ql-vscode/src/common/vscode/archive-filesystem-provider.ts index 483dafabf..c82225f42 100644 --- a/extensions/ql-vscode/src/common/vscode/archive-filesystem-provider.ts +++ b/extensions/ql-vscode/src/common/vscode/archive-filesystem-provider.ts @@ -244,8 +244,6 @@ export class ArchiveFileSystemProvider implements FileSystemProvider { root = new Directory(""); - constructor() {} - flushCache(zipPath: string) { this.archives.delete(zipPath); } @@ -363,17 +361,19 @@ export class ArchiveFileSystemProvider implements FileSystemProvider { */ export const zipArchiveScheme = "codeql-zip-archive"; -export function activate(ctx: ExtensionContext, dbm: DatabaseManager) { +export function activate(ctx: ExtensionContext, dbm?: DatabaseManager) { const afsp = new ArchiveFileSystemProvider(); - ctx.subscriptions.push( - dbm.onDidChangeDatabaseItem(async ({ kind, item: db }) => { - if (kind === DatabaseEventKind.Remove) { - if (db?.sourceArchive) { - afsp.flushCache(db.sourceArchive.fsPath); + 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( // When a file system archive is removed from the workspace, we should diff --git a/extensions/ql-vscode/src/databases/local-databases/database-events.ts b/extensions/ql-vscode/src/databases/local-databases/database-events.ts index 9cf7c4865..1fab0d110 100644 --- a/extensions/ql-vscode/src/databases/local-databases/database-events.ts +++ b/extensions/ql-vscode/src/databases/local-databases/database-events.ts @@ -18,6 +18,6 @@ export interface DatabaseChangedEvent { item: DatabaseItem | undefined; // If true, event handlers should consider the database manager // to have been fully refreshed. Any state managed by the - /// event handler shouuld be fully refreshed as well. + // event handler shouuld be fully refreshed as well. fullRefresh: boolean; } diff --git a/extensions/ql-vscode/src/databases/local-databases/database-manager.ts b/extensions/ql-vscode/src/databases/local-databases/database-manager.ts index b4b75fd7e..7b4eb482f 100644 --- a/extensions/ql-vscode/src/databases/local-databases/database-manager.ts +++ b/extensions/ql-vscode/src/databases/local-databases/database-manager.ts @@ -722,7 +722,6 @@ export class DatabaseManager extends DisposableObject { ); } - // note that we use undefined as the item in order to reset the entire tree this._onDidChangeDatabaseItem.fire({ item, kind: DatabaseEventKind.Remove, diff --git a/extensions/ql-vscode/test/vscode-tests/minimal-workspace/local-queries/local-databases.test.ts b/extensions/ql-vscode/test/vscode-tests/minimal-workspace/local-queries/local-databases.test.ts index 66fcd8093..b72be5488 100644 --- a/extensions/ql-vscode/test/vscode-tests/minimal-workspace/local-queries/local-databases.test.ts +++ b/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); From 39ad5b28c7cac64f1d1b320a3485bb369024af12 Mon Sep 17 00:00:00 2001 From: Andrew Eisenberg Date: Thu, 23 May 2024 08:43:03 -0700 Subject: [PATCH 3/3] Fix typo in comment Co-authored-by: Koen Vlaswinkel --- .../ql-vscode/src/databases/local-databases/database-events.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/ql-vscode/src/databases/local-databases/database-events.ts b/extensions/ql-vscode/src/databases/local-databases/database-events.ts index 1fab0d110..d41a9f319 100644 --- a/extensions/ql-vscode/src/databases/local-databases/database-events.ts +++ b/extensions/ql-vscode/src/databases/local-databases/database-events.ts @@ -18,6 +18,6 @@ export interface DatabaseChangedEvent { item: DatabaseItem | undefined; // If true, event handlers should consider the database manager // to have been fully refreshed. Any state managed by the - // event handler shouuld be fully refreshed as well. + // event handler should be fully refreshed as well. fullRefresh: boolean; }