graphql-python / graphene-django

Build powerful, efficient, and flexible GraphQL APIs with seamless Django integration.
http://docs.graphene-python.org/projects/django/en/latest/
MIT License
4.3k stars 769 forks source link

DjangoFormMutation does not generate enum from forms.ChoiceField #1040

Open artofhuman opened 4 years ago

artofhuman commented 4 years ago

GEEKS_CHOICES =(("1", "One"), ("2", "Two"))

class UpdateUserStateInTradeForm(forms.Form): state = forms.ChoiceField(choices=GEEKS_CHOICES)

mutation

class UpdateUserStateInTradeMutation(DjangoFormMutation): class Meta: form_class = UpdateUserStateInTradeForm

generated schema

state: String!



* **What is the expected behavior?**

state is enumeration

* **Please tell us about your environment:**

  - Version: 2.13.0, Django 3.0.1
  - Platform: Linux
iorlandini commented 4 years ago

@artofhuman I think the way in which the repository handles the creation of enum types from models fields with choices is too magical, generating an enum type for each field with choices. I think that's a problem because it could be the same enum across your whole application. And if you want to force them to have the same name in order to use the same type by setting a DJANGO_CHOICE_FIELD_ENUM_CUSTOM_NAME function, an error is raised.

On the other hand, the repository does not implement the same magic for the form fields. Which brings us to an opportunity to make it better, more explicit, and less magical. That's why a created a branch in a forked repository and you can check a possible implementation there https://github.com/graphql-python/graphene-django/compare/main...iorlandini:feature/form-choice-field-enum

If you check the test:

def test_form_field_with_choices_convert_enum():
    choices_a = (("choice-a-0", "Choice A 0"), ("choice-a-1", "Choice A 1"))
    enum_a = convert_choices_to_named_enum_with_descriptions("ChoicesAEnum", choices_a)

    choices_b = (("choice-b-0", "Choice B 0"), ("choice-b-1", "Choice B 1"))
    enum_b = convert_choices_to_named_enum_with_descriptions("ChoicesBEnum", choices_b)

    class TestForm(forms.Form):
        field_1 = EnumField(choices=choices_a, enum=enum_a)
        field_2 = EnumField(choices=choices_a, enum=enum_a)

        field_3 = EnumField(choices=choices_b, enum=enum_b)

    class TestMutation(DjangoFormMutation):
        class Meta:
            form_class = TestForm

    schema = graphene.Schema(mutation=TestMutation)

    # Check ChoicesAEnum: field_1 and field_2
    _type = schema.get_type("ChoicesAEnum")
    assert _type.name == "ChoicesAEnum"

    assert _type.values[0].name == "CHOICE_A_0"
    assert _type.values[0].value == "choice-a-0"

    assert _type.values[1].name == "CHOICE_A_1"
    assert _type.values[1].value == "choice-a-1"

    # Check ChoicesBEnum: field_3
    _type = schema.get_type("ChoicesBEnum")
    assert _type.name == "ChoicesBEnum"

    assert _type.values[0].name == "CHOICE_B_0"
    assert _type.values[0].value == "choice-b-0"

    assert _type.values[1].name == "CHOICE_B_1"
    assert _type.values[1].value == "choice-b-1"

You are going to see that only one enum type is generated for all choices based on that enum without raising an error.

I did not create a PR yet because I want to know what do the maintainers think about this solution instead of the magical kind implemented for the model fields.

artofhuman commented 4 years ago

Maybe we're can use convert_choices_to_enum option like for models. https://docs.graphene-python.org/projects/django/en/latest/queries/#choices-to-enum-conversion

class UpdateUserStateInTradeMutation(DjangoFormMutation):
    class Meta:
        form_class = UpdateUserStateInTradeForm
        convert_choices_to_enum = ["state"]

My point is the reuse enums thats generated in models mutations and form mutations. Do we have workaround for this case?

iorlandini commented 4 years ago

@artofhuman that's a good point. In that case, check the following branch comparison:

https://github.com/graphql-python/graphene-django/compare/main...iorlandini:feature/form-choices-to-enum-conversion

I think it does the job! Just do not forget to use a Django ModelForm instead of a Form because it needs a reference to the model:

def test_form_field_with_choices_convert_enum():
    class TestModel(models.Model):
        field = models.CharField(
            choices=(("choice-0", "Choice 0"), ("choice-1", "Choice 1"))
        )

    class TestType(DjangoObjectType):
        class Meta:
            model = TestModel

    class TestForm(forms.ModelForm):
        class Meta:
            model = TestModel
            fields = ["field"]

    class TestMutation(DjangoFormMutation):
        class Meta:
            form_class = TestForm

    schema = graphene.Schema(mutation=TestMutation)

    _type = schema.get_type("TestModelField")
    assert _type.name == "TestModelField"

    assert _type.values[0].name == "CHOICE_0"
    assert _type.values[0].value == "choice-0"

    assert _type.values[1].name == "CHOICE_1"
    assert _type.values[1].value == "choice-1"
marcosalcazar commented 3 years ago

About this issue, can you guys let us know what's the proper way to have Enums for a ChoiceField (or CharField with choices) for a DjangoFormMutation? It seems the bug is still there.

zbyte64 commented 3 years ago

I like the idea of extending the enum generation behavior to the form mutations. Since we are reusing the enums from DjangObjectType I think this would be safe. Please open up a PR :+1:

iorlandini commented 3 years ago

I like the idea of extending the enum generation behavior to the form mutations. Since we are reusing the enums from DjangObjectType I think this would be safe. Please open up a PR 👍

Sure, I will.