Fix pagination for all permutations of first, last, before and after

docs/events.rst

@@ -66,14 +66,25 @@ all resolve parameters:
 
 * ``getQueryBuilder`` - Will return a query builder with the user specified
   filters already applied.
-* ``getOffset`` - Will return the offset for the query.  The QueryBuilder passed
-  to the event is not modified with the offset and limit yet.  So if you have a
-  large dataset and need to fetch it within the event, you may use this method
-  to get the offset.
-* ``getLimit`` - Will return the limit for the query.  The QueryBuilder passed
-  to the event is not modified with the offset and limit yet.  So if you have a
-  large dataset and need to fetch it within the event, you may use this method
-  to get the limit.
+* ``getRequestedOffset`` - Will return the offset the client asked for.  The
+  QueryBuilder passed to the event is not modified with the offset and limit
+  yet, so if you have a large dataset and need to fetch it within the event you
+  may use this method to get the offset.
+* ``getRequestedLimit`` - Will return the page size the client asked for, capped
+  by the configured ``limit``.
+
+.. note::
+
+    The event is dispatched **before** the rows are counted so that a listener
+    may modify the QueryBuilder and have that modification reflected in both
+    ``totalCount`` and the rows returned.  The offset and limit are therefore
+    the window the client requested, not the window finally queried.
+
+    A backward request - ``last`` without a ``before`` cursor - reports an
+    offset of zero because its real offset is the row count minus ``last``, and
+    that count has not been taken when the event is dispatched.  Read
+    ``getArgs()['pagination']`` if you need to tell a backward request apart
+    from a request for the first page.
 
 Association QueryBuilder Event
 ==============================
@@ -131,8 +142,8 @@ The ``QueryBuilder`` event for associations has the same methods as the
 QueryBuilder event for entity queries (see above):
 
 * ``getQueryBuilder`` - Returns a QueryBuilder with user-specified filters already applied
-* ``getOffset`` - Returns the offset for the query
-* ``getLimit`` - Returns the limit for the query
+* ``getRequestedOffset`` - Returns the offset the client asked for
+* ``getRequestedLimit`` - Returns the page size the client asked for
 * Plus getters for all resolve parameters (getSource, getArgs, getContext, getInfo)
 
 Performance Benefits

docs/queries.rst

@@ -195,6 +195,36 @@ For the previous page, you would add the startCursor from the current page as th
     }
   }
 
+Combining the arguments
+^^^^^^^^^^^^^^^^^^^^^^^
+
+The four arguments narrow the same range and may be combined freely.  The
+range starts as every row the query matches, then
+
+* ``after`` moves the start of the range past the cursor it names;
+* ``before`` moves the end of the range to the cursor it names;
+* ``first`` moves the end of the range to ``first`` rows after the start;
+* ``last`` moves the start of the range to ``last`` rows before the end.
+
+No argument is discarded when another is present, so
+``{ after: "cursor", before: "cursor" }`` returns the rows between the two
+cursors.  A range which cannot match a row, such as
+``{ before: "<the first cursor>" }``, returns an empty ``edges`` list rather
+than a full page.  An empty page has no first or last node, so
+``pageInfo.startCursor`` and ``pageInfo.endCursor`` are both null.
+
+The configured ``limit`` remains a hard cap.  A request for more rows than the
+limit allows is truncated: a forward request keeps the start of its range and a
+backward (``last``) request keeps the end.
+
+Invalid arguments
+^^^^^^^^^^^^^^^^^
+
+``first`` and ``last`` must be non-negative integers and ``after`` and
+``before`` must be cursors taken from a previous result.  A negative count or a
+cursor which cannot be decoded is reported to the client as a GraphQL error
+rather than silently ignored.
+
 .. role:: raw-html(raw)
    :format: html
 

docs/upgrade.rst

@@ -2,6 +2,103 @@
 Upgrade from previous versions
 ==============================
 
+13.0 to 13.2
+============
+
+Version 13.2 corrects the handling of the pagination arguments.  Every
+combination of ``first``, ``last``, ``before`` and ``after`` is now well
+defined and no argument is discarded when another is present.
+
+Behaviour changes
+-----------------
+
+**Cursors for index zero are no longer read as absent arguments**
+
+``{ before: "<the first cursor>" }`` previously returned a full page.  It now
+returns an empty ``edges`` list, because no row precedes the first one.
+Combined with ``last`` it previously returned the rows at the *end* of the
+result set; it now returns nothing.
+
+**Arguments are no longer discarded**
+
+``{ after: "...", before: "..." }`` previously ignored ``before`` and
+``{ last: n, after: "..." }`` previously ignored ``after``.  Both arguments now
+narrow the range.  ``{ first: n, before: "..." }`` now returns the *first* ``n``
+rows before the cursor rather than the last ``n``.
+
+**A zero count returns no rows**
+
+``{ first: 0 }`` previously returned a full page.  It now returns an empty
+``edges`` list.
+
+**Invalid arguments are reported**
+
+A negative ``first`` or ``last``, or a cursor which cannot be decoded, was
+previously coerced to something harmless and silently applied.  It is now
+reported to the client as a GraphQL error of type
+``ApiSkeletons\Doctrine\ORM\GraphQL\Exception\Pagination``.
+
+**A ``last`` larger than the result set no longer fails**
+
+``{ last: 1000 }`` against a shorter collection previously produced
+``Offset must be a positive integer or zero`` from Doctrine.  It now returns
+every row.
+
+Schema change
+-------------
+
+``PageInfo.startCursor`` and ``PageInfo.endCursor`` are now nullable ``String``
+rather than ``String!``, matching the GraphQL Complete Connection Model.  They
+are null when ``edges`` is empty.  Regenerate any client types built from the
+schema.  A client which only feeds the cursors back as ``after`` or ``before``
+needs no change.
+
+Event change
+------------
+
+``Event\QueryBuilder::getOffset()`` and ``getLimit()`` are renamed to
+``getRequestedOffset()`` and ``getRequestedLimit()``.
+
+The event is dispatched before the rows are counted so a listener can modify
+the QueryBuilder, so these values are the window the client requested rather
+than the window finally queried.  A backward request - ``last`` without a
+``before`` cursor - reports an offset of zero.
+
+**Old (13.1)**:
+
+.. code-block:: php
+
+    function (QueryBuilder $event): void {
+        $offset = $event->getOffset();
+        $limit  = $event->getLimit();
+    }
+
+**New (13.2)**:
+
+.. code-block:: php
+
+    function (QueryBuilder $event): void {
+        $offset = $event->getRequestedOffset();
+        $limit  = $event->getRequestedLimit();
+    }
+
+``PaginationService``
+---------------------
+
+The shared pagination service changed shape.  Nothing else in the library calls
+it, but if you use it directly:
+
+* ``decodePaginationFields()`` returns ``int|null`` per field instead of ``int``
+  so that an absent argument is distinct from index zero, and throws
+  ``Exception\Pagination`` on invalid input.
+* ``calculateOffsetAndLimit()`` takes ``int $itemCount`` as a required third
+  argument instead of an optional nullable one.
+* ``buildCursors()`` takes ``(int $offset, int $resultCount)``; the ``$itemCount``
+  argument is gone and the returned array has ``start`` and ``end`` keys only.
+* ``buildPaginationResponse()`` takes a fourth argument, ``int $offset``.
+* ``calculateRequestedOffsetAndLimit()`` is new and produces the values the
+  QueryBuilder event carries.
+
 12.x to 13.x
 ============
 

src/Event/QueryBuilder.php

@@ -11,6 +11,11 @@
 
 /**
  * This event is fired when the QueryBuilder is created for an entity
+ *
+ * The event is dispatched before the rows are counted so that a listener may
+ * modify the QueryBuilder and have that modification reflected in both the
+ * total count and the rows returned.  The offset and limit it carries are
+ * therefore the window the client requested, not the window finally queried.
  */
 final class QueryBuilder implements
     HasEventName
@@ -19,8 +24,8 @@ final class QueryBuilder implements
     public function __construct(
         protected readonly string $eventName,
         protected readonly DoctrineQueryBuilder $queryBuilder,
-        protected readonly int $offset,
-        protected readonly int $limit,
+        protected readonly int $requestedOffset,
+        protected readonly int $requestedLimit,
         protected readonly mixed $objectValue,
         protected readonly array $args,
         protected readonly mixed $context,
@@ -39,14 +44,25 @@ public function getQueryBuilder(): DoctrineQueryBuilder
         return $this->queryBuilder;
     }
 
-    public function getOffset(): int
+    /**
+     * The offset the client asked for
+     *
+     * A backward request - `last` without a `before` cursor - reports zero
+     * because its offset depends on a row count which has not been taken when
+     * this event is dispatched.  Read `getArgs()['pagination']` to tell a
+     * backward request from a request for the first page.
+     */
+    public function getRequestedOffset(): int
     {
-        return $this->offset;
+        return $this->requestedOffset;
     }
 
-    public function getLimit(): int
+    /**
+     * The page size the client asked for, capped by the configured limit
+     */
+    public function getRequestedLimit(): int
     {
-        return $this->limit;
+        return $this->requestedLimit;
     }
 
     public function getObjectValue(): mixed

src/Exception/Pagination.php

@@ -0,0 +1,22 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ApiSkeletons\Doctrine\ORM\GraphQL\Exception;
+
+use GraphQL\Language\AST\Node;
+use Throwable;
+
+/**
+ * Thrown when a pagination argument is invalid
+ */
+final class Pagination extends GraphQL
+{
+    public function __construct(
+        string $message,
+        Node|null $node = null,
+        Throwable|null $previous = null,
+    ) {
+        parent::__construct($message, $node, null, [], null, $previous);
+    }
+}