Skip to content

Commit

Permalink
Fix sort with pagination and composite fields (twentyhq#9150)
Browse files Browse the repository at this point in the history
Fixes twentyhq#8863

## Description
This PR fixes an issue with cursor-based pagination when dealing with
composite fields (like `fullName`). Previously, the pagination direction
was incorrectly determined for composite fields because the code wasn't
properly handling nested object structures in the `orderBy` parameter.
Refactored the code accordingly.
  • Loading branch information
Weiko authored and mdrazak2001 committed Dec 20, 2024
1 parent 9db9b87 commit 30fe169
Show file tree
Hide file tree
Showing 4 changed files with 228 additions and 33 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@ export class GraphqlQueryOrderFieldParser {
Object.assign(acc, compositeOrder);
} else {
acc[`"${objectNameSingular}"."${key}"`] =
this.convertOrderByToFindOptionsOrder(value, isForwardPagination);
this.convertOrderByToFindOptionsOrder(
value as OrderByDirection,
isForwardPagination,
);
}
});

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
import { OrderByDirection } from 'src/engine/api/graphql/workspace-query-builder/interfaces/object-record.interface';

import { GraphqlQueryRunnerException } from 'src/engine/api/graphql/graphql-query-runner/errors/graphql-query-runner.exception';
import { computeCursorArgFilter } from 'src/engine/api/graphql/graphql-query-runner/utils/compute-cursor-arg-filter';
import { FieldMetadataType } from 'src/engine/metadata-modules/field-metadata/field-metadata.entity';

describe('computeCursorArgFilter', () => {
const mockFieldMetadataMap = {
name: {
type: FieldMetadataType.TEXT,
id: 'name-id',
name: 'name',
label: 'Name',
objectMetadataId: 'object-id',
},
age: {
type: FieldMetadataType.NUMBER,
id: 'age-id',
name: 'age',
label: 'Age',
objectMetadataId: 'object-id',
},
fullName: {
type: FieldMetadataType.FULL_NAME,
id: 'fullname-id',
name: 'fullName',
label: 'Full Name',
objectMetadataId: 'object-id',
},
};

describe('basic cursor filtering', () => {
it('should return empty array when cursor is empty', () => {
const result = computeCursorArgFilter({}, [], mockFieldMetadataMap, true);

expect(result).toEqual([]);
});

it('should compute forward pagination filter for single field', () => {
const cursor = { name: 'John' };
const orderBy = [{ name: OrderByDirection.AscNullsLast }];

const result = computeCursorArgFilter(
cursor,
orderBy,
mockFieldMetadataMap,
true,
);

expect(result).toEqual([{ name: { gt: 'John' } }]);
});

it('should compute backward pagination filter for single field', () => {
const cursor = { name: 'John' };
const orderBy = [{ name: OrderByDirection.AscNullsLast }];

const result = computeCursorArgFilter(
cursor,
orderBy,
mockFieldMetadataMap,
false,
);

expect(result).toEqual([{ name: { lt: 'John' } }]);
});
});

describe('multiple fields cursor filtering', () => {
it('should handle multiple cursor fields with forward pagination', () => {
const cursor = { name: 'John', age: 30 };
const orderBy = [
{ name: OrderByDirection.AscNullsLast },
{ age: OrderByDirection.DescNullsLast },
];

const result = computeCursorArgFilter(
cursor,
orderBy,
mockFieldMetadataMap,
true,
);

expect(result).toEqual([
{ name: { gt: 'John' } },
{ name: { eq: 'John' }, age: { lt: 30 } },
]);
});
});

describe('composite field handling', () => {
it('should handle fullName composite field', () => {
const cursor = {
fullName: { firstName: 'John', lastName: 'Doe' },
};
const orderBy = [
{
fullName: {
firstName: OrderByDirection.AscNullsLast,
lastName: OrderByDirection.AscNullsLast,
},
},
];

const result = computeCursorArgFilter(
cursor,
orderBy,
mockFieldMetadataMap,
true,
);

expect(result).toEqual([
{
fullName: {
firstName: { gt: 'John' },
lastName: { gt: 'Doe' },
},
},
]);
});
});

describe('error handling', () => {
it('should throw error for invalid field metadata', () => {
const cursor = { invalidField: 'value' };
const orderBy = [{ invalidField: OrderByDirection.AscNullsLast }];

expect(() =>
computeCursorArgFilter(cursor, orderBy, mockFieldMetadataMap, true),
).toThrow(GraphqlQueryRunnerException);
});

it('should throw error for missing orderBy entry', () => {
const cursor = { name: 'John' };
const orderBy = [{ age: OrderByDirection.AscNullsLast }];

expect(() =>
computeCursorArgFilter(cursor, orderBy, mockFieldMetadataMap, true),
).toThrow(GraphqlQueryRunnerException);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,42 @@ import { FieldMetadataType } from 'src/engine/metadata-modules/field-metadata/fi
import { isCompositeFieldMetadataType } from 'src/engine/metadata-modules/field-metadata/utils/is-composite-field-metadata-type.util';
import { FieldMetadataMap } from 'src/engine/metadata-modules/types/field-metadata-map';

const computeOperator = (
isAscending: boolean,
isForwardPagination: boolean,
defaultOperator?: string,
): string => {
if (defaultOperator) return defaultOperator;

return isAscending
? isForwardPagination
? 'gt'
: 'lt'
: isForwardPagination
? 'lt'
: 'gt';
};

const validateAndGetOrderBy = (
key: string,
orderBy: ObjectRecordOrderBy,
): Record<string, any> => {
const keyOrderBy = orderBy.find((order) => key in order);

if (!keyOrderBy) {
throw new GraphqlQueryRunnerException(
'Invalid cursor',
GraphqlQueryRunnerExceptionCode.INVALID_CURSOR,
);
}

return keyOrderBy;
};

const isAscendingOrder = (direction: OrderByDirection): boolean =>
direction === OrderByDirection.AscNullsFirst ||
direction === OrderByDirection.AscNullsLast;

export const computeCursorArgFilter = (
cursor: Record<string, any>,
orderBy: ObjectRecordOrderBy,
Expand Down Expand Up @@ -40,35 +76,22 @@ export const computeCursorArgFilter = (
cursorKeys[subConditionIndex],
cursorValues[subConditionIndex],
fieldMetadataMapByName,
orderBy,
isForwardPagination,
'eq',
),
};
}

const keyOrderBy = orderBy.find((order) => key in order);

if (!keyOrderBy) {
throw new GraphqlQueryRunnerException(
'Invalid cursor',
GraphqlQueryRunnerExceptionCode.INVALID_CURSOR,
);
}

const isAscending =
keyOrderBy[key] === OrderByDirection.AscNullsFirst ||
keyOrderBy[key] === OrderByDirection.AscNullsLast;

const operator = isAscending
? isForwardPagination
? 'gt'
: 'lt'
: isForwardPagination
? 'lt'
: 'gt';

return {
...whereCondition,
...buildWhereCondition(key, value, fieldMetadataMapByName, operator),
...buildWhereCondition(
key,
value,
fieldMetadataMapByName,
orderBy,
isForwardPagination,
),
} as ObjectRecordFilter;
});
};
Expand All @@ -77,7 +100,9 @@ const buildWhereCondition = (
key: string,
value: any,
fieldMetadataMapByName: FieldMetadataMap,
operator: string,
orderBy: ObjectRecordOrderBy,
isForwardPagination: boolean,
operator?: string,
): Record<string, any> => {
const fieldMetadata = fieldMetadataMapByName[key];

Expand All @@ -93,18 +118,30 @@ const buildWhereCondition = (
key,
value,
fieldMetadata.type,
orderBy,
isForwardPagination,
operator,
);
}

return { [key]: { [operator]: value } };
const keyOrderBy = validateAndGetOrderBy(key, orderBy);
const isAscending = isAscendingOrder(keyOrderBy[key]);
const computedOperator = computeOperator(
isAscending,
isForwardPagination,
operator,
);

return { [key]: { [computedOperator]: value } };
};

const buildCompositeWhereCondition = (
key: string,
value: any,
fieldType: FieldMetadataType,
operator: string,
orderBy: ObjectRecordOrderBy,
isForwardPagination: boolean,
operator?: string,
): Record<string, any> => {
const compositeType = compositeTypeDefinitions.get(fieldType);

Expand All @@ -115,18 +152,30 @@ const buildCompositeWhereCondition = (
);
}

const keyOrderBy = validateAndGetOrderBy(key, orderBy);
const result: Record<string, any> = {};

compositeType.properties.forEach((property) => {
if (
property.type !== FieldMetadataType.RAW_JSON &&
value[property.name] !== undefined
property.type === FieldMetadataType.RAW_JSON ||
value[property.name] === undefined
) {
result[key] = {
...result[key],
[property.name]: { [operator]: value[property.name] },
};
return;
}

const isAscending = isAscendingOrder(keyOrderBy[key][property.name]);
const computedOperator = computeOperator(
isAscending,
isForwardPagination,
operator,
);

result[key] = {
...result[key],
[property.name]: {
[computedOperator]: value[property.name],
},
};
});

return result;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ export enum OrderByDirection {
}

export type ObjectRecordOrderBy = Array<{
[Property in keyof ObjectRecord]?: OrderByDirection;
[Property in keyof ObjectRecord]?:
| OrderByDirection
| Record<string, OrderByDirection>;
}>;

export interface ObjectRecordDuplicateCriteria {
Expand Down

0 comments on commit 30fe169

Please sign in to comment.