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

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 27 additions & 0 deletions src/plugins/expressions/common/expression_types/filter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const name = 'filter';
* Represents an object that is a Filter.
*/
export interface Filter {
version?: number;
type?: string;
value?: string;
column?: string;
Expand All @@ -34,9 +35,35 @@ export interface Filter {
query?: string | null;
}

const migrator = {
isUnsupported: (p: any) => !p.version || p.version < 2,
isLegacy: (p: any) => p.version && p.version < 3,
migrate: (legacyFilter: any) => ({ ...legacyFilter, version: 3 }),
};
Comment on lines +38 to +42
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.


function parseAndValidate(s: string): Filter {
try {
const parsed = JSON.parse(s);
if (migrator.isUnsupported(parsed)) {
throw new Error('LEGACY');
}
return parsed;
} catch (e) {
if (e.message === 'LEGACY') {
throw new Error('Provided filter string is malformed or no longer supported');
}
throw new Error('Unable to parse filter string.');
}
}

export const filter = (): ExpressionType<typeof name, Filter> => ({
name,
from: {
string: s => {
const { isLegacy, migrate } = migrator;
const parsed = parseAndValidate(s);
return isLegacy(parsed) ? migrate(parsed) : parsed;
},
Comment on lines +62 to +66
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.

null: () => {
return {
type: name,
Expand Down
51 changes: 51 additions & 0 deletions src/plugins/expressions/public/functions/combine_filters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { Filter } from '../../common/expression_types';
import { ExpressionFunction } from '../../common/types';

interface Arguments {
filter: string;
}

type Output = Filter;

export function combineFilters(): ExpressionFunction<'combineFilters', null, Arguments, Output> {
return {
name: 'combineFilters',
type: 'filter',
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.

aliases: ['_'],
multi: true,
required: true,
help: 'filter object',
},
},
fn: (_context, args) => {
return {
type: 'filter',
...Object.assign({}, ...args.filter),
and: [args.filter], // only here for illustrative purposes
};
},
};
}
2 changes: 2 additions & 0 deletions src/plugins/expressions/public/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
setNotifications,
} from './services';
import { clog as clogFunction } from './functions/clog';
import { combineFilters as combineFiltersFunction } from './functions/combine_filters';
import { font as fontFunction } from './functions/font';
import { kibana as kibanaFunction } from './functions/kibana';
import { kibanaContext as kibanaContextFunction } from './functions/kibana_context';
Expand Down Expand Up @@ -113,6 +114,7 @@ export class ExpressionsPublicPlugin
};

registerFunction(clogFunction);
registerFunction(combineFiltersFunction);
registerFunction(fontFunction);
registerFunction(kibanaFunction);
registerFunction(kibanaContextFunction);
Expand Down