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

[expressions] changes fork to use namespacing #125957

Merged
merged 13 commits into from
Mar 15, 2022
17 changes: 10 additions & 7 deletions examples/partial_results_example/public/plugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
*/
import React from 'react';
import ReactDOM from 'react-dom';
import type { ExpressionsService, ExpressionsServiceSetup } from 'src/plugins/expressions';
import type { ExpressionsServiceSetup } from 'src/plugins/expressions';
import { ExpressionsServiceFork } from 'src/plugins/expressions/common/service/expressions_fork';
import { AppMountParameters, AppNavLinkStatus, CoreSetup, Plugin } from '../../../src/core/public';
import type { DeveloperExamplesSetup } from '../../developer_examples/public';
import { App, ExpressionsContext } from './app';
Expand All @@ -19,21 +20,23 @@ interface SetupDeps {
}

export class PartialResultsExamplePlugin implements Plugin<void, void, SetupDeps> {
private expressions?: ExpressionsService;
private expressions?: ExpressionsServiceFork;

setup({ application }: CoreSetup, { expressions, developerExamples }: SetupDeps) {
this.expressions = expressions.fork();
this.expressions.registerFunction(countEvent);
this.expressions.registerFunction(getEvents);
this.expressions.registerFunction(pluck);
this.expressions = expressions.fork('test');
const expressionsSetup = this.expressions.setup();
expressionsSetup.registerFunction(countEvent);
expressionsSetup.registerFunction(getEvents);
expressionsSetup.registerFunction(pluck);
const expressionsStart = this.expressions.start();

application.register({
id: 'partialResultsExample',
title: 'Partial Results Example',
navLinkStatus: AppNavLinkStatus.hidden,
mount: async ({ element }: AppMountParameters) => {
ReactDOM.render(
<ExpressionsContext.Provider value={this.expressions}>
<ExpressionsContext.Provider value={expressionsStart}>
<App />
</ExpressionsContext.Provider>,
element
Expand Down
6 changes: 5 additions & 1 deletion src/plugins/expressions/common/execution/execution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,11 @@ export class Execution<
...(chainArr.map((link) =>
switchMap((currentInput) => {
const { function: fnName, arguments: fnArgs } = link;
const fn = getByAlias(this.state.get().functions, fnName);
const fn = getByAlias(
this.state.get().functions,
fnName,
this.execution.params.namespace
);

if (!fn) {
throw createError({
Expand Down
26 changes: 15 additions & 11 deletions src/plugins/expressions/common/executor/executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { ExpressionType } from '../expression_types/expression_type';
import { AnyExpressionTypeDefinition } from '../expression_types/types';
import { ExpressionAstExpression, ExpressionAstFunction } from '../ast';
import { ExpressionValueError, typeSpecs } from '../expression_types/specs';
import { getByAlias } from '../util';
import { ALL_NAMESPACES, getByAlias } from '../util';
import { SavedObjectReference } from '../../../../core/types';
import {
MigrateFunctionsObject,
Expand Down Expand Up @@ -146,15 +146,17 @@ export class Executor<Context extends Record<string, unknown> = Record<string, u
this.container.transitions.addFunction(fn);
}

public getFunction(name: string): ExpressionFunction | undefined {
return this.container.get().functions[name] ?? this.parent?.getFunction(name);
public getFunction(name: string, namespace?: string): ExpressionFunction | undefined {
const fn = this.container.get().functions[name];
if (!fn?.namespace || fn.namespace === namespace) return fn;
}

public getFunctions(): Record<string, ExpressionFunction> {
return {
...(this.parent?.getFunctions() ?? {}),
...this.container.get().functions,
};
public getFunctions(namespace?: string): Record<string, ExpressionFunction> {
const fns = Object.entries(this.container.get().functions);
const filtered = fns.filter(
([key, value]) => !value.namespace || value.namespace === namespace
);
return Object.fromEntries(filtered);
}

public registerType(
Expand Down Expand Up @@ -222,9 +224,10 @@ export class Executor<Context extends Record<string, unknown> = Record<string, u
ast: ExpressionAstExpression,
action: (fn: ExpressionFunction, link: ExpressionAstFunction) => void
) {
const functions = this.container.get().functions;
for (const link of ast.chain) {
const { function: fnName, arguments: fnArgs } = link;
const fn = getByAlias(this.getFunctions(), fnName);
const fn = getByAlias(functions, fnName, ALL_NAMESPACES);

if (fn) {
// if any of arguments are expressions we should migrate those first
Expand All @@ -249,12 +252,13 @@ export class Executor<Context extends Record<string, unknown> = Record<string, u
) => ExpressionAstFunction | ExpressionAstExpression
): ExpressionAstExpression {
let additionalFunctions = 0;
const functions = this.container.get().functions;
return (
ast.chain.reduce<ExpressionAstExpression>(
(newAst: ExpressionAstExpression, funcAst: ExpressionAstFunction, index: number) => {
const realIndex = index + additionalFunctions;
const { function: fnName, arguments: fnArgs } = funcAst;
const fn = getByAlias(this.getFunctions(), fnName);
const fn = getByAlias(functions, fnName, ALL_NAMESPACES);
if (!fn) {
return newAst;
}
Expand Down Expand Up @@ -331,7 +335,7 @@ export class Executor<Context extends Record<string, unknown> = Record<string, u

public getAllMigrations() {
const uniqueVersions = new Set(
Object.values(this.getFunctions())
Object.values(this.container.get().functions)
.map((fn) => Object.keys(fn.migrations))
.flat(1)
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ export class ExpressionFunction implements PersistableState<ExpressionAstFunctio
*/
name: string;

namespace?: string;

/**
* Aliases that can be used instead of `name`.
*/
Expand Down Expand Up @@ -90,9 +92,11 @@ export class ExpressionFunction implements PersistableState<ExpressionAstFunctio
inject,
extract,
migrations,
namespace,
} = functionDefinition;

this.name = name;
this.namespace = namespace;
ppisljar marked this conversation as resolved.
Show resolved Hide resolved
this.type = type;
this.aliases = aliases || [];
this.fn = fn as ExpressionFunction['fn'];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ export interface ExpressionFunctionDefinition<
*/
disabled?: boolean;

namespace?: string;

/**
* Name of type of value this function outputs.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,18 @@ import { ExpressionRenderDefinition } from './types';

export class ExpressionRenderer<Config = unknown> {
public readonly name: string;
public readonly namespace?: string;
public readonly displayName: string;
public readonly help: string;
public readonly validate: () => void | Error;
public readonly reuseDomNode: boolean;
public readonly render: ExpressionRenderDefinition<Config>['render'];

constructor(config: ExpressionRenderDefinition<Config>) {
const { name, displayName, help, validate, reuseDomNode, render } = config;
const { name, displayName, help, validate, reuseDomNode, render, namespace } = config;

this.name = name;
this.namespace = namespace;
this.displayName = displayName || name;
this.help = help || '';
this.validate = validate || (() => {});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export interface ExpressionRenderDefinition<Config = unknown> {
* function that is used to create the `type: render` object.
*/
name: string;
namespace?: string;

/**
* A user friendly name of the renderer as will be displayed to user in UI.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { getType } from './get_type';

export class ExpressionType {
name: string;

namespace?: string;
/**
* A short help text.
*/
Expand All @@ -33,9 +33,10 @@ export class ExpressionType {
private readonly definition: AnyExpressionTypeDefinition;

constructor(definition: AnyExpressionTypeDefinition) {
const { name, help, deserialize, serialize, validate } = definition;
const { name, help, deserialize, serialize, validate, namespace } = definition;

this.name = name;
this.namespace = namespace;
this.help = help || '';
this.validate = validate || (() => {});

Expand Down
1 change: 1 addition & 0 deletions src/plugins/expressions/common/expression_types/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export interface ExpressionTypeDefinition<
SerializedType = undefined
> {
name: Name;
namespace?: string;
validate?(type: unknown): void | Error;
serialize?(type: Value): SerializedType;
deserialize?(type: SerializedType): Value;
Expand Down
140 changes: 140 additions & 0 deletions src/plugins/expressions/common/service/expressions_fork.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import {
ExpressionExecutionParams,
ExpressionsService,
ExpressionsServiceSetup,
ExpressionsServiceStart,
} from '.';
import { ExpressionAstExpression } from '../ast';
import { AnyExpressionFunctionDefinition } from '../expression_functions';
import { AnyExpressionTypeDefinition } from '../expression_types';
import { AnyExpressionRenderDefinition } from '../expression_renderers';

export interface ExpressionServiceFork {
setup(): ExpressionsServiceSetup;
start(): ExpressionsServiceStart;
}

/**
* `ExpressionsService` class is used for multiple purposes:
*
* 1. It implements the same Expressions service that can be used on both:
* (1) server-side and (2) browser-side.
* 2. It implements the same Expressions service that users can fork/clone,
* thus have their own instance of the Expressions plugin.
* 3. `ExpressionsService` defines the public contracts of *setup* and *start*
* Kibana Platform life-cycles for ease-of-use on server-side and browser-side.
* 4. `ExpressionsService` creates a bound version of all exported contract functions.
* 5. Functions are bound the way there are:
*
* ```ts
* registerFunction = (...args: Parameters<Executor['registerFunction']>
* ): ReturnType<Executor['registerFunction']> => this.executor.registerFunction(...args);
* ```
*
* so that JSDoc appears in developers IDE when they use those `plugins.expressions.registerFunction(`.
*/
export class ExpressionsServiceFork implements ExpressionServiceFork {
/**
* @note Workaround since the expressions service is frozen.
*/
constructor(private namespace: string, private expressionsService: ExpressionsService) {
this.registerFunction = this.registerFunction.bind(this);
this.registerRenderer = this.registerRenderer.bind(this);
this.registerType = this.registerType.bind(this);
this.run = this.run.bind(this);
this.execute = this.execute.bind(this);
this.getFunction = this.getFunction.bind(this);
this.getFunctions = this.getFunctions.bind(this);
}

protected registerFunction(
definition: AnyExpressionFunctionDefinition | (() => AnyExpressionFunctionDefinition)
) {
if (typeof definition === 'function') definition = definition();
return this.expressionsService.registerFunction({
...definition,
namespace: this.namespace,
});
}

protected registerRenderer(
definition: AnyExpressionRenderDefinition | (() => AnyExpressionRenderDefinition)
) {
if (typeof definition === 'function') definition = definition();
return this.expressionsService.registerRenderer({
...definition,
namespace: this.namespace,
});
}

protected registerType(
definition: AnyExpressionTypeDefinition | (() => AnyExpressionTypeDefinition)
) {
if (typeof definition === 'function') definition = definition();
return this.expressionsService.registerType({ ...definition, namespace: this.namespace });
}

protected run<Input, Output>(
ast: string | ExpressionAstExpression,
input: Input,
params?: ExpressionExecutionParams
) {
return this.expressionsService.run<Input, Output>(ast, input, {
...params,
namespace: this.namespace,
});
}

protected execute<Input = unknown, Output = unknown>(
ast: string | ExpressionAstExpression,
input: Input,
params?: ExpressionExecutionParams
) {
return this.expressionsService.execute<Input, Output>(ast, input, {
...params,
namespace: this.namespace,
});
}

protected getFunction(name: string) {
return this.expressionsService.getFunction(name, this.namespace);
}

protected getFunctions() {
return this.expressionsService.getFunctions(this.namespace);
}
/**
* Returns Kibana Platform *setup* life-cycle contract. Useful to return the
* same contract on server-side and browser-side.
*/
public setup(): ExpressionsServiceSetup {
Copy link
Contributor

@dokmic dokmic Mar 14, 2022

Choose a reason for hiding this comment

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

Using a decorator pattern here is a great idea. What do you think about breaking down the methods to make the class more straightforward?

I see the following points for this:

  • If we make methods protected, we will be able to extend this class in the future if we have to diverge server or public implementation.
  • Placing logic in separate methods should simplify readability and testability since we can test every method independently.
  • In general, it's a good practice to make methods as straightforward as possible and have only one single responsibility. The setup and start are factory methods, so it's better to keep them only producing new instances.
export class ExpressionsServiceFork implements ExpressionServiceFork {
  constructor(private namespace: string, private expressions: ExpressionsService) {
    this.registerFunction = this.registerFunction.bind(this);
  }

  setup(): ExpressionsServiceSetup {
    return {
      ...this.expressionsService,
      registerFunction: this.registerFunction,
      // ...
    };
  }

  protected registerFunction(definition: /* ... */) {
    if (typeof definition === 'function') {
      definition = definition();
    }

    return this.expressionsService.registerFunction({
      ...definition,
      namespace: this.namespace,
    });
  }
}

Some notes regarding the example above:

  • We should define protected methods as methods and not as properties so they will end up in the prototype. In this case, we will be able to use super in the extended class.
  • We should bind methods in the constructor to prevent producing new boundaries on every start/setup call.

Copy link
Member Author

Choose a reason for hiding this comment

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

  • I don't think this will be extended internally and it shouldn't be extended externally.
  • we don't want to make the methods visible on the ExpressionServiceFork class, if we make them protected how can we test this from the outside then ?
  • for me personally in this specific case readability is better as it is now, makes it obvious that the fork is just a wrapper around ExpressionsService and we don't need to do the binding

Copy link
Contributor

@dokmic dokmic Mar 15, 2022

Choose a reason for hiding this comment

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

I don't think this will be extended internally and it shouldn't be extended externally.

Well, we cannot predict that. That's not about the extension necessity but more about abstraction. Those methods seem to be like internal API and not an implementation detail. Even though we export only start and setup, they define the forked service.

we don't want to make the methods visible on the ExpressionServiceFork class, if we make them protected how can we test this from the outside then ?

That's possible to call them through an extended anonymous class. But in cases like that, we can just put related tests under the describe('method') block since it is not a private method.

for me personally in this specific case readability is better as it is now, makes it obvious that the fork is just a wrapper around ExpressionsService and we don't need to do the binding

I think it's better if we can extract those into methods. It should not make it less straightforward, but it will be more consistent with the ExpressionsService. The latter makes sense because we are actually forking that service.
If we do that, it will make the decorator pattern more visible so that it still should be clear that the class is a wrapper.

return {
...this.expressionsService,
registerFunction: this.registerFunction,
registerRenderer: this.registerRenderer,
registerType: this.registerType,
};
}

/**
* Returns Kibana Platform *start* life-cycle contract. Useful to return the
* same contract on server-side and browser-side.
*/
public start(): ExpressionsServiceStart {
return {
...this.expressionsService,
run: this.run,
execute: this.execute,
getFunction: this.getFunction,
getFunctions: this.getFunctions,
};
}
}
Loading