From d4e5d2959c0adbc7d722201eadc113a087b854fd Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Thu, 13 Sep 2018 12:25:35 +0100 Subject: [PATCH] fix(pagination): fixes bug with hasPreviousPage when paginating without unique orderBy (#297) Fixes graphile/postgraphile#844 --- .../graphile-build-pg/src/QueryBuilder.js | 12 +- .../src/queryFromResolveData.js | 207 +++++++++++++++--- .../fixtures/queries/procedure-query.graphql | 10 + .../__snapshots__/queries.test.js.snap | 127 ++++++++++- 4 files changed, 320 insertions(+), 36 deletions(-) diff --git a/packages/graphile-build-pg/src/QueryBuilder.js b/packages/graphile-build-pg/src/QueryBuilder.js index bda1b3240..71df4b491 100644 --- a/packages/graphile-build-pg/src/QueryBuilder.js +++ b/packages/graphile-build-pg/src/QueryBuilder.js @@ -237,9 +237,17 @@ class QueryBuilder { offset(offsetGen: NumberGen) { this.checkLock("offset"); if (this.data.offset != null) { - throw new Error("Must only set offset once"); + // Add the offsets together (this should be able to recurse) + const previous = this.data.offset; + this.data.offset = context => { + return ( + callIfNecessary(previous, context) + + callIfNecessary(offsetGen, context) + ); + }; + } else { + this.data.offset = offsetGen; } - this.data.offset = offsetGen; } first(first: number) { this.checkLock("first"); diff --git a/packages/graphile-build-pg/src/queryFromResolveData.js b/packages/graphile-build-pg/src/queryFromResolveData.js index 0f3ac63b6..fefdd203f 100644 --- a/packages/graphile-build-pg/src/queryFromResolveData.js +++ b/packages/graphile-build-pg/src/queryFromResolveData.js @@ -4,6 +4,7 @@ import * as sql from "pg-sql2"; import type { SQL } from "pg-sql2"; import type { DataForType } from "graphile-build"; import isSafeInteger from "lodash/isSafeInteger"; +import assert from "assert"; const identity = _ => _ !== null && _ !== undefined; @@ -56,45 +57,189 @@ export default ( offset = 0, invert = false ) { - // if invert is true queryHasBefore means queryHasAfter; queryHasFirst means queryHasLast; etc - const sqlCommon = sql.fragment` + /* + * Strap in, 'coz this function gets hairy! + * + * The point of this function is to return SQL which will resolve to a + * boolean true/false depending on whether or not there is a (invert ? + * "previous" : "next") page. + * + * Connections have before, after, first, last and offset. + * - Users are forbidden from setting both first and last. + * - Users are forbidden from setting both offset and last. + * + * Further there are two main modes of paginating, one works by adding a + * where clause (this is preferred, but is not always possible, and is + * indicated by `canHaveCursorInWhere === true`) and the other works using + * standard LIMIT/OFFSET SQL pagination (and is indicated by + * `canHaveCursorInWhere === false`). + * + * The following diagram shows a full collection of records, #, starting at + * START and ending at END. The key after, before, offset, first and last + * variables are shown. One thing not show is that it's possible to have + * fewer records between before and after than requested by first or last. + * Another detail not clearly show is that if there is no `after` then + * `START` is used, similarly if there is no `before` then `END` is used. + * + * #################################################### < collection + * ^ ^<-offset->^<-first->^ ^<-last->^ ^ + * | | | | | | | + * | | +---------+ +--------+ | + * | | | DATA1 DATA2 | | + * | | | | | + * | | | | | + * | | +-------------------------+ | + * | | DATA3 | | + * | after before | + * | | + * START END + * + * We want one of the three DATA blocks: + * + * - If `first` is set, then we want DATA1. + * - If `last` is set then we want DATA2. + * - If neither is set then we want DATA3. + * + * (Remember: you cannot set both `first` and `last` at the same time.) + * + * When invert === false: + * + * Note that both DATA2 and DATA3 end at the same point, and we only care + * if there's data *after* the relevant DATA block, so really we only + * care if the query specified `first` (`queryHasFirst`) which makes + * things complex (ending at the end of DATA1), otherwise we can use + * `before` as the bound (end of DATA2/DATA3). + * + * When invert === true: + * + * Similarly, DATA1 and DATA3 start at the same point, and we're going + * backwards so we only care if there's data *before* the DATA block, so + * really we just need to know if the query set `last` or not, but since + * this is inverted we call it `queryHasFirst`. + * + * When `invert` is false we're calculating `hasNextPage`, when true we're + * calculating `hasPreviousPage`. + * + * Because of the near-symmetry of requesting hasPreviousPage vs + * hasNextPage we always pretend we're determining `hasNextPage`, and we + * just invert everything. + */ + + const sqlCommonUnbounded = sql.fragment` select 1 from ${queryBuilder.getTableExpression()} as ${queryBuilder.getTableAlias()} + `; + /* + * This variable is a fragment to go into an `EXISTS(...)` call (after some tweaks). + * + * The buildWhereClause takes three arguments: + * + * - includeLowerBound (we want this for hasNextPage but not hasPreviousPage) + * - includeUpperBound (we want this for hasPreviousPage but not hasNextPage) + * - options (specifically `{addNullCase}`) - we just pass this through. + * + * So in hasNextPage mode (invert === false), this common SQL ends up + * representing the collection from `(after || START)` onwards with no + * upper bound. In hasPreviousPage mode (invert === true), it represents + * everything from `(before || END)` backwards, with no lower bound. + */ + const sqlCommon = sql.fragment` + ${sqlCommonUnbounded} where ${queryBuilder.buildWhereClause(!invert, invert, options)} `; - if (!queryHasBefore && !queryHasFirst && (!invert || offset === 0)) { - // There can be no next page since there's no upper bound - return sql.literal(false); - } else if (queryHasBefore && (!invert || offset === 0)) { - // Simply see if there are any records after the before cursor - return sql.fragment`exists( - ${sqlCommon} - and not (${queryBuilder.buildWhereBoundClause(invert)}) - )`; - } else if (canHaveCursorInWhere && (!invert || offset === 0)) { - // Query must have "first" - // Drop the limit, see if there are any records that aren't already in the list we've fetched - return sql.fragment`exists( - ${sqlCommon} - and (${queryBuilder.getSelectCursor()})::text not in (select __cursor::text from ${sqlQueryAlias}) - ${offset === 0 ? sql.blank : sql.fragment`offset ${sql.value(offset)}`} - )`; - } else { - if (!invert) { - // Skip over the already known entries, are there any left? + + /* + * Since the offset makes the diagram asymmetric, if offset === 0 + * then the diagram is symmetric and things are simplified a little. + */ + const isForwardOrSymmetric = !invert || offset === 0; + + if (!isForwardOrSymmetric) { + assert(invert); + assert(offset > 0); + // We're looking for a previous page, and there's an offset, so lets just + // assume there's a previous page where offset is smaller. + return sql.literal(true); + } else if (canHaveCursorInWhere) { + assert(isForwardOrSymmetric); + if (!queryHasBefore && !queryHasFirst) { + assert(isForwardOrSymmetric); + // There can be no next page since there's no upper bound + return sql.literal(false); + } else if (queryHasBefore && !queryHasFirst) { + /* + * We invert the upper buildWhereBoundClause to only represent the data + * after `before`, then check if there's at least one record in that set. + * + * This only works if the `before` cursor can be represented in the + * SQL WHERE clause, otherwise we're doing limit/offset pagination + * which requires different logic. It also only works if there's no + * `first` clause, otherwise there could be a next page before the + * `before` clause. + */ + return sql.fragment`exists( + ${sqlCommonUnbounded} + where ${queryBuilder.buildWhereClause(false, false, options)} + and not (${queryBuilder.buildWhereBoundClause(invert)}) + )`; + } else { + assert(queryHasFirst); + // queryHasBefore could be true or false. + /* + * There's a few ways that we could determine if there's a next page. + * + * If !queryHasBefore, we could COUNT(*) the number of rows in + * `sqlCommon` and see if it's larger than `first`: + * `(select count(*) > ${first} from (${sqlCommon}) __random_table_alias__)` + * + * If !queryHasBefore, we could build a subquery table of offsetData + * from sqlCommon and see if it contains any rows: + * `EXISTS(select 1 from (${sqlCommon} OFFSET ${first}) __random_table_alias__)`. + * + * We could see if there's at least one row in sqlCommon that's not + * already in our chosen result set. + * + * We've chosen the latter approach here because it doesn't place a limit + * on queryHasBefore. + */ + // Drop the `first` limit, see if there are any records that aren't + // already in the list we've fetched. return sql.fragment`exists( ${sqlCommon} - offset (select coalesce((select count(*) from ${sqlQueryAlias}), 0) + ${sql.value( - offset - )}) + and (${queryBuilder.getSelectCursor()})::text not in (select __cursor::text from ${sqlQueryAlias}) + ${ + offset === 0 ? sql.blank : sql.fragment`offset ${sql.value(offset)}` + } )`; + } + } else { + assert(!invert || offset === 0); // isForwardOrSymmetric + assert(!canHaveCursorInWhere); + // We're dealing with LIMIT/OFFSET pagination here, which means `natural` + // cursors, so the `queryBuilder` factors the before/after, first/last + // into the limit / offset. + const { limit } = queryBuilder.getFinalLimitAndOffset(); + + if (limit == null) { + // If paginating backwards, then offset > 0 has already been dealt + // with. Unbounded, so there's no next page. + return sql.fragment`false`; + } else if (invert) { + assert(offset === 0); + // Paginating backwards and there's no offset (which factors in before/after), so there's no previous page. + return sql.fragment`false`; } else { - // Things get somewhat more complex here... Let's just assume if offset > 0 there's a previous page. - if (offset > 0) { - return sql.literal(true); - } - // And here (offset === 0 && invert) so we'd have hit an earlier case; since we haven't there must be no previous page. - return sql.literal(false); + assert(!invert); + /* + * We're paginating forwards; either there's a before, there's a first, + * or both. + * + * We want to see if there's more than limit+offset records in sqlCommon. + */ + return sql.fragment`exists( + ${sqlCommon} + offset ${sql.literal(limit + offset)} + )`; } } } diff --git a/packages/postgraphile-core/__tests__/fixtures/queries/procedure-query.graphql b/packages/postgraphile-core/__tests__/fixtures/queries/procedure-query.graphql index 7adc3d95e..a8b6cefd8 100644 --- a/packages/postgraphile-core/__tests__/fixtures/queries/procedure-query.graphql +++ b/packages/postgraphile-core/__tests__/fixtures/queries/procedure-query.graphql @@ -26,6 +26,16 @@ query { tableSetQueryWithOffset4: tableSetQuery(before: "WyJuYXR1cmFsIiw1XQ==", first: 2) { edges { cursor node { name } } pageInfo { startCursor endCursor hasNextPage hasPreviousPage } } tableSetQueryWithOffset5: tableSetQuery(after: "WyJuYXR1cmFsIiwzXQ==", first: 2) { edges { cursor node { name } } pageInfo { startCursor endCursor hasNextPage hasPreviousPage } } tableSetQueryWithOffset6: tableSetQuery(first: 2, offset: 2) { edges { cursor node { name } } pageInfo { startCursor endCursor hasNextPage hasPreviousPage } } + # There's only 6 people, so there should be no next page + tableSetQueryWithOffset7: tableSetQuery(first: 2, offset: 4) { edges { cursor node { name } } pageInfo { startCursor endCursor hasNextPage hasPreviousPage } } + # There should be no previous page + tableSetQueryWithOffset8: tableSetQuery(first: 2, offset: 0) { edges { cursor node { name } } pageInfo { startCursor endCursor hasNextPage hasPreviousPage } } + # There should be no previous or next page + tableSetQueryWithOffset9: tableSetQuery(first: 6, offset: 0) { edges { cursor node { name } } pageInfo { startCursor endCursor hasNextPage hasPreviousPage } } + # Requesting before the first record returns no results, and thus has hasNextPage = false and hasPreviousPage = false as there's no cursors to use for pagination + tableSetQueryWithOffset10: tableSetQuery(before: "WyJuYXR1cmFsIiwxXQ==", last: 2) { edges { cursor node { name } } pageInfo { startCursor endCursor hasNextPage hasPreviousPage } } + # Using after and offset together should not throw + tableSetQueryWithOffset11: tableSetQuery(after: "WyJuYXR1cmFsIiwxXQ==", offset: 1, first: 2) { edges { cursor node { name } } pageInfo { startCursor endCursor hasNextPage hasPreviousPage } } intSetQuery(x: 5, z: 6) { edges { cursor node } } noArgsQuery staticBigInteger { edges { node } } diff --git a/packages/postgraphile-core/__tests__/integration/__snapshots__/queries.test.js.snap b/packages/postgraphile-core/__tests__/integration/__snapshots__/queries.test.js.snap index 17141afe6..f38c55303 100644 --- a/packages/postgraphile-core/__tests__/integration/__snapshots__/queries.test.js.snap +++ b/packages/postgraphile-core/__tests__/integration/__snapshots__/queries.test.js.snap @@ -4117,11 +4117,42 @@ Object { ], "pageInfo": Object { "endCursor": "WyJuYXR1cmFsIiw0XQ==", - "hasNextPage": false, + "hasNextPage": true, "hasPreviousPage": true, "startCursor": "WyJuYXR1cmFsIiw0XQ==", }, }, + "tableSetQueryWithOffset10": Object { + "edges": Array [], + "pageInfo": Object { + "endCursor": null, + "hasNextPage": false, + "hasPreviousPage": false, + "startCursor": null, + }, + }, + "tableSetQueryWithOffset11": Object { + "edges": Array [ + Object { + "cursor": "WyJuYXR1cmFsIiwzXQ==", + "node": Object { + "name": "Budd Deey", + }, + }, + Object { + "cursor": "WyJuYXR1cmFsIiw0XQ==", + "node": Object { + "name": "Kathryn Ramirez", + }, + }, + ], + "pageInfo": Object { + "endCursor": "WyJuYXR1cmFsIiw0XQ==", + "hasNextPage": true, + "hasPreviousPage": true, + "startCursor": "WyJuYXR1cmFsIiwzXQ==", + }, + }, "tableSetQueryWithOffset2": Object { "edges": Array [ Object { @@ -4133,7 +4164,7 @@ Object { ], "pageInfo": Object { "endCursor": "WyJuYXR1cmFsIiw0XQ==", - "hasNextPage": false, + "hasNextPage": true, "hasPreviousPage": true, "startCursor": "WyJuYXR1cmFsIiw0XQ==", }, @@ -4155,7 +4186,7 @@ Object { ], "pageInfo": Object { "endCursor": "WyJuYXR1cmFsIiw0XQ==", - "hasNextPage": false, + "hasNextPage": true, "hasPreviousPage": true, "startCursor": "WyJuYXR1cmFsIiwzXQ==", }, @@ -4226,6 +4257,96 @@ Object { "startCursor": "WyJuYXR1cmFsIiwzXQ==", }, }, + "tableSetQueryWithOffset7": Object { + "edges": Array [ + Object { + "cursor": "WyJuYXR1cmFsIiw1XQ==", + "node": Object { + "name": "Joe Tucker", + }, + }, + Object { + "cursor": "WyJuYXR1cmFsIiw2XQ==", + "node": Object { + "name": "Twenty Seventwo", + }, + }, + ], + "pageInfo": Object { + "endCursor": "WyJuYXR1cmFsIiw2XQ==", + "hasNextPage": false, + "hasPreviousPage": true, + "startCursor": "WyJuYXR1cmFsIiw1XQ==", + }, + }, + "tableSetQueryWithOffset8": Object { + "edges": Array [ + Object { + "cursor": "WyJuYXR1cmFsIiwxXQ==", + "node": Object { + "name": "John Smith", + }, + }, + Object { + "cursor": "WyJuYXR1cmFsIiwyXQ==", + "node": Object { + "name": "Sara Smith", + }, + }, + ], + "pageInfo": Object { + "endCursor": "WyJuYXR1cmFsIiwyXQ==", + "hasNextPage": true, + "hasPreviousPage": false, + "startCursor": "WyJuYXR1cmFsIiwxXQ==", + }, + }, + "tableSetQueryWithOffset9": Object { + "edges": Array [ + Object { + "cursor": "WyJuYXR1cmFsIiwxXQ==", + "node": Object { + "name": "John Smith", + }, + }, + Object { + "cursor": "WyJuYXR1cmFsIiwyXQ==", + "node": Object { + "name": "Sara Smith", + }, + }, + Object { + "cursor": "WyJuYXR1cmFsIiwzXQ==", + "node": Object { + "name": "Budd Deey", + }, + }, + Object { + "cursor": "WyJuYXR1cmFsIiw0XQ==", + "node": Object { + "name": "Kathryn Ramirez", + }, + }, + Object { + "cursor": "WyJuYXR1cmFsIiw1XQ==", + "node": Object { + "name": "Joe Tucker", + }, + }, + Object { + "cursor": "WyJuYXR1cmFsIiw2XQ==", + "node": Object { + "name": "Twenty Seventwo", + }, + }, + ], + "pageInfo": Object { + "endCursor": "WyJuYXR1cmFsIiw2XQ==", + "hasNextPage": false, + "hasPreviousPage": false, + "startCursor": "WyJuYXR1cmFsIiwxXQ==", + }, + }, "typesQuery": false, }, }