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

Different types with the same name in the schema - Django fields with choices #185

Closed
kozickikarol opened this issue May 26, 2017 · 47 comments · Fixed by #860
Closed

Different types with the same name in the schema - Django fields with choices #185

kozickikarol opened this issue May 26, 2017 · 47 comments · Fixed by #860
Assignees

Comments

@kozickikarol
Copy link

Hi,

When you try to create a model with choices field and then create a second model which inherit from the previous one, you will receive error while schema generating.

Here you have a simple project with models and basic configuration. Try to generate schema and you will get following error:

AssertionError: Found different types with the same name in the schema: PageType, PageType.

It seems that it is unable to inherit models with choices fields.

@ncrmro
Copy link

ncrmro commented Jun 10, 2017

Getting same

AssertionError: Found different types with the same name in the schema: PartMarketState, PartMarketState.

This is using django-fsm (functional state machine) which is guess creates set of enums as well

@fangaofeng
Copy link

I getting a same error.
File "E:\pythonvenv\django11\lib\site-packages\graphql\type\definition.py", line 180, in fields
return define_field_map(self, self._fields)
File "E:\pythonvenv\django11\lib\site-packages\graphql\type\definition.py", line 189, in define_field_map
field_map = field_map()
File "E:\pythonvenv\django11\lib\site-packages\graphene\types\typemap.py", line 235, in construct_fields_for_type
map = self.reducer(map, field.type)
File "E:\pythonvenv\django11\lib\site-packages\graphene\types\typemap.py", line 72, in reducer
return self.graphene_reducer(map, type)
File "E:\pythonvenv\django11\lib\site-packages\graphene\types\typemap.py", line 77, in graphene_reducer
return self.reducer(map, type.of_type)
File "E:\pythonvenv\django11\lib\site-packages\graphene\types\typemap.py", line 72, in reducer
return self.graphene_reducer(map, type)
File "E:\pythonvenv\django11\lib\site-packages\graphene\types\typemap.py", line 83, in graphene_reducer
).format(_type.graphene_type, type)
AssertionError: Found different types with the same name in the schema: UserUserType, UserUserType.


class User(BDAbstractUser):

    TYPE_CHOICES = [('agent', 'agent'), ('nomal', 'nomal.'), ('superuser',
     """                                                          'superuser')]
    user_type = models.CharField(max_length=10, blank=True)
    """
    user_type = models.CharField(
        _('user type'),
        max_length=10,
        choices=TYPE_CHOICES,
        default='nomal',
        help_text=_('need user type'))

@Tyrdall
Copy link

Tyrdall commented Jul 10, 2017

It seems you're not allowed to include type in your field's name. I tried type = models.CharField() and foo_type = models.CharField() and both caused that error.

When I changed the field's name to foo the error was gone.

@alexche8
Copy link

alexche8 commented Oct 6, 2017

Still have this issue after renaming of models fields.
But this hack works for me #87 (comment)

@darkowic
Copy link

darkowic commented Jan 26, 2018

Is it documented anywhere that we can not use field name type in models when using this library?

@sanfilippopablo
Copy link

@darkowic I was wondering why I was getting this error. Now that you mention, the problematic model has a type field in it.

@NathanLawrence
Copy link

This is quite a glaring bug to still have.

What can be done about it? Thoughts?

@denis-ryzhkov
Copy link

When graphene_django converts a field with choices to graphene.Enum, it creates a new type for this field.

If it was type field of model Foo, new autogenerated type will be FooType.

And you probably already have your own

class FooType(DjangoObjectType):
     class Meta:
         model = Foo

So there is AssertionError: Found different types with the same name in the schema: FooType, FooType.

This conversion raised a lot of questions already: https://github.com/graphql-python/graphene-django/issues?q=is%3Aissue+choices

Maybe someone adds GRAPHENE_CONVERT_CHOICES_TO_ENUMS = False setting to graphene_django once.

Until then I am going to monkey-patch it somewhere before schema is imported, for example in settings file.

Patch: https://gist.github.com/denis-ryzhkov/fcb944f6ebfad4775efb74848703f0d7

@jkimbo
Copy link
Member

jkimbo commented May 13, 2018

@denis-ryzhkov thanks for the investigation! You're right, the issues seems to be that the auto naming of the Enum field that a Django choice field gets converted to is likely to clash with an existing type in your schema since it's just the object type name plus the field name:

name = to_camel_case('{}_{}'.format(meta.object_name, field.name))

I think the fix for that is make the generated name more explicit and less likely to clash with anything in the schema. Something like DjangoModel{object_name}{field_name}Choices (e.g. DjangoModelFooTypeChoices).

Separately, disabling the conversion of Django choice fields to Enums is a good idea since it's not always desired. Though I think it's probably something that should be done on a field level rather than globally.

@bastiW
Copy link

bastiW commented Dec 31, 2018

I fixed it by just using the import statement. I imported the type from another class instead of rewriting it again.

But that maybe does not solve the issues if you overwrite a class

@GasimGasimzada
Copy link

I have also had this issue and @denis-ryzhkov explanation helped me resolve the issue. I have a model Organization with field type with choices in it. For schema I had the following:

class OrganizationType(DjangoObjectType):
    class Meta:
        model = Organization

As a result, I was getting:

AssertionError: Found different types with the same name in the schema: OrganizationType, OrganizationType.

After reading through this, I just changed the class name to OrganizationGQLType. Now, everyhing works as expected. If this decision is not going to be changed, I think it would be beneficial to add this to the docs to clear out the confusion.

@aindong
Copy link

aindong commented Apr 26, 2019

I also encountered the same error and renaming the field type into something fixed my problem

@phalt
Copy link
Contributor

phalt commented Apr 26, 2019

Is it documented anywhere that we can not use field name type in models when using this library?

It's generally not a good idea to use this as a variable name in Python, as you're overwriting the built-in type keyword. So this isn't documented in this package because it's probably more of a Python gotcha?

@denis-ryzhkov
Copy link

as you're overwriting the built-in type keyword

There is a difference between reserved keywords that cannot be used as identifiers vs built-in identifiers that are always available (either without import or via builtins module)

Both type and id are examples of built-in identifiers, and id is widely shadowed, e.g. in Django ORM

This shadowing usually happens on the class definition level, where built-in functions like type() and id() are of no use, but class fields with these names are very natural

Once such class fields are defined, item.id and item.type (including self.id and self.type) don't shadow built-ins any more

Imagine graphene-django expected you to never have id field in your classes and also felt no need to document this expectation and naming conflict it leads to, just because id is a built-in identifier - that would raise a ton of issues instantly, but even type raised multiple issues here

So please either document this poor design decision and its workaround or solve it somehow

@phalt
Copy link
Contributor

phalt commented Apr 29, 2019

There is a difference between reserved keywords that cannot be used as identifiers vs built-in identifiers that are always available (either without import or via builtins module)

I mixed them up, woops!

So please either document this poor design decision and its workaround or solve it somehow

I appreciate this hasn't been looked at yet. I'm a new collaborator to this project, I'll see what I can do to make sure this bug is added to a list of top priority things.

@stale
Copy link

stale bot commented Jun 28, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jun 28, 2019
@jkimbo
Copy link
Member

jkimbo commented Jul 1, 2019

Looks like this is still an issue. Don't close

@stale stale bot removed the wontfix label Jul 1, 2019
@GasimGasimzada
Copy link

I think this can be solved by adding double underscore or something similar to the name (e.g “__OrganizationType” instead of “OrganizationType”). I just got curious about this to be honest. Why does a choice based field have a type? Is it possible to completely remove this auto-generated field type for choice field?

@jkimbo
Copy link
Member

jkimbo commented Jul 1, 2019

@GasimGasimzada there is an option in master (not yet released) that allows you to turn off the automatic choice to enum conversion: #674

@jkimbo jkimbo mentioned this issue Jul 9, 2019
7 tasks
@zbyte64
Copy link
Collaborator

zbyte64 commented Jul 9, 2019

I am trying to figure out why our unit tests don't catch this. We have a test model Reporter which has a field reporter_type which has options and it seems to work. See: https://github.com/graphql-python/graphene-django/blob/master/graphene_django/tests/models.py#L45

@mfdeux
Copy link

mfdeux commented Aug 23, 2019

@zbyte64 Isn't it just if the field is named "type"? I have the same issue.

@nikita-davydov
Copy link

To fix it you have to use ConvertedPageType = Enum.from_enum(PageType) only once, and use ConvertedPageType everywhere

@arielnmz
Copy link

arielnmz commented Dec 5, 2019

To fix it you have to use ConvertedPageType = Enum.from_enum(PageType) only once, and use ConvertedPageType everywhere

This doesn't work if you are using RelaySerializerMutation, as it will create a type on its own

@berekuk
Copy link
Contributor

berekuk commented Dec 29, 2019

Related issue: meta.object_name is not unique enough, since it's possible to have multiple models with the same name in different Django apps which are running at the same time.

I think the fix for that is make the generated name more explicit and less likely to clash with anything in the schema. Something like DjangoModel{object_name}{field_name}Choices (e.g. DjangoModelFooTypeChoices).

So this is not enough, it should be DjangoModel{app_label}{object_name}{field_name}Choices.

@arielnmz
Copy link

Related issue: meta.object_name is not unique enough, since it's possible to have multiple models with the same name in different Django apps which are running at the same time.

I think the fix for that is make the generated name more explicit and less likely to clash with anything in the schema. Something like DjangoModel{object_name}{field_name}Choices (e.g. DjangoModelFooTypeChoices).

So this is not enough, it should be DjangoModel{app_label}{object_name}{field_name}Choices.

I agree. But maybe check first for name collisions first? Otherwise new and very simple projects will have over-complicated names.

@jkimbo
Copy link
Member

jkimbo commented Dec 29, 2019

Related issue: meta.object_name is not unique enough, since it's possible to have multiple models with the same name in different Django apps which are running at the same time.

I think the fix for that is make the generated name more explicit and less likely to clash with anything in the schema. Something like DjangoModel{object_name}{field_name}Choices (e.g. DjangoModelFooTypeChoices).

So this is not enough, it should be DjangoModel{app_label}{object_name}{field_name}Choices.

I agree. But maybe check first for name collisions first? Otherwise new and very simple projects will have over-complicated names.

Very good points and I especially like the suggestion of checking for name collisions first to progressively improve this. That way it’s not a breaking change either. I’ll try and create a PR for this as soon as possible.

@jkimbo jkimbo self-assigned this Dec 29, 2019
@MisterGlass
Copy link
Contributor

@jkimbo have you had a chance to work on that? Just ran into this, seems like your solution would solve my problems.

@jkimbo
Copy link
Member

jkimbo commented Jan 10, 2020

@MisterGlass I have had a look and unfortunately I don’t it would be possible to check for name collisions since the enum name is defined before it has access to the schema. I think the only way to solve this would be an opt in flag to change the naming generation. Currently not sure if this should be per object type or globally for the whole schema. Anyone have any suggestions?

@MisterGlass
Copy link
Contributor

I think using the long names without checking for name collisions is fine. Certainly better than the collisions IMO.

@jkimbo
Copy link
Member

jkimbo commented Jan 11, 2020

@MisterGlass @arielnmz would it be very disruptive for you we added a global option to change all enums generated from field choices to the new long name format? It would be all or nothing.

@arielnmz
Copy link

@MisterGlass @arielnmz would it be very disruptive for you we added a global option to change all enums generated from field choices to the new long name format? It would be all or nothing.

Not at all. I think its an excellent middle ground and by the way aligns pretty well with the actual workarounds I've put in place (at least in my case).

@MisterGlass
Copy link
Contributor

@jkimbo Sounds fine to me. Another thing I've seen some frameworks do is Just have a configurable string that is part of the generated names. Default is often the project name (EG Graphene, so our example of PageType would be PageGrapheneType) , but to keep existing deployments the same you can default to empty string. Then all people with these conflicts would have to do is override the empty string with something that won't generate a conflict.

@jkimbo
Copy link
Member

jkimbo commented Jan 29, 2020

Sorry for the delay with this but I've just created #860 which should resolve the issues in this thread. I would appreciate if people could have a look at the PR to make sure that it would solve your issue in a reasonable way.

@MisterGlass
Copy link
Contributor

That looks fabulous @jkimbo, thank you

@rarenatoe
Copy link

I am using version 2.10.1 and this definitely hasn't been solved.

I have a field called type with choices option. I am forced to name the ObjectType anything other than ModelNameType in order to avoid conflicts.

Did I miss something? If not, then what was fixed?

@jkimbo
Copy link
Member

jkimbo commented Aug 12, 2020

@rarenatoe have you tried enabling the DJANGO_CHOICE_FIELD_ENUM_V3_NAMING option?

@rarenatoe
Copy link

rarenatoe commented Aug 12, 2020

@jkimbo like DJANGO_CHOICE_FIELD_ENUM_V3_NAMING = True on my settings.py?

Ah figured it out. I had to add something like this to settings.py:

GRAPHENE = {
    "DJANGO_CHOICE_FIELD_ENUM_V3_NAMING": True,
}

Might be a good idea to include this in the online docs.

@jkimbo
Copy link
Member

jkimbo commented Aug 12, 2020

@rarenatoe it is in the docs: https://docs.graphene-python.org/projects/django/en/latest/settings/#django-choice-field-enum-v3-naming

Glad you found it and it works anyway.

@coler-j
Copy link

coler-j commented Aug 18, 2020

I am also getting this with a field called "status" (instead of "type")...

And DJANGO_CHOICE_FIELD_ENUM_V3_NAMING https://docs.graphene-python.org/projects/django/en/latest/settings/#django-choice-field-enum-v3-naming does not fix it.

It appears that this is occuring because I have a query and a SerializerMutation (for the same type) that both use the model with a choice field. Can someone reference the tests surrounding choice fields? Is there one the also uses a SeralizerMutation?

@arielnmz
Copy link

arielnmz commented Aug 19, 2020 via email

@CBuiVNG
Copy link
Contributor

CBuiVNG commented Aug 21, 2020

@coler-j I have the same issue (using SerializerMutation). I've got a field_type. Setting DJANGO_CHOICE_FIELD_ENUM_V3_NAMING does not help

What did help was #851. Set
convert_choices_to_enum = False in your SerializerMutation class.

@coler-j
Copy link

coler-j commented Aug 21, 2020

@arielnmz I didn't. I am more interested in an example test that shows this working.

@CBuiVNG I will try convert_choices_to_enum = False for now, I just removed the status field from my serializer (as I don't need it for current scope).

@coler-j
Copy link

coler-j commented Feb 11, 2021

@CBuiVNG I tried to use convert_choices_to_enum = False and it did not work. Still unable to use choicefield -> enums in my project. I am going to try and narrow down why this is happening.

Edit: NVM convert_choices_to_enum = False did resolve the issue.

@rarenatoe
Copy link

@Instrumedley the solution is right above your post.

@samjhill
Copy link

"DJANGO_CHOICE_FIELD_ENUM_V3_NAMING": True,

this fixed it for me

@overide
Copy link

overide commented Jul 9, 2021

I have a different case -

  1. One of my models has severity as a field
  2. Other model has choice fields value as severity

And I'm getting an error AssertionError: Found different types with the same name in the schema: severity, severity. I've tried all the possible solutions mentioned above but none works for me

@zilehuda
Copy link

To fix it you have to use ConvertedPageType = Enum.from_enum(PageType) only once, and use ConvertedPageType everywhere

it works for me. thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet