Skip to content

Commit 376d6b7

Browse files
committed
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.
1 parent 233cb1c commit 376d6b7

File tree

5 files changed

+48
-26
lines changed

5 files changed

+48
-26
lines changed

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

Lines changed: 33 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ import {
2626
// All path operations in this file must be on paths *within* the zip
2727
// archive.
2828
import { posix } from "path";
29+
import { DatabaseEventKind } from "../../databases/local-databases/database-events";
30+
import type { DatabaseManager } from "../../databases/local-databases/database-manager";
2931

3032
const path = posix;
3133

@@ -242,15 +244,10 @@ export class ArchiveFileSystemProvider implements FileSystemProvider {
242244

243245
root = new Directory("");
244246

245-
constructor() {
246-
// When a file system archive is removed from the workspace, we should
247-
// also remove it from our cache.
248-
workspace.onDidChangeWorkspaceFolders((event) => {
249-
for (const removed of event.removed) {
250-
const zipPath = removed.uri.fsPath;
251-
this.archives.delete(zipPath);
252-
}
253-
});
247+
constructor() {}
248+
249+
flushCache(zipPath: string) {
250+
this.archives.delete(zipPath);
254251
}
255252

256253
// metadata
@@ -366,15 +363,33 @@ export class ArchiveFileSystemProvider implements FileSystemProvider {
366363
*/
367364
export const zipArchiveScheme = "codeql-zip-archive";
368365

369-
export function activate(ctx: ExtensionContext) {
366+
export function activate(ctx: ExtensionContext, dbm: DatabaseManager) {
367+
const afsp = new ArchiveFileSystemProvider();
370368
ctx.subscriptions.push(
371-
workspace.registerFileSystemProvider(
372-
zipArchiveScheme,
373-
new ArchiveFileSystemProvider(),
374-
{
375-
isCaseSensitive: true,
376-
isReadonly: true,
377-
},
378-
),
369+
dbm.onDidChangeDatabaseItem(async ({ kind, item: db }) => {
370+
if (kind === DatabaseEventKind.Remove) {
371+
if (db?.sourceArchive) {
372+
afsp.flushCache(db.sourceArchive.fsPath);
373+
}
374+
}
375+
}),
376+
);
377+
378+
ctx.subscriptions.push(
379+
// When a file system archive is removed from the workspace, we should
380+
// also remove it from our cache.
381+
workspace.onDidChangeWorkspaceFolders((event) => {
382+
for (const removed of event.removed) {
383+
const zipPath = removed.uri.fsPath;
384+
afsp.flushCache(zipPath);
385+
}
386+
}),
387+
);
388+
389+
ctx.subscriptions.push(
390+
workspace.registerFileSystemProvider(zipArchiveScheme, afsp, {
391+
isCaseSensitive: true,
392+
isReadonly: true,
393+
}),
379394
);
380395
}

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -109,9 +109,8 @@ class DatabaseTreeDataProvider
109109
// Note that events from the database manager are instances of DatabaseChangedEvent
110110
// and events fired by the UI are instances of DatabaseItem
111111

112-
// When event.item is undefined, then the entire tree is refreshed.
113-
// When event.item is a db item, then only that item is refreshed.
114-
this._onDidChangeTreeData.fire(event.item);
112+
// When a full refresh has occurred, then all items are refreshed by passing undefined.
113+
this._onDidChangeTreeData.fire(event.fullRefresh ? undefined : event.item);
115114
}
116115

117116
private handleDidChangeCurrentDatabaseItem(

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,4 +16,8 @@ export enum DatabaseEventKind {
1616
export interface DatabaseChangedEvent {
1717
kind: DatabaseEventKind;
1818
item: DatabaseItem | undefined;
19+
// If true, event handlers should consider the database manager
20+
// to have been fully refreshed. Any state managed by the
21+
/// event handler shouuld be fully refreshed as well.
22+
fullRefresh: boolean;
1923
}

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -613,6 +613,7 @@ export class DatabaseManager extends DisposableObject {
613613
this._onDidChangeCurrentDatabaseItem.fire({
614614
item,
615615
kind: DatabaseEventKind.Change,
616+
fullRefresh: false,
616617
});
617618
}
618619
}
@@ -662,18 +663,19 @@ export class DatabaseManager extends DisposableObject {
662663
}
663664
// note that we use undefined as the item in order to reset the entire tree
664665
this._onDidChangeDatabaseItem.fire({
665-
item: undefined,
666+
item,
666667
kind: DatabaseEventKind.Add,
668+
fullRefresh: true,
667669
});
668670
}
669671

670672
public async renameDatabaseItem(item: DatabaseItem, newName: string) {
671673
item.name = newName;
672674
await this.updatePersistedDatabaseList();
673675
this._onDidChangeDatabaseItem.fire({
674-
// pass undefined so that the entire tree is rebuilt in order to re-sort
675-
item: undefined,
676+
item,
676677
kind: DatabaseEventKind.Rename,
678+
fullRefresh: true,
677679
});
678680
}
679681

@@ -722,8 +724,9 @@ export class DatabaseManager extends DisposableObject {
722724

723725
// note that we use undefined as the item in order to reset the entire tree
724726
this._onDidChangeDatabaseItem.fire({
725-
item: undefined,
727+
item,
726728
kind: DatabaseEventKind.Remove,
729+
fullRefresh: true,
727730
});
728731
}
729732

@@ -776,6 +779,7 @@ export class DatabaseManager extends DisposableObject {
776779
this._onDidChangeDatabaseItem.fire({
777780
kind: DatabaseEventKind.Refresh,
778781
item: databaseItem,
782+
fullRefresh: false,
779783
});
780784
}
781785
}

extensions/ql-vscode/src/extension.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -947,7 +947,7 @@ async function activateWithInstalledDistribution(
947947
ctx.subscriptions.push(compareView);
948948

949949
void extLogger.log("Initializing source archive filesystem provider.");
950-
archiveFilesystemProvider_activate(ctx);
950+
archiveFilesystemProvider_activate(ctx, dbm);
951951

952952
const qhelpTmpDir = dirSync({
953953
prefix: "qhelp_",

0 commit comments

Comments
 (0)