Skip to content

Conversation

@lukeelmers
Copy link
Member

[skip-ci]

The recent changes in #33773 got me thinking that until each saved vis object is stored as a true expression, our build_pipeline utility for converting a stored vis into an expression string is going to become increasingly complex and error prone, especially as we continue refactoring each of the functions.

I thought that perhaps we could reduce our likelihood for errors and eliminate some duplication by having a chainable helper class that could be used to help build the expression string. This a quick POC I put together; if it feels like we want to move in this direction, I will update this PR with tests & convert everything over.

const exp = new ExpressionBuilder();

// add a new function to the expression (functions are stored in the order they are added)
exp.addFn('kibana');

// add a new function with some arguments
exp.addFn('tagcloud')
  .addArg('visConfig', { foo: 'bar' }); // will handle stringifying & escaping objects

exp.addFn('render')
  .addArg('baz', '"hello"'); // "hello" gets escaped

// whoops, forgot to add dimensions to the tagcloud function earlier
exp.selectFn('tagcloud')
  .addArg('metric', '{ vis_dimension 1 }'); // works when given a subexpression as a string

// print an individual function
exp.printFn('tagcloud');
"tagcloud visConfig='{ foo: \'bar\' }' metric={ vis_dimension 1 }"

// print the whole expression string
exp.print();
"kibana | tagcloud visConfig='{ foo: \'bar\' }' metric={ vis_dimension 1 } | render baz='\"hello\"'"

Basically the class takes care of the following:

  1. Storing provided functions in order (FIFO)
  2. Adding spaces between args & pipes between functions
  3. Escaping strings properly
  4. Stringifying JSON when provided as an arg
  5. Skipping any undefined args - currently we have a ton of if (foo) pipeline += foo type of checks in the code that this would eliminate the need for
  6. removing or updating functions
  7. removing or updating args on a function

Maybe a utility like this would even have applications in other places, like unit tests? idk

cc @ppisljar

@lukeelmers lukeelmers added discuss WIP Work in progress :AppArch labels Apr 1, 2019
@lukeelmers lukeelmers mentioned this pull request Feb 4, 2020
6 tasks
@lukeelmers
Copy link
Member Author

Closing as we agreed long ago that we'd prefer a utility to manipulate the AST directly (rather than manipulating strings, which don't have type safety). This work is being tracked in #56748

@lukeelmers lukeelmers closed this Feb 4, 2020
@lukeelmers lukeelmers mentioned this pull request Apr 24, 2020
6 tasks
@lukeelmers lukeelmers deleted the poc/expression-builder branch February 11, 2021 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

discuss WIP Work in progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant