Fix open notebook bug (#10930)

* Fix open notebook bug

* cleanup

* clean up spaces

extensions/notebook/src/common/appContext.ts

@@ -5,6 +5,7 @@
 
 import * as vscode from 'vscode';
 import { ApiWrapper } from './apiWrapper';
+import { NotebookUtils } from './notebookUtils';
 
 /**
  * Global context for the application
@@ -12,8 +13,11 @@ import { ApiWrapper } from './apiWrapper';
 export class AppContext {
 
 	private serviceMap: Map<string, any> = new Map();
+	public readonly notebookUtils: NotebookUtils;
+
 	constructor(public readonly extensionContext: vscode.ExtensionContext, public readonly apiWrapper: ApiWrapper) {
 		this.apiWrapper = apiWrapper || new ApiWrapper();
+		this.notebookUtils = new NotebookUtils(apiWrapper);
 	}
 
 	public getService<T>(serviceName: string): T {

extensions/notebook/src/common/notebookUtils.ts

@@ -4,11 +4,11 @@
  *--------------------------------------------------------------------------------------------*/
 
 import * as azdata from 'azdata';
-import * as crypto from 'crypto';
 import * as os from 'os';
 import * as vscode from 'vscode';
 import * as nls from 'vscode-nls';
 import { getErrorMessage, isEditorTitleFree } from '../common/utils';
+import { ApiWrapper } from './apiWrapper';
 
 const localize = nls.loadMessageBundle();
 
@@ -16,24 +16,12 @@ const JUPYTER_NOTEBOOK_PROVIDER = 'jupyter';
 const msgSampleCodeDataFrame = localize('msgSampleCodeDataFrame', "This sample code loads the file into a data frame and shows the first 10 results.");
 const noNotebookVisible = localize('noNotebookVisible', "No notebook editor is active");
 
-/**
+export class NotebookUtils {
- * Creates a random token per https://nodejs.org/api/crypto.html#crypto_crypto_randombytes_size_callback.
- * Defaults to 24 bytes, which creates a 48-char hex string
- */
-export function getRandomToken(size: number = 24): Promise<string> {
-	return new Promise((resolve, reject) => {
-		crypto.randomBytes(size, (err, buffer) => {
-			if (err) {
-				reject(err);
-			}
-			let token = buffer.toString('hex');
-			resolve(token);
-		});
-	});
-}
 
-export async function newNotebook(connectionProfile?: azdata.IConnectionProfile): Promise<azdata.nb.NotebookEditor> {
+	constructor(private _apiWrapper: ApiWrapper) { }
-	const title = findNextUntitledEditorName();
+
+	public async newNotebook(connectionProfile?: azdata.IConnectionProfile): Promise<azdata.nb.NotebookEditor> {
+		const title = this.findNextUntitledEditorName();
 		const untitledUri = vscode.Uri.parse(`untitled:${title}`);
 		const options: azdata.nb.NotebookShowOptions = connectionProfile ? {
 			viewColumn: null,
@@ -46,7 +34,7 @@ export async function newNotebook(connectionProfile?: azdata.IConnectionProfile)
 		return azdata.nb.showNotebookDocument(untitledUri, options);
 	}
 
-function findNextUntitledEditorName(): string {
+	private findNextUntitledEditorName(): string {
 		let nextVal = 0;
 		// Note: this will go forever if it's coded wrong, or you have infinite Untitled notebooks!
 		while (true) {
@@ -58,24 +46,23 @@ function findNextUntitledEditorName(): string {
 		}
 	}
 
-export async function openNotebook(): Promise<void> {
+	public async openNotebook(): Promise<void> {
 		try {
 			let filter: { [key: string]: Array<string> } = {};
 			// TODO support querying valid notebook file types
 			filter[localize('notebookFiles', "Notebooks")] = ['ipynb'];
-		let file = await vscode.window.showOpenDialog({
+			let file = await this._apiWrapper.showOpenDialog({
 				filters: filter
 			});
-		if (file) {
+			if (file && file.length > 0) {
-			let doc = await vscode.workspace.openTextDocument(file[0]);
+				await azdata.nb.showNotebookDocument(file[0]);
-			vscode.window.showTextDocument(doc);
 			}
 		} catch (err) {
-		vscode.window.showErrorMessage(getErrorMessage(err));
+			this._apiWrapper.showErrorMessage(getErrorMessage(err));
 		}
 	}
 
-export async function runActiveCell(): Promise<void> {
+	public async runActiveCell(): Promise<void> {
 		try {
 			let notebook = azdata.nb.activeNotebookEditor;
 			if (notebook) {
@@ -88,7 +75,7 @@ export async function runActiveCell(): Promise<void> {
 		}
 	}
 
-export async function clearActiveCellOutput(): Promise<void> {
+	public async clearActiveCellOutput(): Promise<void> {
 		try {
 			let notebook = azdata.nb.activeNotebookEditor;
 			if (notebook) {
@@ -101,7 +88,7 @@ export async function clearActiveCellOutput(): Promise<void> {
 		}
 	}
 
-export async function runAllCells(startCell?: azdata.nb.NotebookCell, endCell?: azdata.nb.NotebookCell): Promise<void> {
+	public async runAllCells(startCell?: azdata.nb.NotebookCell, endCell?: azdata.nb.NotebookCell): Promise<void> {
 		try {
 			let notebook = azdata.nb.activeNotebookEditor;
 			if (notebook) {
@@ -114,7 +101,7 @@ export async function runAllCells(startCell?: azdata.nb.NotebookCell, endCell?:
 		}
 	}
 
-export async function addCell(cellType: azdata.nb.CellType): Promise<void> {
+	public async addCell(cellType: azdata.nb.CellType): Promise<void> {
 		try {
 			let notebook = azdata.nb.activeNotebookEditor;
 			if (notebook) {
@@ -133,10 +120,10 @@ export async function addCell(cellType: azdata.nb.CellType): Promise<void> {
 		}
 	}
 
-export async function analyzeNotebook(oeContext?: azdata.ObjectExplorerContext): Promise<void> {
+	public async analyzeNotebook(oeContext?: azdata.ObjectExplorerContext): Promise<void> {
 		// Ensure we get a unique ID for the notebook. For now we're using a different prefix to the built-in untitled files
 		// to handle this. We should look into improving this in the future
-	let title = findNextUntitledEditorName();
+		let title = this.findNextUntitledEditorName();
 		let untitledUri = vscode.Uri.parse(`untitled:${title}`);
 
 		let editor = await azdata.nb.showNotebookDocument(untitledUri, {
@@ -165,3 +152,4 @@ export async function analyzeNotebook(oeContext?: azdata.ObjectExplorerContext):
 			}
 		}
 	}
+}

extensions/notebook/src/common/utils.ts

@@ -8,6 +8,7 @@ import * as fs from 'fs-extra';
 import * as nls from 'vscode-nls';
 import * as vscode from 'vscode';
 import * as azdata from 'azdata';
+import * as crypto from 'crypto';
 import { notebookLanguages } from './constants';
 
 const localize = nls.loadMessageBundle();
@@ -300,3 +301,19 @@ function decorate(decorator: (fn: Function, key: string) => Function): Function
 export function getDropdownValue(dropdown: azdata.DropDownComponent): string {
 	return (typeof dropdown.value === 'string') ? dropdown.value : dropdown.value.name;
 }
+
+/**
+ * Creates a random token per https://nodejs.org/api/crypto.html#crypto_crypto_randombytes_size_callback.
+ * Defaults to 24 bytes, which creates a 48-char hex string
+ */
+export async function getRandomToken(size: number = 24): Promise<string> {
+	return new Promise((resolve, reject) => {
+		crypto.randomBytes(size, (err, buffer) => {
+			if (err) {
+				reject(err);
+			}
+			let token = buffer.toString('hex');
+			resolve(token);
+		});
+	});
+}

extensions/notebook/src/extension.ts

@@ -16,7 +16,6 @@ import { CellType } from './contracts/content';
 import { NotebookUriHandler } from './protocol/notebookUriHandler';
 import { BookTreeViewProvider } from './book/bookTreeView';
 import { NavigationProviders } from './common/constants';
-import { newNotebook, openNotebook, runActiveCell, runAllCells, clearActiveCellOutput, addCell, analyzeNotebook } from './common/notebookUtils';
 
 const localize = nls.loadMessageBundle();
 
@@ -26,6 +25,7 @@ let controller: JupyterController;
 type ChooseCellType = { label: string, id: CellType };
 
 export async function activate(extensionContext: vscode.ExtensionContext): Promise<IExtensionApi> {
+	const appContext = new AppContext(extensionContext, new ApiWrapper());
 	const createBookPath: string = path.posix.join(extensionContext.extensionPath, 'resources', 'notebooks', 'JupyterBooksCreate.ipynb');
 	extensionContext.subscriptions.push(vscode.commands.registerCommand('bookTreeView.openBook', (bookPath: string, openAsUntitled: boolean, urlToOpen?: string) => openAsUntitled ? providedBookTreeViewProvider.openBook(bookPath, urlToOpen, true) : bookTreeViewProvider.openBook(bookPath, urlToOpen, true)));
 	extensionContext.subscriptions.push(vscode.commands.registerCommand('bookTreeView.openNotebook', (resource) => bookTreeViewProvider.openNotebook(resource)));
@@ -56,19 +56,19 @@ export async function activate(extensionContext: vscode.ExtensionContext): Promi
 		if (context && context.connectionProfile) {
 			connectionProfile = context.connectionProfile;
 		}
-		return newNotebook(connectionProfile);
+		return appContext.notebookUtils.newNotebook(connectionProfile);
 	}));
 	extensionContext.subscriptions.push(vscode.commands.registerCommand('notebook.command.open', async () => {
-		await openNotebook();
+		await appContext.notebookUtils.openNotebook();
 	}));
 	extensionContext.subscriptions.push(vscode.commands.registerCommand('notebook.command.runactivecell', async () => {
-		await runActiveCell();
+		await appContext.notebookUtils.runActiveCell();
 	}));
 	extensionContext.subscriptions.push(vscode.commands.registerCommand('notebook.command.runallcells', async () => {
-		await runAllCells();
+		await appContext.notebookUtils.runAllCells();
 	}));
 	extensionContext.subscriptions.push(vscode.commands.registerCommand('notebook.command.clearactivecellresult', async () => {
-		await clearActiveCellOutput();
+		await appContext.notebookUtils.clearActiveCellOutput();
 	}));
 	extensionContext.subscriptions.push(vscode.commands.registerCommand('notebook.command.addcell', async () => {
 		let cellType: CellType;
@@ -91,17 +91,17 @@ export async function activate(extensionContext: vscode.ExtensionContext): Promi
 			return;
 		}
 		if (cellType) {
-			await addCell(cellType);
+			await appContext.notebookUtils.addCell(cellType);
 		}
 	}));
 	extensionContext.subscriptions.push(vscode.commands.registerCommand('notebook.command.addcode', async () => {
-		await addCell('code');
+		await appContext.notebookUtils.addCell('code');
 	}));
 	extensionContext.subscriptions.push(vscode.commands.registerCommand('notebook.command.addtext', async () => {
-		await addCell('markdown');
+		await appContext.notebookUtils.addCell('markdown');
 	}));
 	extensionContext.subscriptions.push(vscode.commands.registerCommand('notebook.command.analyzeNotebook', async (explorerContext: azdata.ObjectExplorerContext) => {
-		await analyzeNotebook(explorerContext);
+		await appContext.notebookUtils.analyzeNotebook(explorerContext);
 	}));
 	extensionContext.subscriptions.push(vscode.window.registerUriHandler(new NotebookUriHandler()));
 
@@ -110,7 +110,6 @@ export async function activate(extensionContext: vscode.ExtensionContext): Promi
 		await vscode.commands.executeCommand('vscode.open', vscode.Uri.parse(urlToOpen));
 	}));
 
-	let appContext = new AppContext(extensionContext, new ApiWrapper());
 	controller = new JupyterController(appContext);
 	let result = await controller.activate();
 	if (!result) {

extensions/notebook/src/jupyter/serverInstance.ts

@@ -16,7 +16,6 @@ import { IServerInstance } from './common';
 import { JupyterServerInstallation } from './jupyterServerInstallation';
 import * as utils from '../common/utils';
 import * as constants from '../common/constants';
-import * as notebookUtils from '../common/notebookUtils';
 import * as ports from '../common/ports';
 
 const NotebookConfigFilename = 'jupyter_notebook_config.py';
@@ -241,7 +240,7 @@ export class PerFolderServerInstance implements IServerInstance {
 		let notebookDirectory = this.getNotebookDirectory();
 		// Find a port in a given range. If run into trouble, try another port inside the given range
 		let port = await ports.strictFindFreePort(new ports.StrictPortFindOptions(defaultPort, defaultPort + 1000));
-		let token = await notebookUtils.getRandomToken();
+		let token = await utils.getRandomToken();
 		this._uri = vscode.Uri.parse(`http://localhost:${port}/?token=${token}`);
 		this._port = port.toString();
 		let startCommand = `"${this.options.install.pythonExecutable}" -m jupyter notebook --no-browser --no-mathjax --notebook-dir "${notebookDirectory}" --port=${port} --NotebookApp.token=${token}`;

extensions/notebook/src/test/common/notebookUtils.test.ts

@@ -4,15 +4,29 @@
  *--------------------------------------------------------------------------------------------*/
 
 import * as azdata from 'azdata';
-import { newNotebook } from '../../common/notebookUtils';
+import { NotebookUtils } from '../../common/notebookUtils';
 import * as should from 'should';
 import * as vscode from 'vscode';
+import * as TypeMoq from 'typemoq';
+import * as os from 'os';
+import * as path from 'path';
+import * as uuid from 'uuid';
+import { promises as fs } from 'fs';
+import { ApiWrapper } from '../../common/apiWrapper';
+import { tryDeleteFile } from './testUtils';
 
-describe('notebookUtils Tests', async function (): Promise<void> {
+describe('notebookUtils Tests', function (): void {
-	describe('newNotebook', async function (): Promise<void> {
+	let notebookUtils: NotebookUtils;
+	let apiWrapperMock: TypeMoq.IMock<ApiWrapper>;
+	before(function (): void {
+		apiWrapperMock = TypeMoq.Mock.ofInstance(new ApiWrapper());
+		notebookUtils = new NotebookUtils(apiWrapperMock.object);
+	});
+
+	describe('newNotebook', function (): void {
 		it('Should open a new notebook successfully', async function (): Promise<void> {
 			should(azdata.nb.notebookDocuments.length).equal(0, 'There should be not any open Notebook documents');
-			await newNotebook(undefined);
+			await notebookUtils.newNotebook(undefined);
 			should(azdata.nb.notebookDocuments.length).equal(1, 'There should be exactly 1 open Notebook document');
 			await vscode.commands.executeCommand('workbench.action.closeActiveEditor');
 			should(azdata.nb.notebookDocuments.length).equal(0, 'There should be not any open Notebook documents');
@@ -20,12 +34,12 @@ describe('notebookUtils Tests', async function (): Promise<void> {
 
 		it('Opening an untitled editor after closing should re-use previous untitled name', async function (): Promise<void> {
 			should(azdata.nb.notebookDocuments.length).equal(0, 'There should be not any open Notebook documents');
-			await newNotebook(undefined);
+			await notebookUtils.newNotebook(undefined);
 			should(azdata.nb.notebookDocuments.length).equal(1, 'There should be exactly 1 open Notebook document');
 			should(azdata.nb.notebookDocuments[0].fileName).equal('Notebook-0', 'The first Untitled Notebook should have an index of 0');
 			await vscode.commands.executeCommand('workbench.action.closeActiveEditor');
 			should(azdata.nb.notebookDocuments.length).equal(0, 'There should be not any open Notebook documents');
-			await newNotebook(undefined);
+			await notebookUtils.newNotebook(undefined);
 			should(azdata.nb.notebookDocuments.length).equal(1, 'There should be exactly 1 open Notebook document after second opening');
 			should(azdata.nb.notebookDocuments[0].fileName).equal('Notebook-0', 'The first Untitled Notebook should have an index of 0 after closing first Untitled Notebook');
 			await vscode.commands.executeCommand('workbench.action.closeActiveEditor');
@@ -33,9 +47,9 @@ describe('notebookUtils Tests', async function (): Promise<void> {
 
 		it('Untitled Name index should increase', async function (): Promise<void> {
 			should(azdata.nb.notebookDocuments.length).equal(0, 'There should be not any open Notebook documents');
-			await newNotebook(undefined);
+			await notebookUtils.newNotebook(undefined);
 			should(azdata.nb.notebookDocuments.length).equal(1, 'There should be exactly 1 open Notebook document');
-			const secondNotebook = await newNotebook(undefined);
+			const secondNotebook = await notebookUtils.newNotebook(undefined);
 			should(azdata.nb.notebookDocuments.length).equal(2, 'There should be exactly 2 open Notebook documents');
 			should(secondNotebook.document.fileName).equal('Notebook-1', 'The second Untitled Notebook should have an index of 1');
 			await vscode.commands.executeCommand('workbench.action.closeActiveEditor');
@@ -43,4 +57,27 @@ describe('notebookUtils Tests', async function (): Promise<void> {
 			should(azdata.nb.notebookDocuments.length).equal(0, 'There should be not any open Notebook documents');
 		});
 	});
+
+	describe('openNotebook', function () {
+		it('opens a Notebook successfully', async function (): Promise<void> {
+			const notebookPath = path.join(os.tmpdir(), `OpenNotebookTest_${uuid.v4()}.ipynb`);
+			const notebookUri = vscode.Uri.file(notebookPath);
+			try {
+				await fs.writeFile(notebookPath, '');
+				apiWrapperMock.setup(x => x.showOpenDialog(TypeMoq.It.isAny())).returns(() => Promise.resolve([notebookUri]));
+				await notebookUtils.openNotebook();
+				should(azdata.nb.notebookDocuments.find(doc => doc.fileName === notebookUri.fsPath)).not.be.undefined();
+				await vscode.commands.executeCommand('workbench.action.closeActiveEditor');
+			} finally {
+				tryDeleteFile(notebookPath);
+			}
+		});
+
+		it('shows error if unexpected error is thrown', async function (): Promise<void> {
+			apiWrapperMock.setup(x => x.showOpenDialog(TypeMoq.It.isAny())).throws(new Error('Unexpected error'));
+			apiWrapperMock.setup(x => x.showErrorMessage(TypeMoq.It.isAny())).returns(() => Promise.resolve(''));
+			await notebookUtils.openNotebook();
+			apiWrapperMock.verify(x => x.showErrorMessage(TypeMoq.It.isAny()), TypeMoq.Times.once());
+		});
+	});
 });

extensions/notebook/src/test/common/querybookUtils.test.ts

@@ -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 should from 'should';
-import 'mocha';
-
-import * as notebookUtils from '../../common/notebookUtils';
-
-describe('Random Token', () => {
-	it('Should have default length and be hex only', async function (): Promise<void> {
-
-		let token = await notebookUtils.getRandomToken();
-		should(token).have.length(48);
-		let validChars = ['0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'a', 'b', 'c', 'd', 'e', 'f'];
-		for (let i = 0; i < token.length; i++) {
-			let char = token.charAt(i);
-			should(validChars.indexOf(char)).be.greaterThan(-1);
-		}
-	});
-});

extensions/notebook/src/test/common/testUtils.ts

@@ -4,6 +4,7 @@
  *--------------------------------------------------------------------------------------------*/
 
 import * as assert from 'assert';
+import { promises as fs } from 'fs';
 
 export async function assertThrowsAsync(fn: () => Promise<any>, msg: string): Promise<void> {
 	let f = () => {
@@ -21,3 +22,11 @@ export async function assertThrowsAsync(fn: () => Promise<any>, msg: string): Pr
 export async function sleep(ms: number): Promise<{}> {
 	return new Promise(resolve => setTimeout(resolve, ms));
 }
+
+export async function tryDeleteFile(path: string): Promise<void> {
+	try {
+		await fs.unlink(path);
+	} catch {
+		console.warn(`Could not delete file ${path}`);
+	}
+}

extensions/notebook/src/test/common/utils.test.ts

@@ -318,4 +318,17 @@ describe('Utils Tests', function () {
 			}).throw();
 		});
 	});
+
+	describe('getRandomToken', function (): void {
+		it('Should have default length and be hex only', async function (): Promise<void> {
+
+			let token = await utils.getRandomToken();
+			should(token).have.length(48);
+			let validChars = ['0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'a', 'b', 'c', 'd', 'e', 'f'];
+			for (let i = 0; i < token.length; i++) {
+				let char = token.charAt(i);
+				should(validChars.indexOf(char)).be.greaterThan(-1);
+			}
+		});
+	});
 });