[Lens] Fix table sorting on time picker interval change (#182173)

## Summary

Fixes #182153 

Due to some specific implementation of the schema sorting in the EUI
datagrid, sometimes the table row can be `null/undefined` and the
comparison function utility wasn't prepared for that.
Now the utility is resilient to `null` rows.

### Checklist

- [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
This commit is contained in:
Marco Liberati 2024-05-01 07:04:29 +02:00 committed by GitHub
parent 088e70d00e
commit 085d2368b0
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 82 additions and 19 deletions

View file

@ -9,6 +9,7 @@ This package contains a flexible sorting function who supports the following typ
* 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)
* if it matters the difference: null values are sorted always before undefined ones
* 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)

View file

@ -46,6 +46,56 @@ function testSorting({
}
describe('Data sorting criteria', () => {
describe('null rows', () => {
// in these tests it needs to skip the testSorting utility in order to pass null rows
// mind that [].sort() will never pass `undefined` values to the comparison function
// so we test it with null values instead
it('should not crash with null rows with strings', () => {
const datatable = ['a', 'b', 'c', 'd', '12'];
const datatableWithNulls = datatable.flatMap((v) => [{ a: v }, null]);
const criteria = getSortingCriteria('string', 'a', getMockFormatter());
expect(
datatableWithNulls
.sort((a, b) => criteria(a, b, 'asc'))
.map((row) => (row == null ? row : row.a))
).toEqual(['12', 'a', 'b', 'c', 'd', ...Array(datatable.length).fill(null)]);
expect(
datatableWithNulls
.sort((a, b) => criteria(a, b, 'desc'))
.map((row) => (row == null ? row : row.a))
).toEqual(['d', 'c', 'b', 'a', '12', ...Array(datatable.length).fill(null)]);
});
it('should not crash with null rows with version', () => {
const datatable = ['1.21.0', '1.1.0', '1.112.0', '1.0.0', '__other__'];
const datatableWithNulls = datatable.flatMap((v) => [{ a: v }, null]);
const criteria = getSortingCriteria('version', 'a', getMockFormatter());
expect(
datatableWithNulls
.sort((a, b) => criteria(a, b, 'asc'))
.map((row) => (row == null ? row : row.a))
).toEqual([
'1.0.0',
'1.1.0',
'1.21.0',
'1.112.0',
...Array(datatable.length).fill(null),
'__other__',
]);
expect(
datatableWithNulls
.sort((a, b) => criteria(a, b, 'desc'))
.map((row) => (row == null ? row : row.a))
).toEqual([
'1.112.0',
'1.21.0',
'1.1.0',
'1.0.0',
...Array(datatable.length).fill(null),
'__other__',
]);
});
});
describe('Date values', () => {
for (const direction of ['asc', 'desc'] as const) {
it(`should provide the date criteria for date values (${direction})`, () => {
@ -229,7 +279,7 @@ describe('Data sorting criteria', () => {
it('should sort non-version stuff to the end', () => {
testSorting({
input: ['1.21.0', undefined, '1.1.0', null, '1.112.0', '__other__', '1.0.0'],
output: ['1.0.0', '1.1.0', '1.21.0', '1.112.0', undefined, null, '__other__'],
output: ['1.0.0', '1.1.0', '1.21.0', '1.112.0', null, undefined, '__other__'],
direction: 'asc',
type: 'version',
reverseOutput: false,
@ -237,7 +287,7 @@ describe('Data sorting criteria', () => {
testSorting({
input: ['1.21.0', undefined, '1.1.0', null, '1.112.0', '__other__', '1.0.0'],
output: ['1.112.0', '1.21.0', '1.1.0', '1.0.0', undefined, null, '__other__'],
output: ['1.112.0', '1.21.0', '1.1.0', '1.0.0', null, undefined, '__other__'],
direction: 'desc',
type: 'version',
reverseOutput: false,

View file

@ -133,11 +133,17 @@ function getSafeIpAddress(ip: string | undefined, directionFactor: number) {
}
const versionComparison: CompareFn<string> = (v1, v2, direction) => {
const valueA = String(v1 ?? '');
const valueB = String(v2 ?? '');
const valueA = String(v1 == null ? '' : v1);
const valueB = String(v2 == null ? '' : v2);
const aInvalid = !valueA || !valid(valueA);
const bInvalid = !valueB || !valid(valueB);
if (aInvalid && bInvalid) {
if (v1 == null && v1 !== v2) {
return direction * -1;
}
if (v2 == null && v1 !== v2) {
return direction * 1;
}
return 0;
}
// need to fight the direction multiplication of the parent function
@ -164,30 +170,32 @@ const rangeComparison: CompareFn<Omit<Range, 'type'>> = (v1, v2) => {
function createArrayValuesHandler(sortBy: string, formatter: FieldFormat) {
return function <T>(criteriaFn: CompareFn<T>) {
return (
rowA: Record<string, unknown>,
rowB: Record<string, unknown>,
rowA: Record<string, unknown> | undefined | null,
rowB: Record<string, unknown> | undefined | null,
direction: 'asc' | 'desc'
) => {
// handle the direction with a multiply factor.
const directionFactor = direction === 'asc' ? 1 : -1;
// make it handle null/undefined values
// this masks null/undefined rows into null/undefined values so it can benefit from shared invalid logic
// and enable custom sorting for invalid values (like for version type)
const valueA = rowA == null ? rowA : rowA[sortBy];
const valueB = rowB == null ? rowB : rowB[sortBy];
// if either side of the comparison is an array, make it also the other one become one
// then perform an array comparison
if (Array.isArray(rowA[sortBy]) || Array.isArray(rowB[sortBy])) {
if (Array.isArray(valueA) || Array.isArray(valueB)) {
return (
directionFactor *
compareArrays(
(Array.isArray(rowA[sortBy]) ? rowA[sortBy] : [rowA[sortBy]]) as T[],
(Array.isArray(rowB[sortBy]) ? rowB[sortBy] : [rowB[sortBy]]) as T[],
(Array.isArray(valueA) ? valueA : [valueA]) as T[],
(Array.isArray(valueB) ? valueB : [valueB]) as T[],
directionFactor,
formatter,
criteriaFn
)
);
}
return (
directionFactor *
criteriaFn(rowA[sortBy] as T, rowB[sortBy] as T, directionFactor, formatter)
);
return directionFactor * criteriaFn(valueA as T, valueB as T, directionFactor, formatter);
};
};
}
@ -201,12 +209,12 @@ function getUndefinedHandler(
) => number
) {
return (
rowA: Record<string, unknown>,
rowB: Record<string, unknown>,
rowA: Record<string, unknown> | undefined | null,
rowB: Record<string, unknown> | undefined | null,
direction: 'asc' | 'desc'
) => {
const valueA = rowA[sortBy];
const valueB = rowB[sortBy];
const valueA = rowA?.[sortBy];
const valueB = rowB?.[sortBy];
// 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)) {
@ -218,7 +226,7 @@ function getUndefinedHandler(
if (valueB == null || Number.isNaN(valueB)) {
return -1;
}
return sortingCriteria(rowA, rowB, direction);
return sortingCriteria(rowA!, rowB!, direction);
};
}
@ -226,7 +234,11 @@ export function getSortingCriteria(
type: string | undefined,
sortBy: string,
formatter: FieldFormat
) {
): (
rowA: Record<string, unknown> | undefined | null,
rowB: Record<string, unknown> | undefined | null,
direction: 'asc' | 'desc'
) => number {
const arrayValueHandler = createArrayValuesHandler(sortBy, formatter);
if (type === 'date') {