Convert Django form / DRF decimals to Graphene decimals by ulgens · Pull Request #958 · graphql-python/graphene-django (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation11 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Fixes #91
(Branch name is outdated. I can create a new PR if it's necessary.)
This was referenced
May 11, 2020
#91 is fixed, is this still relevant? What's this about test_should_query_filter_node_limit ?
@zbyte64 I don't think this is still relevant, i'll test and update it asap. The issue with the test, as far as i remember, registering a decimal as string affected incoming data type in pagination, paginator was expecting a decimal value (not sure how and why).
@zbyte64 This is still relevant. #1083 doesn't touch Django form based conversions at all. I'll try to make it work with that new graphene.Decimal type.
Register decimals as string Convert Django form decimals to Graphene decimals
| @@ -671,7 +671,7 @@ def resolve_all_reporters(self, info, **args): |
|---|
| schema = Schema(query=Query) |
| query = """ |
| query NodeFilteringQuery { |
| allReporters(limit: 1) { |
| allReporters(limit: "1") { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zbyte64 I couldn't understand why this change is necessary and i can use some help 🙂
The filter here uses NumberFilter from django-filters and it expects a decimal input. Decimal(1) == Decimal("1") is true but when that input is numeric 1, the test fails with
FAILED graphene_django/filter/tests/test_fields.py::test_should_query_filter_node_limit - assert not [GraphQLError('Argument "limit" has invalid value 1.\nExpected type "Decimal", found 1.')]
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I experimented with adding the following above line 671:
assert Query.all_reporters.filtering_args["limit"].type is Decimal, str(
Query.all_reporters.filtering_args["limit"].type
)
Interestingly this line did not fail. So we know we're generating the right graphene type at least.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is easier to get a stack trace with v3 than v2, still working on this :/
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something interesting happened when I branched and rebased off v3, that particular error went away but then it stopped applying the limit, filter_limit no longer gets called. BUT if I then switch the form conversion back to float the method gets called by django filter. Digging further the args being passed into DjangoFilterConnectoinField.resolve_queryset as limit as None when we convert to Decimal but not Float.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Traced the issue to graphene, I'll be sending them a PR in a moment
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convert Django form decimals to Graphene decimals Convert Django form / DRF decimals to Graphene decimals
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks @ulgens
(this should probably be part of the v3 release since it's a breaking change)
@zbyte64 Why did we just merge it? 🙂 graphene didn't release a v2 version with decimal fix. I was intended to revert 1 -> "1" after that fix.