[GraphQl] Better pagination support #2142

Reduxx · 2018-08-03T14:24:30Z

Q	A
Bug fix?	yes
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#2139
License	MIT
Doc PR	api-platform/docs#561

This is a WIP Pull Request. It aims to make the pagination support closer to the graphql-relay-js implementation.

Bug Fixes:
pageInfo.endCursor is now the last item of the current page

New Features:
pageInfo.startCursor: new Type for the first item of the current page
queryArguments last and before: the counterpart for first and after

PHPUnit Tests: passing

Behat:

all tests: 5 not passing
GraphQl tests: passing (although there are no tests for startCursor, last and before at the moment)
I haven't figured out why the other tests aren't running properly, I have to check this.

Any help with the tests are appreciated, this is the first time I've ever seen Behat and used prophecies in PHPUnit

I'll open a PR for the docs and finish this up according to the guidelines when I have the time :)

Reduxx · 2018-08-07T13:43:15Z

The phpstan errors seem to be unrelated to this PR, they are also thrown on the base master branch.
I couldn't figure out why it says phpunit-coverage failed, the tests are all passing and circleci says "0 failures" but it has an exit code of 1.

The new behavior now working as intended. PHPUnit and Behat tests are updated to reflect this.

The docs PR is almost ready, just figuring out the wording and spelling. And then I'll finalize this :)

soyuka · 2018-08-07T17:00:46Z

rebasing should do !

Reduxx · 2018-08-08T07:13:59Z

I've rebased my commits, but unfortunately something doesn't seem right :-/

But at least I've included the docs PR api-platform/docs#561 :)

alanpoulain · 2018-08-09T16:05:23Z

src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php

+            if (isset($collectionArgs[$resourceClass]['before'])) {
+                $before = \base64_decode($collectionArgs[$resourceClass]['before'], true);
+                $firstResult = (int) $before - $itemsPerPage;
+                $firstResult = (false === $before) ? 0 : (int) $firstResult;


The cast is not needed.

alanpoulain · 2018-08-09T16:07:24Z

src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php

+                $before = \base64_decode($collectionArgs[$resourceClass]['before'], true);
+                $firstResult = (int) $before - $itemsPerPage;
+                $firstResult = (false === $before) ? 0 : (int) $firstResult;
+                if (false !== $before && 0 > $firstResult) {


I think you can remove the false !== $before part. Since $firstResult will be negative only if $before is not false.

Is this part tested?

This is correct I'll change that.
It is tested in features/graphql/collection.feature starting on line 424

alanpoulain · 2018-08-09T16:25:49Z

src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php

@@ -121,6 +122,19 @@ public function applyToCollection(QueryBuilder $queryBuilder, QueryNameGenerator
                $firstResult = (int) $after;
                $firstResult = false === $after ? $firstResult : ++$firstResult;
            }
+            if (isset($collectionArgs[$resourceClass]['last'])) {


You are doing an extra query inside a building of a query. Maybe there is a better way to do this.

Yeah, that's kinda hacky. It's for the case that before isn't set, so you can get the last items in the correct order. I have to think about another way

I didn't come up with an idea so far :-/ How should we handle this?

I think doing backwards pagination in SQL is tricky and you need to know the total rows the first time in order to do it.
The current implementation needs to be changed though because getArrayResult can be very big and I think it can eat all the memory. You need to use an SQL count.
The Doctrine Paginator is doing it anyway when you get the total items (see its method getCountQuery). You need to do something like this.

alanpoulain · 2018-08-09T16:52:59Z

src/GraphQl/Resolver/Factory/CollectionResolverFactory.php

            }

            foreach ($collection as $index => $object) {
+                if ($index === $collectionLastIndex) {


Why is this needed? The $collectionLastIndex is always the endCursor doesn't it?

You are completly right, it's not needed at all

alanpoulain · 2018-08-09T16:53:23Z

src/GraphQl/Resolver/Factory/CollectionResolverFactory.php

                ];
            }

+            if ($collection instanceof PaginatorInterface && ($totalItems = $collection->getTotalItems()) > 0) {


It should be moved inside the previous if.

Add support for graphql before, last, startcursor

Reduxx · 2018-08-22T09:46:58Z

No further changes, only updated branch to match base branch

alanpoulain · 2018-09-12T11:58:52Z

@Reduxx Could you try to use an SQL count?

alanpoulain · 2019-02-24T14:44:34Z

This PR should be good.
@api-platform/core-team, I have introduced an ArrayIterator in order to have pagination information when returning an array, please tell me what you think of it.

soyuka

LGTM

The ___ field and the hardcoded limit0 disturb me but I guess they're fine?

alanpoulain · 2019-02-25T13:50:01Z

@soyuka I have cherry-picked the commit (f91d3d8) for the limit 0 which uses constants.

meyerbaptiste · 2019-02-25T14:02:23Z

src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php

+        }
+
+        if (isset($context['filters']['last']) && !isset($context['filters']['before'])) {
+            $context['count'] = (new DoctrineOrmPaginator($queryBuilder))->count();


Should I assume that partial pagination is not compatible with Graphql?

Yes. We could make it compatible later.

The code you annotated is only if you are doing backwards pagination though.

src/DataProvider/ArrayPaginator.php

src/GraphQl/Resolver/Factory/CollectionResolverFactory.php

alanpoulain · 2019-02-26T16:31:54Z

This PR misses the hasPreviousPage field. Please do not merge it yet.

alanpoulain · 2019-02-26T17:15:11Z

hasPreviousPage added.

alanpoulain · 2019-02-27T10:49:47Z

Thank @Reduxx 🙂

luca-simonetti · 2019-05-13T12:36:34Z

any plans to have this released soon?

rajasimon · 2021-11-27T03:23:14Z

Need this now. My after cursor resulting in empty result after five iteration. Why this not in production?

Reduxx force-pushed the master branch from 84b8745 to db9f87d Compare August 8, 2018 06:59

Reduxx changed the title ~~[WIP] [GraphQl] Better pagination support~~ [GraphQl] Better pagination support Aug 8, 2018

alanpoulain reviewed Aug 9, 2018

View reviewed changes

Reduxx force-pushed the master branch 2 times, most recently from d2ad79f to 10ef076 Compare August 10, 2018 09:31

Reduxx mentioned this pull request Aug 22, 2018

Adding graphql pagination section api-platform/docs#561

Merged

dunglas approved these changes Aug 22, 2018

View reviewed changes

Better support for graphql pagination

2999de4

Add support for graphql before, last, startcursor

Reduxx force-pushed the master branch from 10ef076 to 2999de4 Compare August 22, 2018 09:44

alanpoulain added [WFC] Waiting for Change GraphQL labels Oct 11, 2018

Merge branch 'master' into master

0a38259

alanpoulain mentioned this pull request Feb 16, 2019

GraphQL pagination seems not working api-platform/api-platform#530

Closed

Merge branch 'master' into better-pagination

7dfcb40

alanpoulain mentioned this pull request Feb 21, 2019

[MongoDB] Allow limit 0 #2549

Merged

alanpoulain force-pushed the master branch 3 times, most recently from 2d4f1b7 to 0160051 Compare February 24, 2019 14:35

alanpoulain added [RFR] Ready for Review and removed [WFC] Waiting for Change labels Feb 24, 2019

alanpoulain force-pushed the master branch 2 times, most recently from 6536a32 to 6141b39 Compare February 24, 2019 15:48

soyuka approved these changes Feb 25, 2019

View reviewed changes

Allow limit 0 for MongoDB

3fc31d2

alanpoulain force-pushed the master branch from 6141b39 to b93335a Compare February 25, 2019 13:47

meyerbaptiste reviewed Feb 25, 2019

View reviewed changes

src/DataProvider/ArrayPaginator.php Outdated Show resolved Hide resolved

meyerbaptiste reviewed Feb 25, 2019

View reviewed changes

src/GraphQl/Resolver/Factory/CollectionResolverFactory.php Outdated Show resolved Hide resolved

alanpoulain force-pushed the master branch from b93335a to fbd2824 Compare February 25, 2019 14:09

Fix backwards pagination

6651085

alanpoulain force-pushed the master branch from fbd2824 to 6651085 Compare February 25, 2019 14:23

Merge branch 'master' into better-pagination

8b738a6

alanpoulain mentioned this pull request Feb 26, 2019

[GraphQL] Fix pagination #2560

Merged

Add hasPreviousPage

b00dcd9

Merge branch 'master' into better-pagination

7a4be7f

alanpoulain merged commit 9eb6662 into api-platform:master Feb 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GraphQl] Better pagination support #2142

[GraphQl] Better pagination support #2142

Reduxx commented Aug 3, 2018 •

edited

Loading

Reduxx commented Aug 7, 2018

soyuka commented Aug 7, 2018

Reduxx commented Aug 8, 2018

alanpoulain Aug 9, 2018

alanpoulain Aug 9, 2018

alanpoulain Aug 9, 2018

Reduxx Aug 10, 2018

alanpoulain Aug 9, 2018 •

edited

Loading

Reduxx Aug 10, 2018

Reduxx Aug 17, 2018 •

edited

Loading

alanpoulain Aug 25, 2018

alanpoulain Aug 9, 2018

Reduxx Aug 10, 2018

alanpoulain Aug 9, 2018

Reduxx commented Aug 22, 2018

alanpoulain commented Sep 12, 2018

alanpoulain commented Feb 24, 2019

soyuka left a comment •

edited

Loading

alanpoulain commented Feb 25, 2019

meyerbaptiste Feb 25, 2019

alanpoulain Feb 25, 2019

alanpoulain Feb 25, 2019

alanpoulain commented Feb 26, 2019

alanpoulain commented Feb 26, 2019

alanpoulain commented Feb 27, 2019

luca-simonetti commented May 13, 2019

rajasimon commented Nov 27, 2021

[GraphQl] Better pagination support #2142

[GraphQl] Better pagination support #2142

Conversation

Reduxx commented Aug 3, 2018 • edited Loading

Reduxx commented Aug 7, 2018

soyuka commented Aug 7, 2018

Reduxx commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanpoulain Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reduxx Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reduxx commented Aug 22, 2018

alanpoulain commented Sep 12, 2018

alanpoulain commented Feb 24, 2019

soyuka left a comment • edited Loading

Choose a reason for hiding this comment

alanpoulain commented Feb 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanpoulain commented Feb 26, 2019

alanpoulain commented Feb 26, 2019

alanpoulain commented Feb 27, 2019

luca-simonetti commented May 13, 2019

rajasimon commented Nov 27, 2021

Reduxx commented Aug 3, 2018 •

edited

Loading

alanpoulain Aug 9, 2018 •

edited

Loading

Reduxx Aug 17, 2018 •

edited

Loading

soyuka left a comment •

edited

Loading