[CodeEditor] add support of triple quotes #112656
Conversation
alexwizp
added
Feature:Visualizations
|
Pinging @elastic/kibana-vis-editors (Team:VisEditors) |
There was a problem hiding this comment.
i have my doubts about this:
- is this something a lot of customers are asking for ? seems very specific
- why would we support tripple quotes in
visualize aggregation advanced configbut not in other places json can be entered ? seems we should either support this everywhere or not support it at all.
|
@ppisljar See the linked issue for the reason behind switching - actually I think it would be nice for all raw JSON inputs in Kibana to support xjson as well, we can work on this incrementally (even though I have a hard time thinking of other places where JSON can be entered directly). The only place I can think of is dev tools which is already supporting xjson, so it's more about aligning with this (making it easier to copy over something from dev tools which is a common thing to do): |
There was a problem hiding this comment.
Hi @alexwizp! Thanks for working on this.
I know the existing code is probably not the best example, but I think it would be helpful to add some code comments in the grammar file to help explain the implementation as well as tests. If I'm not mistaken, triple quotes was already supported via #67485, but this is more specifically implementing multiline support. Is that correct?
The XJSON lang is currently being used in Ingest Node Pipelines UI so I'd like to make sure there are no regressions.
|
@alisonelizabeth I see that we have no tests for the |
| } | ||
| else string += ch; | ||
| } | ||
| for (; next(); ) { |
There was a problem hiding this comment.
in this part, only the if statement has been removed.
if (peek('""')) {
.....
}
There was a problem hiding this comment.
This is looking cool @alexwizp 🍻 ! I think it makes sense to support XJSON everywhere 👍
looks like only syntax highlighting was added here
That PR did add grammar parsing too (see comments for more details).
I'd like to get your (and perhaps @alisonelizabeth's) thoughts on my comments. Let me know what you think!
| 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(''); | ||
| } |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
@jloleysens not sure that it's a working idea due to two thoughts:
es-ui-sharedplugin is responsible only for ui code and not sure that it's semantically right to create a common folder here.es-ui-sharedalready depends ondataplugin 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
There was a problem hiding this comment.
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 👍🏻
| @@ -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) { | |||
There was a problem hiding this comment.
The original functionality was airlifted out of:
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…le-quotes # Conflicts: # src/plugins/vis_default_editor/kibana.json
|
@elasticmachine merge upstream |
There was a problem hiding this comment.
Thanks for adding the tests @alexwizp and explaining the reasoning.
I still think it would be best to cherry-pick the parser-related changes onto a separate PR since they are not related to this PR's goal.
That being said, I won't block moving ahead but I think the PR description should explain that the parser was slightly refactored and we expect the same behaviour and added some tests.
Left a suggestion for one more test, thanks for adding those 🍻
| expect( | ||
| parser(` | ||
| {"menu": { | ||
| "id": """ |
There was a problem hiding this comment.
I wonder what would happen if we added \" at the end of a triple quote value, so \""""
There was a problem hiding this comment.
thank you, that case was broken 🥇
|
@elasticmachine merge upstream |
There was a problem hiding this comment.
This is looking in a good state, @alexwizp. Happy to merge pending green CI.
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elastic/kibana-stack-management please review |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/ml/jobs/categorization_field_examples·ts.apis Machine Learning jobs Categorization example endpoint - invalid, too many tokens.Standard OutStack TraceMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @alexwizp |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @alexwizp |
There was a problem hiding this comment.
Thanks for working on this @alexwizp! Code changes LGTM, tested locally also and triple quotes work as expected 🚀
* [CodeEditor] add support of triple quotes * add tests for grammar * an escaped quote can be appended to the end of triple quotes' Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* [CodeEditor] add support of triple quotes * add tests for grammar * an escaped quote can be appended to the end of triple quotes' Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* [CodeEditor] add support of triple quotes * add tests for grammar * an escaped quote can be appended to the end of triple quotes' Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Closes: #107723
Summary:
[Vis editor] Triple quote for multiline script support in JSON input
Testing notes:
AreaAverageaggregation for Y-AxisAdvancedpanel and set the followingJSONExpected result:JSONshould be valid, script should be passed intoESrequestScreens: