Supports keeping the cursors for pagination #1915

sroze · 2018-05-01T17:28:09Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	ø
License	MIT
Doc PR	ø

The background of this pull-request is explained in the following (public but non-published) blog post:
https://medium.com/@sroze/cursor-based-pagination-with-apiplatform-74fd1d324723

This is the first step, allowing to manually use the filters to achieve a cursor-based pagination. A follow-up PR will come with an obfuscation mechanism if this one goes through :)

sroze · 2018-05-02T08:11:15Z

(CircleCI failure in unrelated by the way)

sroze · 2018-05-02T08:12:41Z

src/Hydra/Serializer/PartialCollectionViewNormalizer.php

+            $lastObject = end($objects);
+
+            if (false !== $lastObject) {
+                $data['hydra:view']['@id'] = IriHelper::createIri($parsed['parts'], $parsed['parameters']);


This... doesn't require the $lastObject to be not null

soyuka · 2018-05-02T08:16:33Z

(CircleCI failure in unrelated by the way)

Issue with the merging stuff. Just rebase should be fine on master since #1917 .

antograssiot · 2018-05-02T07:47:04Z

src/Hydra/Serializer/PartialCollectionViewNormalizer.php

    private $pageParameterName;
    private $enabledParameterName;

-    public function __construct(NormalizerInterface $collectionNormalizer, string $pageParameterName = 'page', string $enabledParameterName = 'pagination')
+    public function __construct(NormalizerInterface $collectionNormalizer, ResourceMetadataFactoryInterface $resourceMetadataFactory, string $pageParameterName = 'page', string $enabledParameterName = 'pagination')


new arguments should have a default value according to SF BC promise IIRC

Well, if we apply the BC policy here I shouldn't change the argument order either 😄

Indeed, we can't do that :)

antograssiot · 2018-05-02T08:17:02Z

src/Hydra/Serializer/PartialCollectionViewNormalizer.php

@@ -71,12 +75,40 @@ public function normalize($object, $format = null, array $context = [])
            return $data;
        }

+        $metadata = isset($context['resource_class']) ? $this->resourceMetadataFactory->create($context['resource_class']) : null;
+        $isPaginatedWithCursor = $paginated && null !== $metadata && null !== $cursorPaginationAttribute = $metadata->getAttribute('pagination_via_cursor');


what about using getCollectionOperationAttribute() instead of getAttribute() ?
isn't collection_operation_name key set in the context ?

bendavies · 2018-05-02T08:48:10Z

tests/Fixtures/TestBundle/Entity/SoMany.php

+ * @ORM\Entity
+ * @ApiResource(attributes={
+ *     "pagination_partial"=true,
+ *     "pagination_via_cursor"={


what about supporting multiple fields? this is often how it is done in postgres...
SELECT * FROM table WHERE (date, id) < (prev_date, prev_id) ORDER BY date DESC, id DESC LIMIT 10;

I was thinking about adding more later but I think you're right, we can start with multiple.

Multiple fields added 👍

Rather than just supporting fields, it might be needed. Unlike the sorting for a regular pagination, the cursor must be unique. Although not efficient performance-wise, the safest way to achieve this is to always append (when necessary as it requires what fields are already present) a sort by ID as a last order by.

I also think you need to account for the NULL values as they might mess up the order

bendavies · 2018-05-02T08:48:46Z

tests/Fixtures/TestBundle/Entity/SoMany.php

+ *     }
+ * })
+ *
+ * @ApiFilter(RangeFilter::class, properties={"id"})


so these filters must match the pagination_via_cursor. should there be a check for that?

Actually, shouldn't we put these filters automatically based on the pagination_via_cursor configuration?

Yes. Think so!

I've looked at it, will do that in another PR while refactoring the pagination actually.

dunglas · 2018-05-02T21:25:58Z

src/Hydra/Serializer/PartialCollectionViewNormalizer.php

+            $forwardRangeOperator = 'desc' === strtolower($cursorPaginationAttribute['direction']) ? 'lt' : 'gt';
+            $backwardRangeOperator = 'gt' === $forwardRangeOperator ? 'lt' : 'gt';
+
+            $accessor = PropertyAccess::createPropertyAccessor();


You should reuse the existing service if it exists (and create it only if null is passed in the constructor). This way we'll benefit from the component's cache.

sroze · 2018-05-12T14:53:26Z

Status: Needs review

soyuka · 2018-05-13T02:42:50Z

src/Hydra/Serializer/PartialCollectionViewNormalizer.php

@@ -71,12 +78,29 @@ public function normalize($object, $format = null, array $context = [])
            return $data;
        }

+        $metadata = isset($context['resource_class']) && null !== $this->resourceMetadataFactory ? $this->resourceMetadataFactory->create($context['resource_class']) : null;
+        $isPaginatedWithCursor = $paginated && null !== $metadata && null !== $cursorPaginationAttribute = $metadata->getCollectionOperationAttribute($context['collection_operation_name'], 'pagination_via_cursor', null, true);


I find these lines a bit long but apart from that 👍

If I split them, it will be longer (i.e. repeating these metadata !== null) 🙊

Could be extracted into a private method?

meyerbaptiste · 2018-05-14T09:26:52Z

What about HAL (ApiPlatform\Core\Hal\Serializer\CollectionNormalizer)?

theofidry · 2018-05-14T09:58:23Z

src/Hydra/Serializer/PartialCollectionViewNormalizer.php

-        if ($paginated) {
+        if ($isPaginatedWithCursor) {
+            $objects = iterator_to_array($object);
+            $firstObject = current($objects);


this will fail if there is no object

This will return false if there is no object and this is tested below, isn't it?

yup, what @meyerbaptiste said

sroze · 2018-05-14T16:47:50Z

What about HAL (ApiPlatform\Core\Hal\Serializer\CollectionNormalizer)?

Whoever needs it will add it? 🤷‍♂️

bendavies · 2018-05-14T17:05:20Z

it would be nice if all output formats support a major feature like this. otherwise it sets a precedent that contributors only add features to the formats they use, which will result in not great framework caveats of "Awesome Feature XXXX (only supported for JSON API)"

sroze · 2018-05-14T19:14:51Z

@bendavies I do agree with you. Do you want to add HAL support in this PR? 😉

bendavies · 2018-05-14T19:33:10Z

i could but it would not be for a few weeks. currently on vacation

teohhanhui · 2018-05-22T15:35:45Z

About the format support, on the other hand, we would not want to discourage contribution because of the need to support all formats? And sometimes it's just better for people who are actually familiar with the format / who actually use it to add the feature.

teohhanhui · 2018-05-22T15:38:40Z

"Awesome Feature XXXX (only supported for JSON API)"

Only supported for JSON-LD / Hydra is acceptable to me, as it's the default / recommended(?) format. But not for the other formats. 😛

trsteel88 · 2018-07-06T02:06:58Z

This PR doesn't support duplicate values for something you are sorting by. e.g if you are sorting by "createdAt" and there are multiple entities it needs to check for a backup field like id.

Query would be something like:

entity.createdAt > [DATE from first/last collection] or (entity.createdAt = [DATE from first/last collection] and entity.id > [FIRST OR LAST ID OF COLLECTION])

direction of the > is going to change depending on which way you are going in the pagination.

bendavies · 2018-07-06T08:56:44Z

it supports multiple fields. it's up to the developer to use the fields they need to do the sorting.

sroze · 2018-08-28T17:35:16Z

What do you expect as a change in here except fixing the merge conflicts? Can we go ahead without all the formats?

dunglas · 2018-08-28T17:43:43Z

Let’s go ahead

sroze · 2018-08-28T17:47:50Z

Rebased on master 👍

dunglas · 2018-08-28T20:21:18Z

src/Hydra/Serializer/PartialCollectionViewNormalizer.php

    {
        $this->collectionNormalizer = $collectionNormalizer;
        $this->pageParameterName = $pageParameterName;
        $this->enabledParameterName = $enabledParameterName;
+        $this->resourceMetadataFactory = $resourceMetadataFactory;
+        $this->propertyAccessor = $propertyAccessor ?: PropertyAccess::createPropertyAccessor();


soyuka · 2018-10-04T10:37:50Z

ping @sroze we'd like to merge this one

bendavies · 2019-01-27T13:20:05Z

Let's get this going again. It's an awesome feature.
I'd like to add hal support too

soyuka · 2019-02-20T14:26:15Z

Thanks for the feature @sroze, I merged this onto master in #2532!

sroze · 2019-02-20T18:59:11Z

My please. Thanks for taking it over the line!

…

On Wed, 20 Feb 2019 at 14:26, Antoine Bluchet ***@***.***> wrote: Closed #1915 <#1915>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1915 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxHEea9E9UHoAXRI1Lei0LPA_HVd4Lnks5vPVsLgaJpZM4TuOwf> .

sroze force-pushed the cursor-pagination branch 2 times, most recently from d3627b1 to c4731df Compare May 1, 2018 17:50

sroze commented May 2, 2018

View reviewed changes

antograssiot reviewed May 2, 2018

View reviewed changes

bendavies reviewed May 2, 2018

View reviewed changes

dunglas requested changes May 2, 2018

View reviewed changes

sroze force-pushed the cursor-pagination branch from 86e3e03 to 8f31842 Compare May 12, 2018 14:48

sroze force-pushed the cursor-pagination branch from 8f31842 to f2d247a Compare May 12, 2018 14:57

soyuka reviewed May 13, 2018

View reviewed changes

theofidry reviewed May 14, 2018

View reviewed changes

sroze added 4 commits August 28, 2018 18:45

Enable the pagination via cursor (no page-based pagination)

9c91cb0

Add tests for the SoMany collection

43c0046

Uses the PropertyAccessor service and don't break the BC

7efd9f8

Allow to use multiple fields

a60b771

sroze force-pushed the cursor-pagination branch from f2d247a to a60b771 Compare August 28, 2018 17:47

dunglas requested changes Aug 28, 2018

View reviewed changes

This was referenced Oct 6, 2018

Is there any plan for supporting cursor based pagination? #1328

Closed

Token Pagination #1713

Closed

soyuka self-assigned this Jan 28, 2019

soyuka mentioned this pull request Feb 19, 2019

Enable the pagination via cursor (no page-based pagination) #2532

Merged

soyuka closed this Feb 20, 2019

Uh oh!

Supports keeping the cursors for pagination #1915

Supports keeping the cursors for pagination #1915

Uh oh!

Conversation

sroze commented May 1, 2018

Uh oh!

sroze commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bendavies May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bendavies May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sroze commented May 12, 2018

Uh oh!

soyuka May 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meyerbaptiste commented May 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sroze commented May 14, 2018

Uh oh!

bendavies commented May 14, 2018

Uh oh!

sroze commented May 14, 2018

Uh oh!

bendavies commented May 14, 2018

Uh oh!

teohhanhui commented May 22, 2018

Uh oh!

teohhanhui commented May 22, 2018

Uh oh!

trsteel88 commented Jul 6, 2018

Uh oh!

bendavies commented Jul 6, 2018

sroze commented May 2, 2018 •

edited

Loading

soyuka commented May 2, 2018 •

edited

Loading

bendavies May 2, 2018 •

edited

Loading

bendavies May 2, 2018 •

edited

Loading

soyuka May 13, 2018 •

edited

Loading