Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CodeEditor] add support of triple quotes #112656

Merged
merged 10 commits into from
Oct 21, 2021
169 changes: 169 additions & 0 deletions packages/kbn-monaco/src/xjson/grammar.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { createParser } from './grammar';

describe('createParser', () => {
let parser: ReturnType<typeof createParser>;

beforeEach(() => {
parser = createParser();
});

test('should create a xjson grammar parser', () => {
expect(createParser()).toBeInstanceOf(Function);
});

test('should return no annotations in case of valid json', () => {
expect(
parser(`
{"menu": {
"id": "file",
"value": "File",
"popup": {
"actions": [
"new",
"open",
"close"
],
"menuitem": [
{"value": "New"},
{"value": "Open"},
{"value": "Close"}
]
}
}}
`)
).toMatchInlineSnapshot(`
Object {
"annotations": Array [],
}
`);
});

test('should support triple quotes', () => {
expect(
parser(`
{"menu": {
"id": """
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what would happen if we added \" at the end of a triple quote value, so \""""

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you, that case was broken 🥇

file
""",
"value": "File"
}}
`)
).toMatchInlineSnapshot(`
Object {
"annotations": Array [],
}
`);
});

test('triple quotes should be correctly closed', () => {
expect(
parser(`
{"menu": {
"id": """"
file
"",
"value": "File"
}}
`)
).toMatchInlineSnapshot(`
Object {
"annotations": Array [
Object {
"at": 36,
"text": "Expected ',' instead of '\\"'",
"type": "error",
},
],
}
`);
});

test('text values should be wrapper into quotes', () => {
expect(
parser(`
{"menu": {
"id": id,
"value": "File"
}}
`)
).toMatchInlineSnapshot(`
Object {
"annotations": Array [
Object {
"at": 36,
"text": "Unexpected 'i'",
"type": "error",
},
],
}
`);
});
test('check for close quotes', () => {
expect(
parser(`
{"menu": {
"id": "id,
"value": "File"
}}
`)
).toMatchInlineSnapshot(`
Object {
"annotations": Array [
Object {
"at": 52,
"text": "Expected ',' instead of 'v'",
"type": "error",
},
],
}
`);
});
test('no duplicate keys', () => {
expect(
parser(`
{"menu": {
"id": "id",
"id": "File"
}}
`)
).toMatchInlineSnapshot(`
Object {
"annotations": Array [
Object {
"at": 53,
"text": "Duplicate key \\"id\\"",
"type": "warning",
},
],
}
`);
});
test('all curly quotes should be closed', () => {
expect(
parser(`
{"menu": {
"id": "id",
"name": "File"
}
`)
).toMatchInlineSnapshot(`
Object {
"annotations": Array [
Object {
"at": 82,
"text": "Expected ',' instead of ''",
"type": "error",
},
],
}
`);
});
});
72 changes: 34 additions & 38 deletions packages/kbn-monaco/src/xjson/grammar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,6 @@ export const createParser = () => {
text: m,
});
},
reset = function (newAt: number) {
ch = text.charAt(newAt);
at = newAt + 1;
},
next = function (c?: string) {
return (
c && c !== ch && error("Expected '" + c + "' instead of '" + ch + "'"),
Expand All @@ -69,15 +65,6 @@ export const createParser = () => {
ch
);
},
nextUpTo = function (upTo: any, errorMessage: string) {
let currentAt = at,
i = text.indexOf(upTo, currentAt);
if (i < 0) {
error(errorMessage || "Expected '" + upTo + "'");
}
reset(i + upTo.length);
return text.substring(currentAt, i);
},
peek = function (c: string) {
return text.substr(at, c.length) === c; // nocommit - double check
},
Expand All @@ -96,37 +83,42 @@ export const createParser = () => {
(string += ch), next();
return (number = +string), isNaN(number) ? (error('Bad number'), void 0) : number;
},
stringLiteral = function(quotes: string) {
Copy link
Contributor

@jloleysens jloleysens Sep 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original functionality was airlifted out of:

}), ace.define("ace/mode/json/json_parse", ["require", "exports", "module"], function() {

This is, unfortunately, not exactly the most reader-friendly code and so without tests it's all the more tough to say if a regression has snuck in or not. I know with it being "the same-ish" as Console we had more guarantees that the code worked as expected (according to the tests on Console). It has also been fairly "manually" tested since then. I apologise for not adding tests here at the time 🤦🏻.

I think it would be useful to add tests for the difference you're expecting to see with these changes, but AFAICT it looks like this was just a refactor and we are expecting the same behaviour? If that's the case I'd recommend breaking this change out into a follow up PR that adds tests for the "before" and "after" behaviour against this parser.

Let me know what you think, or whether I've missed something!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be useful to add tests for the difference you're expecting to see with these changes, but AFAICT it looks like this was just a refactor and we are expecting the same behaviour? If that's the case I'd recommend breaking this change out into a follow up PR that adds tests for the "before" and "after" behaviour against this parser.

++ Great suggestion. I'd also feel more comfortable with this change if I better understood the difference in behavior we're expecting (if any) to help ensure we're not introducing any regressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed some tests to be sure that everything works as before. Please if you see some specific case which you want to cover please let me know.

const end = text.indexOf(quotes, at + quotes.length);

if (end >= 0) {
for (let l = end - at + quotes.length; l > 0; l--) {
next();
}
}
return next();
},
string = function () {
let hex: any,
i: any,
uffff: any,
string = '';

if ('"' === ch) {
if (peek('""')) {
// literal
next('"');
next('"');
return nextUpTo('"""', 'failed to find closing \'"""\'');
} else {
for (; next(); ) {
if ('"' === ch) return next(), string;
if ('\\' === ch)
if ((next(), 'u' === ch)) {
for (
uffff = 0, i = 0;
4 > i && ((hex = parseInt(next(), 16)), isFinite(hex));
i += 1
)
uffff = 16 * uffff + hex;
string += String.fromCharCode(uffff);
} else {
if ('string' != typeof escapee[ch]) break;
string += escapee[ch];
}
else string += ch;
}
for (; next(); ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this part, only the if statement has been removed.

if (peek('""')) {
.....
}

if ('"' === ch) return next(), string;
if ('\\' === ch)
if ((next(), 'u' === ch)) {
for (
uffff = 0, i = 0;
4 > i && ((hex = parseInt(next(), 16)), isFinite(hex));
i += 1
)
uffff = 16 * uffff + hex;
string += String.fromCharCode(uffff);
} else {
if ('string' != typeof escapee[ch]) break;
string += escapee[ch];
}
else string += ch;
}
}

error('Bad string');
},
white = function () {
Expand Down Expand Up @@ -165,9 +157,9 @@ export const createParser = () => {
((key = string()),
white(),
next(':'),
Object.hasOwnProperty.call(object, key) &&
Object.hasOwnProperty.call(object, key!) &&
warning('Duplicate key "' + key + '"', latchKeyStart),
(object[key] = value()),
(object[key!] = value()),
white(),
'}' === ch)
)
Expand All @@ -179,6 +171,10 @@ export const createParser = () => {
};
return (
(value = function () {
const tripleQuotes = '"""';
if (peek(tripleQuotes)) {
return stringLiteral(tripleQuotes);
}
switch ((white(), ch)) {
case '{':
return object();
Expand Down
32 changes: 28 additions & 4 deletions src/plugins/data/common/search/aggs/param_types/json.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,34 @@ describe('JSON', function () {
aggParam.write(aggConfig, output);
expect(aggConfig.params).toHaveProperty(paramName);

expect(output.params).toEqual({
existing: 'true',
new_param: 'should exist in output',
});
expect(output.params).toMatchInlineSnapshot(`
Object {
"existing": "true",
"new_param": "should exist in output",
}
`);
});

it('should append param when valid JSON with triple quotes', () => {
const aggParam = initAggParam();
const jsonData = `{
"a": """
multiline string - line 1
"""
}`;

aggConfig.params[paramName] = jsonData;

aggParam.write(aggConfig, output);
expect(aggConfig.params).toHaveProperty(paramName);

expect(output.params).toMatchInlineSnapshot(`
Object {
"a": "
multiline string - line 1
",
}
`);
});

it('should not overwrite existing params', () => {
Expand Down
14 changes: 12 additions & 2 deletions src/plugins/data/common/search/aggs/param_types/json.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,17 @@ import _ from 'lodash';
import { IAggConfig } from '../agg_config';
import { BaseParamType } from './base';

function collapseLiteralStrings(xjson: string) {
const tripleQuotes = '"""';
const splitData = xjson.split(tripleQuotes);

for (let idx = 1; idx < splitData.length - 1; idx += 2) {
splitData[idx] = JSON.stringify(splitData[idx]);
}

return splitData.join('');
}
Comment on lines +14 to +23
Copy link
Contributor

@jloleysens jloleysens Sep 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this was copy-pasted from src/plugins/es_ui_shared/__packages_do_not_import__/xjson/json_xjson_translation_tools/index.ts?

I wonder whether it is worth moving that function to common in es-ui-shared and import from there (then also re-exporting it from where it was originally exported). Not sure you have to do that in this PR, but would be nice to not have too many of these around :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jloleysens not sure that it's a working idea due to two thoughts:

  • es-ui-shared plugin is responsible only for ui code and not sure that it's semantically right to create a common folder here.
  • es-ui-shared already depends on data plugin and this will cause cross-dependency import problem

If you want to reuse that code probably it should be moved into kibanaUtils. But in this case probably all XJSON related code should be there.

I thing now you see why that code was duplicated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

es-ui-shared already depends on data plugin and this will cause cross-dependency import problem

Ah I see, that does make sense.

@alisonelizabeth Looking closer at the dependence on data only the const value indexPatterns is being imported and only the value ILLEGAL_CHARACTERS_VISIBLE is being used. This is actually from the data_views plugin now. We should be able to remove the dependence on data from es_ui_shared and replace with dataViews. That way we can import collapseLiteralStrings from es_ui_shared/common in data to only have one function defined. Your call though 👍🏻


export class JsonParamType extends BaseParamType {
constructor(config: Record<string, any>) {
super(config);
Expand All @@ -26,9 +37,8 @@ export class JsonParamType extends BaseParamType {
return;
}

// handle invalid Json input
try {
paramJson = JSON.parse(param);
paramJson = JSON.parse(collapseLiteralStrings(param));
} catch (err) {
return;
}
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/vis_default_editor/kibana.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"version": "kibana",
"ui": true,
"optionalPlugins": ["visualize"],
"requiredBundles": ["kibanaUtils", "kibanaReact", "data", "fieldFormats", "discover"],
"requiredBundles": ["kibanaUtils", "kibanaReact", "data", "fieldFormats", "discover", "esUiShared"],
"owner": {
"name": "Vis Editors",
"githubTeam": "kibana-vis-editors"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { EuiFormRow, EuiIconTip, EuiScreenReaderOnly } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { XJsonLang } from '@kbn/monaco';
import { CodeEditor } from '../../../../kibana_react/public';
import { XJson } from '../../../../es_ui_shared/public';

import { AggParamEditorProps } from '../agg_param_props';

Expand Down Expand Up @@ -58,7 +59,7 @@ function RawJsonParamEditor({
let isJsonValid = true;
try {
if (newValue) {
JSON.parse(newValue);
JSON.parse(XJson.collapseLiteralStrings(newValue));
}
} catch (e) {
isJsonValid = false;
Expand Down