Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

has_previous_page and has_next_page always False while navigating #12

Open
robmoorman opened this issue Sep 5, 2017 · 4 comments · May be fixed by #14
Open

has_previous_page and has_next_page always False while navigating #12

robmoorman opened this issue Sep 5, 2017 · 4 comments · May be fixed by #14

Comments

@robmoorman
Copy link

robmoorman commented Sep 5, 2017

While using the pageInfo hasPreviousPage and hasNextPage cursor based navigation (before and after), it seems these results are always set to False.

The corresponding code (https://github.com/graphql-python/graphql-relay-py/blob/master/graphql_relay/connection/arrayconnection.py#L104-L105):

return connection_type(
    edges=edges,
    page_info=pageinfo_type(
        start_cursor=first_edge_cursor,
        end_cursor=last_edge_cursor,
        has_previous_page=isinstance(last, int) and start_offset > lower_bound,
        has_next_page=isinstance(first, int) and end_offset < upper_bound
    )
)

Querying with first: 5, after: "xxx" in this case always results in hasPreviousPage: false.
The same for last: 5, before: "xxx", which results always in hasNextPage: false.

@sciyoshi sciyoshi linked a pull request Nov 22, 2017 that will close this issue
@sciyoshi
Copy link

Fixed by #14 - @robmoorman could you test to see if that fix works for you?

@robmoorman
Copy link
Author

Hi @sciyoshi regardless the fix you made, it's probably not in the relay spec at all (till today I have no clue why relay put this in the spec).

See https://facebook.github.io/relay/graphql/connections.htm#sec-undefined.PageInfo.Fields

@sciyoshi
Copy link

@robmoorman I'm looking at that spec as well. For example, for hasPreviousPage:

hasPreviousPage is used to indicate whether more edges exist prior to the set defined by the clients arguments. If the client is paginating with last/before, then the server must return true if prior edges exist, otherwise false. 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.

(emphasis mine)

@ipmcc
Copy link

ipmcc commented Apr 24, 2018

I just got bitten by this too. As previously quoted, the spec says (emphasis mine):

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.

By my reading, the current behavior is technically within spec because the behavior is prescribed as "may" and not "must". That said, I suspect that like me, it's very counter-intuitive to consumers, and it cost quite a bit of time and debugging to figure out.

My sense is that this specific PR would dramatically improve the greenfield developer experience, however, I can see how it would likely break existing usages, especially considering that the default value for an unspecified slice_start param is 0. This behavior makes sense for the case where the slice itself begins in the correct place, and where it is not possible to efficiently determine what came before it.

Is there some way this could be reworked to accommodate both cases? For instance what about an optional boolean param that indicates whether the slice_start, list_length, and list_slice_length are known to be correct with respect to the entire dataset? (i.e. start_lengths_valid or something) That seems like it could avoid breaking existing consumers while dramatically reducing confusion for situations where this info is available and known to be valid (which I suspect would include many, many situations backed by traditional relational databases).

open-dynaMIX added a commit to open-dynaMIX/caluma that referenced this issue Jun 3, 2019
This commit implements a fix for
graphql-python/graphql-relay-py#12

The project `graphql-relay-py` seems unmaintained, so there is
little hope that
[this PR](graphql-python/graphql-relay-py#14)
gets merged any time soon.

Closes projectcaluma#469
open-dynaMIX added a commit to open-dynaMIX/caluma that referenced this issue Jun 3, 2019
This commit implements a fix for
graphql-python/graphql-relay-py#12

The project `graphql-relay-py` seems unmaintained, so there is
little hope that
[this PR](graphql-python/graphql-relay-py#14)
gets merged any time soon.

Closes projectcaluma#469
open-dynaMIX added a commit to open-dynaMIX/caluma that referenced this issue Jun 3, 2019
This commit implements a fix for
graphql-python/graphql-relay-py#12

The project `graphql-relay-py` seems unmaintained, so there is
little hope that
[this PR](graphql-python/graphql-relay-py#14)
gets merged any time soon.

Closes projectcaluma#469
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants