Bugfix: call resolver function in DjangoConnectionField as documented
#1529
Conversation
that is, the one specified using DjangoConnectionField(..., resolver=some_func)
| @convert_django_field.register(models.AutoField) | ||
| @convert_django_field.register(models.BigAutoField) | ||
| @convert_django_field.register(models.SmallAutoField) | ||
| def convert_field_to_id(field, registry=None): | ||
| return ID(description=get_django_field_description(field), required=not field.null) | ||
|
|
||
|
|
There was a problem hiding this comment.
SmallAutoField has been arouns since Django 3.0 from 2019-12-02, which we don't support anymore. So the check for models.SmallAutoField shouldn't be necessary anymore.
| def test_should_uuid_convert_string(): | ||
| if hasattr(forms, "UUIDField"): | ||
| assert_conversion(forms.UUIDField, UUID) | ||
| assert_conversion(forms.UUIDField, UUID) |
There was a problem hiding this comment.
forms.UUIDField has been around since Django 1.8 from 2015-04-01.
| def test_should_uuid_convert_string(): | ||
| if hasattr(serializers, "UUIDField"): | ||
| assert_conversion(serializers.UUIDField, graphene.String) | ||
| assert_conversion(serializers.UUIDField, graphene.String) | ||
|
|
There was a problem hiding this comment.
DRF added support for UUIDField in 3.0.4 from 2015-01-28. We require DRF >= 3.6.3
| @@ -166,14 +164,14 @@ def test_field_with_choices_convert_enum(): | |||
| help_text="Language", choices=(("es", "Spanish"), ("en", "English")) | |||
| ) | |||
|
|
|||
| class TranslatedModel(models.Model): | |||
| class ChoicesModel(models.Model): | |||
There was a problem hiding this comment.
This fixes a Django warning about the TranslatedModel being already registered:
RuntimeWarning: Model 'test.translatedmodel' was already registered. Reloading models is not advised as it can lead to inconsistencies, most notably with related models.
| def resolve_ships(self): | ||
| def resolve_ships(self, info): | ||
| return get_ships() |
There was a problem hiding this comment.
resolve_only_args is deprecated
|
I just noticed that this is basically a duplicate of #1513 😅 |
|
@sjdemartini @kiendang any thoughts on this? |
There was a problem hiding this comment.
@PoByBolek Sorry for the delay, I haven't had much time to look at graphene-django recently. Overall the changes look good to me, and I appreciate the comments clarifying the updates you made (as well as the efforts to reduce noise with warnings, etc). I would generally prefer that the changes unrelated to the DjangoConnectionField resolver fix were put into their own PR to make this simpler to review and more targeted, but the separate commits helps. This seems a tad cleaner than the solution in #1513 (where the super call looks a bit odd, even if the effect is the same), so merging this seems good to me.
| if hasattr(models, "SmallAutoField"): | ||
|
|
||
| @convert_django_field.register(models.SmallAutoField) | ||
| def convert_field_small_to_id(field, registry=None): | ||
| return convert_field_to_id(field, registry) | ||
|
|
||
|
|
There was a problem hiding this comment.
Note that this could be considered a backwards-incompatible change as it removes the (undocumented) convert_field_small_to_id() function from the graphene_django.converter module.
So if you want, I can turn this into two PR: one only including c127aec (the bugfix) and the remaining commits that fix the warnings.
This fixes a bug in that
DjangoConnectionFielddoesn't call itsresolverfunction despite it being documented in the Graphene docs: "Resolvers outside the class".I also fixed some warnings and running the tests now treats warnings as errors. But I really only care about c127aec here 😄