[Expressions] Fix setup and start contracts (#110841) (#112923)

* Refactor executor forking to implement state inheritance * Fix setup and start contracts typings * Add support of named forks * Add expressions service life-cycle assertions Co-authored-by: Kibana Machine <[email protected]>
elastic · Sep 23, 2021 · 7ce80d5 · 7ce80d5
1 parent 59d2271
commit 7ce80d5
Show file tree

Hide file tree

Showing 14 changed files with 320 additions and 231 deletions.
diff --git a/src/plugins/expressions/common/execution/execution.ts b/src/plugins/expressions/common/execution/execution.ts
@@ -180,7 +180,7 @@ export class Execution<
     const ast = execution.ast || parseExpression(this.expression);
 
     this.state = createExecutionContainer({
-      ...executor.state.get(),
+      ...executor.state,
       state: 'not-started',
       ast,
     });

diff --git a/src/plugins/expressions/common/executor/executor.ts b/src/plugins/expressions/common/executor/executor.ts
@@ -49,7 +49,7 @@ export class TypesRegistry implements IRegistry<ExpressionType> {
   }
 
   public get(id: string): ExpressionType | null {
-    return this.executor.state.selectors.getType(id);
+    return this.executor.getType(id) ?? null;
   }
 
   public toJS(): Record<string, ExpressionType> {
@@ -71,7 +71,7 @@ export class FunctionsRegistry implements IRegistry<ExpressionFunction> {
   }
 
   public get(id: string): ExpressionFunction | null {
-    return this.executor.state.selectors.getFunction(id);
+    return this.executor.getFunction(id) ?? null;
   }
 
   public toJS(): Record<string, ExpressionFunction> {
@@ -95,22 +95,44 @@ export class Executor<Context extends Record<string, unknown> = Record<string, u
     return executor;
   }
 
-  public readonly state: ExecutorContainer<Context>;
+  public readonly container: ExecutorContainer<Context>;
 
   /**
    * @deprecated
    */
-  public readonly functions: FunctionsRegistry;
+  public readonly functions = new FunctionsRegistry(this);
 
   /**
    * @deprecated
    */
-  public readonly types: TypesRegistry;
+  public readonly types = new TypesRegistry(this);
+
+  protected parent?: Executor<Context>;
 
   constructor(state?: ExecutorState<Context>) {
-    this.state = createExecutorContainer<Context>(state);
-    this.functions = new FunctionsRegistry(this);
-    this.types = new TypesRegistry(this);
+    this.container = createExecutorContainer<Context>(state);
+  }
+
+  public get state(): ExecutorState<Context> {
+    const parent = this.parent?.state;
+    const state = this.container.get();
+
+    return {
+      ...(parent ?? {}),
+      ...state,
+      types: {
+        ...(parent?.types ?? {}),
+        ...state.types,
+      },
+      functions: {
+        ...(parent?.functions ?? {}),
+        ...state.functions,
+      },
+      context: {
+        ...(parent?.context ?? {}),
+        ...state.context,
+      },
+    };
   }
 
   public registerFunction(
@@ -119,15 +141,18 @@ export class Executor<Context extends Record<string, unknown> = Record<string, u
     const fn = new ExpressionFunction(
       typeof functionDefinition === 'object' ? functionDefinition : functionDefinition()
     );
-    this.state.transitions.addFunction(fn);
+    this.container.transitions.addFunction(fn);
   }
 
   public getFunction(name: string): ExpressionFunction | undefined {
-    return this.state.get().functions[name];
+    return this.container.get().functions[name] ?? this.parent?.getFunction(name);
   }
 
   public getFunctions(): Record<string, ExpressionFunction> {
-    return { ...this.state.get().functions };
+    return {
+      ...(this.parent?.getFunctions() ?? {}),
+      ...this.container.get().functions,
+    };
   }
 
   public registerType(
@@ -136,23 +161,30 @@ export class Executor<Context extends Record<string, unknown> = Record<string, u
     const type = new ExpressionType(
       typeof typeDefinition === 'object' ? typeDefinition : typeDefinition()
     );
-    this.state.transitions.addType(type);
+
+    this.container.transitions.addType(type);
   }
 
   public getType(name: string): ExpressionType | undefined {
-    return this.state.get().types[name];
+    return this.container.get().types[name] ?? this.parent?.getType(name);
   }
 
   public getTypes(): Record<string, ExpressionType> {
-    return { ...this.state.get().types };
+    return {
+      ...(this.parent?.getTypes() ?? {}),
+      ...this.container.get().types,
+    };
   }
 
   public extendContext(extraContext: Record<string, unknown>) {
-    this.state.transitions.extendContext(extraContext);
+    this.container.transitions.extendContext(extraContext);
   }
 
   public get context(): Record<string, unknown> {
-    return this.state.selectors.getContext();
+    return {
+      ...(this.parent?.context ?? {}),
+      ...this.container.selectors.getContext(),
+    };
   }
 
   /**
@@ -199,18 +231,15 @@ export class Executor<Context extends Record<string, unknown> = Record<string, u
   ) {
     for (const link of ast.chain) {
       const { function: fnName, arguments: fnArgs } = link;
-      const fn = getByAlias(this.state.get().functions, fnName);
+      const fn = getByAlias(this.getFunctions(), fnName);
 
       if (fn) {
         // if any of arguments are expressions we should migrate those first
-        link.arguments = mapValues(fnArgs, (asts, argName) => {
-          return asts.map((arg) => {
-            if (arg && typeof arg === 'object') {
-              return this.walkAst(arg, action);
-            }
-            return arg;
-          });
-        });
+        link.arguments = mapValues(fnArgs, (asts) =>
+          asts.map((arg) =>
+            arg != null && typeof arg === 'object' ? this.walkAst(arg, action) : arg
+          )
+        );
 
         action(fn, link);
       }
@@ -275,39 +304,19 @@ export class Executor<Context extends Record<string, unknown> = Record<string, u
 
   private migrate(ast: SerializableRecord, version: string) {
     return this.walkAst(cloneDeep(ast) as ExpressionAstExpression, (fn, link) => {
-      if (!fn.migrations[version]) return link;
-      const updatedAst = fn.migrations[version](link) as ExpressionAstFunction;
-      link.arguments = updatedAst.arguments;
-      link.type = updatedAst.type;
+      if (!fn.migrations[version]) {
+        return;
+      }
+
+      ({ arguments: link.arguments, type: link.type } = fn.migrations[version](
+        link
+      ) as ExpressionAstFunction);
     });
   }
 
   public fork(): Executor<Context> {
-    const initialState = this.state.get();
-    const fork = new Executor<Context>(initialState);
-
-    /**
-     * Synchronize registry state - make any new types, functions and context
-     * also available in the forked instance of `Executor`.
-     */
-    this.state.state$.subscribe(({ types, functions, context }) => {
-      const state = fork.state.get();
-      fork.state.set({
-        ...state,
-        types: {
-          ...types,
-          ...state.types,
-        },
-        functions: {
-          ...functions,
-          ...state.functions,
-        },
-        context: {
-          ...context,
-          ...state.context,
-        },
-      });
-    });
+    const fork = new Executor<Context>();
+    fork.parent = this;
 
     return fork;
   }

diff --git a/src/plugins/expressions/common/service/expressions_services.test.ts b/src/plugins/expressions/common/service/expressions_services.test.ts
@@ -17,11 +17,16 @@ describe('ExpressionsService', () => {
     const expressions = new ExpressionsService();
 
     expect(expressions.setup()).toMatchObject({
+      getFunction: expect.any(Function),
       getFunctions: expect.any(Function),
+      getRenderer: expect.any(Function),
+      getRenderers: expect.any(Function),
+      getType: expect.any(Function),
+      getTypes: expect.any(Function),
       registerFunction: expect.any(Function),
       registerType: expect.any(Function),
       registerRenderer: expect.any(Function),
-      run: expect.any(Function),
+      fork: expect.any(Function),
     });
   });
 
@@ -30,7 +35,16 @@ describe('ExpressionsService', () => {
     expressions.setup();
 
     expect(expressions.start()).toMatchObject({
+      getFunction: expect.any(Function),
       getFunctions: expect.any(Function),
+      getRenderer: expect.any(Function),
+      getRenderers: expect.any(Function),
+      getType: expect.any(Function),
+      getTypes: expect.any(Function),
+      registerFunction: expect.any(Function),
+      registerType: expect.any(Function),
+      registerRenderer: expect.any(Function),
+      execute: expect.any(Function),
       run: expect.any(Function),
     });
   });
@@ -54,21 +68,21 @@ describe('ExpressionsService', () => {
       const service = new ExpressionsService();
       const fork = service.fork();
 
-      expect(fork.executor.state.get().types).toEqual(service.executor.state.get().types);
+      expect(fork.getTypes()).toEqual(service.getTypes());
     });
 
     test('fork keeps all functions of the origin service', () => {
       const service = new ExpressionsService();
       const fork = service.fork();
 
-      expect(fork.executor.state.get().functions).toEqual(service.executor.state.get().functions);
+      expect(fork.getFunctions()).toEqual(service.getFunctions());
     });
 
     test('fork keeps context of the origin service', () => {
       const service = new ExpressionsService();
       const fork = service.fork();
 
-      expect(fork.executor.state.get().context).toEqual(service.executor.state.get().context);
+      expect(fork.executor.state.context).toEqual(service.executor.state.context);
     });
 
     test('newly registered functions in origin are also available in fork', () => {
@@ -82,7 +96,7 @@ describe('ExpressionsService', () => {
         fn: () => {},
       });
 
-      expect(fork.executor.state.get().functions).toEqual(service.executor.state.get().functions);
+      expect(fork.getFunctions()).toEqual(service.getFunctions());
     });
 
     test('newly registered functions in fork are NOT available in origin', () => {
@@ -96,14 +110,15 @@ describe('ExpressionsService', () => {
         fn: () => {},
       });
 
-      expect(Object.values(fork.executor.state.get().functions)).toHaveLength(
-        Object.values(service.executor.state.get().functions).length + 1
+      expect(Object.values(fork.getFunctions())).toHaveLength(
+        Object.values(service.getFunctions()).length + 1
       );
     });
 
     test('fork can execute an expression with newly registered function', async () => {
       const service = new ExpressionsService();
       const fork = service.fork();
+      fork.start();
 
       service.registerFunction({
         name: '__test__',
@@ -118,5 +133,28 @@ describe('ExpressionsService', () => {
 
       expect(result).toBe('123');
     });
+
+    test('throw on fork if the service is already started', async () => {
+      const service = new ExpressionsService();
+      service.start();
+
+      expect(() => service.fork()).toThrow();
+    });
+  });
+
+  describe('.execute()', () => {
+    test('throw if the service is not started', () => {
+      const expressions = new ExpressionsService();
+
+      expect(() => expressions.execute('foo', null)).toThrow();
+    });
+  });
+
+  describe('.run()', () => {
+    test('throw if the service is not started', () => {
+      const expressions = new ExpressionsService();
+
+      expect(() => expressions.run('foo', null)).toThrow();
+    });
   });
 });