From 139816d5274c8596601f30ca8912fef9c590af28 Mon Sep 17 00:00:00 2001 From: Will Schurman Date: Thu, 26 Feb 2026 11:50:23 -0800 Subject: [PATCH] fix: correct pagination behavior for standard pagination with descending ordering clauses --- .../PostgresEntityIntegration-test.ts | 184 ++++++++++++++++-- .../src/internal/EntityKnexDataManager.ts | 116 +++++++---- 2 files changed, 240 insertions(+), 60 deletions(-) diff --git a/packages/entity-database-adapter-knex/src/__integration-tests__/PostgresEntityIntegration-test.ts b/packages/entity-database-adapter-knex/src/__integration-tests__/PostgresEntityIntegration-test.ts index 59294c699..b04317624 100644 --- a/packages/entity-database-adapter-knex/src/__integration-tests__/PostgresEntityIntegration-test.ts +++ b/packages/entity-database-adapter-knex/src/__integration-tests__/PostgresEntityIntegration-test.ts @@ -1641,22 +1641,22 @@ describe('postgres entity integration', () => { strategy: PaginationStrategy.STANDARD, orderBy: [ { fieldName: 'hasACat', order: OrderByOrdering.DESCENDING }, // true comes before false - { fieldName: 'name', order: OrderByOrdering.ASCENDING }, - { fieldName: 'id', order: OrderByOrdering.ASCENDING }, + { fieldName: 'name', order: OrderByOrdering.DESCENDING }, + { fieldName: 'id', order: OrderByOrdering.DESCENDING }, ], }, }); - // Entities with cats (true) come first, then sorted by name + // Entities with cats (true) come first, then sorted by name descending expect(page.edges).toHaveLength(4); expect(page.edges[0]?.node.getField('hasACat')).toBe(true); - expect(page.edges[0]?.node.getField('name')).toBe('Alice'); + expect(page.edges[0]?.node.getField('name')).toBe('Grace'); expect(page.edges[1]?.node.getField('hasACat')).toBe(true); - expect(page.edges[1]?.node.getField('name')).toBe('Charlie'); + expect(page.edges[1]?.node.getField('name')).toBe('Eve'); expect(page.edges[2]?.node.getField('hasACat')).toBe(true); - expect(page.edges[2]?.node.getField('name')).toBe('Eve'); + expect(page.edges[2]?.node.getField('name')).toBe('Charlie'); expect(page.edges[3]?.node.getField('hasACat')).toBe(true); - expect(page.edges[3]?.node.getField('name')).toBe('Grace'); + expect(page.edges[3]?.node.getField('name')).toBe('Alice'); }); it('supports pagination with fieldFragment orderBy', async () => { @@ -1817,6 +1817,156 @@ describe('postgres entity integration', () => { }); }); + it('performs forward pagination with ascending order', async () => { + const vc = new ViewerContext( + createKnexIntegrationTestEntityCompanionProvider(knexInstance), + ); + + // Create test data with names that sort in a specific order + const entities = []; + for (let i = 1; i <= 5; i++) { + const entity = await PostgresTestEntity.creator(vc) + .setField('name', `Z_Item_${i}`) // Z_Item_1, Z_Item_2, Z_Item_3, Z_Item_4, Z_Item_5 + .createAsync(); + entities.push(entity); + } + + // Get first page with ASCENDING order + // Sorted ascending: Z_Item_1, Z_Item_2, Z_Item_3, Z_Item_4, Z_Item_5 + const firstPage = await PostgresTestEntity.knexLoader(vc).loadPageAsync({ + first: 3, + pagination: { + strategy: PaginationStrategy.STANDARD, + orderBy: [{ fieldName: 'name', order: OrderByOrdering.ASCENDING }], + }, + }); + + expect(firstPage.edges).toHaveLength(3); + expect(firstPage.edges[0]?.node.getField('name')).toBe('Z_Item_1'); + expect(firstPage.edges[1]?.node.getField('name')).toBe('Z_Item_2'); + expect(firstPage.edges[2]?.node.getField('name')).toBe('Z_Item_3'); + expect(firstPage.pageInfo.hasNextPage).toBe(true); + expect(firstPage.pageInfo.hasPreviousPage).toBe(false); + + // Get second page using cursor + const secondPage = await PostgresTestEntity.knexLoader(vc).loadPageAsync({ + first: 3, + after: firstPage.pageInfo.endCursor!, + pagination: { + strategy: PaginationStrategy.STANDARD, + orderBy: [{ fieldName: 'name', order: OrderByOrdering.ASCENDING }], + }, + }); + + // Remaining items in ascending order: Z_Item_4, Z_Item_5 + expect(secondPage.edges).toHaveLength(2); + expect(secondPage.edges[0]?.node.getField('name')).toBe('Z_Item_4'); + expect(secondPage.edges[1]?.node.getField('name')).toBe('Z_Item_5'); + expect(secondPage.pageInfo.hasNextPage).toBe(false); + expect(secondPage.pageInfo.hasPreviousPage).toBe(false); + }); + + it('performs backward pagination with ascending order', async () => { + const vc = new ViewerContext( + createKnexIntegrationTestEntityCompanionProvider(knexInstance), + ); + + // Create test data with names that sort in a specific order + const entities = []; + for (let i = 1; i <= 5; i++) { + const entity = await PostgresTestEntity.creator(vc) + .setField('name', `Z_Item_${i}`) // Z_Item_1, Z_Item_2, Z_Item_3, Z_Item_4, Z_Item_5 + .createAsync(); + entities.push(entity); + } + + // Test backward pagination with ASCENDING order + // This internally flips ASCENDING to DESCENDING for the query + const page = await PostgresTestEntity.knexLoader(vc).loadPageAsync({ + last: 3, + pagination: { + strategy: PaginationStrategy.STANDARD, + orderBy: [{ fieldName: 'name', order: OrderByOrdering.ASCENDING }], + }, + }); + + // With `last: 3` and ASCENDING order, we get the last 3 items when sorted ascending + // Sorted ascending: Z_Item_1, Z_Item_2, Z_Item_3, Z_Item_4, Z_Item_5 + // Last 3: Z_Item_3, Z_Item_4, Z_Item_5 + expect(page.edges).toHaveLength(3); + expect(page.edges[0]?.node.getField('name')).toBe('Z_Item_3'); + expect(page.edges[1]?.node.getField('name')).toBe('Z_Item_4'); + expect(page.edges[2]?.node.getField('name')).toBe('Z_Item_5'); + expect(page.pageInfo.hasPreviousPage).toBe(true); + expect(page.pageInfo.hasNextPage).toBe(false); + + // Get previous page using cursor + const previousPage = await PostgresTestEntity.knexLoader(vc).loadPageAsync({ + last: 3, + before: page.pageInfo.startCursor!, + pagination: { + strategy: PaginationStrategy.STANDARD, + orderBy: [{ fieldName: 'name', order: OrderByOrdering.ASCENDING }], + }, + }); + + // Remaining items in ascending order: Z_Item_1, Z_Item_2 + expect(previousPage.edges).toHaveLength(2); + expect(previousPage.edges[0]?.node.getField('name')).toBe('Z_Item_1'); + expect(previousPage.edges[1]?.node.getField('name')).toBe('Z_Item_2'); + expect(previousPage.pageInfo.hasPreviousPage).toBe(false); + expect(previousPage.pageInfo.hasNextPage).toBe(false); + }); + + it('performs forward pagination with descending order', async () => { + const vc = new ViewerContext( + createKnexIntegrationTestEntityCompanionProvider(knexInstance), + ); + + // Create test data with names that sort in a specific order + const entities = []; + for (let i = 1; i <= 5; i++) { + const entity = await PostgresTestEntity.creator(vc) + .setField('name', `Z_Item_${i}`) // Z_Item_1, Z_Item_2, Z_Item_3, Z_Item_4, Z_Item_5 + .createAsync(); + entities.push(entity); + } + + // Get first page with DESCENDING order + // Sorted descending: Z_Item_5, Z_Item_4, Z_Item_3, Z_Item_2, Z_Item_1 + const firstPage = await PostgresTestEntity.knexLoader(vc).loadPageAsync({ + first: 3, + pagination: { + strategy: PaginationStrategy.STANDARD, + orderBy: [{ fieldName: 'name', order: OrderByOrdering.DESCENDING }], + }, + }); + + expect(firstPage.edges).toHaveLength(3); + expect(firstPage.edges[0]?.node.getField('name')).toBe('Z_Item_5'); + expect(firstPage.edges[1]?.node.getField('name')).toBe('Z_Item_4'); + expect(firstPage.edges[2]?.node.getField('name')).toBe('Z_Item_3'); + expect(firstPage.pageInfo.hasNextPage).toBe(true); + expect(firstPage.pageInfo.hasPreviousPage).toBe(false); + + // Get second page using cursor + const secondPage = await PostgresTestEntity.knexLoader(vc).loadPageAsync({ + first: 3, + after: firstPage.pageInfo.endCursor!, + pagination: { + strategy: PaginationStrategy.STANDARD, + orderBy: [{ fieldName: 'name', order: OrderByOrdering.DESCENDING }], + }, + }); + + // Remaining items in descending order: Z_Item_2, Z_Item_1 + expect(secondPage.edges).toHaveLength(2); + expect(secondPage.edges[0]?.node.getField('name')).toBe('Z_Item_2'); + expect(secondPage.edges[1]?.node.getField('name')).toBe('Z_Item_1'); + expect(secondPage.pageInfo.hasNextPage).toBe(false); + expect(secondPage.pageInfo.hasPreviousPage).toBe(false); + }); + it('performs backward pagination with descending order', async () => { const vc = new ViewerContext( createKnexIntegrationTestEntityCompanionProvider(knexInstance), @@ -1852,22 +2002,22 @@ describe('postgres entity integration', () => { expect(page.pageInfo.hasPreviousPage).toBe(true); expect(page.pageInfo.hasNextPage).toBe(false); - // Verify the order is maintained correctly with forward pagination too - const forwardPage = await PostgresTestEntity.knexLoader(vc).loadPageAsync({ - first: 3, + // Get previous page using cursor + const previousPage = await PostgresTestEntity.knexLoader(vc).loadPageAsync({ + last: 3, + before: page.pageInfo.startCursor!, pagination: { strategy: PaginationStrategy.STANDARD, orderBy: [{ fieldName: 'name', order: OrderByOrdering.DESCENDING }], }, }); - // First 3 in descending order - expect(forwardPage.edges).toHaveLength(3); - expect(forwardPage.edges[0]?.node.getField('name')).toBe('Z_Item_5'); - expect(forwardPage.edges[1]?.node.getField('name')).toBe('Z_Item_4'); - expect(forwardPage.edges[2]?.node.getField('name')).toBe('Z_Item_3'); - expect(forwardPage.pageInfo.hasNextPage).toBe(true); - expect(forwardPage.pageInfo.hasPreviousPage).toBe(false); + // Remaining items in descending order: Z_Item_5, Z_Item_4 + expect(previousPage.edges).toHaveLength(2); + expect(previousPage.edges[0]?.node.getField('name')).toBe('Z_Item_5'); + expect(previousPage.edges[1]?.node.getField('name')).toBe('Z_Item_4'); + expect(previousPage.pageInfo.hasPreviousPage).toBe(false); + expect(previousPage.pageInfo.hasNextPage).toBe(false); }); it('always includes ID field in orderBy for stability', async () => { diff --git a/packages/entity-database-adapter-knex/src/internal/EntityKnexDataManager.ts b/packages/entity-database-adapter-knex/src/internal/EntityKnexDataManager.ts index 6e28d0318..32741617e 100644 --- a/packages/entity-database-adapter-knex/src/internal/EntityKnexDataManager.ts +++ b/packages/entity-database-adapter-knex/src/internal/EntityKnexDataManager.ts @@ -131,12 +131,11 @@ const CURSOR_ROW_TABLE_ALIAS = 'cursor_row'; interface PaginationProvider, TIDField extends keyof TFields> { whereClause: SQLFragment | undefined; - buildOrderBy: (direction: PaginationDirection) => { - clauses?: readonly PostgresOrderByClause[]; - }; + buildOrderBy: (direction: PaginationDirection) => readonly PostgresOrderByClause[]; buildCursorCondition: ( decodedCursorId: TFields[TIDField], direction: PaginationDirection, + orderByClauses: readonly PostgresOrderByClause[], ) => SQLFragment; } @@ -265,6 +264,7 @@ export class EntityKnexDataManager< if (pagination.strategy === PaginationStrategy.STANDARD) { // Standard pagination EntityKnexDataManager.validateOrderByClauses(pagination.orderBy); + EntityKnexDataManager.validateOrderByClausesHaveConsistentDirection(pagination.orderBy); const idField = this.entityConfiguration.idField; const augmentedOrderByClauses = this.augmentOrderByIfNecessary(pagination.orderBy, idField); @@ -277,7 +277,7 @@ export class EntityKnexDataManager< // Create strategy for regular pagination const strategy: PaginationProvider = { whereClause: where, - buildOrderBy: (direction) => { + buildOrderBy: (direction: PaginationDirection) => { // For backward pagination, we flip the ORDER BY and NULLS direction to fetch records // in reverse order. This allows us to use a simple "less than" cursor comparison // instead of complex SQL. We'll reverse the results array later to restore @@ -286,25 +286,30 @@ export class EntityKnexDataManager< // 1. Flip to name DESC to get the last items first // 2. Apply cursor with < comparison // 3. Reverse the final array to present items in name ASC order - const clauses = - direction === PaginationDirection.FORWARD - ? augmentedOrderByClauses - : augmentedOrderByClauses.map( - (clause): PostgresOrderByClause => ({ - ...clause, - order: - clause.order === OrderByOrdering.ASCENDING - ? OrderByOrdering.DESCENDING - : OrderByOrdering.ASCENDING, - nulls: clause.nulls - ? EntityKnexDataManager.flipNullsOrderingSpread(clause.nulls) - : undefined, - }), - ); - return { clauses }; + return direction === PaginationDirection.FORWARD + ? augmentedOrderByClauses + : augmentedOrderByClauses.map( + (clause): PostgresOrderByClause => ({ + ...clause, + order: + clause.order === OrderByOrdering.ASCENDING + ? OrderByOrdering.DESCENDING + : OrderByOrdering.ASCENDING, + nulls: clause.nulls + ? EntityKnexDataManager.flipNullsOrderingSpread(clause.nulls) + : undefined, + }), + ); + }, + buildCursorCondition: (decodedCursorId, _direction, orderByClauses) => { + // all clauses are guaranteed to have the same order due to validation, so we can just look at the first one for effective ordering + const effectiveOrdering = orderByClauses[0]?.order ?? OrderByOrdering.ASCENDING; + return this.buildCursorCondition( + decodedCursorId, + fieldsToUseInPostgresTupleCursor, + effectiveOrdering, + ); }, - buildCursorCondition: (decodedCursorId, direction) => - this.buildCursorCondition(decodedCursorId, fieldsToUseInPostgresTupleCursor, direction), }; return await this.loadPageInternalAsync(queryContext, args, strategy); @@ -333,19 +338,17 @@ export class EntityKnexDataManager< const strategy: PaginationProvider = { whereClause, buildOrderBy: (direction) => { - const clauses = - direction === PaginationDirection.FORWARD - ? searchOrderByClauses - : searchOrderByClauses.map( - (clause): DistributiveOmit, 'nulls'> => ({ - ...clause, - order: - clause.order === OrderByOrdering.ASCENDING - ? OrderByOrdering.DESCENDING - : OrderByOrdering.ASCENDING, - }), - ); - return { clauses }; + return direction === PaginationDirection.FORWARD + ? searchOrderByClauses + : searchOrderByClauses.map( + (clause): DistributiveOmit, 'nulls'> => ({ + ...clause, + order: + clause.order === OrderByOrdering.ASCENDING + ? OrderByOrdering.DESCENDING + : OrderByOrdering.ASCENDING, + }), + ); }, buildCursorCondition: (decodedCursorId, direction) => search.strategy === PaginationStrategy.TRIGRAM_SEARCH @@ -353,7 +356,10 @@ export class EntityKnexDataManager< : this.buildCursorCondition( decodedCursorId, fieldsToUseInPostgresTupleCursor, - direction, + // ILIKE search always orders ASC, so effective ordering matches direction + direction === PaginationDirection.FORWARD + ? OrderByOrdering.ASCENDING + : OrderByOrdering.DESCENDING, ), }; @@ -410,17 +416,21 @@ export class EntityKnexDataManager< // Decode cursor const decodedExternalCursorEntityID = cursor ? this.decodeOpaqueCursor(cursor) : null; + // Get ordering from strategy + const orderByClauses = paginationProvider.buildOrderBy(direction); + // Build WHERE clause with cursor condition const baseWhere = paginationProvider.whereClause; const cursorCondition = decodedExternalCursorEntityID - ? paginationProvider.buildCursorCondition(decodedExternalCursorEntityID, direction) + ? paginationProvider.buildCursorCondition( + decodedExternalCursorEntityID, + direction, + orderByClauses, + ) : null; const whereClause = this.combineWhereConditions(baseWhere, cursorCondition); - // Get ordering from strategy - const { clauses: orderByClauses } = paginationProvider.buildOrderBy(direction); - // Determine query modifiers const queryModifiers: PostgresQuerySelectionModifiers = { ...(orderByClauses !== undefined && { orderBy: orderByClauses }), @@ -491,7 +501,9 @@ export class EntityKnexDataManager< // if ID is already included as a fragment, but that is preferable to the risk of incorrect pagination behavior. const hasId = clauses.some((spec) => 'fieldName' in spec && spec.fieldName === idField); if (!hasId) { - return [...clauses, { fieldName: idField, order: OrderByOrdering.ASCENDING }]; + const lastClauseOrder = + clauses.length > 0 ? clauses[clauses.length - 1]!.order : OrderByOrdering.ASCENDING; + return [...clauses, { fieldName: idField, order: lastClauseOrder }]; } return clauses; } @@ -519,6 +531,24 @@ export class EntityKnexDataManager< } } + /** + * Cursor-based pagination uses Postgres tuple comparison (e.g., (a, b) \> (x, y)) which + * applies a single comparison direction to all columns. Mixed ordering directions would + * produce incorrect pagination results. + */ + private static validateOrderByClausesHaveConsistentDirection>( + orderBy: readonly PostgresOrderByClause[] | undefined, + ): void { + if (orderBy === undefined || orderBy.length <= 1) { + return; + } + const firstOrder = orderBy[0]!.order; + assert( + orderBy.every((clause) => clause.order === firstOrder), + 'All orderBy clauses must have the same ordering direction. Mixed ordering directions are not supported with cursor-based pagination.', + ); + } + private encodeOpaqueCursor(idField: TFields[TIDField]): string { return Buffer.from(JSON.stringify({ id: idField })).toString('base64url'); } @@ -574,14 +604,14 @@ export class EntityKnexDataManager< | SQLFragment | DataManagerSearchFieldSQLFragmentFnSpecification )[], - direction: PaginationDirection, + effectiveOrdering: OrderByOrdering, ): SQLFragment { // We build a tuple comparison for fieldsToUseInPostgresTupleCursor fields of the // entity identified by the external cursor to ensure correct pagination behavior // even in cases where multiple rows have the same value all fields other than id. // If the cursor entity has been deleted, the subquery returns no rows and the // comparison evaluates to NULL, filtering out all results (empty page). - const operator = direction === PaginationDirection.FORWARD ? '>' : '<'; + const operator = effectiveOrdering === OrderByOrdering.ASCENDING ? '>' : '<'; const idField = getDatabaseFieldForEntityField( this.entityConfiguration,