From db0a9cc61ef78b7865e74862895a5b63fe50154e Mon Sep 17 00:00:00 2001 From: Maryia Lapata Date: Mon, 24 Feb 2020 14:03:16 +0300 Subject: [PATCH 1/4] [Bug fix] Update nav link when it belongs to the same plugin (#58008) * Update nav link when it belongs to the same plugin * Move the plugin name check to history listener * Add isUrlBelongsToApp function * Code review comments * Update unit tests Co-authored-by: Elastic Machine --- .../dashboard/np_ready/dashboard_constants.ts | 2 + .../kibana/public/dashboard/plugin.ts | 15 +++- .../url/kbn_url_tracker.test.ts | 79 ++++++++++++++----- .../state_management/url/kbn_url_tracker.ts | 19 ++++- 4 files changed, 89 insertions(+), 26 deletions(-) diff --git a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_constants.ts b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_constants.ts index fe42e07912799..0820ebd371004 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_constants.ts +++ b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_constants.ts @@ -23,6 +23,8 @@ export const DashboardConstants = { CREATE_NEW_DASHBOARD_URL: '/dashboard', ADD_EMBEDDABLE_ID: 'addEmbeddableId', ADD_EMBEDDABLE_TYPE: 'addEmbeddableType', + DASHBOARDS_ID: 'dashboards', + DASHBOARD_ID: 'dashboard', }; export function createDashboardEditUrl(id: string) { diff --git a/src/legacy/core_plugins/kibana/public/dashboard/plugin.ts b/src/legacy/core_plugins/kibana/public/dashboard/plugin.ts index 7d330676e79ed..7d64ee969212f 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/plugin.ts +++ b/src/legacy/core_plugins/kibana/public/dashboard/plugin.ts @@ -83,7 +83,14 @@ export class DashboardPlugin implements Plugin { ); const { appMounted, appUnMounted, stop: stopUrlTracker } = createKbnUrlTracker({ baseUrl: core.http.basePath.prepend('/app/kibana'), - defaultSubUrl: '#/dashboards', + defaultSubUrl: `#${DashboardConstants.LANDING_PAGE_PATH}`, + shouldTrackUrlUpdate: pathname => { + const targetAppName = pathname.split('/')[1]; + return ( + targetAppName === DashboardConstants.DASHBOARDS_ID || + targetAppName === DashboardConstants.DASHBOARD_ID + ); + }, storageKey: 'lastUrl:dashboard', navLinkUpdater$: this.appStateUpdater, toastNotifications: core.notifications.toasts, @@ -150,15 +157,15 @@ export class DashboardPlugin implements Plugin { }; kibanaLegacy.registerLegacyApp({ ...app, - id: 'dashboard', + id: DashboardConstants.DASHBOARD_ID, // only register the updater in once app, otherwise all updates would happen twice updater$: this.appStateUpdater.asObservable(), navLinkId: 'kibana:dashboard', }); - kibanaLegacy.registerLegacyApp({ ...app, id: 'dashboards' }); + kibanaLegacy.registerLegacyApp({ ...app, id: DashboardConstants.DASHBOARDS_ID }); home.featureCatalogue.register({ - id: 'dashboard', + id: DashboardConstants.DASHBOARD_ID, title: i18n.translate('kbn.dashboard.featureCatalogue.dashboardTitle', { defaultMessage: 'Dashboard', }), diff --git a/src/plugins/kibana_utils/public/state_management/url/kbn_url_tracker.test.ts b/src/plugins/kibana_utils/public/state_management/url/kbn_url_tracker.test.ts index 4cf74d991ceb9..701154c06a2ff 100644 --- a/src/plugins/kibana_utils/public/state_management/url/kbn_url_tracker.test.ts +++ b/src/plugins/kibana_utils/public/state_management/url/kbn_url_tracker.test.ts @@ -38,7 +38,7 @@ describe('kbnUrlTracker', () => { let navLinkUpdaterSubject: BehaviorSubject<(app: AppBase) => { activeUrl?: string } | undefined>; let toastService: jest.Mocked; - function createTracker() { + function createTracker(shouldTrackUrlUpdate?: (pathname: string) => boolean) { urlTracker = createKbnUrlTracker({ baseUrl: '/app/test', defaultSubUrl: '#/start', @@ -57,6 +57,7 @@ describe('kbnUrlTracker', () => { ], navLinkUpdater$: navLinkUpdaterSubject, toastNotifications: toastService, + shouldTrackUrlUpdate, }); } @@ -82,44 +83,44 @@ describe('kbnUrlTracker', () => { }); test('set nav link to session storage value if defined', () => { - storage.setItem('storageKey', '#/deep/path'); + storage.setItem('storageKey', '#/start/deep/path'); createTracker(); - expect(getActiveNavLinkUrl()).toEqual('/app/test#/deep/path'); + expect(getActiveNavLinkUrl()).toEqual('/app/test#/start/deep/path'); }); test('set nav link to default if app gets mounted', () => { - storage.setItem('storageKey', '#/deep/path'); + storage.setItem('storageKey', '#/start/deep/path'); createTracker(); urlTracker.appMounted(); expect(getActiveNavLinkUrl()).toEqual('/app/test#/start'); }); test('keep nav link to default if path gets changed while app mounted', () => { - storage.setItem('storageKey', '#/deep/path'); + storage.setItem('storageKey', '#/start/deep/path'); createTracker(); urlTracker.appMounted(); - history.push('/deep/path/2'); + history.push('/start/deep/path/2'); expect(getActiveNavLinkUrl()).toEqual('/app/test#/start'); }); test('change nav link to last visited url within app after unmount', () => { createTracker(); urlTracker.appMounted(); - history.push('/deep/path/2'); - history.push('/deep/path/3'); + history.push('/start/deep/path/2'); + history.push('/start/deep/path/3'); urlTracker.appUnMounted(); - expect(getActiveNavLinkUrl()).toEqual('/app/test#/deep/path/3'); + expect(getActiveNavLinkUrl()).toEqual('/app/test#/start/deep/path/3'); }); test('unhash all urls that are recorded while app is mounted', () => { (unhashUrl as jest.Mock).mockImplementation(x => x + '?unhashed'); createTracker(); urlTracker.appMounted(); - history.push('/deep/path/2'); - history.push('/deep/path/3'); + history.push('/start/deep/path/2'); + history.push('/start/deep/path/3'); urlTracker.appUnMounted(); expect(unhashUrl).toHaveBeenCalledTimes(2); - expect(getActiveNavLinkUrl()).toEqual('/app/test#/deep/path/3?unhashed'); + expect(getActiveNavLinkUrl()).toEqual('/app/test#/start/deep/path/3?unhashed'); }); test('show warning and use hashed url if unhashing does not work', () => { @@ -128,17 +129,17 @@ describe('kbnUrlTracker', () => { }); createTracker(); urlTracker.appMounted(); - history.push('/deep/path/2'); + history.push('/start/deep/path/2'); urlTracker.appUnMounted(); - expect(getActiveNavLinkUrl()).toEqual('/app/test#/deep/path/2'); + expect(getActiveNavLinkUrl()).toEqual('/app/test#/start/deep/path/2'); expect(toastService.addDanger).toHaveBeenCalledWith('unhash broke'); }); test('change nav link back to default if app gets mounted again', () => { createTracker(); urlTracker.appMounted(); - history.push('/deep/path/2'); - history.push('/deep/path/3'); + history.push('/start/deep/path/2'); + history.push('/start/deep/path/3'); urlTracker.appUnMounted(); urlTracker.appMounted(); expect(getActiveNavLinkUrl()).toEqual('/app/test#/start'); @@ -151,11 +152,11 @@ describe('kbnUrlTracker', () => { }); test('update state param without overwriting rest of the url when app is not mounted', () => { - storage.setItem('storageKey', '#/deep/path?extrastate=1'); + storage.setItem('storageKey', '#/start/deep/path?extrastate=1'); createTracker(); state1Subject.next({ key1: 'abc' }); expect(getActiveNavLinkUrl()).toMatchInlineSnapshot( - `"/app/test#/deep/path?extrastate=1&state1=(key1:abc)"` + `"/app/test#/start/deep/path?extrastate=1&state1=(key1:abc)"` ); }); @@ -184,7 +185,45 @@ describe('kbnUrlTracker', () => { test('set url to storage when setActiveUrl was called', () => { createTracker(); - urlTracker.setActiveUrl('/deep/path/4'); - expect(storage.getItem('storageKey')).toEqual('#/deep/path/4'); + urlTracker.setActiveUrl('/start/deep/path/4'); + expect(storage.getItem('storageKey')).toEqual('#/start/deep/path/4'); + }); + + describe('shouldTrackUrlUpdate', () => { + test('change nav link when shouldTrackUrlUpdate is not overridden', () => { + storage.setItem('storageKey', '#/start/deep/path'); + createTracker(); + urlTracker.appMounted(); + history.push('/start/path'); + urlTracker.appUnMounted(); + expect(getActiveNavLinkUrl()).toEqual('/app/test#/start/path'); + }); + + test('not change nav link when shouldTrackUrlUpdate is not overridden', () => { + storage.setItem('storageKey', '#/start/deep/path'); + createTracker(); + urlTracker.appMounted(); + history.push('/setup/path/2'); + urlTracker.appUnMounted(); + expect(getActiveNavLinkUrl()).toEqual('/app/test#/start/deep/path'); + }); + + test('change nav link when shouldTrackUrlUpdate is overridden', () => { + storage.setItem('storageKey', '#/start/deep/path'); + createTracker(() => true); + urlTracker.appMounted(); + history.push('/setup/path/2'); + urlTracker.appUnMounted(); + expect(getActiveNavLinkUrl()).toEqual('/app/test#/setup/path/2'); + }); + + test('not change nav link when shouldTrackUrlUpdate is overridden', () => { + storage.setItem('storageKey', '#/start/deep/path'); + createTracker(() => false); + urlTracker.appMounted(); + history.push('/setup/path/2'); + urlTracker.appUnMounted(); + expect(getActiveNavLinkUrl()).toEqual('/app/test#/start/deep/path'); + }); }); }); diff --git a/src/plugins/kibana_utils/public/state_management/url/kbn_url_tracker.ts b/src/plugins/kibana_utils/public/state_management/url/kbn_url_tracker.ts index 2edd135c184ec..b778535a2d428 100644 --- a/src/plugins/kibana_utils/public/state_management/url/kbn_url_tracker.ts +++ b/src/plugins/kibana_utils/public/state_management/url/kbn_url_tracker.ts @@ -58,6 +58,12 @@ export function createKbnUrlTracker({ toastNotifications, history, storage, + shouldTrackUrlUpdate = pathname => { + const currentAppName = defaultSubUrl.slice(2); // cut hash and slash symbols + const targetAppName = pathname.split('/')[1]; + + return currentAppName === targetAppName; + }, }: { /** * Base url of the current app. This will be used as a prefix for the @@ -82,7 +88,7 @@ export function createKbnUrlTracker({ stateUpdate$: Observable; }>; /** - * Key used to store the current sub url in session storage. This key should only be used for one active url tracker at any given ntime. + * Key used to store the current sub url in session storage. This key should only be used for one active url tracker at any given time. */ storageKey: string; /** @@ -101,6 +107,13 @@ export function createKbnUrlTracker({ * Storage object to use to persist currently active url. If this isn't provided, the browser wide session storage instance will be used. */ storage?: Storage; + /** + * Checks if pathname belongs to current app. It's used in history listener to define whether it's necessary to set pathname as active url or not. + * The default implementation compares the app name to the first part of pathname. Consumers can override this function for more complex cases. + * + * @param {string} pathname A location's pathname which comes to history listener + */ + shouldTrackUrlUpdate?: (pathname: string) => boolean; }): KbnUrlTracker { const historyInstance = history || createHashHistory(); const storageInstance = storage || sessionStorage; @@ -148,7 +161,9 @@ export function createKbnUrlTracker({ unsubscribe(); // track current hash when within app unsubscribeURLHistory = historyInstance.listen(location => { - setActiveUrl(location.pathname + location.search); + if (shouldTrackUrlUpdate(location.pathname)) { + setActiveUrl(location.pathname + location.search); + } }); } From e64eff0a3d5fde205256ab74b731a63766822f9a Mon Sep 17 00:00:00 2001 From: Tyler Smalley Date: Mon, 24 Feb 2020 06:02:27 -0800 Subject: [PATCH 2/4] Temporarily removes kbn-optimizer cache key tests (#58318) While we investigate why they are interfering with other tests. Signed-off-by: Tyler Smalley Co-authored-by: Elastic Machine --- .../src/optimizer/cache_keys.test.ts | 206 ------------------ 1 file changed, 206 deletions(-) delete mode 100644 packages/kbn-optimizer/src/optimizer/cache_keys.test.ts diff --git a/packages/kbn-optimizer/src/optimizer/cache_keys.test.ts b/packages/kbn-optimizer/src/optimizer/cache_keys.test.ts deleted file mode 100644 index 2337017f54ed8..0000000000000 --- a/packages/kbn-optimizer/src/optimizer/cache_keys.test.ts +++ /dev/null @@ -1,206 +0,0 @@ -/* - * 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 Path from 'path'; - -import jestDiff from 'jest-diff'; -import { REPO_ROOT, createAbsolutePathSerializer } from '@kbn/dev-utils'; - -import { reformatJestDiff, getOptimizerCacheKey, diffCacheKey } from './cache_keys'; -import { OptimizerConfig } from './optimizer_config'; - -jest.mock('./get_changes.ts', () => ({ - getChanges: async () => - new Map([ - ['/foo/bar/a', 'modified'], - ['/foo/bar/b', 'modified'], - ['/foo/bar/c', 'deleted'], - ]), -})); - -jest.mock('./get_mtimes.ts', () => ({ - getMtimes: async (paths: string[]) => new Map(paths.map(path => [path, 12345])), -})); - -jest.mock('execa'); - -jest.mock('fs', () => { - const realFs = jest.requireActual('fs'); - jest.spyOn(realFs, 'readFile'); - return realFs; -}); - -expect.addSnapshotSerializer(createAbsolutePathSerializer()); - -jest.requireMock('execa').mockImplementation(async (cmd: string, args: string[], opts: object) => { - expect(cmd).toBe('git'); - expect(args).toEqual([ - 'log', - '-n', - '1', - '--pretty=format:%H', - '--', - expect.stringContaining('kbn-optimizer'), - ]); - expect(opts).toEqual({ - cwd: REPO_ROOT, - }); - - return { - stdout: '', - }; -}); - -describe('getOptimizerCacheKey()', () => { - it('uses latest commit, bootstrap cache, and changed files to create unique value', async () => { - jest - .requireMock('fs') - .readFile.mockImplementation( - (path: string, enc: string, cb: (err: null, file: string) => void) => { - expect(path).toBe( - Path.resolve(REPO_ROOT, 'packages/kbn-optimizer/target/.bootstrap-cache') - ); - expect(enc).toBe('utf8'); - cb(null, ''); - } - ); - - const config = OptimizerConfig.create({ - repoRoot: REPO_ROOT, - }); - - await expect(getOptimizerCacheKey(config)).resolves.toMatchInlineSnapshot(` - Object { - "bootstrap": "", - "deletedPaths": Array [ - "/foo/bar/c", - ], - "lastCommit": "", - "modifiedTimes": Object { - "/foo/bar/a": 12345, - "/foo/bar/b": 12345, - }, - "workerConfig": Object { - "browserslistEnv": "dev", - "cache": true, - "dist": false, - "optimizerCacheKey": "♻", - "profileWebpack": false, - "repoRoot": , - "watch": false, - }, - } - `); - }); -}); - -describe('diffCacheKey()', () => { - it('returns undefined if values are equal', () => { - expect(diffCacheKey('1', '1')).toBe(undefined); - expect(diffCacheKey(1, 1)).toBe(undefined); - expect(diffCacheKey(['1', '2', { a: 'b' }], ['1', '2', { a: 'b' }])).toBe(undefined); - expect( - diffCacheKey( - { - a: '1', - b: '2', - }, - { - b: '2', - a: '1', - } - ) - ).toBe(undefined); - }); - - it('returns a diff if the values are different', () => { - expect(diffCacheKey(['1', '2', { a: 'b' }], ['1', '2', { b: 'a' }])).toMatchInlineSnapshot(` - "- Expected - + Received - -  Array [ -  \\"1\\", -  \\"2\\", -  Object { - - \\"a\\": \\"b\\", - + \\"b\\": \\"a\\", -  }, -  ]" - `); - expect( - diffCacheKey( - { - a: '1', - b: '1', - }, - { - b: '2', - a: '2', - } - ) - ).toMatchInlineSnapshot(` - "- Expected - + Received - -  Object { - - \\"a\\": \\"1\\", - - \\"b\\": \\"1\\", - + \\"a\\": \\"2\\", - + \\"b\\": \\"2\\", -  }" - `); - }); -}); - -describe('reformatJestDiff()', () => { - it('reformats large jestDiff output to focus on the changed lines', () => { - const diff = jestDiff( - { - a: ['1', '1', '1', '1', '1', '1', '1', '2', '1', '1', '1', '1', '1', '1', '1', '1', '1'], - }, - { - b: ['1', '1', '1', '1', '1', '1', '1', '1', '1', '1', '1', '1', '2', '1', '1', '1', '1'], - } - ); - - expect(reformatJestDiff(diff)).toMatchInlineSnapshot(` - "- Expected - + Received - -  Object { - - \\"a\\": Array [ - + \\"b\\": Array [ -  \\"1\\", -  \\"1\\", -  ... -  \\"1\\", -  \\"1\\", - - \\"2\\", -  \\"1\\", -  \\"1\\", -  ... -  \\"1\\", -  \\"1\\", - + \\"2\\", -  \\"1\\", -  \\"1\\", -  ..." - `); - }); -}); From 581f1bd6e9329b66db5bc3cb3348475595db5aa2 Mon Sep 17 00:00:00 2001 From: Vadim Dalecky Date: Mon, 24 Feb 2020 16:26:34 +0100 Subject: [PATCH 3/4] Expressions debug mode (#57841) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: 🎸 add ability to execute expression in debug mode * feat: 🎸 store resolved arguments in debug information * feat: 🎸 track function execution time and set "success" flag * feat: 🎸 store debug information for functions that throw * feat: 🎸 use performance.now, safe "fn" reference, fix typo --- src/plugins/expressions/common/ast/types.ts | 53 ++++ .../common/execution/execution.test.ts | 253 +++++++++++++++++- .../expressions/common/execution/execution.ts | 67 ++++- .../expressions/common/executor/executor.ts | 26 +- 4 files changed, 379 insertions(+), 20 deletions(-) diff --git a/src/plugins/expressions/common/ast/types.ts b/src/plugins/expressions/common/ast/types.ts index 82a7578dd4b89..0b505f117a580 100644 --- a/src/plugins/expressions/common/ast/types.ts +++ b/src/plugins/expressions/common/ast/types.ts @@ -17,6 +17,9 @@ * under the License. */ +import { ExpressionValue, ExpressionValueError } from '../expression_types'; +import { ExpressionFunction } from '../../public'; + export type ExpressionAstNode = | ExpressionAstExpression | ExpressionAstFunction @@ -31,6 +34,56 @@ export interface ExpressionAstFunction { type: 'function'; function: string; arguments: Record; + + /** + * Debug information added to each function when expression is executed in *debug mode*. + */ + debug?: ExpressionAstFunctionDebug; +} + +export interface ExpressionAstFunctionDebug { + /** + * True if function successfully returned output, false if function threw. + */ + success: boolean; + + /** + * Reference to the expression function this AST node represents. + */ + fn: ExpressionFunction; + + /** + * Input that expression function received as its first argument. + */ + input: ExpressionValue; + + /** + * Map of resolved arguments expression function received as its second argument. + */ + args: Record; + + /** + * Result returned by the expression function. Including an error result + * if it was returned by the function (not thrown). + */ + output?: ExpressionValue; + + /** + * Error that function threw normalized to `ExpressionValueError`. + */ + error?: ExpressionValueError; + + /** + * Raw error that was thrown by the function, if any. + */ + rawError?: any | Error; + + /** + * Time in milliseconds it took to execute the function. Duration can be + * `undefined` if error happened during argument resolution, because function + * timing starts after the arguments have been resolved. + */ + duration: number | undefined; } export type ExpressionAstArgument = string | boolean | number | ExpressionAstExpression; diff --git a/src/plugins/expressions/common/execution/execution.test.ts b/src/plugins/expressions/common/execution/execution.test.ts index b6c1721e33eef..f6ff9efca848b 100644 --- a/src/plugins/expressions/common/execution/execution.test.ts +++ b/src/plugins/expressions/common/execution/execution.test.ts @@ -18,20 +18,28 @@ */ import { Execution } from './execution'; -import { parseExpression } from '../ast'; +import { parseExpression, ExpressionAstExpression } from '../ast'; import { createUnitTestExecutor } from '../test_helpers'; import { ExpressionFunctionDefinition } from '../../public'; import { ExecutionContract } from './execution_contract'; +beforeAll(() => { + if (typeof performance === 'undefined') { + (global as any).performance = { now: Date.now }; + } +}); + const createExecution = ( expression: string = 'foo bar=123', - context: Record = {} + context: Record = {}, + debug: boolean = false ) => { const executor = createUnitTestExecutor(); const execution = new Execution({ executor, ast: parseExpression(expression), context, + debug, }); return execution; }; @@ -406,4 +414,245 @@ describe('Execution', () => { }); }); }); + + describe('debug mode', () => { + test('can execute expression in debug mode', async () => { + const execution = createExecution('add val=1 | add val=2 | add val=3', {}, true); + execution.start(-1); + const result = await execution.result; + + expect(result).toEqual({ + type: 'num', + value: 5, + }); + }); + + test('can execute expression with sub-expression in debug mode', async () => { + const execution = createExecution( + 'add val={var_set name=foo value=5 | var name=foo} | add val=10', + {}, + true + ); + execution.start(0); + const result = await execution.result; + + expect(result).toEqual({ + type: 'num', + value: 15, + }); + }); + + describe('when functions succeed', () => { + test('sets "success" flag on all functions to true', async () => { + const execution = createExecution('add val=1 | add val=2 | add val=3', {}, true); + execution.start(-1); + await execution.result; + + for (const node of execution.state.get().ast.chain) { + expect(node.debug?.success).toBe(true); + } + }); + + test('stores "fn" reference to the function', async () => { + const execution = createExecution('add val=1 | add val=2 | add val=3', {}, true); + execution.start(-1); + await execution.result; + + for (const node of execution.state.get().ast.chain) { + expect(node.debug?.fn.name).toBe('add'); + } + }); + + test('saves duration it took to execute each function', async () => { + const execution = createExecution('add val=1 | add val=2 | add val=3', {}, true); + execution.start(-1); + await execution.result; + + for (const node of execution.state.get().ast.chain) { + expect(typeof node.debug?.duration).toBe('number'); + expect(node.debug?.duration).toBeLessThan(100); + expect(node.debug?.duration).toBeGreaterThanOrEqual(0); + } + }); + + test('sets duration to 10 milliseconds when function executes 10 milliseconds', async () => { + const execution = createExecution('sleep 10', {}, true); + execution.start(-1); + await execution.result; + + const node = execution.state.get().ast.chain[0]; + expect(typeof node.debug?.duration).toBe('number'); + expect(node.debug?.duration).toBeLessThan(50); + expect(node.debug?.duration).toBeGreaterThanOrEqual(5); + }); + + test('adds .debug field in expression AST on each executed function', async () => { + const execution = createExecution('add val=1 | add val=2 | add val=3', {}, true); + execution.start(-1); + await execution.result; + + for (const node of execution.state.get().ast.chain) { + expect(typeof node.debug).toBe('object'); + expect(!!node.debug).toBe(true); + } + }); + + test('stores input of each function', async () => { + const execution = createExecution('add val=1 | add val=2 | add val=3', {}, true); + execution.start(-1); + await execution.result; + + const { chain } = execution.state.get().ast; + + expect(chain[0].debug!.input).toBe(-1); + expect(chain[1].debug!.input).toEqual({ + type: 'num', + value: 0, + }); + expect(chain[2].debug!.input).toEqual({ + type: 'num', + value: 2, + }); + }); + + test('stores output of each function', async () => { + const execution = createExecution('add val=1 | add val=2 | add val=3', {}, true); + execution.start(-1); + await execution.result; + + const { chain } = execution.state.get().ast; + + expect(chain[0].debug!.output).toEqual({ + type: 'num', + value: 0, + }); + expect(chain[1].debug!.output).toEqual({ + type: 'num', + value: 2, + }); + expect(chain[2].debug!.output).toEqual({ + type: 'num', + value: 5, + }); + }); + + test('stores resolved arguments of a function', async () => { + const execution = createExecution( + 'add val={var_set name=foo value=5 | var name=foo} | add val=10', + {}, + true + ); + execution.start(-1); + await execution.result; + + const { chain } = execution.state.get().ast; + + expect(chain[0].debug!.args).toEqual({ + val: 5, + }); + + expect((chain[0].arguments.val[0] as ExpressionAstExpression).chain[0].debug!.args).toEqual( + { + name: 'foo', + value: 5, + } + ); + }); + + test('store debug information about sub-expressions', async () => { + const execution = createExecution( + 'add val={var_set name=foo value=5 | var name=foo} | add val=10', + {}, + true + ); + execution.start(0); + await execution.result; + + const { chain } = execution.state.get().ast.chain[0].arguments + .val[0] as ExpressionAstExpression; + + expect(typeof chain[0].debug).toBe('object'); + expect(typeof chain[1].debug).toBe('object'); + expect(!!chain[0].debug).toBe(true); + expect(!!chain[1].debug).toBe(true); + + expect(chain[0].debug!.input).toBe(0); + expect(chain[0].debug!.output).toBe(0); + expect(chain[1].debug!.input).toBe(0); + expect(chain[1].debug!.output).toBe(5); + }); + }); + + describe('when expression throws', () => { + const executor = createUnitTestExecutor(); + executor.registerFunction({ + name: 'throws', + args: {}, + help: '', + fn: () => { + throw new Error('foo'); + }, + }); + + test('stores debug information up until the function that throws', async () => { + const execution = new Execution({ + executor, + ast: parseExpression('add val=1 | throws | add val=3'), + debug: true, + }); + execution.start(0); + await execution.result; + + const node1 = execution.state.get().ast.chain[0]; + const node2 = execution.state.get().ast.chain[1]; + const node3 = execution.state.get().ast.chain[2]; + + expect(typeof node1.debug).toBe('object'); + expect(typeof node2.debug).toBe('object'); + expect(typeof node3.debug).toBe('undefined'); + }); + + test('stores error thrown in debug information', async () => { + const execution = new Execution({ + executor, + ast: parseExpression('add val=1 | throws | add val=3'), + debug: true, + }); + execution.start(0); + await execution.result; + + const node2 = execution.state.get().ast.chain[1]; + + expect(node2.debug?.error).toMatchObject({ + type: 'error', + error: { + message: '[throws] > foo', + }, + }); + expect(node2.debug?.rawError).toBeInstanceOf(Error); + }); + + test('sets .debug object to expected shape', async () => { + const execution = new Execution({ + executor, + ast: parseExpression('add val=1 | throws | add val=3'), + debug: true, + }); + execution.start(0); + await execution.result; + + const node2 = execution.state.get().ast.chain[1]; + + expect(node2.debug).toMatchObject({ + success: false, + fn: expect.any(Object), + input: expect.any(Object), + args: expect.any(Object), + error: expect.any(Object), + rawError: expect.any(Error), + duration: expect.any(Number), + }); + }); + }); + }); }); diff --git a/src/plugins/expressions/common/execution/execution.ts b/src/plugins/expressions/common/execution/execution.ts index 2a272e187cffc..7e7df822724ae 100644 --- a/src/plugins/expressions/common/execution/execution.ts +++ b/src/plugins/expressions/common/execution/execution.ts @@ -23,7 +23,7 @@ import { createExecutionContainer, ExecutionContainer } from './container'; import { createError } from '../util'; import { Defer } from '../../../kibana_utils/common'; import { RequestAdapter, DataAdapter } from '../../../inspector/common'; -import { isExpressionValueError } from '../expression_types/specs/error'; +import { isExpressionValueError, ExpressionValueError } from '../expression_types/specs/error'; import { ExpressionAstExpression, ExpressionAstFunction, @@ -32,7 +32,7 @@ import { parseExpression, } from '../ast'; import { ExecutionContext, DefaultInspectorAdapters } from './types'; -import { getType } from '../expression_types'; +import { getType, ExpressionValue } from '../expression_types'; import { ArgumentType, ExpressionFunction } from '../expression_functions'; import { getByAlias } from '../util/get_by_alias'; import { ExecutionContract } from './execution_contract'; @@ -44,6 +44,13 @@ export interface ExecutionParams< ast?: ExpressionAstExpression; expression?: string; context?: ExtraContext; + + /** + * Whether to execute expression in *debug mode*. In *debug mode* inputs and + * outputs as well as all resolved arguments and time it took to execute each + * function are saved and are available for introspection. + */ + debug?: boolean; } const createDefaultInspectorAdapters = (): DefaultInspectorAdapters => ({ @@ -190,23 +197,55 @@ export class Execution< } const { function: fnName, arguments: fnArgs } = link; - const fnDef = getByAlias(this.state.get().functions, fnName); + const fn = getByAlias(this.state.get().functions, fnName); - if (!fnDef) { + if (!fn) { return createError({ message: `Function ${fnName} could not be found.` }); } + let args: Record = {}; + let timeStart: number | undefined; + try { - // Resolve arguments before passing to function - // resolveArgs returns an object because the arguments themselves might - // actually have a 'then' function which would be treated as a promise - const { resolvedArgs } = await this.resolveArgs(fnDef, input, fnArgs); - const output = await this.invokeFunction(fnDef, input, resolvedArgs); + // `resolveArgs` returns an object because the arguments themselves might + // actually have a `then` function which would be treated as a `Promise`. + const { resolvedArgs } = await this.resolveArgs(fn, input, fnArgs); + args = resolvedArgs; + timeStart = this.params.debug ? performance.now() : 0; + const output = await this.invokeFunction(fn, input, resolvedArgs); + + if (this.params.debug) { + const timeEnd: number = performance.now(); + (link as ExpressionAstFunction).debug = { + success: true, + fn, + input, + args: resolvedArgs, + output, + duration: timeEnd - timeStart, + }; + } + if (getType(output) === 'error') return output; input = output; - } catch (e) { - e.message = `[${fnName}] > ${e.message}`; - return createError(e); + } catch (rawError) { + const timeEnd: number = this.params.debug ? performance.now() : 0; + rawError.message = `[${fnName}] > ${rawError.message}`; + const error = createError(rawError) as ExpressionValueError; + + if (this.params.debug) { + (link as ExpressionAstFunction).debug = { + success: false, + fn, + input, + args, + error, + rawError, + duration: timeStart ? timeEnd - timeStart : undefined, + }; + } + + return error; } } @@ -327,7 +366,9 @@ export class Execution< const resolveArgFns = mapValues(argAstsWithDefaults, (asts, argName) => { return asts.map((item: ExpressionAstExpression) => { return async (subInput = input) => { - const output = await this.params.executor.interpret(item, subInput); + const output = await this.params.executor.interpret(item, subInput, { + debug: this.params.debug, + }); if (isExpressionValueError(output)) throw output.error; const casted = this.cast(output, argDefs[argName as any].types); return casted; diff --git a/src/plugins/expressions/common/executor/executor.ts b/src/plugins/expressions/common/executor/executor.ts index af3662d13de4e..2ecbc5f75a9e8 100644 --- a/src/plugins/expressions/common/executor/executor.ts +++ b/src/plugins/expressions/common/executor/executor.ts @@ -31,6 +31,15 @@ import { ExpressionAstExpression, ExpressionAstNode } from '../ast'; import { typeSpecs } from '../expression_types/specs'; import { functionSpecs } from '../expression_functions/specs'; +export interface ExpressionExecOptions { + /** + * Whether to execute expression in *debug mode*. In *debug mode* inputs and + * outputs as well as all resolved arguments and time it took to execute each + * function are saved and are available for introspection. + */ + debug?: boolean; +} + export class TypesRegistry implements IRegistry { constructor(private readonly executor: Executor) {} @@ -145,10 +154,14 @@ export class Executor = Record(ast: ExpressionAstNode, input: T): Promise { + public async interpret( + ast: ExpressionAstNode, + input: T, + options?: ExpressionExecOptions + ): Promise { switch (getType(ast)) { case 'expression': - return await this.interpretExpression(ast as ExpressionAstExpression, input); + return await this.interpretExpression(ast as ExpressionAstExpression, input, options); case 'string': case 'number': case 'null': @@ -161,9 +174,10 @@ export class Executor = Record( ast: string | ExpressionAstExpression, - input: T + input: T, + options?: ExpressionExecOptions ): Promise { - const execution = this.createExecution(ast); + const execution = this.createExecution(ast, undefined, options); execution.start(input); return await execution.result; } @@ -192,7 +206,8 @@ export class Executor = Record( ast: string | ExpressionAstExpression, - context: ExtraContext = {} as ExtraContext + context: ExtraContext = {} as ExtraContext, + { debug }: ExpressionExecOptions = {} as ExpressionExecOptions ): Execution { const params: ExecutionParams = { executor: this, @@ -200,6 +215,7 @@ export class Executor = Record Date: Mon, 24 Feb 2020 16:40:20 +0100 Subject: [PATCH 4/4] no sparse array by default. (#58212) Co-authored-by: Elastic Machine --- .../src/types/array_type.test.ts | 37 ++++++++++++++++--- .../kbn-config-schema/src/types/array_type.ts | 4 +- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/packages/kbn-config-schema/src/types/array_type.test.ts b/packages/kbn-config-schema/src/types/array_type.test.ts index 73661ef849cf4..66b72096a593d 100644 --- a/packages/kbn-config-schema/src/types/array_type.test.ts +++ b/packages/kbn-config-schema/src/types/array_type.test.ts @@ -85,14 +85,29 @@ test('fails if mixed types of content in array', () => { ); }); -test('returns empty array if input is empty but type has default value', () => { - const type = schema.arrayOf(schema.string({ defaultValue: 'test' })); +test('fails if sparse content in array', () => { + const type = schema.arrayOf(schema.string()); expect(type.validate([])).toEqual([]); + expect(() => type.validate([undefined])).toThrowErrorMatchingInlineSnapshot( + `"[0]: sparse array are not allowed"` + ); }); -test('returns empty array if input is empty even if type is required', () => { - const type = schema.arrayOf(schema.string()); +test('fails if sparse content in array if optional', () => { + const type = schema.arrayOf(schema.maybe(schema.string())); + expect(type.validate([])).toEqual([]); + expect(() => type.validate([undefined])).toThrowErrorMatchingInlineSnapshot( + `"[0]: sparse array are not allowed"` + ); +}); + +test('fails if sparse content in array if nullable', () => { + const type = schema.arrayOf(schema.nullable(schema.string())); expect(type.validate([])).toEqual([]); + expect(type.validate([null])).toEqual([null]); + expect(() => type.validate([undefined])).toThrowErrorMatchingInlineSnapshot( + `"[0]: sparse array are not allowed"` + ); }); test('fails for null values if optional', () => { @@ -102,9 +117,19 @@ test('fails for null values if optional', () => { ); }); +test('returns empty array if input is empty but type has default value', () => { + const type = schema.arrayOf(schema.string({ defaultValue: 'test' })); + expect(type.validate([])).toEqual([]); +}); + +test('returns empty array if input is empty even if type is required', () => { + const type = schema.arrayOf(schema.string()); + expect(type.validate([])).toEqual([]); +}); + test('handles default values for undefined values', () => { - const type = schema.arrayOf(schema.string({ defaultValue: 'foo' })); - expect(type.validate([undefined])).toEqual(['foo']); + const type = schema.arrayOf(schema.string(), { defaultValue: ['foo'] }); + expect(type.validate(undefined)).toEqual(['foo']); }); test('array within array', () => { diff --git a/packages/kbn-config-schema/src/types/array_type.ts b/packages/kbn-config-schema/src/types/array_type.ts index ad74f375588ad..a0353e8348ddd 100644 --- a/packages/kbn-config-schema/src/types/array_type.ts +++ b/packages/kbn-config-schema/src/types/array_type.ts @@ -31,7 +31,7 @@ export class ArrayType extends Type { let schema = internals .array() .items(type.getSchema().optional()) - .sparse(); + .sparse(false); if (options.minSize !== undefined) { schema = schema.min(options.minSize); @@ -49,6 +49,8 @@ export class ArrayType extends Type { case 'any.required': case 'array.base': return `expected value of type [array] but got [${typeDetect(value)}]`; + case 'array.sparse': + return `sparse array are not allowed`; case 'array.parse': return `could not parse array value from [${value}]`; case 'array.min':