fix(graphql): remove count query if paginationInfo is not requested

[xavierleune]

Q	A
Branch?	current stable version branch for bug fixes
License	MIT

This pull request is a draft to fix a bug discovered on the current graphql implementation.

If the following request is sent:

{
  nameMetricFirstnameDepartmentOccurrences {
    collection {
      geoEntityDepartment {
        label
      }
    }
  }
}

The current implementation executes the count query on the database, to populate paginationInfo in the result until this information is dropped by the method ReferenceExecutor::collectAndExecuteSubFields.
I would expect the count query to be executed only if the paginationInfo is requested, like with the following request:

{
  nameMetricFirstnameDepartmentOccurrences {
    paginationInfo {
      itemsPerPage
      lastPage
      totalCount
    }
    collection {
      geoEntityDepartment {
        label
      }
    }
  }
}

I'm not quite sure that this is the best way to implement that fix, I open this pull request to start the discussion. There is probably some other implementations I should look at (like cursor based pagination, perhaps the NormalizeProcessor) and add some tests. In the meantime what do you think of this approach ?

Thanks,

[xavierleune]

I think this would fix api-platform/api-platform#2310 api-platform/api-platform#1996 and #4764 (after full implementation)

[soyuka]

I like this approach, can you add a test?

[xavierleune]

@soyuka sure, I think this should do the trick, WDYT ?

[xavierleune]

Thanks for your feedback @alanpoulain
As I've now a better understanding of the mechanics here, I think I can go further and offer a better implementation for the partial pagination and solve the bug described in this issue.

I think to control partial pagination in "paged" pagination in graphql, it would make sense to offer a hasNextPage in the paginationInfo. It would be a boolean to indicate if there is more results to the current query (so the client can query the next page).

I think this should be done only with a new interface HasNextPagePaginatorInterface describing only one method: public function hasNextPage(): bool.

So we will have the following paginationInfo available for the client:

paginationInfo {
    totalCount
    lastPage
    itemsPerPage
    hasNextPage
}

Each item may trigger one query to response, only if declared in the client query.

So in the case described in the current issue, no additionnal query would be triggerred:

{
  nameMetricFirstnameDepartmentOccurrences {
    collection {
      geoEntityDepartment {
        label
      }
    }
  }
}

If paginationInfo.totalCount or paginationInfo.lastPage is declared, the count query would be issued. If hasNextPage is declared (and the paginator implements HasNextPagePaginatorInterface) a query to fetch the row +1 would be triggered. A better approach here could be to always fetch itemsPerPage + 1, count the number of rows (and drop the last row if that was an overfetch), but I'm not sure if that's easy or not.

[alanpoulain]

Sure why not, maybe this interface could be used for REST as well. IMO it should be done in another PR though.

[xavierleune]

Sure why not, maybe this interface could be used for REST as well. IMO it should be done in another PR though.

I'll go with another PR for that part ^^

On the main topic of this pull request, beside the falling test (because of php-parser 5.0.0), I think we're good to go. @alanpoulain may you take a look please ?

[xavierleune]

Hi @soyuka @alanpoulain do you have a new feedback on this pull request ? Maybe something I should improve before merging it ? Thanks !

[soyuka]

can you rebase against 3.2 so we get proper coverage?

[xavierleune]

@soyuka It seems that coverage is ok after rebase 👍

[soyuka]

Thanks @xavierleune !

[xavierleune]

@alanpoulain following this pull request, I opened the PR #6120 about the hasNextPagePaginatorInterface