From 2235ebaf20079b4b0847fa8131b469da7f81c266 Mon Sep 17 00:00:00 2001 From: Kevin Cunnane Date: Tue, 8 Jan 2019 15:24:16 -0800 Subject: [PATCH] Fix #3680 Notebooks: outputs with string arrays rendered incorrectly (#3681) * Refactor JSON and format files to model and fix tabs -> spaces issues This is in prep for some work to reuse these code files inside the model, so pushing as its own PR to keep the next piece of work clean. * Fix #3680 Notebooks: outputs with string arrays rendered incorrectly - Add support for processing v4 format files loaded from disk - Prep support for v3 notebooks by adding placeholder code for that * Fix failing tests and add specific one for this bug * Remove references to v5 --- .../services/notebook/localContentManager.ts | 189 ++++++++++++++++-- src/sql/sqlops.proposed.d.ts | 40 +++- .../notebook/model/contentManagers.test.ts | 41 +++- .../notebook/model/notebookModel.test.ts | 12 +- 4 files changed, 251 insertions(+), 31 deletions(-) diff --git a/src/sql/services/notebook/localContentManager.ts b/src/sql/services/notebook/localContentManager.ts index 41ac81c79e..e7d83745ce 100644 --- a/src/sql/services/notebook/localContentManager.ts +++ b/src/sql/services/notebook/localContentManager.ts @@ -3,6 +3,8 @@ * Licensed under the Source EULA. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ +// Note: the code in the v3 and v4 namespaces has been adapted (with significant changes) from https://github.com/nteract/nteract/tree/master/packages/commutable + 'use strict'; import { nb } from 'sqlops'; @@ -10,26 +12,179 @@ import { nb } from 'sqlops'; import * as json from 'vs/base/common/json'; import * as pfs from 'vs/base/node/pfs'; import URI from 'vs/base/common/uri'; +import { localize } from 'vs/nls'; +import { JSONObject } from 'sql/parts/notebook/models/jsonext'; import ContentManager = nb.ContentManager; +import { OutputTypes } from 'sql/parts/notebook/models/contracts'; + +type MimeBundle = { [key: string]: string | string[] | undefined }; export class LocalContentManager implements ContentManager { - public async getNotebookContents(notebookUri: URI): Promise { - if (!notebookUri) { - return undefined; - } - // TODO validate this is an actual file URI, and error if not - let path = notebookUri.fsPath; - // Note: intentionally letting caller handle exceptions - let notebookFileBuffer = await pfs.readFile(path); - return json.parse(notebookFileBuffer.toString()); - } + public async getNotebookContents(notebookUri: URI): Promise { + if (!notebookUri) { + return undefined; + } + // TODO validate this is an actual file URI, and error if not + let path = notebookUri.fsPath; + // Note: intentionally letting caller handle exceptions + let notebookFileBuffer = await pfs.readFile(path); + let contents: JSONObject = json.parse(notebookFileBuffer.toString()); + + if (contents) { + if (contents.nbformat === 4) { + return v4.readNotebook(contents); + } else if (contents.nbformat === 3) { + return v3.readNotebook(contents); + } + if (contents.nbformat) { + throw new TypeError(localize('nbformatNotRecognized', 'nbformat v{0}.{1} not recognized', contents.nbformat, contents.nbformat_minor)); + } + } + // else, fallthrough condition + throw new TypeError(localize('nbNotSupported', 'This notebook format is not supported')); + + } + + public async save(notebookUri: URI, notebook: nb.INotebookContents): Promise { + // Convert to JSON with pretty-print functionality + let contents = JSON.stringify(notebook, undefined, ' '); + let path = notebookUri.fsPath; + await pfs.writeFile(path, contents); + return notebook; + } - public async save(notebookUri: URI, notebook: nb.INotebookContents): Promise { - // Convert to JSON with pretty-print functionality - let contents = JSON.stringify(notebook, undefined, ' '); - let path = notebookUri.fsPath; - await pfs.writeFile(path, contents); - return notebook; - } +} + +namespace v4 { + export function readNotebook(contents: nb.INotebookContents): nb.INotebookContents { + let notebook: nb.INotebookContents = { + cells: [], + metadata: contents.metadata, + nbformat: 4, + nbformat_minor: contents.nbformat_minor + }; + + for (let cell of contents.cells) { + notebook.cells.push(readCell(cell)); + } + + return notebook; + } + function readCell(cell: nb.ICellContents): nb.ICellContents { + switch (cell.cell_type) { + case 'markdown': + case 'raw': + return createDefaultCell(cell); + case 'code': + return createCodeCell(cell); + default: + throw new TypeError(localize('unknownCellType', 'Cell type {0} unknown', cell.cell_type)); + } + } + + function createDefaultCell(cell: nb.ICellContents): nb.ICellContents { + return { + cell_type: cell.cell_type, + source: demultiline(cell.source), + metadata: cell.metadata + }; + } + function createCodeCell(cell: nb.ICellContents): nb.ICellContents { + return { + cell_type: cell.cell_type, + source: demultiline(cell.source), + metadata: cell.metadata, + execution_count: cell.execution_count, + outputs: createOutputs(cell) + }; + } + + function createOutputs(cell: nb.ICellContents): nb.ICellOutput[] { + return cell.outputs && cell.outputs.length > 0 ? cell.outputs.map(output => createOutput(output as nb.Output)) : []; + } + + function createOutput(output: nb.Output): nb.ICellOutput { + switch (output.output_type) { + case OutputTypes.ExecuteResult: + return { + output_type: output.output_type, + execution_count: output.execution_count, + data: createMimeBundle(output.data), + metadata: output.metadata + }; + case OutputTypes.DisplayData: + case OutputTypes.UpdateDisplayData: + return { + output_type: output.output_type, + data: createMimeBundle(output.data), + metadata: output.metadata + }; + case 'stream': + return { + output_type: output.output_type, + name: output.name, + text: demultiline(output.text) + }; + case 'error': + return { + output_type: 'error', + ename: output.ename, + evalue: output.evalue, + // Note: this is one of the cases where the Array of strings (for + // traceback) is part of the format, not a multiline string + traceback: output.traceback + }; + default: + // Should never get here + throw new TypeError(localize('unrecognizedOutput', 'Output type {0} not recognized', (output).output_type)); + } + } + + function createMimeBundle(oldMimeBundle: MimeBundle): MimeBundle { + let mimeBundle: MimeBundle = {}; + for (let key of Object.keys(oldMimeBundle)) { + mimeBundle[key] = cleanMimeData(key, oldMimeBundle[key]); + } + return mimeBundle; + } + + /** + * Cleans mimedata, primarily converts an array of strings into a single string + * joined by newlines. + * + * @param key The key, usually a mime type, that is associated with the mime data. + * @param data The mime data to clean. + * + * @returns The cleaned mime data. + */ + function cleanMimeData(key: string, data: string | string[] | undefined) { + // See https://github.com/jupyter/nbformat/blob/62d6eb8803616d198eaa2024604d1fe923f2a7b3/nbformat/v4/nbformat.v4.schema.json#L368 + if (isJSONKey(key)) { + // Data stays as is for JSON types + return data; + } + + if (typeof data === 'string' || Array.isArray(data)) { + return demultiline(data); + } + + throw new TypeError(localize('invalidMimeData', 'Data for {0} is expected to be a string or an Array of strings', key)); + } + + function demultiline(value: nb.MultilineString): string { + return Array.isArray(value) ? value.join('') : value; + } + + function isJSONKey(key: string): boolean { + return /^application\/(.*\+)?json$/.test(key); + } +} + +namespace v3 { + + export function readNotebook(contents: nb.INotebookContents): nb.INotebookContents { + // TODO will add v3 support in future update + throw new TypeError(localize('nbNotSupported', 'This notebook format is not supported')); + } } diff --git a/src/sql/sqlops.proposed.d.ts b/src/sql/sqlops.proposed.d.ts index 45950715af..23da9370ed 100644 --- a/src/sql/sqlops.proposed.d.ts +++ b/src/sql/sqlops.proposed.d.ts @@ -1779,24 +1779,36 @@ declare module 'sqlops' { metadata: { language?: string; }; - execution_count: number; + execution_count?: number; outputs?: ICellOutput[]; } export type CellType = 'code' | 'markdown' | 'raw'; export interface ICellOutput { - output_type: OutputType; + output_type: OutputTypeName; } + + /** + * An alias for a stream type. + */ + export type StreamType = 'stdout' | 'stderr'; + + /** + * A multiline string. + */ + export type MultilineString = string | string[]; + export interface IStreamResult extends ICellOutput { + output_type: 'stream'; /** * Stream output field defining the stream name, for example stdout */ - name: string; + name: StreamType; /** * Stream output field defining the multiline stream text */ - text: string | Buffer; + text: MultilineString; } export interface IDisplayResult extends ICellOutput { /** @@ -1809,13 +1821,27 @@ declare module 'sqlops' { */ metadata?: {}; } + export interface IDisplayData extends IDisplayResult { + output_type: 'display_data'; + } + export interface IUpdateDisplayData extends IDisplayResult { + output_type: 'update_display_data'; + } export interface IExecuteResult extends IDisplayResult { + /** + * Type of cell output. + */ + output_type: 'execute_result'; /** * Number of times the cell was executed */ - executionCount: number; + execution_count: number; } export interface IErrorResult extends ICellOutput { + /** + * Type of cell output. + */ + output_type: 'error'; /** * Exception name */ @@ -1830,13 +1856,15 @@ declare module 'sqlops' { traceback?: string[]; } - export type OutputType = + export type OutputTypeName = | 'execute_result' | 'display_data' | 'stream' | 'error' | 'update_display_data'; + export type Output = nb.IDisplayData | nb.IUpdateDisplayData | nb.IExecuteResult | nb.IErrorResult | nb.IStreamResult; + //#endregion //#region Session APIs diff --git a/src/sqltest/parts/notebook/model/contentManagers.test.ts b/src/sqltest/parts/notebook/model/contentManagers.test.ts index 468aabce1a..14ef0ac411 100644 --- a/src/sqltest/parts/notebook/model/contentManagers.test.ts +++ b/src/sqltest/parts/notebook/model/contentManagers.test.ts @@ -29,8 +29,8 @@ let expectedNotebookContent: nb.INotebookContents = { language: 'sql' } }, - nbformat: 5, - nbformat_minor: 0 + nbformat: 4, + nbformat_minor: 2 }; let notebookContentString = JSON.stringify(expectedNotebookContent); @@ -74,4 +74,41 @@ describe('Local Content Manager', function(): void { // then I expect notebook format to still be valid verifyMatchesExpectedNotebook(notebook); }); + it('Should inline mime data into a single string', async function(): Promise { + let mimeNotebook: nb.INotebookContents = { + cells: [{ + cell_type: CellTypes.Code, + source: 'insert into t1 values (c1, c2)', + metadata: { language: 'python' }, + execution_count: 1, + outputs: [ + { + output_type: 'display_data', + data: { + 'text/html': [ + '
', + '
' + ] + } + } + ] + }], + metadata: { + kernelspec: { + name: 'mssql', + language: 'sql' + } + }, + nbformat: 4, + nbformat_minor: 2 + }; + let mimeContentString = JSON.stringify(mimeNotebook); + // Given a file containing a valid notebook with multiline mime type + let localFile = tempWrite.sync(mimeContentString, 'notebook.ipynb'); + // when I read the content + let notebook = await contentManager.getNotebookContents(URI.file(localFile)); + // then I expect output to have been normalized into a single string + let displayOutput = notebook.cells[0].outputs[0]; + should(displayOutput.data['text/html']).equal('
'); + }); }); diff --git a/src/sqltest/parts/notebook/model/notebookModel.test.ts b/src/sqltest/parts/notebook/model/notebookModel.test.ts index 85748d8572..1f5915cadf 100644 --- a/src/sqltest/parts/notebook/model/notebookModel.test.ts +++ b/src/sqltest/parts/notebook/model/notebookModel.test.ts @@ -44,8 +44,8 @@ let expectedNotebookContent: nb.INotebookContents = { language: 'sql' } }, - nbformat: 5, - nbformat_minor: 0 + nbformat: 4, + nbformat_minor: 5 }; let expectedNotebookContentOneCell: nb.INotebookContents = { @@ -61,8 +61,8 @@ let expectedNotebookContentOneCell: nb.INotebookContents = { language: 'sql' } }, - nbformat: 5, - nbformat_minor: 0 + nbformat: 4, + nbformat_minor: 5 }; let defaultUri = URI.file('/some/path.ipynb'); @@ -112,8 +112,8 @@ describe('notebook model', function(): void { language: 'sql' } }, - nbformat: 5, - nbformat_minor: 0 + nbformat: 4, + nbformat_minor: 5 }; let mockContentManager = TypeMoq.Mock.ofType(LocalContentManager);