Pagination with "after" cursor results in `ScrollPosition` that skips one extra item

[tata-dr-dk]

Using Graphql Query 'after' option with a cursor of position N, yields a result-set starting from position N+2 upwards, skipping the document right after the cursor that should have been included (N+1).

Using the latest graphql and mongodb starters

	implementation 'org.springframework.boot:spring-boot-starter-data-mongodb'
	implementation 'org.springframework.boot:spring-boot-starter-graphql'

Using graphql @QueryMapping annotation and ScrollSubrange on a controller

    @QueryMapping
    public Window<Customer> customers(@Argument String storeNumber, ScrollSubrange subrange) {
       //...
    }

and a query:

query MyQuery {
  customers(storeNumber: "123", after: "T18x") {
    edges {
      cursor
      node {
        name
        storeNumber
      }
    }
  }
}

where 'T18x' is the cursor of the first document, results in subrange.position() of 2.

Using that position in a mongodb Query:

        var query = Query.query(criteria)
                .with(scrollPosition);

would actually skip 2 documents, and not start from document of position 2 (thus not inclusive).
Seems to be related to #916

A project reproducing the bug can be found here: graphql-bug

[JIngrahamTG]

I've noticed the same issue. Also, the last and before includes the cursor provided as the "before", so last:2 before:"cursor" only includes one node before the cursor as well as the before cursor itself. I am also using the latest released version of spring graphql.

[pax95]

It seems to be a regression from #916.
Downgrading to spring-graphql:1.2.4 solves the pagination problem.

[rstoyanchev]

@tata-dr-dk, thanks for the sample.

Indeed #916 was a breaking change, but it was necessary to fix a problem that had been there from the start, and reverse another that was introduced in 1.2.4. That's pretty confusing I'll admit, but I believe the current behavior is correct.

In the sample, there are 3 customers. Cursor "T18x" decodes to "O_1", which is offset 1. That points to the 2nd customer, and the server correctly returns only the 3rd customer.

If I use cursor "T18w" instead, or "O_0", which is offset 0, that points to the first customer, and the server returns the 2nd and 3rd customers.

only includes one node before the cursor as well as the before cursor itself

If I use last:2 before:"T18x" in same the sample, the resulting ScrollSubrange has offset 0 and count 1 as expected. However, the controller uses 10, in effect ignoring the count, and the server returns all 3 customers. If I change the count to subrange.count().orElse(10), then the server returns only the 1st customer.

[tata-dr-dk]

Thanks @rstoyanchev for your response

I am totally new to GraphQL, but for me there still seems to be a incompatible behavior. I would expect the cursor X returned for any document to be usable in the after clause, which then would result in the next page starting from the document that exactly follows it, e.g. with cursor Y.

I updated the example unit tests with two cases, one using the first returned document.cursor, the other using the pageInfo.endCursor to query the next page. Both seem to show that there is a missing document, and an extra erroneous skip indeed is happening.

It would surprise me if that is indeed the correct behavior. In that case, requesting the next page would require the client to use the cursor with index "pageSize - 1", and the endCursor would not be of any value

The first document cursor returned is T18x and not T18w

[rstoyanchev]

That helps better to demonstrate the issue, thanks.

It looks like OffsetScrollPosition adds 1 to its offset, so when we render cursors starting from 1. However, when the cursor is sent back and used, the repository seems to treat the offset as zero based. I'm not sure if this is expected behavior for Spring Data or not, but I will investigate.

[rstoyanchev]

After some discussions with the Spring Data team, there is now a change planned on their side, see spring-projects/spring-data-commons#3070. After that we will see if we need any follow-up changes here.