From f7344efedc08cbc35400612f905a55dce518f664 Mon Sep 17 00:00:00 2001 From: Yaroslav Kuznietsov Date: Tue, 16 Nov 2021 13:57:39 +0200 Subject: [PATCH] [Expressions] `columns`. Fixes Bugs caused by using name instead of ID. (#118470) * Fixed behavior of the columns function. * updated tests to check for `id` matching in addition to the `name`. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> --- .../common/__fixtures__/test_tables.ts | 14 ++++++ .../functions/common/columns.test.js | 30 ++++++++---- .../functions/common/columns.ts | 48 +++++++++++++------ 3 files changed, 68 insertions(+), 24 deletions(-) diff --git a/x-pack/plugins/canvas/canvas_plugin_src/functions/common/__fixtures__/test_tables.ts b/x-pack/plugins/canvas/canvas_plugin_src/functions/common/__fixtures__/test_tables.ts index 18aa70534b0ba..22c4d16d9810a 100644 --- a/x-pack/plugins/canvas/canvas_plugin_src/functions/common/__fixtures__/test_tables.ts +++ b/x-pack/plugins/canvas/canvas_plugin_src/functions/common/__fixtures__/test_tables.ts @@ -21,6 +21,11 @@ const testTable: Datatable = { name: 'name', meta: { type: 'string' }, }, + { + id: 'title_id', + name: 'title', + meta: { type: 'string' }, + }, { id: 'time', name: 'time', @@ -49,6 +54,7 @@ const testTable: Datatable = { price: 605, quantity: 100, in_stock: true, + title_id: 'title1', }, { name: 'product1', @@ -56,6 +62,7 @@ const testTable: Datatable = { price: 583, quantity: 200, in_stock: true, + title_id: 'title2', }, { name: 'product1', @@ -63,6 +70,7 @@ const testTable: Datatable = { price: 420, quantity: 300, in_stock: true, + title_id: 'title3', }, { name: 'product2', @@ -70,6 +78,7 @@ const testTable: Datatable = { price: 216, quantity: 350, in_stock: false, + title_id: 'title4', }, { name: 'product2', @@ -77,6 +86,7 @@ const testTable: Datatable = { price: 200, quantity: 256, in_stock: false, + title_id: 'title5', }, { name: 'product2', @@ -84,6 +94,7 @@ const testTable: Datatable = { price: 190, quantity: 231, in_stock: false, + title_id: 'title6', }, { name: 'product3', @@ -91,6 +102,7 @@ const testTable: Datatable = { price: 67, quantity: 240, in_stock: true, + title_id: 'title7', }, { name: 'product4', @@ -98,6 +110,7 @@ const testTable: Datatable = { price: 311, quantity: 447, in_stock: false, + title_id: 'title8', }, { name: 'product5', @@ -105,6 +118,7 @@ const testTable: Datatable = { price: 288, quantity: 384, in_stock: true, + title_id: 'title9', }, ], }; diff --git a/x-pack/plugins/canvas/canvas_plugin_src/functions/common/columns.test.js b/x-pack/plugins/canvas/canvas_plugin_src/functions/common/columns.test.js index d7f28559ee0ef..0184920285a6d 100644 --- a/x-pack/plugins/canvas/canvas_plugin_src/functions/common/columns.test.js +++ b/x-pack/plugins/canvas/canvas_plugin_src/functions/common/columns.test.js @@ -20,15 +20,20 @@ describe('columns', () => { it('returns a datatable with included columns and without excluded columns', () => { const arbitraryRowIndex = 7; const result = fn(testTable, { - include: 'name, price, quantity, foo, bar', + include: 'name, title_id, price, quantity, foo, bar', exclude: 'price, quantity, fizz, buzz', }); expect(result.columns[0]).toHaveProperty('name', 'name'); + expect(result.columns[1]).toHaveProperty('id', 'title_id'); expect(result.rows[arbitraryRowIndex]).toHaveProperty( 'name', testTable.rows[arbitraryRowIndex].name ); + expect(result.rows[arbitraryRowIndex]).toHaveProperty( + 'title_id', + testTable.rows[arbitraryRowIndex].title_id + ); expect(result.rows[arbitraryRowIndex]).not.toHaveProperty('price'); expect(result.rows[arbitraryRowIndex]).not.toHaveProperty('quantity'); expect(result.rows[arbitraryRowIndex]).not.toHaveProperty('foo'); @@ -46,8 +51,8 @@ describe('columns', () => { expect( fn(testTable, { - include: 'price, quantity, in_stock', - exclude: 'price, quantity, in_stock', + include: 'price, quantity, in_stock, title_id', + exclude: 'price, quantity, in_stock, title_id', }) ).toEqual(emptyTable); }); @@ -56,15 +61,17 @@ describe('columns', () => { it('returns a datatable with included columns only', () => { const arbitraryRowIndex = 3; const result = fn(testTable, { - include: 'name, time, in_stock', + include: 'name, time, in_stock, title_id', }); - expect(result.columns).toHaveLength(3); - expect(Object.keys(result.rows[0])).toHaveLength(3); + expect(result.columns).toHaveLength(4); + expect(Object.keys(result.rows[0])).toHaveLength(4); expect(result.columns[0]).toHaveProperty('name', 'name'); expect(result.columns[1]).toHaveProperty('name', 'time'); expect(result.columns[2]).toHaveProperty('name', 'in_stock'); + expect(result.columns[3]).toHaveProperty('id', 'title_id'); + expect(result.rows[arbitraryRowIndex]).toHaveProperty( 'name', testTable.rows[arbitraryRowIndex].name @@ -77,6 +84,10 @@ describe('columns', () => { 'in_stock', testTable.rows[arbitraryRowIndex].in_stock ); + expect(result.rows[arbitraryRowIndex]).toHaveProperty( + 'title_id', + testTable.rows[arbitraryRowIndex].title_id + ); }); it('ignores invalid columns', () => { @@ -102,14 +113,15 @@ describe('columns', () => { describe('exclude', () => { it('returns a datatable without excluded columns', () => { const arbitraryRowIndex = 5; - const result = fn(testTable, { exclude: 'price, quantity, foo, bar' }); + const result = fn(testTable, { exclude: 'price, quantity, foo, bar, title_id' }); - expect(result.columns.length).toEqual(testTable.columns.length - 2); - expect(Object.keys(result.rows[0])).toHaveLength(testTable.columns.length - 2); + expect(result.columns.length).toEqual(testTable.columns.length - 3); + expect(Object.keys(result.rows[0])).toHaveLength(testTable.columns.length - 3); expect(result.rows[arbitraryRowIndex]).not.toHaveProperty('price'); expect(result.rows[arbitraryRowIndex]).not.toHaveProperty('quantity'); expect(result.rows[arbitraryRowIndex]).not.toHaveProperty('foo'); expect(result.rows[arbitraryRowIndex]).not.toHaveProperty('bar'); + expect(result.rows[arbitraryRowIndex]).not.toHaveProperty('title_id'); }); it('ignores invalid columns', () => { diff --git a/x-pack/plugins/canvas/canvas_plugin_src/functions/common/columns.ts b/x-pack/plugins/canvas/canvas_plugin_src/functions/common/columns.ts index a81368e23a477..e627086d70d9d 100644 --- a/x-pack/plugins/canvas/canvas_plugin_src/functions/common/columns.ts +++ b/x-pack/plugins/canvas/canvas_plugin_src/functions/common/columns.ts @@ -14,6 +14,26 @@ interface Arguments { exclude: string; } +const prepareFields = (fields: string) => fields.split(',').map((field) => field.trim()); + +const getFieldsIds = (cols: DatatableColumn[]) => cols.map((col) => col.id ?? col.name); + +const splitColumnsByFields = ( + cols: DatatableColumn[], + fields: string[], + saveOther: boolean = false +) => + cols.reduce<{ matched: DatatableColumn[]; other: DatatableColumn[] }>( + (splitColumns, col) => { + if (fields.includes(col.id) || fields.includes(col.name)) { + return { ...splitColumns, matched: [...splitColumns.matched, col] }; + } + + return saveOther ? { ...splitColumns, other: [...splitColumns.other, col] } : splitColumns; + }, + { matched: [], other: [] } + ); + export function columns(): ExpressionFunctionDefinition< 'columns', Datatable, @@ -44,27 +64,25 @@ export function columns(): ExpressionFunctionDefinition< let result = { ...input }; if (exclude) { - const fields = exclude.split(',').map((field) => field.trim()); - const cols = contextColumns.filter((col) => !fields.includes(col.name)); - const rows = cols.length > 0 ? contextRows.map((row) => omit(row, fields)) : []; - - result = { rows, columns: cols, ...rest }; + const fields = prepareFields(exclude); + const { matched: excluded, other } = splitColumnsByFields(result.columns, fields, true); + const fieldsIds = getFieldsIds(excluded); + const rows = excluded.length ? result.rows.map((row) => omit(row, fieldsIds)) : result.rows; + result = { rows, columns: other, ...rest }; } if (include) { - const fields = include.split(',').map((field) => field.trim()); - // const columns = result.columns.filter(col => fields.includes(col.name)); + const fields = prepareFields(include); + const { matched: included } = splitColumnsByFields(result.columns, fields); + const fieldsIds = getFieldsIds(included); // Include columns in the order the user specified - const cols: DatatableColumn[] = []; + const cols = fields.reduce((includedCols, field) => { + const column = find(included, (col) => col.id === field || col.name === field); + return column ? [...includedCols, column] : includedCols; + }, []); - fields.forEach((field) => { - const column = find(result.columns, { name: field }); - if (column) { - cols.push(column); - } - }); - const rows = cols.length > 0 ? result.rows.map((row) => pick(row, fields)) : []; + const rows = cols.length ? result.rows.map((row) => pick(row, fieldsIds)) : []; result = { rows, columns: cols, ...rest }; }