Skip to content

Commit afc155b

Browse files
rchiodorchiodo
authored
A different way of fixing escaping (#14186)
* Move escaping to just output * Add some tests to verify round tripping * Fixup test for rountrip and make roundtripping actually work * Add news entry * Add to manual test file * Fix streaming problem and add more to the test * Fix traceback unit test * Fix problem caught by functional tests :) * Another functional test catch
1 parent cfe12a7 commit afc155b

File tree

14 files changed

+239
-79
lines changed

14 files changed

+239
-79
lines changed

‎news/2 Fixes/14182.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Do not escape output in the actual ipynb file.

‎src/client/datascience/editor-integration/cellhashprovider.ts

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,9 @@ import {
2929

3030
// tslint:disable-next-line:no-require-imports no-var-requires
3131
const _escapeRegExp = require('lodash/escapeRegExp') as typeof import('lodash/escapeRegExp'); // NOSONAR
32-
const LineNumberMatchRegex = /(;32m[ ->]*?)(\d+)/g;
32+
// tslint:disable-next-line: no-require-imports no-var-requires
33+
const _escape = require('lodash/escape') as typeof import('lodash/escape'); // NOSONAR
34+
const LineNumberMatchRegex = /(;32m[ ->]*?)(\d+)(.*)/g;
3335

3436
interface IRangedCellHash extends ICellHash {
3537
code: string;
@@ -133,7 +135,7 @@ export class CellHashProvider implements ICellHashProvider, INotebookExecutionLo
133135
...msg,
134136
content: {
135137
...msg.content,
136-
traceback: this.modifyTraceback(msg as KernelMessage.IErrorMsg) // NOSONAR
138+
transient: this.modifyTraceback(msg as KernelMessage.IErrorMsg) // NOSONAR
137139
}
138140
};
139141
}
@@ -423,14 +425,16 @@ export class CellHashProvider implements ICellHashProvider, INotebookExecutionLo
423425
// Now attempt to find a cell that matches these source lines
424426
const offset = this.findCellOffset(this.hashes.get(match[0]), sourceLines);
425427
if (offset !== undefined) {
426-
return traceFrame.replace(LineNumberMatchRegex, (_s, prefix, num) => {
428+
return traceFrame.replace(LineNumberMatchRegex, (_s, prefix, num, suffix) => {
427429
const n = parseInt(num, 10);
428430
const newLine = offset + n - 1;
429-
return `${prefix}<a href='file://${match[0]}?line=${newLine}'>${newLine + 1}</a>`;
431+
return `${_escape(prefix)}<a href='file://${match[0]}?line=${newLine}'>${newLine + 1}</a>${_escape(
432+
suffix
433+
)}`;
430434
});
431435
}
432436
}
433-
return traceFrame;
437+
return _escape(traceFrame);
434438
}
435439
}
436440

‎src/client/datascience/jupyter/jupyterDebugger.ts

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,6 @@
33
'use strict';
44
import type { nbformat } from '@jupyterlab/coreutils';
55
import { inject, injectable, named } from 'inversify';
6-
// tslint:disable-next-line: no-require-imports
7-
import unescape = require('lodash/unescape');
86
import * as path from 'path';
97
import * as uuid from 'uuid/v4';
108
import { DebugConfiguration, Disposable } from 'vscode';
@@ -475,10 +473,8 @@ export class JupyterDebugger implements IJupyterDebugger, ICellHashListener {
475473
if (outputs.length > 0) {
476474
const data = outputs[0].data;
477475
if (data && data.hasOwnProperty('text/plain')) {
478-
// Plain text should be escaped by our execution engine. Unescape it so
479-
// we can parse it.
480476
// tslint:disable-next-line:no-any
481-
return unescape((data as any)['text/plain']);
477+
return (data as any)['text/plain'];
482478
}
483479
if (outputs[0].output_type === 'stream') {
484480
const stream = outputs[0] as nbformat.IStream;

‎src/client/datascience/jupyter/jupyterNotebook.ts

Lines changed: 33 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -40,11 +40,7 @@ import { KernelConnectionMetadata } from './kernels/types';
4040

4141
// tslint:disable-next-line: no-require-imports
4242
import cloneDeep = require('lodash/cloneDeep');
43-
// tslint:disable-next-line: no-require-imports
44-
import escape = require('lodash/escape');
45-
// tslint:disable-next-line: no-require-imports
46-
import unescape = require('lodash/unescape');
47-
import { concatMultilineString, formatStreamText } from '../../../datascience-ui/common';
43+
import { concatMultilineString, formatStreamText, splitMultilineString } from '../../../datascience-ui/common';
4844
import { RefBool } from '../../common/refBool';
4945
import { PythonEnvironment } from '../../pythonEnvironments/info';
5046
import { getInterpreterFromKernelConnectionMetadata, isPythonKernelConnection } from './kernels/helpers';
@@ -787,12 +783,12 @@ export class JupyterNotebookBase implements INotebook {
787783
outputs.forEach((o) => {
788784
if (o.output_type === 'stream') {
789785
const stream = o as nbformat.IStream;
790-
result = result.concat(formatStreamText(unescape(concatMultilineString(stream.text, true))));
786+
result = result.concat(formatStreamText(concatMultilineString(stream.text, true)));
791787
} else {
792788
const data = o.data;
793789
if (data && data.hasOwnProperty('text/plain')) {
794790
// tslint:disable-next-line:no-any
795-
result = result.concat(unescape((data as any)['text/plain']));
791+
result = result.concat((data as any)['text/plain']);
796792
}
797793
}
798794
});
@@ -1206,12 +1202,7 @@ export class JupyterNotebookBase implements INotebook {
12061202

12071203
private addToCellData = (
12081204
cell: ICell,
1209-
output:
1210-
| nbformat.IUnrecognizedOutput
1211-
| nbformat.IExecuteResult
1212-
| nbformat.IDisplayData
1213-
| nbformat.IStream
1214-
| nbformat.IError,
1205+
output: nbformat.IExecuteResult | nbformat.IDisplayData | nbformat.IStream | nbformat.IError,
12151206
clearState: RefBool
12161207
) => {
12171208
const data: nbformat.ICodeCell = cell.data as nbformat.ICodeCell;
@@ -1237,7 +1228,10 @@ export class JupyterNotebookBase implements INotebook {
12371228
) {
12381229
// Check our length on text output
12391230
if (msg.content.data && msg.content.data.hasOwnProperty('text/plain')) {
1240-
msg.content.data['text/plain'] = escape(trimFunc(msg.content.data['text/plain'] as string));
1231+
msg.content.data['text/plain'] = splitMultilineString(
1232+
// tslint:disable-next-line: no-any
1233+
trimFunc(concatMultilineString(msg.content.data['text/plain'] as any))
1234+
);
12411235
}
12421236

12431237
this.addToCellData(
@@ -1265,14 +1259,15 @@ export class JupyterNotebookBase implements INotebook {
12651259
reply.payload.forEach((o) => {
12661260
if (o.data && o.data.hasOwnProperty('text/plain')) {
12671261
// tslint:disable-next-line: no-any
1268-
const str = (o.data as any)['text/plain'].toString();
1269-
const data = escape(trimFunc(str)) as string;
1262+
const str = concatMultilineString((o.data as any)['text/plain']); // NOSONAR
1263+
const data = trimFunc(str);
12701264
this.addToCellData(
12711265
cell,
12721266
{
12731267
// Mark as stream output so the text is formatted because it likely has ansi codes in it.
12741268
output_type: 'stream',
1275-
text: data,
1269+
text: splitMultilineString(data),
1270+
name: 'stdout',
12761271
metadata: {},
12771272
execution_count: reply.execution_count
12781273
},
@@ -1313,23 +1308,25 @@ export class JupyterNotebookBase implements INotebook {
13131308
? data.outputs[data.outputs.length - 1]
13141309
: undefined;
13151310
if (existing) {
1316-
// tslint:disable-next-line:restrict-plus-operands
1317-
existing.text = existing.text + escape(msg.content.text);
1318-
const originalText = formatStreamText(concatMultilineString(existing.text));
1311+
const originalText = formatStreamText(
1312+
// tslint:disable-next-line: no-any
1313+
`${concatMultilineString(existing.text as any)}${concatMultilineString(msg.content.text)}`
1314+
);
13191315
originalTextLength = originalText.length;
1320-
existing.text = trimFunc(originalText);
1321-
trimmedTextLength = existing.text.length;
1316+
const newText = trimFunc(originalText);
1317+
trimmedTextLength = newText.length;
1318+
existing.text = splitMultilineString(newText);
13221319
} else {
1323-
const originalText = formatStreamText(concatMultilineString(escape(msg.content.text)));
1320+
const originalText = formatStreamText(concatMultilineString(msg.content.text));
13241321
originalTextLength = originalText.length;
13251322
// Create a new stream entry
13261323
const output: nbformat.IStream = {
13271324
output_type: 'stream',
13281325
name: msg.content.name,
1329-
text: trimFunc(originalText)
1326+
text: [trimFunc(originalText)]
13301327
};
13311328
data.outputs = [...data.outputs, output];
1332-
trimmedTextLength = output.text.length;
1329+
trimmedTextLength = output.text[0].length;
13331330
cell.data = data;
13341331
}
13351332

@@ -1338,23 +1335,16 @@ export class JupyterNotebookBase implements INotebook {
13381335
// the output is trimmed and what setting changes that.
13391336
// * If data.metadata.tags is undefined, define it so the following
13401337
// code is can rely on it being defined.
1341-
if (data.metadata.tags === undefined) {
1342-
data.metadata.tags = [];
1343-
}
1344-
1345-
data.metadata.tags = data.metadata.tags.filter((t) => t !== 'outputPrepend');
1346-
13471338
if (trimmedTextLength < originalTextLength) {
1339+
if (data.metadata.tags === undefined) {
1340+
data.metadata.tags = [];
1341+
}
1342+
data.metadata.tags = data.metadata.tags.filter((t) => t !== 'outputPrepend');
13481343
data.metadata.tags.push('outputPrepend');
13491344
}
13501345
}
13511346

13521347
private handleDisplayData(msg: KernelMessage.IDisplayDataMsg, clearState: RefBool, cell: ICell) {
1353-
// Escape text output
1354-
if (msg.content.data && msg.content.data.hasOwnProperty('text/plain')) {
1355-
msg.content.data['text/plain'] = escape(msg.content.data['text/plain'] as string);
1356-
}
1357-
13581348
const output: nbformat.IDisplayData = {
13591349
output_type: 'display_data',
13601350
data: msg.content.data,
@@ -1402,10 +1392,14 @@ export class JupyterNotebookBase implements INotebook {
14021392
private handleError(msg: KernelMessage.IErrorMsg, clearState: RefBool, cell: ICell) {
14031393
const output: nbformat.IError = {
14041394
output_type: 'error',
1405-
ename: escape(msg.content.ename),
1406-
evalue: escape(msg.content.evalue),
1407-
traceback: msg.content.traceback.map(escape)
1395+
ename: msg.content.ename,
1396+
evalue: msg.content.evalue,
1397+
traceback: msg.content.traceback
14081398
};
1399+
if (msg.content.hasOwnProperty('transient')) {
1400+
// tslint:disable-next-line: no-any
1401+
output.transient = (msg.content as any).transient;
1402+
}
14091403
this.addToCellData(cell, output, clearState);
14101404
cell.state = CellState.error;
14111405

‎src/client/datascience/jupyter/kernelVariables.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,6 @@ import { inject, injectable } from 'inversify';
66
import stripAnsi from 'strip-ansi';
77
import * as uuid from 'uuid/v4';
88

9-
// tslint:disable-next-line: no-require-imports
10-
import unescape = require('lodash/unescape');
119
import { CancellationToken, Event, EventEmitter, Uri } from 'vscode';
1210
import { PYTHON_LANGUAGE } from '../../common/constants';
1311
import { traceError } from '../../common/logger';
@@ -248,7 +246,7 @@ export class KernelVariables implements IJupyterVariables {
248246

249247
// Pull our text result out of the Jupyter cell
250248
private deserializeJupyterResult<T>(cells: ICell[]): T {
251-
const text = unescape(this.extractJupyterResultText(cells));
249+
const text = this.extractJupyterResultText(cells);
252250
return JSON.parse(text) as T;
253251
}
254252

@@ -373,7 +371,7 @@ export class KernelVariables implements IJupyterVariables {
373371
// Now execute the query
374372
if (notebook && query) {
375373
const cells = await notebook.execute(query.query, Identifiers.EmptyFileName, 0, uuid(), token, true);
376-
const text = unescape(this.extractJupyterResultText(cells));
374+
const text = this.extractJupyterResultText(cells);
377375

378376
// Apply the expression to it
379377
const matches = this.getAllMatches(query.parser, text);

‎src/client/datascience/jupyter/oldJupyterVariables.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,6 @@ import { Event, EventEmitter, Uri } from 'vscode';
1111
import { PYTHON_LANGUAGE } from '../../common/constants';
1212
import { traceError } from '../../common/logger';
1313

14-
// tslint:disable-next-line: no-require-imports
15-
import unescape = require('lodash/unescape');
1614
import { IConfigurationService } from '../../common/types';
1715
import * as localize from '../../common/utils/localize';
1816
import { EXTENSION_ROOT_DIR } from '../../constants';
@@ -234,7 +232,7 @@ export class OldJupyterVariables implements IJupyterVariables {
234232

235233
// Pull our text result out of the Jupyter cell
236234
private deserializeJupyterResult<T>(cells: ICell[]): T {
237-
const text = unescape(this.extractJupyterResultText(cells));
235+
const text = this.extractJupyterResultText(cells);
238236
return JSON.parse(text) as T;
239237
}
240238

@@ -359,7 +357,7 @@ export class OldJupyterVariables implements IJupyterVariables {
359357
// Now execute the query
360358
if (notebook && query) {
361359
const cells = await notebook.execute(query.query, Identifiers.EmptyFileName, 0, uuid(), undefined, true);
362-
const text = unescape(this.extractJupyterResultText(cells));
360+
const text = this.extractJupyterResultText(cells);
363361

364362
// Apply the expression to it
365363
const matches = this.getAllMatches(query.parser, text);

‎src/datascience-ui/interactive-common/cellOutput.tsx

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ import { getRichestMimetype, getTransform, isIPyWidgetOutput, isMimeTypeSupporte
2020

2121
// tslint:disable-next-line: no-var-requires no-require-imports
2222
const ansiToHtml = require('ansi-to-html');
23+
// tslint:disable-next-line: no-var-requires no-require-imports
24+
const lodashEscape = require('lodash/escape');
2325

2426
// tslint:disable-next-line: no-require-imports no-var-requires
2527
const cloneDeep = require('lodash/cloneDeep');
@@ -328,7 +330,7 @@ export class CellOutput extends React.Component<ICellOutputProps> {
328330
// tslint:disable-next-line: no-any
329331
const text = (input as any)['text/plain'];
330332
input = {
331-
'text/html': text // XML tags should have already been escaped.
333+
'text/html': lodashEscape(concatMultilineString(text))
332334
};
333335
} else if (output.output_type === 'stream') {
334336
mimeType = 'text/html';
@@ -337,7 +339,7 @@ export class CellOutput extends React.Component<ICellOutputProps> {
337339
renderWithScrollbars = true;
338340
// Sonar is wrong, TS won't compile without this AS
339341
const stream = output as nbformat.IStream; // NOSONAR
340-
const concatted = concatMultilineString(stream.text);
342+
const concatted = lodashEscape(concatMultilineString(stream.text));
341343
input = {
342344
'text/html': concatted // XML tags should have already been escaped.
343345
};
@@ -363,14 +365,18 @@ export class CellOutput extends React.Component<ICellOutputProps> {
363365
const error = output as nbformat.IError; // NOSONAR
364366
try {
365367
const converter = new CellOutput.ansiToHtmlClass(CellOutput.getAnsiToHtmlOptions());
366-
const trace = error.traceback.length ? converter.toHtml(error.traceback.join('\n')) : error.evalue;
368+
// Modified traceback may exist. If so use that instead. It's only at run time though
369+
const traceback: string[] = error.transient
370+
? (error.transient as string[])
371+
: error.traceback.map(lodashEscape);
372+
const trace = traceback ? converter.toHtml(traceback.join('\n')) : error.evalue;
367373
input = {
368374
'text/html': trace
369375
};
370376
} catch {
371377
// This can fail during unit tests, just use the raw data
372378
input = {
373-
'text/html': error.evalue
379+
'text/html': lodashEscape(error.evalue)
374380
};
375381
}
376382
} else if (input) {
@@ -395,6 +401,12 @@ export class CellOutput extends React.Component<ICellOutputProps> {
395401
data = fixMarkdown(concatMultilineString(data as nbformat.MultilineString, true), true);
396402
}
397403

404+
// Make sure text output is escaped (nteract texttransform won't)
405+
if (mimeType === 'text/plain' && data) {
406+
data = lodashEscape(data.toString());
407+
mimeType = 'text/html';
408+
}
409+
398410
return {
399411
isText,
400412
isError,

‎src/datascience-ui/react-common/postOffice.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ export class PostOffice implements IDisposable {
8888
// See ./src/datascience-ui/native-editor/index.html
8989
// tslint:disable-next-line: no-any
9090
const api = (this.vscodeApi as any) as { handleMessage?: Function };
91-
if (api.handleMessage) {
91+
if (api && api.handleMessage) {
9292
api.handleMessage(this.handleMessages.bind(this));
9393
}
9494
} catch {

‎src/test/datascience/interactiveWindow.functional.test.tsx

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,8 @@ import {
6363
verifyLastCellInputState
6464
} from './testHelpers';
6565
import { ITestInteractiveWindowProvider } from './testInteractiveWindowProvider';
66+
// tslint:disable-next-line: no-require-imports no-var-requires
67+
const _escape = require('lodash/escape') as typeof import('lodash/escape'); // NOSONAR
6668

6769
// tslint:disable:max-func-body-length trailing-comma no-any no-multiline-string
6870
suite('DataScience Interactive Window output tests', () => {
@@ -385,6 +387,9 @@ for _ in range(50):
385387
time.sleep(0.1)
386388
sys.stdout.write('\\r')`;
387389

390+
const exception = 'raise Exception("<html check>")';
391+
addMockData(ioc, exception, `"<html check>"`, 'text/html', 'error');
392+
388393
addMockData(ioc, badPanda, `pandas has no attribute 'read'`, 'text/html', 'error');
389394
addMockData(ioc, goodPanda, `<td>A table</td>`, 'text/html');
390395
addMockData(ioc, matPlotLib, matPlotLibResults, 'text/html');
@@ -401,6 +406,9 @@ for _ in range(50):
401406
return Promise.resolve({ result: result, haveMore: loops > 0 });
402407
});
403408

409+
await addCode(ioc, exception, true);
410+
verifyHtmlOnInteractiveCell(_escape(`<html check>`), CellPosition.Last);
411+
404412
await addCode(ioc, badPanda, true);
405413
verifyHtmlOnInteractiveCell(`has no attribute 'read'`, CellPosition.Last);
406414

0 commit comments

Comments
 (0)