From 6b1ef0e2ad062cc2477e4a31189a54f5b80793a0 Mon Sep 17 00:00:00 2001 From: Charles Gagnon Date: Thu, 28 Jul 2022 11:13:18 -0700 Subject: [PATCH] Store query history items as data, not tree node (#20195) --- extensions/query-history/src/main.ts | 20 +++---- .../query-history/src/queryHistoryItem.ts | 13 ++++ .../query-history/src/queryHistoryNode.ts | 22 ------- .../query-history/src/queryHistoryProvider.ts | 37 +++++++----- .../src/test/queryHistoryProvider.test.ts | 60 +++++++++---------- 5 files changed, 75 insertions(+), 77 deletions(-) create mode 100644 extensions/query-history/src/queryHistoryItem.ts delete mode 100644 extensions/query-history/src/queryHistoryNode.ts diff --git a/extensions/query-history/src/main.ts b/extensions/query-history/src/main.ts index 2ca3c57506..2b95dfeb71 100644 --- a/extensions/query-history/src/main.ts +++ b/extensions/query-history/src/main.ts @@ -5,29 +5,29 @@ import * as azdata from 'azdata'; import * as vscode from 'vscode'; -import { QueryHistoryNode } from './queryHistoryNode'; +import { QueryHistoryItem } from './queryHistoryItem'; import { QueryHistoryProvider } from './queryHistoryProvider'; export async function activate(context: vscode.ExtensionContext): Promise { const provider = new QueryHistoryProvider(); context.subscriptions.push(provider); context.subscriptions.push(vscode.window.registerTreeDataProvider('queryHistory', provider)); - context.subscriptions.push(vscode.commands.registerCommand('queryHistory.open', async (node: QueryHistoryNode) => { + context.subscriptions.push(vscode.commands.registerCommand('queryHistory.open', async (item: QueryHistoryItem) => { return azdata.queryeditor.openQueryDocument( { - content: node.queryText - }, node.connectionProfile?.providerId); + content: item.queryText + }, item.connectionProfile?.providerId); })); - context.subscriptions.push(vscode.commands.registerCommand('queryHistory.run', async (node: QueryHistoryNode) => { + context.subscriptions.push(vscode.commands.registerCommand('queryHistory.run', async (item: QueryHistoryItem) => { const doc = await azdata.queryeditor.openQueryDocument( { - content: node.queryText - }, node.connectionProfile?.providerId); - await azdata.queryeditor.connect(doc.uri, node.connectionProfile?.connectionId || ''); + content: item.queryText + }, item.connectionProfile?.providerId); + await azdata.queryeditor.connect(doc.uri, item.connectionProfile?.connectionId || ''); azdata.queryeditor.runQuery(doc.uri); })); - context.subscriptions.push(vscode.commands.registerCommand('queryHistory.delete', (node: QueryHistoryNode) => { - provider.deleteNode(node); + context.subscriptions.push(vscode.commands.registerCommand('queryHistory.delete', (item: QueryHistoryItem) => { + provider.deleteItem(item); })); context.subscriptions.push(vscode.commands.registerCommand('queryHistory.clear', () => { provider.clearAll(); diff --git a/extensions/query-history/src/queryHistoryItem.ts b/extensions/query-history/src/queryHistoryItem.ts new file mode 100644 index 0000000000..2129f166f7 --- /dev/null +++ b/extensions/query-history/src/queryHistoryItem.ts @@ -0,0 +1,13 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the Source EULA. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import * as azdata from 'azdata'; + +export interface QueryHistoryItem { + readonly queryText: string, + readonly connectionProfile: azdata.connection.ConnectionProfile | undefined, + readonly timestamp: Date, + readonly isSuccess: boolean +} diff --git a/extensions/query-history/src/queryHistoryNode.ts b/extensions/query-history/src/queryHistoryNode.ts deleted file mode 100644 index 08d4fdbd7b..0000000000 --- a/extensions/query-history/src/queryHistoryNode.ts +++ /dev/null @@ -1,22 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Microsoft Corporation. All rights reserved. - * Licensed under the Source EULA. See License.txt in the project root for license information. - *--------------------------------------------------------------------------------------------*/ - -import * as azdata from 'azdata'; -import * as vscode from 'vscode'; -import { removeNewLines } from './utils'; - -export class QueryHistoryNode extends vscode.TreeItem { - constructor( - public readonly queryText: string, - public readonly connectionProfile: azdata.connection.ConnectionProfile | undefined, - timestamp: Date, - isSuccess: boolean - ) { - super(removeNewLines(queryText), vscode.TreeItemCollapsibleState.None); - this.iconPath = isSuccess ? new vscode.ThemeIcon('check', new vscode.ThemeColor('testing.iconPassed')) : new vscode.ThemeIcon('error', new vscode.ThemeColor('testing.iconFailed')); - this.tooltip = queryText; - this.description = connectionProfile ? `${connectionProfile.serverName}|${connectionProfile.databaseName} ${timestamp.toLocaleString()}` : timestamp.toLocaleString(); - } -} diff --git a/extensions/query-history/src/queryHistoryProvider.ts b/extensions/query-history/src/queryHistoryProvider.ts index ad16bd83bf..c737e07f74 100644 --- a/extensions/query-history/src/queryHistoryProvider.ts +++ b/extensions/query-history/src/queryHistoryProvider.ts @@ -6,18 +6,21 @@ import * as vscode from 'vscode'; import * as azdata from 'azdata'; import { EOL } from 'os'; -import { QueryHistoryNode } from './queryHistoryNode'; +import { QueryHistoryItem } from './queryHistoryItem'; +import { removeNewLines } from './utils'; const QUERY_HISTORY_CONFIG_SECTION = 'queryHistory'; const CAPTURE_ENABLED_CONFIG_SECTION = 'captureEnabled'; const DEFAULT_CAPTURE_ENABLED = true; +const successIcon = new vscode.ThemeIcon('check', new vscode.ThemeColor('testing.iconPassed')); +const failedIcon = new vscode.ThemeIcon('error', new vscode.ThemeColor('testing.iconFailed')); -export class QueryHistoryProvider implements vscode.TreeDataProvider, vscode.Disposable { +export class QueryHistoryProvider implements vscode.TreeDataProvider, vscode.Disposable { - private _onDidChangeTreeData: vscode.EventEmitter = new vscode.EventEmitter(); - readonly onDidChangeTreeData: vscode.Event = this._onDidChangeTreeData.event; + private _onDidChangeTreeData: vscode.EventEmitter = new vscode.EventEmitter(); + readonly onDidChangeTreeData: vscode.Event = this._onDidChangeTreeData.event; - private _queryHistoryNodes: QueryHistoryNode[] = []; + private _queryHistoryItems: QueryHistoryItem[] = []; private _captureEnabled: boolean = true; private _disposables: vscode.Disposable[] = []; @@ -37,10 +40,10 @@ export class QueryHistoryProvider implements vscode.TreeDataProvider textDocument.getText(r) ?? '').join(EOL); - const connProfile = await azdata.connection.getConnection(document.uri); - const isError = queryInfo.messages.find(m => m.isError) ? false : true; + const connectionProfile = await azdata.connection.getConnection(document.uri); + const isSuccess = queryInfo.messages.find(m => m.isError) ? false : true; // Add to the front of the list so the new item appears at the top - this._queryHistoryNodes.unshift(new QueryHistoryNode(queryText, connProfile, new Date(), isError)); + this._queryHistoryItems.unshift({ queryText, connectionProfile, timestamp: new Date(), isSuccess }); this._onDidChangeTreeData.fire(undefined); } } @@ -54,21 +57,25 @@ export class QueryHistoryProvider implements vscode.TreeDataProvider n !== node); + public deleteItem(item: QueryHistoryItem): void { + this._queryHistoryItems = this._queryHistoryItems.filter(n => n !== item); this._onDidChangeTreeData.fire(undefined); } - public getTreeItem(node: QueryHistoryNode): vscode.TreeItem { - return node; + public getTreeItem(item: QueryHistoryItem): vscode.TreeItem { + const treeItem = new vscode.TreeItem(removeNewLines(item.queryText), vscode.TreeItemCollapsibleState.None); + treeItem.iconPath = item.isSuccess ? successIcon : failedIcon; + treeItem.tooltip = item.queryText; + treeItem.description = item.connectionProfile ? `${item.connectionProfile.serverName}|${item.connectionProfile.databaseName} ${item.timestamp.toLocaleString()}` : item.timestamp.toLocaleString(); + return treeItem; } - public getChildren(element?: QueryHistoryNode): QueryHistoryNode[] { + public getChildren(element?: QueryHistoryItem): QueryHistoryItem[] { // We only have top level items - return this._queryHistoryNodes; + return this._queryHistoryItems; } public dispose(): void { diff --git a/extensions/query-history/src/test/queryHistoryProvider.test.ts b/extensions/query-history/src/test/queryHistoryProvider.test.ts index 18dc3772fc..9465823078 100644 --- a/extensions/query-history/src/test/queryHistoryProvider.test.ts +++ b/extensions/query-history/src/test/queryHistoryProvider.test.ts @@ -10,7 +10,7 @@ import 'mocha'; import * as sinon from 'sinon'; import * as azdataTest from '@microsoft/azdata-test'; import { QueryHistoryProvider } from '../queryHistoryProvider'; -import { QueryHistoryNode } from '../queryHistoryNode'; +import { QueryHistoryItem } from '../queryHistoryItem'; import { EOL } from 'os'; describe('QueryHistoryProvider', () => { @@ -77,7 +77,7 @@ describe('QueryHistoryProvider', () => { }); const children = testProvider.getChildren(); should(children).length(1, 'Should have one child after adding item'); - should(children[0].queryText).be.equal(`${rangeWithContent1.content}${EOL}${rangeWithContent2.content}`, 'node content should be combined from both source ranges'); + should(children[0].queryText).be.equal(`${rangeWithContent1.content}${EOL}${rangeWithContent2.content}`, 'item content should be combined from both source ranges'); }); it('event with errors is marked as error', async function () { @@ -87,7 +87,7 @@ describe('QueryHistoryProvider', () => { await fireQueryEventAndWaitForRefresh('queryStop', { uri: testUri.toString() }, { messages: [ message1, message2, message3 ], batchRanges: []}); const children = testProvider.getChildren(); should(children).length(1, 'Should have one child after adding item'); - should((children[0].iconPath).id).be.equal('error', 'Event with errors should have error icon'); + should(children[0].isSuccess).be.false('Event with errors should have error icon'); }); it('event without errors is marked as success', async function () { @@ -97,12 +97,12 @@ describe('QueryHistoryProvider', () => { await fireQueryEventAndWaitForRefresh('queryStop', { uri: testUri.toString() }, { messages: [ message1, message2, message3 ], batchRanges: []}); const children = testProvider.getChildren(); should(children).length(1, 'Should have one child after adding item'); - should((children[0].iconPath).id).be.equal('check', 'Event without errors should have check icon'); + should(children[0].isSuccess).be.true('Event without errors should have check icon'); }); it('queryStop events from unknown document are ignored', async function () { const unknownUri = vscode.Uri.parse('untitled://query2'); - // Since we didn't find the text document we'll never update the node list so add a timeout since that event will never fire + // Since we didn't find the text document we'll never update the item list so add a timeout since that event will never fire await fireQueryEventAndWaitForRefresh('queryStop', { uri: unknownUri.toString() }, { messages: [], batchRanges: [] }, 2000); const children = testProvider.getChildren(); should(children).length(0, 'Should not have any children'); @@ -113,7 +113,7 @@ describe('QueryHistoryProvider', () => { let children = testProvider.getChildren(); should(children).length(1, 'Should have one child after adding item'); - await waitForNodeRefresh(() => testProvider.clearAll()); + await waitForItemRefresh(() => testProvider.clearAll()); children = testProvider.getChildren(); should(children).length(0, 'Should have no children after clearing'); }); @@ -125,53 +125,53 @@ describe('QueryHistoryProvider', () => { let children = testProvider.getChildren(); should(children).length(3, 'Should have 3 children after adding item'); - await waitForNodeRefresh(() => testProvider.clearAll()); + await waitForItemRefresh(() => testProvider.clearAll()); children = testProvider.getChildren(); should(children).length(0, 'Should have no children after clearing'); }); - it('delete node when no nodes doesn\'t throw', async function () { - const testNode: QueryHistoryNode = { queryText: 'SELECT 1', connectionProfile: azdataTest.stubs.connectionProfile.createConnectionProfile() }; - await waitForNodeRefresh(() => testProvider.deleteNode(testNode)); + it('delete item when no items doesn\'t throw', async function () { + const testItem: QueryHistoryItem = { queryText: 'SELECT 1', connectionProfile: azdataTest.stubs.connectionProfile.createConnectionProfile(), timestamp: new Date(), isSuccess: true }; + await waitForItemRefresh(() => testProvider.deleteItem(testItem)); const children = testProvider.getChildren(); - should(children).length(0, 'Should have no children after deleting node'); + should(children).length(0, 'Should have no children after deleting item'); }); - it('delete node that doesn\'t exist doesn\'t throw', async function () { + it('delete item that doesn\'t exist doesn\'t throw', async function () { await fireQueryEventAndWaitForRefresh('queryStop', { uri: testUri.toString() }, { messages: [], batchRanges: [] }); let children = testProvider.getChildren(); should(children).length(1, 'Should have 1 child initially'); - const testNode: QueryHistoryNode = { queryText: 'SELECT 1', connectionProfile: azdataTest.stubs.connectionProfile.createConnectionProfile() }; - await waitForNodeRefresh(() => testProvider.deleteNode(testNode)); + const testItem: QueryHistoryItem = { queryText: 'SELECT 1', connectionProfile: azdataTest.stubs.connectionProfile.createConnectionProfile(), timestamp: new Date(), isSuccess: true }; + await waitForItemRefresh(() => testProvider.deleteItem(testItem)); children = testProvider.getChildren(); - should(children).length(1, 'Should still have 1 child after deleting node'); + should(children).length(1, 'Should still have 1 child after deleting item'); }); - it('can delete single node', async function () { + it('can delete single item', async function () { await fireQueryEventAndWaitForRefresh('queryStop', { uri: testUri.toString() }, { messages: [], batchRanges: [] }); await fireQueryEventAndWaitForRefresh('queryStop', { uri: testUri.toString() }, { messages: [], batchRanges: [] }); await fireQueryEventAndWaitForRefresh('queryStop', { uri: testUri.toString() }, { messages: [], batchRanges: [] }); const firstChildren = testProvider.getChildren(); should(firstChildren).length(3, 'Should have 3 children initially'); - let nodeToDelete: QueryHistoryNode = firstChildren[1]; - await waitForNodeRefresh(() => testProvider.deleteNode(nodeToDelete)); + let itemToDelete: QueryHistoryItem = firstChildren[1]; + await waitForItemRefresh(() => testProvider.deleteItem(itemToDelete)); const secondChildren = testProvider.getChildren(); - should(secondChildren).length(2, 'Should still have 2 child after deleting node'); - should(secondChildren[0]).be.equal(firstChildren[0], 'First node should still exist after deleting first node'); - should(secondChildren[1]).be.equal(firstChildren[2], 'Second node should still exist after deleting first node'); + should(secondChildren).length(2, 'Should still have 2 child after deleting item'); + should(secondChildren[0]).be.equal(firstChildren[0], 'First item should still exist after deleting first item'); + should(secondChildren[1]).be.equal(firstChildren[2], 'Second item should still exist after deleting first item'); - nodeToDelete = secondChildren[0]; - await waitForNodeRefresh(() => testProvider.deleteNode(nodeToDelete)); + itemToDelete = secondChildren[0]; + await waitForItemRefresh(() => testProvider.deleteItem(itemToDelete)); const thirdChildren = testProvider.getChildren(); - should(thirdChildren).length(1, 'Should still have 1 child after deleting node'); - should(thirdChildren[0]).be.equal(secondChildren[1], 'Second node should still exist after deleting second node'); + should(thirdChildren).length(1, 'Should still have 1 child after deleting item'); + should(thirdChildren[0]).be.equal(secondChildren[1], 'Second item should still exist after deleting second item'); - nodeToDelete = thirdChildren[0]; - await waitForNodeRefresh(() => testProvider.deleteNode(nodeToDelete)); + itemToDelete = thirdChildren[0]; + await waitForItemRefresh(() => testProvider.deleteItem(itemToDelete)); const fourthChildren = testProvider.getChildren(); - should(fourthChildren).length(0, 'Should have no children after deleting all nodes'); + should(fourthChildren).length(0, 'Should have no children after deleting all items'); }); it('pausing capture causes children not to be added', async function () { @@ -192,10 +192,10 @@ describe('QueryHistoryProvider', () => { }); async function fireQueryEventAndWaitForRefresh(type: azdata.queryeditor.QueryEventType, document: azdata.queryeditor.QueryDocument, queryInfo: azdata.queryeditor.QueryInfo, timeoutMs?: number): Promise { - await waitForNodeRefresh(() => testListener.onQueryEvent(type, document, undefined, queryInfo), timeoutMs); + await waitForItemRefresh(() => testListener.onQueryEvent(type, document, undefined, queryInfo), timeoutMs); } - async function waitForNodeRefresh(func: Function, timeoutMs?: number): Promise { + async function waitForItemRefresh(func: Function, timeoutMs?: number): Promise { const promises: Promise[] = [azdataTest.helpers.eventToPromise(testProvider.onDidChangeTreeData)]; const timeoutPromise = timeoutMs ? new Promise(r => setTimeout(() => r(), timeoutMs)) : undefined; if (timeoutPromise) {