hasPreviousPage returns true incorrectly

[wsaca]

Hi, I have a problem where hasPreviousPage returns true in the following query:

query Applications {
    applications {
        edges {
            cursor
            node {
                id
                name
            }
        }
        pageInfo {
            startCursor
            hasPreviousPage
            hasNextPage
            endCursor
        }
    }
}

The specification says:

HasPreviousPage(allEdges, before, after, first, last)

If last is set:

• Let edges be the result of calling (allEdges, before, after).

• If edges contains more than last elements return true, otherwise false.

  If after is set:

• If the server can efficiently determine that elements exist prior to after, return true.

  Return false.

I thought the problem was on this line:

• https://github.com/spring-projects/spring-graphql/blob/2a99c1ffa0251b76c95f242a181acb638be102fb/spring-graphql/src/main/java/org/springframework/graphql/data/query/WindowConnectionAdapter.java#L56

and I reported the problem in spring data but the team closed my issue with an answer:

• https://github.com/spring-projects/spring-data-jpa/issues/3180

[rstoyanchev]

Thanks for the report.

Under https://github.com/spring-projects/spring-data-jpa/issues/3180 you show invoking the repository with a ScrollPosition. What is the actual position passed since the query doesn't have any arguments like after or before, and have you tried calling the repository without a position for this query?

I think with keyset scrolling, when there is a non-empty keyset, there may be more items in the overall result set that come ahead of that keyset, and there isn't an easy way to know if there are previous items because the query returns only items from the keyset forward. The only way to be certain time, with forward pagination at least, is when the keyset is empty, then we now it's the very first page, and that's how KeysetScrollPosition#isInitial() is implemented.

[rstoyanchev]

One more thought. The spec says this for hasPreviousPage:

If the client is paginating with first/after, then the client may return true if edges prior to after exist, if it can do so efficiently, otherwise may return false.

So for forward pagination we can always return false, and maybe we need to improve that code in WindowConnectionAdapter so that if the pagination is forward and keyset scrolling is in use, then we always return false.

If the client is paginating with last/before, then the server must return true if prior edges exist, otherwise false.

For backward pagination however, maybe we could do better. @mp911de any thoughts on that? It seems like for backwards pagination it would be possible to know if there are more to go which would be hasPreviousPage.

[rstoyanchev]

And of course the same is also true in reverse for hasNextPage. When paginating forward, we should know if there are more windows, but not when paginating backward and again in that case the spec allows us to always return false.

[wsaca]

My code is simple and I didn't try excluding the position:

    @QueryMapping
    public Window<Application> applications(ScrollSubrange subrange, Sort sort) {
        int count = subrange.count().orElse(20);
        KeysetScrollPosition defaultPosition =
                subrange.forward() ? ScrollPosition.keyset() : ScrollPosition.keyset().backward();
        ScrollPosition position = subrange.position().orElse(defaultPosition);
        return applicationRepository.findAllBy(Limit.of(count), position, sort);
    }

I have a custom implementation with support to use connections with dataloaders, what I'm trying to do is use keyset scrolling support in spring graphql for non nested connections.

If for example the database has 20 records these cases should work:

1. when first: 50 then hasPreviousPage should return false.
2. when first: 50 and after: cursor-value then hasPreviousPage should return true ("if edges prior to after exist, if it can do so efficiently").

Here is what I do to get a right value:

• hasNextPage: increment by 1 the number of records requested by the user (like spring data).
• hasPreviousPage: add a subquery in the select clause.

Maybe you have a better solution for hasPreviousPage.

[mp911de]

Basically, what Rossen suggested.

We should consider the scroll direction when providing has previous/has next pagination details. From forward-scrolling, you cannot know, whether there is a previous page and likewise, from backwards scrolling, you cannot know whether there is a next page because we do not have information on whether there is data before the position you've started from.

For offset-based queries, we could indeed optimize and check whether the offset is zero, but that would handle only a very specific case while adding some complexity.

[rstoyanchev]

I've created #843 to fix the behavior of hasNext/hasPrevious with keyset scrolling. After the change hasPrevious will always return false with forward-scroll, and likewise hasNext will always return false with backward-scroll.

This is spec compliant and about as much as we can do.