Fix hasNextPage - revert to count. Fix after #986
Conversation
| # Need first: 4 here to trigger the `has_next_page` logic in graphql-relay | ||
| # See `arrayconnection.py::connection_from_list_slice`: | ||
| # has_next_page=isinstance(first, int) and end_offset < upper_bound |
There was a problem hiding this comment.
IMO this is a bug in graphql-relay. Without first param, you get has_next_page=False even if there are more elements... Sigh.
pcraciunoiu
changed the title
graphene_django/fields.py
Outdated
| after = get_offset_with_default(args.get("after"), -1) + 1 | ||
| list_slice_length += after |
There was a problem hiding this comment.
Without this, graphql-relay v2 slices an empty list here:
Might be fixed in 3.0, but I didn't try that since it might be a rabbit hole.
graphene_django/fields.py
Outdated
| if isinstance(iterable, QuerySet): | ||
| _len = max_limit or iterable.count() | ||
| list_length = iterable.count() | ||
| list_slice_length = max_limit or list_length |
There was a problem hiding this comment.
| list_slice_length = max_limit or list_length | |
| list_slice_length = max(max_limit, list_length) |
With this change you don't need to add the lines on 143 and 144
There was a problem hiding this comment.
I would think we want min? No sense in slicing more than the list has.
There was a problem hiding this comment.
That was my first thought as well but it's max because connection_from_list_slice needs to know what the whole list length is to be able to slice correctly.
There was a problem hiding this comment.
@jkimbo so this doesn't work. It returns 6 results even when the max limit of 4 is set.
There was a problem hiding this comment.
I pushed that so you can see the failing test.
Co-authored-by: Jonathan Kim <jkimbo@gmail.com>
Co-authored-by: Jonathan Kim <jkimbo@gmail.com>
|
Looks like needs to handle `None. Gonna update this a bit later |
|
@jkimbo thoughts on this? |
|
@pcraciunoiu sorry for the delay with this. Looking into it now. |
|
OK @pcraciunoiu I think I've found the solution. You were right you should use If you can either let me push to your branch or cherry pick that commit then we can get this merged. |
|
I tried following this, but I don't see a button to allow. I cherry-picked exactly what you had and left a comment, but I think this makes sense. |
|
@pcraciunoiu I fixed the issue you've mentioned here: #986 (comment) : #993 |
|
@jkimbo awesome! Thanks for taking this one step further! |
This is a follow up fix from #965.
Unfortunately, that broke the logic for
has_next_page(which is still... confusing anyways).This fixes it to perform a count again, since we need the total length to decide if there is a next page. Except for the new test and the changes in
fields.py, the other test changes are a revert, essentially.This should make it possible to do infinite scroll style pagination on
DjangoConnectionFields now!