[FEATURE] Add pagination metadata

I'm wondering whether there are many usecases to go to the last page rather than just to the next one. If you limit to have just one additional page you can get rid of the often really expensive count query by fetching N+1 items and show a next link in case there is a (N+1)th result.

Also, @dawehner do you think that knowing the number of pages for a list is not critical?

Well, for a classical desktop application this might be, but for most mobile applications you don't have pagers where you need to click, but rather have some sort of scrolling, or at least just a next button. A classical pager is hard to use on a phone. One thing I consider an important feature is the use of a total count, but this is not needed for every application. Can we somehow add an option to include the total amount of items, when people want to? In which case we have the total number, we could expose the last link, as it doesn't cost us much more.

This was just me thinking out loud for a while.

Cool, yeah of course. In core we decided to go with minipagers by default, so sites scaling up don't run into issues at some point.

1. +++ b/src/Context/CurrentContextInterface.php
   @@ -65,25 +65,4 @@ interface CurrentContextInterface {
   -   */
   -  public function hasExtension($extension_name);
   -
   -  /**
   -   * Returns a list of requested extensions.
   -   *
   

   Let's ensure to not remove that when we commit

2. +++ b/src/EntityCollectionInterface.php
   @@ -0,0 +1,37 @@
   +
   +  /**
   +   * Sets the has next page flag.
   +   *
   +   * @param bool $has_next_page
   +   *   TRUE if the collection has a next page.
   +   */
   +  public function setHasNextPage($has_next_page);
   +
   

   We should document when to use this setter. This seems not entirely obvious

3. +++ b/src/LinkManager/LinkManager.php
   @@ -14,26 +18,137 @@ use Symfony\Component\HttpFoundation\Request;
   +  public function getRequestLink(Request $request, $query = NULL) {
   +    $query = $query ?: (array) $request->query->getIterator();
   +    $result = $this->router->matchRequest($request);
   +    $route_name = $result[RouteObjectInterface::ROUTE_NAME];
   +    /* @var \Symfony\Component\HttpFoundation\ParameterBag $raw_variables */
   +    $raw_variables = $result['_raw_variables'];
   +    $route_parameters = $raw_variables->all();
   +    $options = [
   +      'absolute' => TRUE,
   +      'query' => $query,
   +    ];
   +    return $this->urlGenerator->generateFromRoute($route_name, $route_parameters, $options);
   +  }
   

   What about using really simply Url::fromRoute('<current>', [], ['query' => $query]) In that case we don't even have to pass along the request.

4. +++ b/src/LinkManager/LinkManager.php
   @@ -14,26 +18,137 @@ use Symfony\Component\HttpFoundation\Request;
   +            'offset' => $offset > $size ? $offset - $size : 0,
   

   In those cases I always use max($offset - $size, 0);

5. +++ b/src/Query/OffsetPagerOption.php
   @@ -34,7 +34,7 @@ class OffsetPagerOption implements QueryOptionInterface {
   -    $this->offset = $offset;
   +    $this->offset = $offset ?: 0;
   
   @@ -48,7 +48,11 @@ class OffsetPagerOption implements QueryOptionInterface {
   +    if (isset($this->offset) && isset($this->size)) {
   

   Nitpick: isn't $offset defaulting to 0 already due to the function signature? The fallback and the isset() here disagree a bit though

6. +++ b/src/Query/QueryBuilder.php
   @@ -79,17 +79,53 @@ class QueryBuilder implements QueryBuilderInterface {
   +    // TODO: Explore the possibility to turn JsonApiParam into a plugin type.
   

   this could be interesting

You can pass a NULL as the second argument and then it goes sour. See https://3v4l.org/0Jmte

Ah I see, fair point!

I would kinda prefer clean code over clean unit tests to be honest :) Feel free to keep it of course!

Sure, let's just commit it, once we have a green patch :)

Thank you @e0ipso!
Thank you testbot!

I disagree with #19. While having standard (<< First < Prev 1 2 3 4 5 Next> Last >>) pagination on mobile is not used very often. Any other interface (web, desktop for example) always has pagination with a last button, and multiple pages (<< First < Prev 1 2 3 4 5 Next > Last >>) type interfaces. There are also times on mobile where you want to know the number of items (showing 10 out of 150 products).

Also the JSON spec says the api must return a last link.

If this is a performance issue, is there a way to do the query while ignoring aspects (for example you don't need to consider include, and sort while getting the count), ideally you would just want a SELECT COUNT(*) FROM table WHERE filter/conditions. Filtering and conditions sometimes requires joining, but the database is very good at that. This count number can also be cached. Views already murders the database, I doubt jsonapi counts would add much on top.

Do you guys have any performance differences between having the count there and doing just the N+1?

Added a patch to add last link back in. It's based on the original patch above. If you want to test performance in src/Controller/EntityResource.php you can comment line 268 and add $total_results = 0;

Doing some very basic tests (100 entities with a single filter and includes/sort) the difference is around 15-20 ms on a $5 digital ocean box uncached. Once it is cached (second call) it's 0 to 5 ms.

Ideally I think it should be re-added, it adds consistency with the json api spec.

I'd also like the see the total_count exposed in some sort of meta way, so people can do things like (showing 10 of 153 products) and stuff like that.

If you want me to add in the tests (this patch is missing them) let me know and I will.

@dubcanada just as an FYI there is a hard coded limit of 50 results per request in the json api module.

So doing your tests against 100 entities might be misleading.

( see issue: https://www.drupal.org/node/2793233 )

@e0ipso should we add this to json_extras? I'm not sure how best to proceed with this. Or maybe just a module within jsonapi?

Ideally we could expose all pagination meta data, including total results, etc, not just last page link.

@dubcanada I found one mistake in your patch - in [offset] it returned page number instead of count of items that must be skipped. So I attached fixed patch to my comment

@dubcanada, I found one more error in your patch and fixed it in attached file

Sending the patch to automated testing.

The attached patch builds on top of the existing patch work from #2746939-44: [FEATURE] Add pagination metadata to expose the already calculated result count to the meta section as discussed in #2877041: Provide the query count.

Being the "test launcher" guy the second time in this issue :P

Updated patch to fix a couple failures:

1) In queries using operator grouping such as the test running around line ~173 in Drupal\Tests\jsonapi\Functional\JsonApiFunctionalTest::testRead(), it was possible for options to bleed over into subsequent queries using the same query builder object. I fixed this by wiping the options property at the beginning of Drupal\jsonapi\Query\QueryBuilder::newQuery().
2) I updated the link manager test expectations to account for the total count and last page properties.

Here is the plan for this feature per our conversation in #contenta:

1) ResourceType has a method called "includePageCount()" that returns FALSE always.
2) ConfigurableResourceType overwrites that "includePageCount()" and reads from a global configuration option in jsonapi_extras (like the prefix option).

#1 can be done in jsonapi and tracked on this ticket, #2 needs to be in jsonapi extras (https://www.drupal.org/node/2877041)

The patch attached here gates the include count and "last" meta stuff behind an includeCount method on the resource type.

The tests are passing, but someone should make sure they're testing the right thing.

To actually see this work, you need to get the patch from jsonapi extras so that you can configure it to globally allow include counts.

Wrong patch last time. Here is the correct one.