diff --git a/extensions/ql-vscode/CHANGELOG.md b/extensions/ql-vscode/CHANGELOG.md index 6a15d749f9f..057873f0376 100644 --- a/extensions/ql-vscode/CHANGELOG.md +++ b/extensions/ql-vscode/CHANGELOG.md @@ -2,6 +2,8 @@ ## [UNRELEASED] +- Fix bug when removing databases where sometimes the source folder would not be removed from the workspace or the database files would not be removed from the workspace storage location. [#692](https://github.com/github/vscode-codeql/pull/692) + ## 1.3.7 - 24 November 2020 - Editors opened by navigating from the results view are no longer opened in _preview mode_. Now they are opened as a persistent editor. [#630](https://github.com/github/vscode-codeql/pull/630) diff --git a/extensions/ql-vscode/src/databases.ts b/extensions/ql-vscode/src/databases.ts index d3c7e0591b2..5364c1c3aba 100644 --- a/extensions/ql-vscode/src/databases.ts +++ b/extensions/ql-vscode/src/databases.ts @@ -38,7 +38,7 @@ export interface DatabaseOptions { dateAdded?: number | undefined; } -interface FullDatabaseOptions extends DatabaseOptions { +export interface FullDatabaseOptions extends DatabaseOptions { ignoreSourceArchive: boolean; dateAdded: number | undefined; } @@ -674,8 +674,9 @@ export class DatabaseManager extends DisposableObject { } public removeDatabaseItem(item: DatabaseItem) { - if (this._currentDatabaseItem == item) + if (this._currentDatabaseItem == item) { this._currentDatabaseItem = undefined; + } const index = this.databaseItems.findIndex(searchItem => searchItem === item); if (index >= 0) { this._databaseItems.splice(index, 1); @@ -683,8 +684,10 @@ export class DatabaseManager extends DisposableObject { this.updatePersistedDatabaseList(); // Delete folder from workspace, if it is still there - const folderIndex = (vscode.workspace.workspaceFolders || []).findIndex(folder => item.belongsToSourceArchiveExplorerUri(folder.uri)); - if (index >= 0) { + const folderIndex = (vscode.workspace.workspaceFolders || []).findIndex( + folder => item.belongsToSourceArchiveExplorerUri(folder.uri) + ); + if (folderIndex >= 0) { logger.log(`Removing workspace folder at index ${folderIndex}`); vscode.workspace.updateWorkspaceFolders(folderIndex, 1); } @@ -692,9 +695,9 @@ export class DatabaseManager extends DisposableObject { // Delete folder from file system only if it is controlled by the extension if (this.isExtensionControlledLocation(item.databaseUri)) { logger.log('Deleting database from filesystem.'); - fs.remove(item.databaseUri.path).then( - () => logger.log(`Deleted '${item.databaseUri.path}'`), - e => logger.log(`Failed to delete '${item.databaseUri.path}'. Reason: ${e.message}`)); + fs.remove(item.databaseUri.fsPath).then( + () => logger.log(`Deleted '${item.databaseUri.fsPath}'`), + e => logger.log(`Failed to delete '${item.databaseUri.fsPath}'. Reason: ${e.message}`)); } // note that we use undefined as the item in order to reset the entire tree @@ -715,7 +718,13 @@ export class DatabaseManager extends DisposableObject { private isExtensionControlledLocation(uri: vscode.Uri) { const storagePath = this.ctx.storagePath || this.ctx.globalStoragePath; - return uri.path.startsWith(storagePath); + // the uri.fsPath function on windows returns a lowercase drive letter, + // but storagePath will have an uppercase drive letter. Be sure to compare + // URIs to URIs only + if (storagePath) { + return uri.fsPath.startsWith(vscode.Uri.file(storagePath).fsPath); + } + return false; } } diff --git a/extensions/ql-vscode/src/vscode-tests/minimal-workspace/databases.test.ts b/extensions/ql-vscode/src/vscode-tests/minimal-workspace/databases.test.ts new file mode 100644 index 00000000000..ce08866a94a --- /dev/null +++ b/extensions/ql-vscode/src/vscode-tests/minimal-workspace/databases.test.ts @@ -0,0 +1,309 @@ +import 'vscode-test'; +import 'mocha'; +import * as sinon from 'sinon'; +import * as tmp from 'tmp'; +import * as fs from 'fs-extra'; +import * as path from 'path'; +import { expect } from 'chai'; +import { ExtensionContext, Uri, workspace } from 'vscode'; + +import { + DatabaseEventKind, + DatabaseManager, + DatabaseItemImpl, + DatabaseContents, + isLikelyDbLanguageFolder, + FullDatabaseOptions +} from '../../databases'; +import { QueryServerConfig } from '../../config'; +import { Logger } from '../../logging'; +import { encodeArchiveBasePath, encodeSourceArchiveUri } from '../../archive-filesystem-provider'; + +describe('databases', () => { + + const MOCK_DB_OPTIONS: FullDatabaseOptions = { + dateAdded: 123, + ignoreSourceArchive: false + }; + + let databaseManager: DatabaseManager; + let updateSpy: sinon.SinonSpy; + let getSpy: sinon.SinonStub; + let dbChangedHandler: sinon.SinonSpy; + + let sandbox: sinon.SinonSandbox; + let dir: tmp.DirResult; + + + + beforeEach(() => { + dir = tmp.dirSync(); + + sandbox = sinon.createSandbox(); + updateSpy = sandbox.spy(); + getSpy = sandbox.stub(); + dbChangedHandler = sandbox.spy(); + databaseManager = new DatabaseManager( + { + workspaceState: { + update: updateSpy, + get: getSpy + }, + // pretend like databases added in the temp dir are controlled by the extension + // so that they are deleted upon removal + storagePath: dir.name + } as unknown as ExtensionContext, + {} as QueryServerConfig, + {} as Logger, + ); + + // Unfortunately, during a test it is not possible to convert from + // a single root workspace to multi-root, so must stub out relevant + // functions + sandbox.stub(workspace, 'updateWorkspaceFolders'); + sandbox.spy(workspace, 'onDidChangeWorkspaceFolders'); + }); + + afterEach(async () => { + dir.removeCallback(); + sandbox.restore(); + }); + + it('should fire events when adding and removing a db item', () => { + const mockDbItem = createMockDB(); + const spy = sinon.spy(); + databaseManager.onDidChangeDatabaseItem(spy); + (databaseManager as any).addDatabaseItem(mockDbItem); + + expect((databaseManager as any)._databaseItems).to.deep.eq([mockDbItem]); + expect(updateSpy).to.have.been.calledWith('databaseList', [{ + options: MOCK_DB_OPTIONS, + uri: dbLocationUri().toString(true) + }]); + expect(spy).to.have.been.calledWith({ + item: undefined, + kind: DatabaseEventKind.Add + }); + + sinon.reset(); + + // now remove the item + databaseManager.removeDatabaseItem(mockDbItem); + expect((databaseManager as any)._databaseItems).to.deep.eq([]); + expect(updateSpy).to.have.been.calledWith('databaseList', []); + expect(spy).to.have.been.calledWith({ + item: undefined, + kind: DatabaseEventKind.Remove + }); + }); + + it('should rename a db item and emit an event', () => { + const mockDbItem = createMockDB(); + const spy = sinon.spy(); + databaseManager.onDidChangeDatabaseItem(spy); + (databaseManager as any).addDatabaseItem(mockDbItem); + sinon.restore(); + + databaseManager.renameDatabaseItem(mockDbItem, 'new name'); + + expect(mockDbItem.name).to.eq('new name'); + expect(updateSpy).to.have.been.calledWith('databaseList', [{ + options: { ...MOCK_DB_OPTIONS, displayName: 'new name' }, + uri: dbLocationUri().toString(true) + }]); + + expect(spy).to.have.been.calledWith({ + item: undefined, + kind: DatabaseEventKind.Rename + }); + }); + + describe('add / remove database items', () => { + it('should add a database item', async () => { + const spy = sandbox.spy(); + databaseManager.onDidChangeDatabaseItem(spy); + const mockDbItem = createMockDB(); + + await (databaseManager as any).addDatabaseItem(mockDbItem); + + expect(databaseManager.databaseItems).to.deep.eq([mockDbItem]); + expect(updateSpy).to.have.been.calledWith('databaseList', [{ + uri: dbLocationUri().toString(true), + options: MOCK_DB_OPTIONS + }]); + + const mockEvent = { + item: undefined, + kind: DatabaseEventKind.Add + }; + expect(spy).to.have.been.calledWith(mockEvent); + }); + + it('should add a database item source archive', async function() { + const mockDbItem = createMockDB(); + mockDbItem.name = 'xxx'; + await (databaseManager as any).addDatabaseSourceArchiveFolder(mockDbItem); + + // workspace folders should be updated. We can only check the mocks since + // when running as a test, we are not allowed to update the workspace folders + expect(workspace.updateWorkspaceFolders).to.have.been.calledWith(1, 0, { + name: '[xxx source archive]', + // must use a matcher here since vscode URIs with the same path + // are not always equal due to internal state. + uri: sinon.match.has('fsPath', encodeArchiveBasePath(sourceLocationUri().fsPath).fsPath) + }); + }); + + it('should remove a database item', async () => { + const mockDbItem = createMockDB(); + sandbox.stub(fs, 'remove').resolves(); + + // pretend that this item is the first workspace folder in the list + sandbox.stub(mockDbItem, 'belongsToSourceArchiveExplorerUri').returns(true); + + await (databaseManager as any).addDatabaseItem(mockDbItem); + updateSpy.resetHistory(); + + await databaseManager.removeDatabaseItem(mockDbItem); + + expect(databaseManager.databaseItems).to.deep.eq([]); + expect(updateSpy).to.have.been.calledWith('databaseList', []); + // should remove the folder + expect(workspace.updateWorkspaceFolders).to.have.been.calledWith(0, 1); + + // should also delete the db contents + expect(fs.remove).to.have.been.calledWith(mockDbItem.databaseUri.fsPath); + }); + + it('should remove a database item outside of the extension controlled area', async () => { + const mockDbItem = createMockDB(); + sandbox.stub(fs, 'remove').resolves(); + + // pretend that this item is the first workspace folder in the list + sandbox.stub(mockDbItem, 'belongsToSourceArchiveExplorerUri').returns(true); + + await (databaseManager as any).addDatabaseItem(mockDbItem); + updateSpy.resetHistory(); + + // pretend that the database location is not controlled by the extension + (databaseManager as any).ctx.storagePath = 'hucairz'; + + await databaseManager.removeDatabaseItem(mockDbItem); + + expect(databaseManager.databaseItems).to.deep.eq([]); + expect(updateSpy).to.have.been.calledWith('databaseList', []); + // should remove the folder + expect(workspace.updateWorkspaceFolders).to.have.been.calledWith(0, 1); + + // should NOT delete the db contents + expect(fs.remove).not.to.have.been.called; + }); + }); + + describe('resolveSourceFile', () => { + it('should fail to resolve when not a uri', () => { + const db = createMockDB(Uri.parse('file:/sourceArchive-uri/')); + (db as any)._contents.sourceArchiveUri = undefined; + expect(() => db.resolveSourceFile('abc')).to.throw('Scheme is missing'); + }); + + it('should fail to resolve when not a file uri', () => { + const db = createMockDB(Uri.parse('file:/sourceArchive-uri/')); + (db as any)._contents.sourceArchiveUri = undefined; + expect(() => db.resolveSourceFile('http://abc')).to.throw('Invalid uri scheme'); + }); + + describe('no source archive', () => { + it('should resolve undefined', () => { + const db = createMockDB(Uri.parse('file:/sourceArchive-uri/')); + (db as any)._contents.sourceArchiveUri = undefined; + const resolved = db.resolveSourceFile(undefined); + expect(resolved.toString(true)).to.eq(dbLocationUri().toString(true)); + }); + + it('should resolve an empty file', () => { + const db = createMockDB(Uri.parse('file:/sourceArchive-uri/')); + (db as any)._contents.sourceArchiveUri = undefined; + const resolved = db.resolveSourceFile('file:'); + expect(resolved.toString()).to.eq('file:///'); + }); + }); + + describe('zipped source archive', () => { + it('should encode a source archive url', () => { + const db = createMockDB(encodeSourceArchiveUri({ + sourceArchiveZipPath: 'sourceArchive-uri', + pathWithinSourceArchive: 'def' + })); + const resolved = db.resolveSourceFile(Uri.file('abc').toString()); + + // must recreate an encoded archive uri instead of typing out the + // text since the uris will be different on windows and ubuntu. + expect(resolved.toString()).to.eq(encodeSourceArchiveUri({ + sourceArchiveZipPath: 'sourceArchive-uri', + pathWithinSourceArchive: 'def/abc' + }).toString()); + }); + + it('should encode a source archive url with trailing slash', () => { + const db = createMockDB(encodeSourceArchiveUri({ + sourceArchiveZipPath: 'sourceArchive-uri', + pathWithinSourceArchive: 'def/' + })); + const resolved = db.resolveSourceFile(Uri.file('abc').toString()); + + // must recreate an encoded archive uri instead of typing out the + // text since the uris will be different on windows and ubuntu. + expect(resolved.toString()).to.eq(encodeSourceArchiveUri({ + sourceArchiveZipPath: 'sourceArchive-uri', + pathWithinSourceArchive: 'def/abc' + }).toString()); + }); + + it('should encode an empty source archive url', () => { + const db = createMockDB(encodeSourceArchiveUri({ + sourceArchiveZipPath: 'sourceArchive-uri', + pathWithinSourceArchive: 'def' + })); + const resolved = db.resolveSourceFile('file:'); + expect(resolved.toString()).to.eq('codeql-zip-archive://1-18/sourceArchive-uri/def/'); + }); + }); + + it('should handle an empty file', () => { + const db = createMockDB(Uri.parse('file:/sourceArchive-uri/')); + const resolved = db.resolveSourceFile(''); + expect(resolved.toString()).to.eq('file:///sourceArchive-uri/'); + }); + }); + + it('should find likely db language folders', () => { + expect(isLikelyDbLanguageFolder('db-javascript')).to.be.true; + expect(isLikelyDbLanguageFolder('dbnot-a-db')).to.be.false; + }); + + function createMockDB( + // source archive location must be a real(-ish) location since + // tests will add this to the workspace location + sourceArchiveUri = sourceLocationUri(), + databaseUri = dbLocationUri() + ): DatabaseItemImpl { + + return new DatabaseItemImpl( + databaseUri, + { + sourceArchiveUri + } as DatabaseContents, + MOCK_DB_OPTIONS, + dbChangedHandler + ); + } + + function sourceLocationUri() { + return Uri.file(path.join(dir.name, 'src.zip')); + } + + function dbLocationUri() { + return Uri.file(path.join(dir.name, 'db')); + } +}); diff --git a/extensions/ql-vscode/src/vscode-tests/minimal-workspace/index.ts b/extensions/ql-vscode/src/vscode-tests/minimal-workspace/index.ts index 26b7bcc6456..320ed14b6da 100644 --- a/extensions/ql-vscode/src/vscode-tests/minimal-workspace/index.ts +++ b/extensions/ql-vscode/src/vscode-tests/minimal-workspace/index.ts @@ -1,4 +1,12 @@ import { runTestsInDirectory } from '../index-template'; + +import * as sinonChai from 'sinon-chai'; +import * as chai from 'chai'; +import * as chaiAsPromised from 'chai-as-promised'; +chai.use(chaiAsPromised); +chai.use(sinonChai); + + export function run(): Promise { return runTestsInDirectory(__dirname); } diff --git a/extensions/ql-vscode/src/vscode-tests/no-workspace/databases.test.ts b/extensions/ql-vscode/src/vscode-tests/no-workspace/databases.test.ts deleted file mode 100644 index 6bbbada3eb2..00000000000 --- a/extensions/ql-vscode/src/vscode-tests/no-workspace/databases.test.ts +++ /dev/null @@ -1,188 +0,0 @@ -import 'vscode-test'; -import 'mocha'; -import * as sinon from 'sinon'; -import { expect } from 'chai'; -import { ExtensionContext, Uri } from 'vscode'; - -import { - DatabaseEventKind, - DatabaseItem, - DatabaseManager, - DatabaseItemImpl, - DatabaseContents, - isLikelyDbLanguageFolder -} from '../../databases'; -import { QueryServerConfig } from '../../config'; -import { Logger } from '../../logging'; -import { encodeSourceArchiveUri } from '../../archive-filesystem-provider'; - -describe('databases', () => { - let databaseManager: DatabaseManager; - let updateSpy: sinon.SinonSpy; - - beforeEach(() => { - updateSpy = sinon.spy(); - databaseManager = new DatabaseManager( - { - workspaceState: { - update: updateSpy, - get: sinon.stub() - }, - } as unknown as ExtensionContext, - {} as QueryServerConfig, - {} as Logger, - ); - }); - - it('should fire events when adding and removing a db item', () => { - const mockDbItem = { - databaseUri: { path: 'file:/abc' }, - name: 'abc', - getPersistedState() { - return this.name; - } - }; - const spy = sinon.spy(); - databaseManager.onDidChangeDatabaseItem(spy); - (databaseManager as any).addDatabaseItem(mockDbItem); - - expect((databaseManager as any)._databaseItems).to.deep.eq([mockDbItem]); - expect(updateSpy).to.have.been.calledWith('databaseList', ['abc']); - expect(spy).to.have.been.calledWith({ - item: undefined, - kind: DatabaseEventKind.Add - }); - - sinon.reset(); - - // now remove the item - databaseManager.removeDatabaseItem(mockDbItem as unknown as DatabaseItem); - expect((databaseManager as any)._databaseItems).to.deep.eq([]); - expect(updateSpy).to.have.been.calledWith('databaseList', []); - expect(spy).to.have.been.calledWith({ - item: undefined, - kind: DatabaseEventKind.Remove - }); - }); - - it('should rename a db item and emit an event', () => { - const mockDbItem = { - databaseUri: 'file:/abc', - name: 'abc', - getPersistedState() { - return this.name; - } - }; - const spy = sinon.spy(); - databaseManager.onDidChangeDatabaseItem(spy); - (databaseManager as any).addDatabaseItem(mockDbItem); - sinon.restore(); - - databaseManager.renameDatabaseItem(mockDbItem as unknown as DatabaseItem, 'new name'); - - expect(mockDbItem.name).to.eq('new name'); - expect(updateSpy).to.have.been.calledWith('databaseList', ['new name']); - expect(spy).to.have.been.calledWith({ - item: undefined, - kind: DatabaseEventKind.Rename - }); - }); - - describe('resolveSourceFile', () => { - it('should fail to resolve when not a uri', () => { - const db = createMockDB(Uri.parse('file:/sourceArchive-uri/')); - (db as any)._contents.sourceArchiveUri = undefined; - expect(() => db.resolveSourceFile('abc')).to.throw('Scheme is missing'); - }); - - it('should fail to resolve when not a file uri', () => { - const db = createMockDB(Uri.parse('file:/sourceArchive-uri/')); - (db as any)._contents.sourceArchiveUri = undefined; - expect(() => db.resolveSourceFile('http://abc')).to.throw('Invalid uri scheme'); - }); - - describe('no source archive', () => { - it('should resolve undefined', () => { - const db = createMockDB(Uri.parse('file:/sourceArchive-uri/')); - (db as any)._contents.sourceArchiveUri = undefined; - const resolved = db.resolveSourceFile(undefined); - expect(resolved.toString()).to.eq('file:///database-uri'); - }); - - it('should resolve an empty file', () => { - const db = createMockDB(Uri.parse('file:/sourceArchive-uri/')); - (db as any)._contents.sourceArchiveUri = undefined; - const resolved = db.resolveSourceFile('file:'); - expect(resolved.toString()).to.eq('file:///'); - }); - }); - - describe('zipped source archive', () => { - it('should encode a source archive url', () => { - const db = createMockDB(encodeSourceArchiveUri({ - sourceArchiveZipPath: 'sourceArchive-uri', - pathWithinSourceArchive: 'def' - })); - const resolved = db.resolveSourceFile(Uri.file('abc').toString()); - - // must recreate an encoded archive uri instead of typing out the - // text since the uris will be different on windows and ubuntu. - expect(resolved.toString()).to.eq(encodeSourceArchiveUri({ - sourceArchiveZipPath: 'sourceArchive-uri', - pathWithinSourceArchive: 'def/abc' - }).toString()); - }); - - it('should encode a source archive url with trailing slash', () => { - const db = createMockDB(encodeSourceArchiveUri({ - sourceArchiveZipPath: 'sourceArchive-uri', - pathWithinSourceArchive: 'def/' - })); - const resolved = db.resolveSourceFile(Uri.file('abc').toString()); - - // must recreate an encoded archive uri instead of typing out the - // text since the uris will be different on windows and ubuntu. - expect(resolved.toString()).to.eq(encodeSourceArchiveUri({ - sourceArchiveZipPath: 'sourceArchive-uri', - pathWithinSourceArchive: 'def/abc' - }).toString()); - }); - - it('should encode an empty source archive url', () => { - const db = createMockDB(encodeSourceArchiveUri({ - sourceArchiveZipPath: 'sourceArchive-uri', - pathWithinSourceArchive: 'def' - })); - const resolved = db.resolveSourceFile('file:'); - expect(resolved.toString()).to.eq('codeql-zip-archive://1-18/sourceArchive-uri/def/'); - }); - }); - - it('should handle an empty file', () => { - const db = createMockDB(Uri.parse('file:/sourceArchive-uri/')); - const resolved = db.resolveSourceFile(''); - expect(resolved.toString()).to.eq('file:///sourceArchive-uri/'); - }); - function createMockDB( - sourceArchiveUri = Uri.parse('file:/sourceArchive-uri'), - databaseUri = Uri.parse('file:/database-uri') - ) { - return new DatabaseItemImpl( - databaseUri, - { - sourceArchiveUri - } as DatabaseContents, - { - dateAdded: 123, - ignoreSourceArchive: false - }, - () => { /**/ } - ); - } - }); - - it('should find likely db language folders', () => { - expect(isLikelyDbLanguageFolder('db-javascript')).to.be.true; - expect(isLikelyDbLanguageFolder('dbnot-a-db')).to.be.false; - }); -}); diff --git a/extensions/ql-vscode/src/vscode-tests/no-workspace/query-history.test.ts b/extensions/ql-vscode/src/vscode-tests/no-workspace/query-history.test.ts index 9af8f6aed86..c5cb4b1cbaf 100644 --- a/extensions/ql-vscode/src/vscode-tests/no-workspace/query-history.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/no-workspace/query-history.test.ts @@ -19,28 +19,26 @@ describe('query-history', () => { let showQuickPickSpy: sinon.SinonStub; let tryOpenExternalFile: Function; + let sandbox: sinon.SinonSandbox; beforeEach(() => { - showTextDocumentSpy = sinon.stub(vscode.window, 'showTextDocument'); - showInformationMessageSpy = sinon.stub( + sandbox = sinon.createSandbox(); + showTextDocumentSpy = sandbox.stub(vscode.window, 'showTextDocument'); + showInformationMessageSpy = sandbox.stub( vscode.window, 'showInformationMessage' ); - showQuickPickSpy = sinon.stub( + showQuickPickSpy = sandbox.stub( vscode.window, 'showQuickPick' ); - executeCommandSpy = sinon.stub(vscode.commands, 'executeCommand'); - sinon.stub(logger, 'log'); + executeCommandSpy = sandbox.stub(vscode.commands, 'executeCommand'); + sandbox.stub(logger, 'log'); tryOpenExternalFile = (QueryHistoryManager.prototype as any).tryOpenExternalFile; }); afterEach(() => { - (vscode.window.showTextDocument as sinon.SinonStub).restore(); - (vscode.commands.executeCommand as sinon.SinonStub).restore(); - (logger.log as sinon.SinonStub).restore(); - (vscode.window.showInformationMessage as sinon.SinonStub).restore(); - (vscode.window.showQuickPick as sinon.SinonStub).restore(); + sandbox.restore(); }); describe('tryOpenExternalFile', () => {