graphql-python / graphql-relay-py

A library to help construct a graphql-py server supporting react-relay
MIT License
146 stars 41 forks source link

has_previous_page and has_next_page always False while navigating #12

Open robmoorman opened 7 years ago

robmoorman commented 7 years ago

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 commented 6 years ago

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

robmoorman commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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).