Skip to content

Adding support for disabling enum creation on SerializerMutation #851

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

Conversation

leewardbound
Copy link
Contributor

@leewardbound leewardbound commented Jan 12, 2020

A workaround for #785 --

Appropriately, DjangoObjectType supports a convert_choices_to_enum which disables the creation of GraphQL Enum types for every ChoiceField on a model.

However, SerializerMutation (similarly a subclass of ObjectType and registered in the GQL schema) does not support such a flag, and forces creating an Enum for every ChoiceField. This behavior can be undesirable because it can trigger a duplicate type registration error Found different types with the same name in the schema:.

For consistency's sake, a flag to disable the Enum creation should be added to the SerializerMutation; this commit implements such changes.

Sidenote --
In my sandbox, this error actually triggered even when the ChoiceField was only registered one single time, on one single Mutation, and not at all present in the DjangoObjectType, which leads me to believe that the SerializerMutation's Enum definition is colliding with itself?

There's actually 0% test coverage for the current existing behavior of creating Enums for SerializerMutation because none of these tests actually implement any Model with a ChoicesField; the test coverage stops at ensuring the automatic type conversion returns a proper value (and I added a second test for my branching logic). I hesitate to suggest it, but it seems that in the current implementation of Enums in SerializerMutations, simply adding a SerializerMutation with any ChoicesField to a schema might be enough to throw the error?

I might have time to dig deeper into that, but for now, here's an effective and reasonable bandaid.

Copy link
Collaborator

@zbyte64 zbyte64 left a comment

Choose a reason for hiding this comment

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

Looks clean, increases test coverage, and I think is a reasonable API improvement 👍

@jkimbo jkimbo merged commit 83bc32b into graphql-python:master Feb 7, 2020
@jkimbo
Copy link
Member

jkimbo commented Feb 7, 2020

Thanks @leewardbound !

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.

3 participants