Skip to content

Convert Django form / DRF decimals to Graphene decimals #958

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 10, 2021
Merged

Convert Django form / DRF decimals to Graphene decimals #958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aaa15c8
Convert Django form decimals to Graphene decimals
ulgens Jan 2, 2021
0d92c53
Ugly fix for test_should_query_filter_node_limit
ulgens Jan 2, 2021
c995be9
Convert DRF serializer decimal to Graphene decimal
ulgens Jan 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions graphene_django/filter/tests/test_fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from django.db.models import TextField, Value
from django.db.models.functions import Concat

from graphene import Argument, Boolean, Field, Float, ObjectType, Schema, String
from graphene import Argument, Boolean, Decimal, Field, ObjectType, Schema, String
from graphene.relay import Node
from graphene_django import DjangoObjectType
from graphene_django.forms import GlobalIDFormField, GlobalIDMultipleChoiceField
Expand Down Expand Up @@ -388,7 +388,7 @@ class Meta:
field = DjangoFilterConnectionField(ArticleNode, filterset_class=ArticleIdFilter)
max_time = field.args["max_time"]
assert isinstance(max_time, Argument)
assert max_time.type == Float
assert max_time.type == Decimal
assert max_time.description == "The maximum time"


Expand Down Expand Up @@ -671,7 +671,7 @@ def resolve_all_reporters(self, info, **args):
schema = Schema(query=Query)
query = """
query NodeFilteringQuery {
allReporters(limit: 1) {
allReporters(limit: "1") {
Copy link
Collaborator Author

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

Copy link
Collaborator

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.

Copy link
Collaborator

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 :/

Copy link
Collaborator

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.

Copy link
Collaborator

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should fix the issue we were seeing: graphql-python/graphene#1295

edges {
node {
id
Expand Down
19 changes: 17 additions & 2 deletions graphene_django/forms/converter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,23 @@
from django import forms
from django.core.exceptions import ImproperlyConfigured

from graphene import ID, Boolean, Float, Int, List, String, UUID, Date, DateTime, Time
from graphene import (
Boolean,
Date,
DateTime,
Decimal,
Float,
ID,
Int,
List,
String,
Time,
UUID,
)

from .forms import GlobalIDFormField, GlobalIDMultipleChoiceField
from ..utils import import_single_dispatch


singledispatch = import_single_dispatch()


Expand Down Expand Up @@ -52,6 +63,10 @@ def convert_form_field_to_nullboolean(field):


@convert_form_field.register(forms.DecimalField)
def convert_field_to_decimal(field):
return Decimal(description=field.help_text, required=field.required)


@convert_form_field.register(forms.FloatField)
def convert_form_field_to_float(field):
return Float(description=field.help_text, required=field.required)
Expand Down
16 changes: 8 additions & 8 deletions graphene_django/forms/tests/test_converter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
from django import forms
from py.test import raises

import graphene
from graphene import (
String,
Int,
Boolean,
Date,
DateTime,
Decimal,
Float,
ID,
UUID,
Int,
List,
NonNull,
DateTime,
Date,
String,
Time,
UUID,
)

from ..converter import convert_form_field
Expand Down Expand Up @@ -97,8 +97,8 @@ def test_should_float_convert_float():
assert_conversion(forms.FloatField, Float)


def test_should_decimal_convert_float():
assert_conversion(forms.DecimalField, Float)
def test_should_decimal_convert_decimal():
assert_conversion(forms.DecimalField, Decimal)


def test_should_multiple_choice_convert_list():
Expand Down
6 changes: 5 additions & 1 deletion graphene_django/rest_framework/serializer_converter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,12 @@ def convert_serializer_field_to_bool(field):
return graphene.Boolean


@get_graphene_type_from_serializer_field.register(serializers.FloatField)
@get_graphene_type_from_serializer_field.register(serializers.DecimalField)
def convert_serializer_field_to_decimal(field):
return graphene.Decimal


@get_graphene_type_from_serializer_field.register(serializers.FloatField)
def convert_serializer_field_to_float(field):
return graphene.Float

Expand Down
4 changes: 2 additions & 2 deletions graphene_django/rest_framework/tests/test_field_converter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,9 @@ def test_should_float_convert_float():
assert_conversion(serializers.FloatField, graphene.Float)


def test_should_decimal_convert_float():
def test_should_decimal_convert_decimal():
assert_conversion(
serializers.DecimalField, graphene.Float, max_digits=4, decimal_places=2
serializers.DecimalField, graphene.Decimal, max_digits=4, decimal_places=2
)


Expand Down
2 changes: 1 addition & 1 deletion graphene_django/tests/test_converter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ def test_should_float_convert_float():
assert_conversion(models.FloatField, graphene.Float)


def test_should_float_convert_decimal():
def test_should_decimal_convert_decimal():
assert_conversion(models.DecimalField, graphene.Decimal)


Expand Down