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

[skip-ci] POC: Using expression types to "migrate" state passed in args. #57354

Closed
wants to merge 1 commit into from

Conversation

lukeelmers
Copy link
Member

POC: Do not merge!

This POC demonstrates one possible way that pieces of state provided as
args to an expression function could be automatically "migrated" from a
legacy shape without the author of the function needing to know about
it.

The sample combineFilters is useless but serves as a simple example:
it takes an arbitrary number of stringified filter objects, and merges them
together, returning a single filter object.

The argument type is set to filter, which still works because it leverages
the interpreter's built-in casting capabilities, which allows us to cast
from a string to a parsed Filter object.

While we are doing that, we can also examine the shape of the provided
object and migrate it if needed.

You can test this PR by running Kibana, and in Canvas put in a debug
element, then modify the expression as follows:

combineFilters
  "{\"version\": 2, \"value\": \"foo\"}"
  "{\"version\": 3, \"query\": \"bar\"}"
| render as=debug

The filter labeled "version 2" will be automatically migrated to
"version 3" before it is passed to the combineFilters function body.

For illustrative purposes, I've included the original filter args inside
the and: [] array in the returned filter, so you can see how they looked as
they were passed into the function:

Screenshot 2020-02-11 11 19 41

This POC demonstrates one possible way that pieces of state provided as
args to an expression function could be automatically "migrated" from a
legacy shape without the author of the function needing to know about
it.

The sample `combineFilters` is useless but serves as a simple example:
it takes an arbitrary number of stringified filter objects, and merges them
together, returning a single filter object.

The argument type is set to `filter`, which still works because it leverages
the interpreter's built-in casting capabilities, which allows us to cast
from a string to a parsed Filter object.

While we are doing that, we can also examine the shape of the provided
object and migrate it if needed.

You can test this PR by running Kibana, and in Canvas put in a `debug`
element, then modify the expression as follows:

```
combineFilters
  "{\"version\": 2, \"value\": \"foo\"}"
  "{\"version\": 3, \"query\": \"bar\"}"
| render as=debug
```
The filter labeled "version 2" will be automatically migrated to
"version 3" before it is passed to the `combineFilters` function body.

For illustrative purposes, I've included the original filter args inside
the `and: []` array in the filter, so you can see how they looked as
they were passed into the function.
help: 'takes two filters and merges them',
args: {
filter: {
types: ['filter'],
Copy link
Member Author

Choose a reason for hiding this comment

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

The key here is that we still specify types: ['filter'] (not string).

The filter type in the types registry will handle parsing & migrating it automatically for us.

If for some reason this function needed a legacy filter shape, either multiple types could be registered as a means of tracking version (types: ['filter_v1']), or the author could just migrate it backwards in the function body itself (maybe the "down" migration function would also be provided by the owner of Filters). Still not sure what the best solution is for this case.

Comment on lines +38 to +42
const migrator = {
isUnsupported: (p: any) => !p.version || p.version < 2,
isLegacy: (p: any) => p.version && p.version < 3,
migrate: (legacyFilter: any) => ({ ...legacyFilter, version: 3 }),
};
Copy link
Member Author

Choose a reason for hiding this comment

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

This is some throwaway code to run a fake migration which simply increments the version number. If version 1 is provided, we consider this to be no longer supported and throw an error. If version 2 is provided, we provide the ability to migrate it up to the latest (version 3).

How these migrations are structured and standardized is a separate topic; this is only an example.

Comment on lines +62 to +66
string: s => {
const { isLegacy, migrate } = migrator;
const parsed = parseAndValidate(s);
return isLegacy(parsed) ? migrate(parsed) : parsed;
},
Copy link
Member Author

Choose a reason for hiding this comment

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

The whole point here is that the "owner" of filters can set up the casting/migrations without the author of the combineFilters function needing to know anything: they just receive the correct, up-to-date filter shape as an object inside their function body as long as they specify their arg as a filter type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also worth pointing out that @streamich has floated the idea of making type converters pluggable by third parties in some future version of the expressions service:

registerTypeConverter('string', 'filter', (string) => { /*...*/ })

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 this could be built on top of the general migration system, but my point was, who is writing migrate: (legacyFilter: any) => ({ ...legacyFilter, version: 3 }), function and what is the implementation?

I think it would work if it used the generic persistableStateFactory we talked about. Code could look something like

 from: {
    string: s => {
    try {
      const { type, ...state } = JSON.parse(s);
      const persistableStateFactory = persistableStateFactories.get(type);

      if (!persistableStateFactory) {
        throw new Error('NOT SUPPORTED');
      }

      if (persistableStateFactory.isDeprecated()) {
        console.warn('LEGACY - upgrade soon to avoid breakage!');
      }
      
      let migratedState = state;
      while (persistableStateFactory.isDeprecated()) {
        const { type: newType, state: migratedState } = persistableStateFactory.migrate(state);
        persistableStateFactory = persistableStateFactories.get(newType);
      }
       return migratedState;
    } catch (e) {
      throw new Error('Unable to parse filter string.');
    }
  }  
}

I was thinking persistableStateFactory.create(state) would return an instance of whatever the factory was for, but since canvas doesn't deal with instances, in this case you'd probably just return the migrated state.

@lukeelmers lukeelmers self-assigned this Feb 11, 2020
@lukeelmers lukeelmers added Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:AppArch labels Feb 11, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@lukeelmers
Copy link
Member Author

One other caveat I will add is that generally, I don't think we should be leaning on stringified objects being passed to functions. While we can handle these magically with casting, I think in most cases it is clearer to either split the necessary items out into separate args, or to use a subexpression to generate the desired state where possible.

For example, something like this would be more in line with the direction we've been moving with expressions thus far:

combineFilters
  {makeFilter version=2 value="foo"}
  {makeFilter version=3 query="bar"}
| render as=debug

Where makeFilter is a subexpression that handles assembling the state object into the correct shape, and presumably migrating as well. This is similar to the strategy we've taken with the visDimension function.

@lukeelmers lukeelmers changed the title [skip-ci] POC: Add combineFilters with typecast args. [skip-ci] POC: Using expression types to "migrate" state passed in args. Feb 11, 2020
@lukeelmers
Copy link
Member Author

Closing as we are working out our approach for migrations & reference injection/extraction in #72438

@lukeelmers lukeelmers closed this Aug 6, 2020
@lukeelmers lukeelmers deleted the poc/expression-types branch February 11, 2021 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants