Skip to content

Commit

Permalink
[Expressions] columns. Fixes Bugs caused by using name instead of I…
Browse files Browse the repository at this point in the history 
…D. (#118470)

* Fixed behavior of the columns function.

* updated tests to check for `id` matching in addition to the `name`.

Co-authored-by: Kibana Machine <[email protected]>
  • Loading branch information
@Kuznietsov @kibanamachine
Kuznietsov and kibanamachine authored Nov 16, 2021
1 parent a247508 commit f7344ef
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 24 deletions.
14 changes: 14 additions & 0 deletions x-pack/plugins/canvas/canvas_plugin_src/functions/common/__fixtures__/test_tables.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ const testTable: Datatable = {
name: 'name',
meta: { type: 'string' },
},
{
id: 'title_id',
name: 'title',
meta: { type: 'string' },
},
{
id: 'time',
name: 'time',
Expand Down Expand Up @@ -49,62 +54,71 @@ const testTable: Datatable = {
price: 605,
quantity: 100,
in_stock: true,
title_id: 'title1',
},
{
name: 'product1',
time: 1517929200950, // 06 Feb 2018 15:00:00 GMT
price: 583,
quantity: 200,
in_stock: true,
title_id: 'title2',
},
{
name: 'product1',
time: 1518015600950, // 07 Feb 2018 15:00:00 GMT
price: 420,
quantity: 300,
in_stock: true,
title_id: 'title3',
},
{
name: 'product2',
time: 1517842800950, // 05 Feb 2018 15:00:00 GMT
price: 216,
quantity: 350,
in_stock: false,
title_id: 'title4',
},
{
name: 'product2',
time: 1517929200950, // 06 Feb 2018 15:00:00 GMT
price: 200,
quantity: 256,
in_stock: false,
title_id: 'title5',
},
{
name: 'product2',
time: 1518015600950, // 07 Feb 2018 15:00:00 GMT
price: 190,
quantity: 231,
in_stock: false,
title_id: 'title6',
},
{
name: 'product3',
time: 1517842800950, // 05 Feb 2018 15:00:00 GMT
price: 67,
quantity: 240,
in_stock: true,
title_id: 'title7',
},
{
name: 'product4',
time: 1517842800950, // 05 Feb 2018 15:00:00 GMT
price: 311,
quantity: 447,
in_stock: false,
title_id: 'title8',
},
{
name: 'product5',
time: 1517842800950, // 05 Feb 2018 15:00:00 GMT
price: 288,
quantity: 384,
in_stock: true,
title_id: 'title9',
},
],
};
Expand Down
30 changes: 21 additions & 9 deletions x-pack/plugins/canvas/canvas_plugin_src/functions/common/columns.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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);
});
Expand All @@ -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
Expand All @@ -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', () => {
Expand All @@ -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', () => {
Expand Down
48 changes: 33 additions & 15 deletions x-pack/plugins/canvas/canvas_plugin_src/functions/common/columns.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<DatatableColumn[]>((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 };
}

Expand Down

0 comments on commit f7344ef

Please sign in to comment.