ckaczor/azuredatastudio
1
0
Fork 0
mirror of https://github.com/ckaczor/azuredatastudio.git synced 2026-01-13 17:22:15 -05:00
Code Issues Packages Projects Releases Wiki Activity

Fix query history text race condition (#20428)

Browse Source 
* Fix query history text race condition

* fix tests
This commit is contained in:
Charles Gagnon
2022-08-22 12:52:56 -07:00
committed by GitHub
parent e6bfdee521
commit 26f7aa5f5d
4 changed files with 88 additions and 50 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
extensions/query-history/package.json
View File

@@ -190,7 +190,7 @@
},
"dependencies": {},
"devDependencies": {
"@microsoft/azdata-test": "^1.5.2",
"@microsoft/azdata-test": "^2.0.0",
"@microsoft/vscodetestcover": "^1.2.1",
"@types/mocha": "^7.0.2",
"@types/node": "^12.11.7",

51
extensions/query-history/src/queryHistoryProvider.ts
View File

@@ -5,7 +5,6 @@
import * as vscode from 'vscode';
import * as azdata from 'azdata';
import { EOL } from 'os';
import { QueryHistoryItem } from './queryHistoryItem';
import { removeNewLines } from './utils';
import { CAPTURE_ENABLED_CONFIG_SECTION, ITEM_SELECTED_COMMAND_ID, QUERY_HISTORY_CONFIG_SECTION } from './constants';
@@ -24,26 +23,44 @@ export class QueryHistoryProvider implements vscode.TreeDataProvider<QueryHistor
private _disposables: vscode.Disposable[] = [];
/**
* Mapping of query URIs to the query text being executed
*/
private queryTextMappings: Map<string, string> = new Map<string, string>();
constructor() {
this._disposables.push(azdata.queryeditor.registerQueryEventListener({
onQueryEvent: async (type: azdata.queryeditor.QueryEventType, document: azdata.queryeditor.QueryDocument, args: azdata.ResultSetSummary | string | undefined, queryInfo?: azdata.queryeditor.QueryInfo) => {
if (this._captureEnabled && queryInfo && type === 'queryStop') {
const textDocuments = vscode.workspace.textDocuments;
// We need to compare URIs, but the event Uri comes in as string so while it should be in the same format as
// the textDocument uri.toString() we parse it into a vscode.Uri first to be absolutely sure.
const textDocument = textDocuments.find(e => e.uri.toString() === vscode.Uri.parse(document.uri).toString());
if (!textDocument) {
// If we couldn't find the document then we can't get the text so just log the error and move on
console.error(`Couldn't find text document with URI ${document.uri} for query event`);
return;
if (this._captureEnabled && queryInfo) {
if (type === 'queryStop') {
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
const queryText = this.queryTextMappings.get(document.uri);
if (queryText === undefined) {
console.error(`Couldn't find query text for URI ${document.uri.toString()}`);
return;
}
this.queryTextMappings.delete(document.uri);
this._queryHistoryItems.unshift({ queryText, connectionProfile, timestamp: new Date(), isSuccess });
this._onDidChangeTreeData.fire(undefined);
} else if (type === 'queryStart') {
// We get the text and save it on queryStart because we want to get the query text immediately when
// the query is started but then only add the item when it finishes (so that we can properly determine the success of the execution).
// This avoids a race condition with the text being modified during execution and ending up with the query text at the end being
// different than when it started.
const textEditor = vscode.window.activeTextEditor;
// We need to compare URIs, but the event Uri comes in as string so while it should be in the same format as
// the textDocument uri.toString() we parse it into a vscode.Uri first to be absolutely sure.
if (textEditor?.document.uri.toString() !== vscode.Uri.parse(document.uri).toString()) {
// If we couldn't find the document then we can't get the text so just log the error and move on
console.error(`Active text editor ${textEditor?.document.uri} does not match URI ${document.uri} for query event`);
return;
}
// Get the text from the current selection - or the entire document if there isn't a selection (mimicking what STS is doing itself)
const queryText = textEditor.document.getText(textEditor.selection.isEmpty ? undefined : textEditor.selection) ?? '';
this.queryTextMappings.set(document.uri, queryText);
}
// Combine all the text from the batches back together
const queryText = queryInfo.batchRanges.map(r => textDocument.getText(r) ?? '').join(EOL);
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._queryHistoryItems.unshift({ queryText, connectionProfile, timestamp: new Date(), isSuccess });
this._onDidChangeTreeData.fire(undefined);
}
}
}));

77
extensions/query-history/src/test/queryHistoryProvider.test.ts
View File

@@ -11,7 +11,6 @@ import * as sinon from 'sinon';
import * as azdataTest from '@microsoft/azdata-test';
import { QueryHistoryProvider } from '../queryHistoryProvider';
import { QueryHistoryItem } from '../queryHistoryItem';
import { EOL } from 'os';
describe('QueryHistoryProvider', () => {
@@ -57,44 +56,51 @@ describe('QueryHistoryProvider', () => {
});
it('queryStop events cause children to be added', async function () {
await fireQueryEventAndWaitForRefresh('queryStop', <any>{ uri: testUri.toString() }, { messages: [], batchRanges: [] });
setupTextEditorMock('SELECT 1');
await fireQueryStartAndStopAndWaitForRefresh(testUri);
const children = testProvider.getChildren();
should(children).length(1, 'Should have one child after adding item');
await fireQueryEventAndWaitForRefresh('queryStop', <any>{ uri: testUri.toString() }, { messages: [], batchRanges: [] });
await fireQueryStartAndStopAndWaitForRefresh(testUri);
should(children).length(2, 'Should have two children after adding another item');
});
it('multiple ranges are combined', async function () {
const rangeWithContent1: azdataTest.mocks.vscode.RangeWithContent = { range: new vscode.Range(new vscode.Position(0, 0), new vscode.Position(2, 0)), content: 'SELECT 1' };
const rangeWithContent2: azdataTest.mocks.vscode.RangeWithContent = { range: new vscode.Range(new vscode.Position(3, 0), new vscode.Position(3, 5)), content: 'SELECT 2' };
const textDocumentMock = azdataTest.mocks.vscode.createTextDocumentMock(testUri, [rangeWithContent1, rangeWithContent2]);
textDocumentSandbox.restore();
textDocumentSandbox.replaceGetter(vscode.workspace, 'textDocuments', () => [textDocumentMock.object]);
await fireQueryEventAndWaitForRefresh('queryStop', <any>{ uri: testUri.toString() }, {
messages: [],
batchRanges: [rangeWithContent1.range, rangeWithContent2.range]
});
it('no selection records entire text', async function () {
const content = 'SELECT 1\nSELECT 2';
setupTextEditorMock(content);
await fireQueryStartAndStopAndWaitForRefresh(testUri);
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}`, 'item content should be combined from both source ranges');
should(children[0].queryText).be.equal(content, 'item content should be full text content');
});
it('active selection records only selected text', async function () {
const rangeWithContent1: azdataTest.mocks.vscode.RangeWithContent = { range: new vscode.Range(new vscode.Position(0, 0), new vscode.Position(2, 0)), content: 'SELECT 1' };
const rangeWithContent2: azdataTest.mocks.vscode.RangeWithContent = { range: new vscode.Range(new vscode.Position(3, 0), new vscode.Position(3, 5)), content: 'SELECT 2' };
setupTextEditorMock([rangeWithContent1, rangeWithContent2], [new vscode.Selection(rangeWithContent1.range.start, rangeWithContent1.range.end)]);
await fireQueryStartAndStopAndWaitForRefresh(testUri);
const children = testProvider.getChildren();
should(children).length(1, 'Should have one child after adding item');
should(children[0].queryText).be.equal(rangeWithContent1.content, 'item content should be only active selection');
});
it('event with errors is marked as error', async function () {
setupTextEditorMock('SELECT 1');
const message1: azdata.queryeditor.QueryMessage = { message: 'Message 1', isError: false };
const message2: azdata.queryeditor.QueryMessage = { message: 'Error message', isError: true };
const message3: azdata.queryeditor.QueryMessage = { message: 'Message 2', isError: false };
await fireQueryEventAndWaitForRefresh('queryStop', <any>{ uri: testUri.toString() }, { messages: [ message1, message2, message3 ], batchRanges: []});
await fireQueryStartAndStopAndWaitForRefresh(testUri, { messages: [message1, message2, message3], batchRanges: [] });
const children = testProvider.getChildren();
should(children).length(1, 'Should have one child after adding item');
should(children[0].isSuccess).be.false('Event with errors should have error icon');
});
it('event without errors is marked as success', async function () {
setupTextEditorMock('SELECT 1');
const message1: azdata.queryeditor.QueryMessage = { message: 'Message 1', isError: false };
const message2: azdata.queryeditor.QueryMessage = { message: 'Message 2', isError: false };
const message3: azdata.queryeditor.QueryMessage = { message: 'Message 3', isError: false };
await fireQueryEventAndWaitForRefresh('queryStop', <any>{ uri: testUri.toString() }, { messages: [ message1, message2, message3 ], batchRanges: []});
await fireQueryStartAndStopAndWaitForRefresh(testUri, { messages: [message1, message2, message3], batchRanges: [] });
const children = testProvider.getChildren();
should(children).length(1, 'Should have one child after adding item');
should(children[0].isSuccess).be.true('Event without errors should have check icon');
@@ -102,14 +108,15 @@ describe('QueryHistoryProvider', () => {
it('queryStop events from unknown document are ignored', async function () {
const unknownUri = vscode.Uri.parse('untitled://query2');
const queryDocumentMock = azdataTest.mocks.azdata.queryeditor.createQueryDocumentMock(unknownUri.toString());
// 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', <any>{ uri: unknownUri.toString() }, { messages: [], batchRanges: [] }, 2000);
await fireQueryEventAndWaitForRefresh('queryStop', queryDocumentMock.object, { messages: [], batchRanges: [] }, 2000);
const children = testProvider.getChildren();
should(children).length(0, 'Should not have any children');
});
it('can clear all with one child', async function () {
await fireQueryEventAndWaitForRefresh('queryStop', <any>{ uri: testUri.toString() }, { messages: [], batchRanges: [] });
await fireQueryStartAndStopAndWaitForRefresh(testUri);
let children = testProvider.getChildren();
should(children).length(1, 'Should have one child after adding item');
@@ -119,9 +126,9 @@ describe('QueryHistoryProvider', () => {
});
it('can clear all with multiple children', async function () {
await fireQueryEventAndWaitForRefresh('queryStop', <any>{ uri: testUri.toString() }, { messages: [], batchRanges: [] });
await fireQueryEventAndWaitForRefresh('queryStop', <any>{ uri: testUri.toString() }, { messages: [], batchRanges: [] });
await fireQueryEventAndWaitForRefresh('queryStop', <any>{ uri: testUri.toString() }, { messages: [], batchRanges: [] });
await fireQueryStartAndStopAndWaitForRefresh(testUri);
await fireQueryStartAndStopAndWaitForRefresh(testUri);
await fireQueryStartAndStopAndWaitForRefresh(testUri);
let children = testProvider.getChildren();
should(children).length(3, 'Should have 3 children after adding item');
@@ -138,7 +145,7 @@ describe('QueryHistoryProvider', () => {
});
it('delete item that doesn\'t exist doesn\'t throw', async function () {
await fireQueryEventAndWaitForRefresh('queryStop', <any>{ uri: testUri.toString() }, { messages: [], batchRanges: [] });
await fireQueryStartAndStopAndWaitForRefresh(testUri);
let children = testProvider.getChildren();
should(children).length(1, 'Should have 1 child initially');
@@ -149,9 +156,9 @@ describe('QueryHistoryProvider', () => {
});
it('can delete single item', async function () {
await fireQueryEventAndWaitForRefresh('queryStop', <any>{ uri: testUri.toString() }, { messages: [], batchRanges: [] });
await fireQueryEventAndWaitForRefresh('queryStop', <any>{ uri: testUri.toString() }, { messages: [], batchRanges: [] });
await fireQueryEventAndWaitForRefresh('queryStop', <any>{ uri: testUri.toString() }, { messages: [], batchRanges: [] });
await fireQueryStartAndStopAndWaitForRefresh(testUri);
await fireQueryStartAndStopAndWaitForRefresh(testUri);
await fireQueryStartAndStopAndWaitForRefresh(testUri);
const firstChildren = testProvider.getChildren();
should(firstChildren).length(3, 'Should have 3 children initially');
@@ -175,22 +182,36 @@ describe('QueryHistoryProvider', () => {
});
it('pausing capture causes children not to be added', async function () {
await fireQueryEventAndWaitForRefresh('queryStop', <any>{ uri: testUri.toString() }, { messages: [], batchRanges: [] });
await fireQueryStartAndStopAndWaitForRefresh(testUri);
const children = testProvider.getChildren();
should(children).length(1, 'Should have one child after adding initial item');
await testProvider.setCaptureEnabled(false);
// Add timeout since the item is never added, thus never triggering the event
await fireQueryEventAndWaitForRefresh('queryStop', <any>{ uri: testUri.toString() }, { messages: [], batchRanges: [] }, 2000);
await fireQueryStartAndStopAndWaitForRefresh(testUri, { messages: [], batchRanges: [] }, 2000);
should(children).length(1, 'Should still have 1 child after adding item when capture paused');
await testProvider.setCaptureEnabled(true);
await fireQueryEventAndWaitForRefresh('queryStop', <any>{ uri: testUri.toString() }, { messages: [], batchRanges: [] });
await fireQueryStartAndStopAndWaitForRefresh(testUri);
should(children).length(2, 'Should have 2 child after adding item when capture was resumed');
});
function setupTextEditorMock(content: string | azdataTest.mocks.vscode.RangeWithContent[], selections?: vscode.Selection[] | undefined): void {
const textDocumentMock = azdataTest.mocks.vscode.createTextDocumentMock(testUri, content);
const textEditorMock = azdataTest.mocks.vscode.createTextEditorMock(textDocumentMock.object, selections);
textDocumentSandbox.replaceGetter(vscode.window, 'activeTextEditor', () => textEditorMock.object);
}
async function fireQueryStartAndStopAndWaitForRefresh(uri: vscode.Uri, queryInfo: azdata.queryeditor.QueryInfo = { messages: [], batchRanges: [] }, timeoutMs?: number): Promise<void> {
const queryDocumentMock = azdataTest.mocks.azdata.queryeditor.createQueryDocumentMock(uri.toString());
// First queryStart message to record text. QueryInfo is always empty for this.
testListener.onQueryEvent('queryStart', queryDocumentMock.object, undefined, { messages: [], batchRanges: [] });
// Fire queryStop message to trigger creation of the history node
await fireQueryEventAndWaitForRefresh('queryStop', queryDocumentMock.object, queryInfo, timeoutMs);
}
async function fireQueryEventAndWaitForRefresh(type: azdata.queryeditor.QueryEventType, document: azdata.queryeditor.QueryDocument, queryInfo: azdata.queryeditor.QueryInfo, timeoutMs?: number): Promise<void> {
await waitForItemRefresh(() => testListener.onQueryEvent(type, document, undefined, queryInfo), timeoutMs);
}

8
extensions/query-history/yarn.lock
View File

@@ -228,10 +228,10 @@
"@jridgewell/resolve-uri" "^3.0.3"
"@jridgewell/sourcemap-codec" "^1.4.10"
"@microsoft/azdata-test@^1.5.2":
version "1.5.2"
resolved "https://registry.yarnpkg.com/@microsoft/azdata-test/-/azdata-test-1.5.2.tgz#0a750eed6c686eaf98e63db580f119916a083d6a"
integrity sha512-m1WVNxkCqU8nkaRB0Q/DEq1wHej0Ev3bfyEmsPgX9znymY7MD9c1l6kRoHtPPmsYYDEjcCxO2QXUJAsNUeAKwg==
"@microsoft/azdata-test@^2.0.0":
version "2.0.0"
resolved "https://registry.yarnpkg.com/@microsoft/azdata-test/-/azdata-test-2.0.0.tgz#5b7d4ba0114ebd5ccae7d88e6982c847b1e76b06"
integrity sha512-vXfmBoc6hlbrdIqXLfbLEzw/cj5CJfpvw1mmMhO6hpSGSYqa1pm7e3dXn3fAbTjiumdSnLN1iIuOba1ggz24nA==
dependencies:
http-proxy-agent "^2.1.0"
https-proxy-agent "^2.2.4"