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

Do not break when after is greater than list_length #999

Merged
merged 1 commit into from Jul 9, 2020

Conversation

bellini666
Copy link
Contributor

I added a test that, without the change do graphene_django/fields.py breaks with the traceback described in the issue bellow.

Fix #998

@jkimbo
Copy link
Member

jkimbo commented Jul 2, 2020

Thanks for the raising this issue @bellini666 . I'm wondering if it would be better if an error was raised (one that it is more helpful) rather than silently accepting an after value which is greater than the list length. What's the situation where your client is passing an after value that is greater than the list length?

@bellini666
Copy link
Contributor Author

Hi @jkimbo .

Yeah, I thought of that too, that probably the client should be more careful when passing an after keyword. But on the other hand, if we think of this in terms of python slicing or even sql limit/offset, if you pass something an offset greater than the list/result's length will be an empty list/set and will be silently regarding the "issue".

Another way to look at this is to do what the relay specifications (or the original node implementation) does. If it says to raise an error then we should definitely raise an error =P

@wkoot
Copy link

wkoot commented Jul 6, 2020

I don't think it would be weird to return an empty set as it is the default behaviour for filtering in general.
Say I filter field_Icontains: "non_existant" I would also recieve an empty result, without errors :)
Perhaps it would be useful to return such warnings in a meta field though. Is there anything about it in the spec?

@jkimbo
Copy link
Member

jkimbo commented Jul 9, 2020

Sorry for the delay @bellini666 . I've been thinking about this a lot and I agree with yours and @wkoot argument so I'm going to merge this.

Perhaps it would be useful to return such warnings in a meta field though. Is there anything about it in the spec?
As far as I know there isn't anything in the spec about this.

@jkimbo jkimbo merged commit d50955a into graphql-python:master Jul 9, 2020
@jkimbo
Copy link
Member

jkimbo commented Jul 9, 2020

Released this in v2.11.1

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 this pull request may close these issues.

"Negative indexing is not supported" after changes to relay
3 participants