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.

Sign up for GitHub

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
Merged

[CodeEditor] add support of triple quotes #112656

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8ed18cc
[CodeEditor] add support of triple quotes
alexwizp Sep 20, 2021
02b748e
Merge remote-tracking branch 'upstream/main' into add-support-of-trip…
alexwizp Oct 15, 2021
781b2f3
add tests for grammar
alexwizp Oct 15, 2021
9dbbd78
Merge branch 'master' into add-support-of-triple-quotes
kibanamachine Oct 18, 2021
9ae8be5
an escaped quote can be appended to the end of triple quotes'
alexwizp Oct 18, 2021
c2d3300
Merge branch 'master' into add-support-of-triple-quotes
kibanamachine Oct 19, 2021
060f17b
Merge branch 'master' into add-support-of-triple-quotes
kibanamachine Oct 19, 2021
2a188f2
Merge branch 'master' into add-support-of-triple-quotes
kibanamachine Oct 20, 2021
614e277
Merge branch 'master' into add-support-of-triple-quotes
kibanamachine Oct 21, 2021
1c23d07
Merge branch 'master' into add-support-of-triple-quotes
kibanamachine Oct 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 34 additions & 38 deletions packages/kbn-monaco/src/xjson/grammar.ts
View file
Open in desktop
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 •
edited
Loading

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:

kibana/packages/kbn-ace/src/ace/modes/x_json/worker/x_json.ace.worker.js

Line 797 in 854e0d4

}), 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
View file
Open in desktop
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
View file
Open in desktop
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 •
edited
Loading

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
View file
Open in desktop
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"],
"requiredBundles": ["kibanaUtils", "kibanaReact", "data", "fieldFormats", "esUiShared"],
"owner": {
"name": "Vis Editors",
"githubTeam": "kibana-vis-editors"
Expand Down
3 changes: 2 additions & 1 deletion src/plugins/vis_default_editor/public/components/controls/raw_json.tsx
View file
Open in desktop
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