From 41f5b45adfda76ccbd6a59bffd54aedf50bbff42 Mon Sep 17 00:00:00 2001 From: Marco Liberati Date: Wed, 21 Feb 2024 09:07:17 +0100 Subject: [PATCH] [Lens] Fix sorting on table when using Last value on date field (#177288) ## Summary Fixes #175659 The bug was due to the format returned from the `Last value` of a date field, which was the ISO string type, but the sorting criteria was assuming a number format for dates. I've revisited the `kbn-sort-predicates` logic for dates to support now both numeric and ISO string format, with dedicated test suite. Screenshot 2024-02-20 at 13 45 07 Screenshot 2024-02-20 at 13 45 01 Also, it was an opportunity to uniform the logic for multi-values comparisons and document it in the package `README`. ### Checklist Delete any items that are not applicable to this PR. - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Stratoula Kalafateli --- packages/kbn-sort-predicates/README.md | 35 +++++- .../kbn-sort-predicates/src/sorting.test.ts | 105 +++++++++++++++--- packages/kbn-sort-predicates/src/sorting.ts | 62 ++++++++--- 3 files changed, 173 insertions(+), 29 deletions(-) diff --git a/packages/kbn-sort-predicates/README.md b/packages/kbn-sort-predicates/README.md index c650be1d6f1f5..bc1a2fa86b028 100644 --- a/packages/kbn-sort-predicates/README.md +++ b/packages/kbn-sort-predicates/README.md @@ -6,10 +6,11 @@ This package contains a flexible sorting function who supports the following typ * number * version * ip addresses (both IPv4 and IPv6) - handles `Others`/strings correcly in this case -* dates +* dates (both as number or ISO string) * ranges open and closed (number type only for now) * null and undefined (always sorted as last entries, no matter the direction) * any multi-value version of the types above (version excluded) + * for multi-values with different length it wins the first non-zero comparison (see note at the bottom) The function is intended to use with objects and to simplify the usage with sorting by a specific column/field. The functions has been extracted from Lens datatable where it was originally used. @@ -71,4 +72,34 @@ return { ... } }} />; -``` \ No newline at end of file +``` + +### Multi-value notes + +In this section there's some more details about multi-value comparison algorithm used in this package. +For multi-values of the same length, the first non-zero comparison wins (ASC example): + +``` +a: [5, 7] +b: [1] +``` + +`b` comes before `a` as `1 < 5`. + +With this other set of data: + +``` +a: [1, 2, 3, 5] +b: [1, 2, 3, 3] +``` + +`a` comes before `b` as the first 3 comparisons will return 0, while the last one (`5 > 3`) returns -1. + +In case of arrays of different length, the `undefined` value is used for the shortest multi-value: + +``` +a; [1, 2] +b: [1] +``` + +In this case `b` wins as on the second comparison `undefined < 2`. \ No newline at end of file diff --git a/packages/kbn-sort-predicates/src/sorting.test.ts b/packages/kbn-sort-predicates/src/sorting.test.ts index 2c50158964505..e2fd1a9448c58 100644 --- a/packages/kbn-sort-predicates/src/sorting.test.ts +++ b/packages/kbn-sort-predicates/src/sorting.test.ts @@ -46,27 +46,107 @@ function testSorting({ } describe('Data sorting criteria', () => { - describe('Numeric values', () => { + describe('Date values', () => { for (const direction of ['asc', 'desc'] as const) { - it(`should provide the number criteria of numeric values (${direction})`, () => { + it(`should provide the date criteria for date values (${direction})`, () => { + const now = Date.now(); testSorting({ - input: [7, 6, 5, -Infinity, Infinity], - output: [-Infinity, 5, 6, 7, Infinity], + input: [now, now - 150000, 0], + output: [0, now - 150000, now], direction, - type: 'number', + type: 'date', }); }); - it(`should provide the number criteria for date values (${direction})`, () => { + it(`should provide the date criteria for array date values (${direction})`, () => { const now = Date.now(); testSorting({ - input: [now, 0, now - 150000], - output: [0, now - 150000, now], + input: [now, [0, now], [0], now - 150000], + output: [[0], [0, now], now - 150000, now], direction, type: 'date', }); }); + it(`should provide the date criteria for ISO string date values (${direction})`, () => { + const now = new Date(Date.now()).toISOString(); + const beforeNow = new Date(Date.now() - 150000).toISOString(); + const originString = new Date(0).toISOString(); + testSorting({ + input: [now, beforeNow, originString], + output: [originString, beforeNow, now], + direction, + type: 'date', + }); + }); + + it(`should provide the date criteria for array ISO string date values (${direction})`, () => { + const now = new Date(Date.now()).toISOString(); + const beforeNow = new Date(Date.now() - 150000).toISOString(); + const originString = new Date(0).toISOString(); + testSorting({ + input: [now, [originString, now], [originString], beforeNow], + output: [[originString], [originString, now], beforeNow, now], + direction, + type: 'date', + }); + }); + + it(`should provide the date criteria for date values (${direction})`, () => { + const now = Date.now(); + const originString = new Date(0).toISOString(); + testSorting({ + input: [now, now - 150000, originString], + output: [originString, now - 150000, now], + direction, + type: 'date', + }); + }); + + it(`should provide the date criteria for array date values of mixed types (${direction})`, () => { + const now = Date.now(); + const beforeNow = Date.now() - 150000; + const originString = new Date(0).toISOString(); + testSorting({ + input: [now, [originString, now], [originString], beforeNow], + output: [[originString], [originString, now], beforeNow, now], + direction, + type: 'date', + }); + }); + } + + it(`should sort undefined and null to the end`, () => { + const now = new Date(Date.now()).toISOString(); + const beforeNow = new Date(Date.now() - 150000).toISOString(); + testSorting({ + input: [null, now, 0, undefined, null, beforeNow], + output: [0, beforeNow, now, null, undefined, null], + direction: 'asc', + type: 'date', + reverseOutput: false, + }); + + testSorting({ + input: [null, now, 0, undefined, null, beforeNow], + output: [now, beforeNow, 0, null, undefined, null], + direction: 'desc', + type: 'date', + reverseOutput: false, + }); + }); + }); + describe('Numeric values', () => { + for (const direction of ['asc', 'desc'] as const) { + it(`should provide the number criteria of numeric values (${direction})`, () => { + testSorting({ + input: [7, 6, 5, -Infinity, Infinity], + output: [-Infinity, 5, 6, 7, Infinity], + direction, + type: 'number', + }); + }); + it(`should provide the number criteria of array numeric values (${direction})`, () => { testSorting({ input: [7, [7, 1], [7, 2], [6], -Infinity, Infinity], @@ -76,13 +156,12 @@ describe('Data sorting criteria', () => { }); }); - it(`should provide the number criteria for array date values (${direction})`, () => { - const now = Date.now(); + it(`should provide the number criteria of array numeric values (${direction})`, () => { testSorting({ - input: [now, [0, now], [0], now - 150000], - output: [[0], [0, now], now - 150000, now], + input: [7, [0, 7], [0], 1], + output: [[0], [0, 7], 1, 7], direction, - type: 'date', + type: 'number', }); }); } diff --git a/packages/kbn-sort-predicates/src/sorting.ts b/packages/kbn-sort-predicates/src/sorting.ts index 555ca22a24bf0..e9fed6e5cfc0d 100644 --- a/packages/kbn-sort-predicates/src/sorting.ts +++ b/packages/kbn-sort-predicates/src/sorting.ts @@ -10,6 +10,7 @@ import versionCompare from 'compare-versions'; import valid from 'semver/functions/valid'; import ipaddr, { type IPv4, type IPv6 } from 'ipaddr.js'; import { FieldFormat } from '@kbn/field-formats-plugin/common'; +import moment from 'moment'; type CompareFn = ( v1: T | undefined, @@ -18,18 +19,47 @@ type CompareFn = ( formatter: FieldFormat ) => number; -const numberCompare: CompareFn = (v1, v2) => (v1 ?? -Infinity) - (v2 ?? -Infinity); +// Catches null, undefined and NaN +const isInvalidValue = (value: unknown): value is null | undefined => + value == null || (typeof value === 'number' && Number.isNaN(value)); -const stringComparison: CompareFn = (v1, v2, _, formatter) => { - const aString = formatter.convert(v1); - const bString = formatter.convert(v2); - if (v1 == null) { +const handleInvalidValues = (v1: unknown, v2: unknown) => { + if (isInvalidValue(v1)) { + if (isInvalidValue(v2)) { + return 0; + } + return -1; + } + if (isInvalidValue(v2)) { + return 1; + } + return undefined; +}; + +const numberCompare: CompareFn = (v1, v2) => handleInvalidValues(v1, v2) ?? v1! - v2!; + +const dateCompare: CompareFn = (v1, v2) => { + const mV1 = moment(v1); + const mV2 = moment(v2); + if (!mV1.isValid() || isInvalidValue(v1)) { + if (!mV2.isValid() || isInvalidValue(v2)) { + return 0; + } return -1; } - if (v2 == null) { + if (!mV2.isValid() || isInvalidValue(v2)) { return 1; } - return aString.localeCompare(bString); + if (mV1.isSame(mV2)) { + return 0; + } + return mV1.isBefore(mV2) ? -1 : 1; +}; + +const stringComparison: CompareFn = (v1, v2, _, formatter) => { + const aString = formatter.convert(v1); + const bString = formatter.convert(v2); + return handleInvalidValues(v1, v2) ?? aString.localeCompare(bString); }; // The maximum length of a IP is 39 chars for a IPv6 @@ -89,7 +119,7 @@ function isIPv6Address(ip: IPv4 | IPv6): ip is IPv6 { } function getSafeIpAddress(ip: string | undefined, directionFactor: number) { - if (ip == null || !ipaddr.isValid(ip)) { + if (isInvalidValue(ip) || !ipaddr.isValid(ip)) { // if ip is null, then it's a part of an array ip value // therefore the comparison might be between a single value [ipA, undefined] vs multiple values ip [ipA, ipB] // set in this case -1 for the undefined of the former to force it to be always before @@ -177,17 +207,18 @@ function getUndefinedHandler( ) => { const valueA = rowA[sortBy]; const valueB = rowB[sortBy]; - if (valueA != null && valueB != null && !Number.isNaN(valueA) && !Number.isNaN(valueB)) { - return sortingCriteria(rowA, rowB, direction); - } + // do not use the utility above as null at root level is handled differently + // than null/undefined within an array type if (valueA == null || Number.isNaN(valueA)) { + if (valueB == null || Number.isNaN(valueB)) { + return 0; + } return 1; } if (valueB == null || Number.isNaN(valueB)) { return -1; } - - return 0; + return sortingCriteria(rowA, rowB, direction); }; } @@ -198,7 +229,10 @@ export function getSortingCriteria( ) { const arrayValueHandler = createArrayValuesHandler(sortBy, formatter); - if (['number', 'date'].includes(type || '')) { + if (type === 'date') { + return getUndefinedHandler(sortBy, arrayValueHandler(dateCompare)); + } + if (type === 'number') { return getUndefinedHandler(sortBy, arrayValueHandler(numberCompare)); } // this is a custom type, and can safely assume the gte and lt fields are all numbers or undefined