From 69dff5a2cbc2158fab91a6c2c5b08fe435c3a205 Mon Sep 17 00:00:00 2001 From: Kevin Cunnane Date: Thu, 7 Feb 2019 10:35:19 -0800 Subject: [PATCH] Fix #3937 Create new notebook (Mac) and receive TypeError (#3965) - Handles empty file scenario, with fixes along the way for missing metadata (bonus win) - In non-empty file still shows error and kernel stuck in loading state. #3964 opened to track this issue and fix later --- .../parts/notebook/models/notebookModel.ts | 51 +++++++++++-------- src/sql/parts/notebook/notebook.component.ts | 2 +- .../notebook/node/localContentManager.ts | 19 ++++++- 3 files changed, 49 insertions(+), 23 deletions(-) diff --git a/src/sql/parts/notebook/models/notebookModel.ts b/src/sql/parts/notebook/models/notebookModel.ts index 9b095b0dd4..fb2e8aeeb9 100644 --- a/src/sql/parts/notebook/models/notebookModel.ts +++ b/src/sql/parts/notebook/models/notebookModel.ts @@ -523,7 +523,7 @@ export class NotebookModel extends Disposable implements INotebookModel { // Get default language if saved in notebook file // Otherwise, default to python private getDefaultLanguageInfo(notebook: nb.INotebookContents): nb.ILanguageInfo { - return notebook!.metadata!.language_info || { + return (notebook && notebook.metadata && notebook.metadata.language_info) ? notebook.metadata.language_info : { name: this._providerId === SQL_NOTEBOOK_PROVIDER ? 'sql' : 'python', version: '', mimetype: this._providerId === SQL_NOTEBOOK_PROVIDER ? 'x-sql' : 'x-python' @@ -532,7 +532,7 @@ export class NotebookModel extends Disposable implements INotebookModel { // Get default kernel info if saved in notebook file private getSavedKernelInfo(notebook: nb.INotebookContents): nb.IKernelInfo { - return notebook!.metadata!.kernelspec; + return (notebook && notebook.metadata && notebook.metadata.kernelspec) ? notebook.metadata.kernelspec : undefined; } private getKernelSpecFromDisplayName(displayName: string): nb.IKernelSpec { @@ -645,28 +645,39 @@ export class NotebookModel extends Disposable implements INotebookModel { * @param kernelSpec KernelSpec for new kernel */ private setProviderIdForKernel(kernelSpec: nb.IKernelSpec): void { - let sessionManagerFound: boolean = false; - for (let i = 0; i < this.notebookManagers.length; i++) { - if (this.notebookManagers[i].sessionManager && this.notebookManagers[i].sessionManager.specs && this.notebookManagers[i].sessionManager.specs.kernels) { - let index = this.notebookManagers[i].sessionManager.specs.kernels.findIndex(kernel => kernel.name === kernelSpec.name); - if (index >= 0) { - this._activeClientSession = this._clientSessions[i]; - if (this.notebookManagers[i].providerId !== this._providerId) { - this._providerId = this.notebookManagers[i].providerId; - this._onProviderIdChanged.fire(this._providerId); + if (!kernelSpec) { + // Just use the 1st non-default provider, we don't have a better heuristic + let notebookManagers = this.notebookOptions.notebookManagers.filter(manager => manager.providerId !== DEFAULT_NOTEBOOK_PROVIDER); + if (!notebookManagers.length) { + notebookManagers = this.notebookOptions.notebookManagers; + } + if (notebookManagers.length > 0) { + this._providerId = notebookManagers[0].providerId; + } + } else { + let sessionManagerFound: boolean = false; + for (let i = 0; i < this.notebookManagers.length; i++) { + if (this.notebookManagers[i].sessionManager && this.notebookManagers[i].sessionManager.specs && this.notebookManagers[i].sessionManager.specs.kernels) { + let index = this.notebookManagers[i].sessionManager.specs.kernels.findIndex(kernel => kernel.name === kernelSpec.name); + if (index >= 0) { + this._activeClientSession = this._clientSessions[i]; + if (this.notebookManagers[i].providerId !== this._providerId) { + this._providerId = this.notebookManagers[i].providerId; + this._onProviderIdChanged.fire(this._providerId); + } + sessionManagerFound = true; + break; } - sessionManagerFound = true; - break; } } - } - // If no SessionManager exists, utilize passed in StandardKernels to see if we can intelligently set _providerId - if (!sessionManagerFound) { - let provider = this._kernelDisplayNameToNotebookProviderIds.get(kernelSpec.display_name); - if (provider) { - this._providerId = provider; - this._onProviderIdChanged.fire(this._providerId); + // If no SessionManager exists, utilize passed in StandardKernels to see if we can intelligently set _providerId + if (!sessionManagerFound) { + let provider = this._kernelDisplayNameToNotebookProviderIds.get(kernelSpec.display_name); + if (provider) { + this._providerId = provider; + this._onProviderIdChanged.fire(this._providerId); + } } } } diff --git a/src/sql/parts/notebook/notebook.component.ts b/src/sql/parts/notebook/notebook.component.ts index a4d9ab9d5c..1068d53622 100644 --- a/src/sql/parts/notebook/notebook.component.ts +++ b/src/sql/parts/notebook/notebook.component.ts @@ -101,7 +101,7 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe } private updateProfile(): void { - this.profile = this.notebookParams!.profile; + this.profile = this.notebookParams ? this.notebookParams.profile : undefined; if (!this.profile) { // use global connection if possible let profile = TaskUtilities.getCurrentGlobalConnection(this.objectExplorerService, this.connectionManagementService, this.editorService); diff --git a/src/sql/workbench/services/notebook/node/localContentManager.ts b/src/sql/workbench/services/notebook/node/localContentManager.ts index 090108de59..8efc2a3176 100644 --- a/src/sql/workbench/services/notebook/node/localContentManager.ts +++ b/src/sql/workbench/services/notebook/node/localContentManager.ts @@ -17,6 +17,7 @@ import { localize } from 'vs/nls'; import { JSONObject } from 'sql/parts/notebook/models/jsonext'; import { OutputTypes } from 'sql/parts/notebook/models/contracts'; import { nbversion } from 'sql/parts/notebook/notebookConstants'; +import { nbformat } from 'sql/parts/notebook/models/nbformat'; type MimeBundle = { [key: string]: string | string[] | undefined }; @@ -29,7 +30,8 @@ export class LocalContentManager implements nb.ContentManager { let path = notebookUri.fsPath; // Note: intentionally letting caller handle exceptions let notebookFileBuffer = await pfs.readFile(path); - let contents: JSONObject = json.parse(notebookFileBuffer.toString()); + let stringContents = notebookFileBuffer.toString(); + let contents: JSONObject = json.parse(stringContents); if (contents) { if (contents.nbformat === 4) { @@ -40,9 +42,13 @@ export class LocalContentManager implements nb.ContentManager { if (contents.nbformat) { throw new TypeError(localize('nbformatNotRecognized', 'nbformat v{0}.{1} not recognized', contents.nbformat, contents.nbformat_minor)); } + } else if (stringContents === '' || stringContents === undefined) { + // Empty? + return v4.createEmptyNotebook(); } + // else, fallthrough condition - throw new TypeError(localize('nbNotSupported', 'This notebook format is not supported')); + throw new TypeError(localize('nbNotSupported', 'This file does not have a valid notebook format')); } @@ -72,6 +78,15 @@ namespace v4 { return notebook; } + export function createEmptyNotebook(): nb.INotebookContents { + return { + cells: [], + metadata: undefined, + nbformat: nbformat.MAJOR_VERSION, + nbformat_minor: nbformat.MINOR_VERSION + }; + } + function readCell(cell: nb.ICellContents): nb.ICellContents { switch (cell.cell_type) { case 'markdown':