Skip to content

Commit aaa1228

Browse files
rchiodorchiodo
authored
A different way of fixing escaping (#14186) (#168)
* A different way of fixing escaping (#14186) * Fix port error * Fix mock file system to ignore case * Code review feedback and linting error
1 parent b03fc29 commit aaa1228

16 files changed

Lines changed: 248 additions & 78 deletions

‎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 uuid from 'uuid/v4';
97
import { DebugConfiguration, Disposable } from 'vscode';
108
import * as vsls from 'vsls/vscode';
@@ -332,10 +330,8 @@ export class JupyterDebugger implements IJupyterDebugger, ICellHashListener {
332330
if (outputs.length > 0) {
333331
const data = outputs[0].data;
334332
if (data && data.hasOwnProperty('text/plain')) {
335-
// Plain text should be escaped by our execution engine. Unescape it so
336-
// we can parse it.
337333
// tslint:disable-next-line:no-any
338-
return unescape((data as any)['text/plain']);
334+
return (data as any)['text/plain'];
339335
}
340336
if (outputs[0].output_type === 'stream') {
341337
const stream = outputs[0] as nbformat.IStream;

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

Lines changed: 31 additions & 33 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
});
@@ -1225,7 +1221,10 @@ export class JupyterNotebookBase implements INotebook {
12251221
) {
12261222
// Check our length on text output
12271223
if (msg.content.data && msg.content.data.hasOwnProperty('text/plain')) {
1228-
msg.content.data['text/plain'] = escape(trimFunc(msg.content.data['text/plain'] as string));
1224+
msg.content.data['text/plain'] = splitMultilineString(
1225+
// tslint:disable-next-line: no-any
1226+
trimFunc(concatMultilineString(msg.content.data['text/plain'] as any))
1227+
);
12291228
}
12301229

12311230
this.addToCellData(
@@ -1253,14 +1252,14 @@ export class JupyterNotebookBase implements INotebook {
12531252
reply.payload.forEach((o) => {
12541253
if (o.data && o.data.hasOwnProperty('text/plain')) {
12551254
// tslint:disable-next-line: no-any
1256-
const str = (o.data as any)['text/plain'].toString();
1257-
const data = escape(trimFunc(str)) as string;
1255+
const str = concatMultilineString((o.data as any)['text/plain']); // NOSONAR
1256+
const data = trimFunc(str);
12581257
this.addToCellData(
12591258
cell,
12601259
{
12611260
// Mark as stream output so the text is formatted because it likely has ansi codes in it.
12621261
output_type: 'stream',
1263-
text: data,
1262+
text: splitMultilineString(data),
12641263
name: 'stdout',
12651264
metadata: {},
12661265
execution_count: reply.execution_count
@@ -1302,23 +1301,25 @@ export class JupyterNotebookBase implements INotebook {
13021301
? data.outputs[data.outputs.length - 1]
13031302
: undefined;
13041303
if (existing) {
1305-
// tslint:disable-next-line:restrict-plus-operands
1306-
existing.text = existing.text + escape(msg.content.text);
1307-
const originalText = formatStreamText(concatMultilineString(existing.text));
1304+
const originalText = formatStreamText(
1305+
// tslint:disable-next-line: no-any
1306+
`${concatMultilineString(existing.text as any)}${concatMultilineString(msg.content.text)}`
1307+
);
13081308
originalTextLength = originalText.length;
1309-
existing.text = trimFunc(originalText);
1310-
trimmedTextLength = existing.text.length;
1309+
const newText = trimFunc(originalText);
1310+
trimmedTextLength = newText.length;
1311+
existing.text = splitMultilineString(newText);
13111312
} else {
1312-
const originalText = formatStreamText(concatMultilineString(escape(msg.content.text)));
1313+
const originalText = formatStreamText(concatMultilineString(msg.content.text));
13131314
originalTextLength = originalText.length;
13141315
// Create a new stream entry
13151316
const output: nbformat.IStream = {
13161317
output_type: 'stream',
13171318
name: msg.content.name,
1318-
text: trimFunc(originalText)
1319+
text: [trimFunc(originalText)]
13191320
};
13201321
data.outputs = [...data.outputs, output];
1321-
trimmedTextLength = output.text.length;
1322+
trimmedTextLength = output.text[0].length;
13221323
cell.data = data;
13231324
}
13241325

@@ -1327,23 +1328,16 @@ export class JupyterNotebookBase implements INotebook {
13271328
// the output is trimmed and what setting changes that.
13281329
// * If data.metadata.tags is undefined, define it so the following
13291330
// code is can rely on it being defined.
1330-
if (data.metadata.tags === undefined) {
1331-
data.metadata.tags = [];
1332-
}
1333-
1334-
data.metadata.tags = data.metadata.tags.filter((t) => t !== 'outputPrepend');
1335-
13361331
if (trimmedTextLength < originalTextLength) {
1332+
if (data.metadata.tags === undefined) {
1333+
data.metadata.tags = [];
1334+
}
1335+
data.metadata.tags = data.metadata.tags.filter((t) => t !== 'outputPrepend');
13371336
data.metadata.tags.push('outputPrepend');
13381337
}
13391338
}
13401339

13411340
private handleDisplayData(msg: KernelMessage.IDisplayDataMsg, clearState: RefBool, cell: ICell) {
1342-
// Escape text output
1343-
if (msg.content.data && msg.content.data.hasOwnProperty('text/plain')) {
1344-
msg.content.data['text/plain'] = escape(msg.content.data['text/plain'] as string);
1345-
}
1346-
13471341
const output: nbformat.IDisplayData = {
13481342
output_type: 'display_data',
13491343
data: msg.content.data,
@@ -1391,10 +1385,14 @@ export class JupyterNotebookBase implements INotebook {
13911385
private handleError(msg: KernelMessage.IErrorMsg, clearState: RefBool, cell: ICell) {
13921386
const output: nbformat.IError = {
13931387
output_type: 'error',
1394-
ename: escape(msg.content.ename),
1395-
evalue: escape(msg.content.evalue),
1396-
traceback: msg.content.traceback.map(escape)
1388+
ename: msg.content.ename,
1389+
evalue: msg.content.evalue,
1390+
traceback: msg.content.traceback
13971391
};
1392+
if (msg.content.hasOwnProperty('transient')) {
1393+
// tslint:disable-next-line: no-any
1394+
output.transient = (msg.content as any).transient;
1395+
}
13981396
this.addToCellData(cell, output, clearState);
13991397
cell.state = CellState.error;
14001398

‎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: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,8 @@ export class PostOffice implements IDisposable {
8787
// In such instances the `acquireVSCodeApi` will return the event handler to get messages from extension.
8888
// See ./src/datascience-ui/native-editor/index.html
8989
// tslint:disable-next-line: no-any
90-
const api = (this.vscodeApi as any) as { handleMessage?: Function };
91-
if (api.handleMessage) {
90+
const api = (this.vscodeApi as any) as undefined | { handleMessage?: Function };
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
@@ -64,6 +64,8 @@ import {
6464
verifyLastCellInputState
6565
} from './testHelpers';
6666
import { ITestInteractiveWindowProvider } from './testInteractiveWindowProvider';
67+
// tslint:disable-next-line: no-require-imports no-var-requires
68+
const _escape = require('lodash/escape') as typeof import('lodash/escape'); // NOSONAR
6769

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

440+
const exception = 'raise Exception("<html check>")';
441+
addMockData(ioc, exception, `"<html check>"`, 'text/html', 'error');
442+
438443
addMockData(ioc, badPanda, `pandas has no attribute 'read'`, 'text/html', 'error');
439444
addMockData(ioc, goodPanda, `<td>A table</td>`, 'text/html');
440445
addMockData(ioc, matPlotLib, matPlotLibResults, 'text/html');
@@ -451,6 +456,9 @@ for _ in range(50):
451456
return Promise.resolve({ result: result, haveMore: loops > 0 });
452457
});
453458

459+
await addCode(ioc, exception, true);
460+
verifyHtmlOnInteractiveCell(_escape(`<html check>`), CellPosition.Last);
461+
454462
await addCode(ioc, badPanda, true);
455463
verifyHtmlOnInteractiveCell(`has no attribute 'read'`, CellPosition.Last);
456464

0 commit comments

Comments
 (0)