Fix workspaceRoot macro for insights (#4686)

* Fix workspaceRoot macro for insights

The workspaceRoot macro wasn't working correctly for finding the queryFile. There were a couple of issues :

1. The path separators were hardcoded as / which wasn't xplat-compatible
2. They required the first section of the path was one of the folders in the workspace - e.g. if the workspace contained a folder named foo you'd have to specify ${workspaceRoot}\foo\myfile.sql. This is inconsistent with the folder logich was just appends the path after ${workspaceRoot} to the folder that's currently open

I changed the logic to just append the relative part of the path to every folder currently open in the workspace and choose the first one that it found that contained the file we were told to look for - which follows the convention the folder logic uses. If the file doesn't exist it'll just fall back to using the path without the macro (which is likely to not resolve and thus will display an error, but there's nothing we can do at that point anyways)

* Switch to using VS Code resolver (support for more than just workspaceRoot) and move resolution code into helper method so it can be used by the multiple places it's called. Added tests for the methods.

* Add test for invalid param

* Change resolveQueryFilePath to be a standalone exported function. Change it to throw if the file can't be resolved/found so callers can display error correctly. Added more tests to covery new scenarios. Switch to using pfs instead of fs for file existance checks.

* Add extra param to InsightsDialogController construction in test

* Fix formatting and test errors.

* Change to suiteSetup and suiteTeardown so the setup/teardown is only ran once instead of once per test - we don't need unique files and this stops a race condition error with deleting the test folder.

* spaces -> tabs

src/sql/parts/dashboard/widgets/insights/insightsWidget.component.ts

@@ -3,7 +3,7 @@
  *  Licensed under the Source EULA. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
 import {
-	Component, Inject, ViewContainerRef, forwardRef, AfterContentInit,
+	Component, Inject, forwardRef, AfterContentInit,
 	ComponentFactoryResolver, ViewChild, ChangeDetectorRef, Injector
 } from '@angular/core';
 import { Observable } from 'rxjs/Observable';
@@ -15,7 +15,8 @@ import { InsightAction, InsightActionContext } from 'sql/workbench/common/action
 import { toDisposableSubscription } from 'sql/base/node/rxjsUtils';
 import { IInsightsConfig, IInsightsView } from './interfaces';
 import { Extensions, IInsightRegistry } from 'sql/platform/dashboard/common/insightRegistry';
-import { insertValueRegex } from 'sql/workbench/services/insights/common/insightsDialogService';
+import { resolveQueryFilePath } from 'sql/workbench/services/insights/common/insightsUtils';
+
 import { RunInsightQueryAction } from './actions';
 
 import { SimpleExecuteResult } from 'azdata';
@@ -25,13 +26,14 @@ import * as types from 'vs/base/common/types';
 import * as pfs from 'vs/base/node/pfs';
 import * as nls from 'vs/nls';
 import { Registry } from 'vs/platform/registry/common/platform';
-import { WorkbenchState, IWorkspaceContextService } from 'vs/platform/workspace/common/workspace';
+import { IWorkspaceContextService } from 'vs/platform/workspace/common/workspace';
 import { IntervalTimer, createCancelablePromise } from 'vs/base/common/async';
 import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
 import { IStorageService, StorageScope } from 'vs/platform/storage/common/storage';
 import { toDisposable } from 'vs/base/common/lifecycle';
 import { isPromiseCanceledError } from 'vs/base/common/errors';
 import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
+import { IConfigurationResolverService } from 'vs/workbench/services/configurationResolver/common/configurationResolver';
 
 const insightRegistry = Registry.as<IInsightRegistry>(Extensions.InsightContribution);
 
@@ -72,8 +74,9 @@ export class InsightsWidget extends DashboardWidget implements IDashboardWidget,
 		@Inject(forwardRef(() => Injector)) private _injector: Injector,
 		@Inject(IInstantiationService) private instantiationService: IInstantiationService,
 		@Inject(IStorageService) private storageService: IStorageService,
-		@Inject(IWorkspaceContextService) private workspaceContextService: IWorkspaceContextService,
+		@Inject(IWorkspaceContextService) private readonly _workspaceContextService: IWorkspaceContextService,
-		@Inject(IConfigurationService) private readonly _configurationService: IConfigurationService
+		@Inject(IConfigurationService) private readonly _configurationService: IConfigurationService,
+		@Inject(IConfigurationResolverService) private readonly _configurationResolverService: IConfigurationResolverService
 	) {
 		super();
 		this.insightConfig = <IInsightsConfig>this._config.widget['insights-widget'];
@@ -111,6 +114,7 @@ export class InsightsWidget extends DashboardWidget implements IDashboardWidget,
 				this._register(toDisposable(() => cancelablePromise.cancel()));
 			}
 		}, error => {
+			this._loading = false;
 			this.showError(error);
 		});
 	}
@@ -279,9 +283,7 @@ export class InsightsWidget extends DashboardWidget implements IDashboardWidget,
 		}
 	}
 
-	private _parseConfig(): Thenable<void[]> {
+	private async _parseConfig(): Promise<void> {
-		let promises: Array<Promise<void>> = [];
-
 		this._typeKey = Object.keys(this.insightConfig.type)[0];
 
 		// When the editor.accessibilitySupport setting is on, we will force the chart type to be table.
@@ -295,47 +297,11 @@ export class InsightsWidget extends DashboardWidget implements IDashboardWidget,
 		if (types.isStringArray(this.insightConfig.query)) {
 			this.insightConfig.query = this.insightConfig.query.join(' ');
 		} else if (this.insightConfig.queryFile) {
-			let filePath = this.insightConfig.queryFile;
+			let filePath = await resolveQueryFilePath(this.insightConfig.queryFile,
-			// check for workspace relative path
+				this._workspaceContextService,
-			let match = filePath.match(insertValueRegex);
+				this._configurationResolverService);
-			if (match && match.length > 0 && match[1] === 'workspaceRoot') {
-				filePath = filePath.replace(match[0], '');
 
-				//filePath = this.dashboardService.workspaceContextService.toResource(filePath).fsPath;
+			this.insightConfig.query = (await pfs.readFile(filePath)).toString();
-				switch (this.workspaceContextService.getWorkbenchState()) {
-					case WorkbenchState.FOLDER:
-						filePath = this.workspaceContextService.getWorkspace().folders[0].toResource(filePath).fsPath;
-						break;
-					case WorkbenchState.WORKSPACE:
-						let filePathArray = filePath.split('/');
-						// filter out empty sections
-						filePathArray = filePathArray.filter(i => !!i);
-						let folder = this.workspaceContextService.getWorkspace().folders.find(i => i.name === filePathArray[0]);
-						if (!folder) {
-							return Promise.reject(new Error(`Could not find workspace folder ${filePathArray[0]}`));
-						}
-						// remove the folder name from the filepath
-						filePathArray.shift();
-						// rejoin the filepath after doing the work to find the right folder
-						filePath = '/' + filePathArray.join('/');
-						filePath = folder.toResource(filePath).fsPath;
-						break;
-				}
-
-			}
-			promises.push(new Promise((resolve, reject) => {
-				pfs.readFile(filePath).then(
-					buffer => {
-						this.insightConfig.query = buffer.toString();
-						resolve();
-					},
-					error => {
-						reject(error);
-					}
-				);
-			}));
 		}
-
-		return Promise.all(promises);
 	}
 }

src/sql/workbench/services/insights/common/insightsUtils.ts

@@ -0,0 +1,41 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the Source EULA. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+import * as pfs from 'vs/base/node/pfs';
+import { localize } from 'vs/nls';
+
+import { IWorkspaceContextService, IWorkspaceFolder } from 'vs/platform/workspace/common/workspace';
+import { IConfigurationResolverService } from 'vs/workbench/services/configurationResolver/common/configurationResolver';
+
+/**
+ * Resolves the given file path using the VS ConfigurationResolver service, replacing macros such as
+ * ${workspaceRoot} with their expected values and then testing each path to see if it exists. It will
+ * return either the first full path that exists or throw an error if none of the resolved paths exist
+ * @param filePath The path to resolve
+ * @param workspaceContextService The workspace context to use for resolving workspace vars
+ * @param configurationResolverService The resolver service to use to resolve the vars
+ */
+export async function resolveQueryFilePath(filePath: string,
+	workspaceContextService: IWorkspaceContextService,
+	configurationResolverService: IConfigurationResolverService): Promise<string> {
+	if (!filePath || !workspaceContextService || !configurationResolverService) {
+		return filePath;
+	}
+
+	let workspaceFolders: IWorkspaceFolder[] = workspaceContextService.getWorkspace().folders;
+	// Resolve the path using each folder in our workspace, or undefined if there aren't any
+	// (so that non-folder vars such as environment vars still resolve)
+	let resolvedFilePaths = (workspaceFolders.length > 0 ? workspaceFolders : [undefined])
+		.map(f => configurationResolverService.resolve(f, filePath));
+
+	// Just need a single query file so use the first we find that exists
+	for (const path of resolvedFilePaths) {
+		if (await pfs.exists(path)) {
+			return path;
+		}
+	}
+
+	throw Error(localize('insightsDidNotFindResolvedFile', 'Could not find query file at any of the following paths :\n {0}', resolvedFilePaths.join('\n')));
+}

src/sql/workbench/services/insights/node/insightsDialogController.ts

@@ -8,9 +8,10 @@ import { IConnectionProfile } from 'sql/platform/connection/common/interfaces';
 import { IInsightsConfigDetails } from 'sql/parts/dashboard/widgets/insights/interfaces';
 import QueryRunner, { EventType as QREvents } from 'sql/platform/query/common/queryRunner';
 import * as Utils from 'sql/platform/connection/common/utils';
-import { IInsightsDialogModel, insertValueRegex } from 'sql/workbench/services/insights/common/insightsDialogService';
+import { IInsightsDialogModel } from 'sql/workbench/services/insights/common/insightsDialogService';
 import { error } from 'sql/base/common/log';
 import { IErrorMessageService } from 'sql/platform/errorMessage/common/errorMessageService';
+import { resolveQueryFilePath } from '../common/insightsUtils';
 
 import { DbCellValue, IDbColumn, QueryExecuteSubsetResult } from 'azdata';
 
@@ -19,8 +20,9 @@ import * as types from 'vs/base/common/types';
 import * as pfs from 'vs/base/node/pfs';
 import * as nls from 'vs/nls';
 import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
-import { IWorkspaceContextService, WorkbenchState } from 'vs/platform/workspace/common/workspace';
+import { IWorkspaceContextService } from 'vs/platform/workspace/common/workspace';
 import { INotificationService } from 'vs/platform/notification/common/notification';
+import { IConfigurationResolverService } from 'vs/workbench/services/configurationResolver/common/configurationResolver';
 
 export class InsightsDialogController {
 	private _queryRunner: QueryRunner;
@@ -35,10 +37,11 @@ export class InsightsDialogController {
 		@IErrorMessageService private _errorMessageService: IErrorMessageService,
 		@IInstantiationService private _instantiationService: IInstantiationService,
 		@IConnectionManagementService private _connectionManagementService: IConnectionManagementService,
-		@IWorkspaceContextService private _workspaceContextService: IWorkspaceContextService
+		@IWorkspaceContextService private _workspaceContextService: IWorkspaceContextService,
+		@IConfigurationResolverService private _configurationResolverService: IConfigurationResolverService
 	) { }
 
-	public update(input: IInsightsConfigDetails, connectionProfile: IConnectionProfile): Thenable<void> {
+	public async update(input: IInsightsConfigDetails, connectionProfile: IConnectionProfile): Promise<void> {
 		// execute string
 		if (typeof input === 'object') {
 			if (connectionProfile === undefined) {
@@ -57,49 +60,32 @@ export class InsightsDialogController {
 					this._errorMessageService.showDialog(Severity.Error, nls.localize("insightsError", "Insights error"), e);
 				}).then(() => undefined);
 			} else if (types.isString(input.queryFile)) {
-				let filePath = input.queryFile;
+				let filePath: string;
-				// check for workspace relative path
+				try {
-				let match = filePath.match(insertValueRegex);
+					filePath = await resolveQueryFilePath(input.queryFile,
-				if (match && match.length > 0 && match[1] === 'workspaceRoot') {
+						this._workspaceContextService,
-					filePath = filePath.replace(match[0], '');
+						this._configurationResolverService);
-
+				}
-					switch (this._workspaceContextService.getWorkbenchState()) {
+				catch (e) {
-						case WorkbenchState.FOLDER:
+					this._notificationService.notify({
-							filePath = this._workspaceContextService.getWorkspace().folders[0].toResource(filePath).fsPath;
+						severity: Severity.Error,
-							break;
+						message: e
-						case WorkbenchState.WORKSPACE:
+					});
-							let filePathArray = filePath.split('/');
+					return Promise.resolve();
-							// filter out empty sections
+				}
-							filePathArray = filePathArray.filter(i => !!i);
+
-							let folder = this._workspaceContextService.getWorkspace().folders.find(i => i.name === filePathArray[0]);
+				try {
-							if (!folder) {
+					let buffer: Buffer = await pfs.readFile(filePath);
-								return Promise.reject(new Error(`Could not find workspace folder ${filePathArray[0]}`));
+					this.createQuery(buffer.toString(), connectionProfile).catch(e => {
-							}
+						this._errorMessageService.showDialog(Severity.Error, nls.localize("insightsError", "Insights error"), e);
-							// remove the folder name from the filepath
+					});
-							filePathArray.shift();
+				}
-							// rejoin the filepath after doing the work to find the right folder
+				catch (e) {
-							filePath = '/' + filePathArray.join('/');
+					this._notificationService.notify({
-							filePath = folder.toResource(filePath).fsPath;
+						severity: Severity.Error,
-							break;
+						message: nls.localize("insightsFileError", "There was an error reading the query file: ") + e
-					}
+					});
-
 				}
-				return new Promise((resolve, reject) => {
-					pfs.readFile(filePath).then(
-						buffer => {
-							this.createQuery(buffer.toString(), connectionProfile).catch(e => {
-								this._errorMessageService.showDialog(Severity.Error, nls.localize("insightsError", "Insights error"), e);
-							}).then(() => resolve());
-						},
-						error => {
-							this._notificationService.notify({
-								severity: Severity.Error,
-								message: nls.localize("insightsFileError", "There was an error reading the query file: ") + error
-							});
-							resolve();
-						}
-					);
-				});
 			} else {
 				error('Error reading details Query: ', input);
 				this._notificationService.notify({

src/sqltest/parts/insights/insightsDialogController.test.ts

@@ -49,6 +49,7 @@ suite('Insights Dialog Controller Tests', () => {
 			undefined,
 			instMoq.object,
 			connMoq.object,
+			undefined,
 			undefined
 		);
 

src/sqltest/parts/insights/insightsUtils.test.ts

@@ -0,0 +1,178 @@
+/*---------------------------------------------------------------------------------------------
+*  Copyright (c) Microsoft Corporation. All rights reserved.
+*  Licensed under the Source EULA. See License.txt in the project root for license information.
+*--------------------------------------------------------------------------------------------*/
+
+import { equal } from 'assert';
+import * as os from 'os';
+
+import { resolveQueryFilePath } from 'sql/workbench/services/insights/common/insightsUtils';
+import { TestWindowService } from 'sqltest/stubs/windowTestService';
+
+import * as path from 'vs/base/common/path';
+import * as pfs from 'vs/base/node/pfs';
+
+import { getRandomTestPath } from 'vs/base/test/node/testUtils';
+import { Workspace, toWorkspaceFolders } from 'vs/platform/workspace/common/workspace';
+import { ConfigurationResolverService } from 'vs/workbench/services/configurationResolver/browser/configurationResolverService';
+import { TestContextService } from 'vs/workbench/test/workbenchTestServices';
+
+suite('Insights Utils tests', function () {
+	let testRootPath: string;
+	let queryFileDir: string;
+	let queryFilePath: string;
+
+	suiteSetup(done => {
+		// Create test file - just needs to exist for verifying the path resolution worked correctly
+		testRootPath = path.join(os.tmpdir(), 'adstests');
+		queryFileDir = getRandomTestPath(testRootPath, 'insightsutils');
+		pfs.mkdirp(queryFileDir).then(() => {
+			queryFilePath = path.join(queryFileDir, 'test.sql');
+			pfs.writeFile(queryFilePath, '').then(done());
+		});
+
+	});
+
+	test('resolveQueryFilePath resolves path correctly with fully qualified path', async () => {
+		let configurationResolverService = new ConfigurationResolverService(
+			new TestWindowService({}),
+			undefined,
+			undefined,
+			undefined,
+			undefined,
+			new TestContextService(),
+			undefined);
+
+		let resolvedPath = await resolveQueryFilePath(queryFilePath, new TestContextService(), configurationResolverService);
+		equal(resolvedPath, queryFilePath);
+	});
+
+	test('resolveQueryFilePath resolves path correctly with workspaceRoot var and non-empty workspace containing file', async () => {
+		// Create mock context service with our test folder added as a workspace folder for resolution
+		let contextService = new TestContextService(
+			new Workspace(
+				'TestWorkspace',
+				toWorkspaceFolders([{ path: queryFileDir }])
+			));
+		let configurationResolverService = new ConfigurationResolverService(
+			new TestWindowService({}),
+			undefined,
+			undefined,
+			undefined,
+			undefined,
+			contextService,
+			undefined);
+
+		let resolvedPath = await resolveQueryFilePath(path.join('${workspaceRoot}', 'test.sql'), contextService, configurationResolverService);
+		equal(resolvedPath, queryFilePath);
+	});
+
+	test('resolveQueryFilePath throws with workspaceRoot var and non-empty workspace not containing file', async (done) => {
+		let tokenizedPath = path.join('${workspaceRoot}', 'test.sql');
+		// Create mock context service with a folder NOT containing our test file to verify it returns original path
+		let contextService = new TestContextService(
+			new Workspace(
+				'TestWorkspace',
+				toWorkspaceFolders([{ path: os.tmpdir() }])
+			));
+		let configurationResolverService = new ConfigurationResolverService(
+			new TestWindowService({}),
+			undefined,
+			undefined,
+			undefined,
+			undefined,
+			contextService,
+			undefined);
+
+		try {
+			await resolveQueryFilePath(tokenizedPath, contextService, configurationResolverService);
+		}
+		catch (e) {
+			done();
+		}
+	});
+
+	test('resolveQueryFilePath throws with workspaceRoot var and empty workspace', async (done) => {
+		let tokenizedPath = path.join('${workspaceRoot}', 'test.sql');
+		// Create mock context service with an empty workspace
+		let contextService = new TestContextService(
+			new Workspace(
+				'TestWorkspace'));
+		let configurationResolverService = new ConfigurationResolverService(
+			new TestWindowService({}),
+			undefined,
+			undefined,
+			undefined,
+			undefined,
+			contextService,
+			undefined);
+
+		try {
+			await resolveQueryFilePath(tokenizedPath, contextService, configurationResolverService);
+		}
+		catch (e) {
+			done();
+		}
+	});
+
+	test('resolveQueryFilePath resolves path correctly with env var and empty workspace', async () => {
+		let contextService = new TestContextService(
+			new Workspace('TestWorkspace'));
+
+		// Create mock window service with env variable containing test folder for resolution
+		let configurationResolverService = new ConfigurationResolverService(
+			new TestWindowService({ TEST_PATH: queryFileDir }),
+			undefined,
+			undefined,
+			undefined,
+			undefined,
+			undefined,
+			undefined);
+
+		let resolvedPath = await resolveQueryFilePath(path.join('${env:TEST_PATH}', 'test.sql'), contextService, configurationResolverService);
+		equal(resolvedPath, queryFilePath);
+	});
+
+	test('resolveQueryFilePath resolves path correctly with env var and non-empty workspace', async () => {
+		let contextService = new TestContextService(
+			new Workspace('TestWorkspace', toWorkspaceFolders([{ path: os.tmpdir() }])));
+
+		// Create mock window service with env variable containing test folder for resolution
+		let configurationResolverService = new ConfigurationResolverService(
+			new TestWindowService({ TEST_PATH: queryFileDir }),
+			undefined,
+			undefined,
+			undefined,
+			undefined,
+			undefined,
+			undefined);
+
+		let resolvedPath = await resolveQueryFilePath(path.join('${env:TEST_PATH}', 'test.sql'), contextService, configurationResolverService);
+		equal(resolvedPath, queryFilePath);
+	});
+
+	test('resolveQueryFilePath throws if invalid param var specified', async (done) => {
+		let invalidPath = path.join('${INVALID}', 'test.sql');
+		let configurationResolverService = new ConfigurationResolverService(
+			new TestWindowService({}),
+			undefined,
+			undefined,
+			undefined,
+			undefined,
+			undefined,
+			undefined);
+
+		try {
+			await resolveQueryFilePath(invalidPath, new TestContextService(), configurationResolverService);
+		}
+		catch (e) {
+			done();
+		}
+
+	});
+
+	suiteTeardown(done => {
+		// Clean up our test files
+		pfs.del(testRootPath).then(done());
+	});
+});

src/sqltest/stubs/windowTestService.ts

@@ -0,0 +1,19 @@
+/*---------------------------------------------------------------------------------------------
+*  Copyright (c) Microsoft Corporation. All rights reserved.
+*  Licensed under the Source EULA. See License.txt in the project root for license information.
+*--------------------------------------------------------------------------------------------*/
+
+import * as platform from 'vs/base/common/platform';
+import { IWindowConfiguration } from 'vs/platform/windows/common/windows';
+import { TestWindowService as vsTestWindowService } from 'vs/workbench/test/workbenchTestServices';
+
+export class TestWindowService extends vsTestWindowService {
+
+	constructor(private env: platform.IProcessEnvironment) {
+		super();
+	}
+
+	getConfiguration(): IWindowConfiguration {
+		return { userEnv: this.env } as IWindowConfiguration;
+	}
+}