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

Bugfix: call resolver function in DjangoConnectionField as documented #1529

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions examples/django_test_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,5 @@
GRAPHENE = {"SCHEMA": "graphene_django.tests.schema_view.schema"}

ROOT_URLCONF = "graphene_django.tests.urls"

USE_TZ = True
11 changes: 4 additions & 7 deletions examples/starwars/schema.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import graphene
from graphene import Schema, relay, resolve_only_args
from graphene import Schema, relay
from graphene_django import DjangoConnectionField, DjangoObjectType

from .data import create_ship, get_empire, get_faction, get_rebels, get_ship, get_ships
Expand Down Expand Up @@ -62,16 +62,13 @@ class Query(graphene.ObjectType):
node = relay.Node.Field()
ships = DjangoConnectionField(Ship, description="All the ships.")

@resolve_only_args
def resolve_ships(self):
def resolve_ships(self, info):
return get_ships()
Copy link
Author

Choose a reason for hiding this comment

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

resolve_only_args is deprecated


@resolve_only_args
def resolve_rebels(self):
def resolve_rebels(self, info):
return get_rebels()

@resolve_only_args
def resolve_empire(self):
def resolve_empire(self, info):
return get_empire()


Expand Down
10 changes: 2 additions & 8 deletions graphene_django/converter.py
Original file line number Diff line number Diff line change
Expand Up @@ -190,19 +190,13 @@ def convert_field_to_string(field, registry=None):
)


@convert_django_field.register(models.BigAutoField)
@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)


Comment on lines 193 to 199
Copy link
Author

Choose a reason for hiding this comment

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

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.

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)


Comment on lines -199 to -205
Copy link
Author

Choose a reason for hiding this comment

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

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.

@convert_django_field.register(models.UUIDField)
def convert_field_to_uuid(field, registry=None):
return UUID(
Expand Down
2 changes: 1 addition & 1 deletion graphene_django/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ def connection_resolver(
def wrap_resolve(self, parent_resolver):
return partial(
self.connection_resolver,
parent_resolver,
self.resolver or parent_resolver,
self.connection_type,
self.get_manager(),
self.get_queryset_resolver(),
Expand Down
22 changes: 15 additions & 7 deletions graphene_django/forms/tests/test_converter.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from django import forms
from django import VERSION as DJANGO_VERSION, forms
from pytest import raises

from graphene import (
Expand All @@ -19,12 +19,16 @@
from ..converter import convert_form_field


def assert_conversion(django_field, graphene_field, *args):
field = django_field(*args, help_text="Custom Help Text")
def assert_conversion(django_field, graphene_field, *args, **kwargs):
# Arrange
help_text = kwargs.setdefault("help_text", "Custom Help Text")
field = django_field(*args, **kwargs)
# Act
graphene_type = convert_form_field(field)
# Assert
assert isinstance(graphene_type, graphene_field)
field = graphene_type.Field()
assert field.description == "Custom Help Text"
assert field.description == help_text
return field


Expand Down Expand Up @@ -59,7 +63,12 @@ def test_should_slug_convert_string():


def test_should_url_convert_string():
assert_conversion(forms.URLField, String)
kwargs = {}
if DJANGO_VERSION >= (5, 0):
# silence RemovedInDjango60Warning
kwargs["assume_scheme"] = "https"

assert_conversion(forms.URLField, String, **kwargs)


def test_should_choice_convert_string():
Expand All @@ -75,8 +84,7 @@ def test_should_regex_convert_string():


def test_should_uuid_convert_string():
if hasattr(forms, "UUIDField"):
assert_conversion(forms.UUIDField, UUID)
assert_conversion(forms.UUIDField, UUID)

Comment on lines 86 to +87
Copy link
Author

Choose a reason for hiding this comment

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

forms.UUIDField has been around since Django 1.8 from 2015-04-01.


def test_should_integer_convert_int():
Expand Down
3 changes: 1 addition & 2 deletions graphene_django/rest_framework/tests/test_field_converter.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,7 @@ def test_should_regex_convert_string():


def test_should_uuid_convert_string():
if hasattr(serializers, "UUIDField"):
assert_conversion(serializers.UUIDField, graphene.String)
assert_conversion(serializers.UUIDField, graphene.String)

Comment on lines 98 to 100
Copy link
Author

@PoByBolek PoByBolek Jul 4, 2024

Choose a reason for hiding this comment

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

DRF added support for UUIDField in 3.0.4 from 2015-01-28. We require DRF >= 3.6.3


def test_should_model_convert_field():
Expand Down
14 changes: 6 additions & 8 deletions graphene_django/tests/test_converter.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,8 @@ def assert_conversion(django_field, graphene_field, *args, **kwargs):


def test_should_unknown_django_field_raise_exception():
with raises(Exception) as excinfo:
with raises(Exception, match="Don't know how to convert the Django field"):
convert_django_field(None)
assert "Don't know how to convert the Django field" in str(excinfo.value)


def test_should_date_time_convert_string():
Expand Down Expand Up @@ -115,8 +114,7 @@ def test_should_big_auto_convert_id():


def test_should_small_auto_convert_id():
if hasattr(models, "SmallAutoField"):
assert_conversion(models.SmallAutoField, graphene.ID, primary_key=True)
assert_conversion(models.SmallAutoField, graphene.ID, primary_key=True)


def test_should_uuid_convert_id():
Expand Down Expand Up @@ -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):
Copy link
Author

Choose a reason for hiding this comment

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

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.

language = field

class Meta:
app_label = "test"

graphene_type = convert_django_field_with_choices(field).type.of_type
assert graphene_type._meta.name == "TestTranslatedModelLanguageChoices"
assert graphene_type._meta.name == "TestChoicesModelLanguageChoices"
assert graphene_type._meta.enum.__members__["ES"].value == "es"
assert graphene_type._meta.enum.__members__["ES"].description == "Spanish"
assert graphene_type._meta.enum.__members__["EN"].value == "en"
Expand All @@ -186,14 +184,14 @@ def get_choices():

field = models.CharField(help_text="Language", choices=get_choices)

class TranslatedModel(models.Model):
class CallableChoicesModel(models.Model):
language = field

class Meta:
app_label = "test"

graphene_type = convert_django_field_with_choices(field).type.of_type
assert graphene_type._meta.name == "TestTranslatedModelLanguageChoices"
assert graphene_type._meta.name == "TestCallableChoicesModelLanguageChoices"
assert graphene_type._meta.enum.__members__["ES"].value == "es"
assert graphene_type._meta.enum.__members__["ES"].description == "Spanish"
assert graphene_type._meta.enum.__members__["EN"].value == "en"
Expand Down
6 changes: 6 additions & 0 deletions graphene_django/tests/test_get_queryset.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ def setup_schema(self):
class ReporterType(DjangoObjectType):
class Meta:
model = Reporter
fields = "__all__"

@classmethod
def get_queryset(cls, queryset, info):
Expand All @@ -36,6 +37,7 @@ def get_queryset(cls, queryset, info):
class ArticleType(DjangoObjectType):
class Meta:
model = Article
fields = "__all__"

@classmethod
def get_queryset(cls, queryset, info):
Expand Down Expand Up @@ -200,6 +202,7 @@ def setup_schema(self):
class ReporterType(DjangoObjectType):
class Meta:
model = Reporter
fields = "__all__"
interfaces = (Node,)

@classmethod
Expand All @@ -211,6 +214,7 @@ def get_queryset(cls, queryset, info):
class ArticleType(DjangoObjectType):
class Meta:
model = Article
fields = "__all__"
interfaces = (Node,)

@classmethod
Expand Down Expand Up @@ -370,6 +374,7 @@ def setup_schema(self):
class FilmDetailsType(DjangoObjectType):
class Meta:
model = FilmDetails
fields = "__all__"

@classmethod
def get_queryset(cls, queryset, info):
Expand All @@ -380,6 +385,7 @@ def get_queryset(cls, queryset, info):
class FilmType(DjangoObjectType):
class Meta:
model = Film
fields = "__all__"

@classmethod
def get_queryset(cls, queryset, info):
Expand Down
57 changes: 53 additions & 4 deletions graphene_django/tests/test_query.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import base64
import datetime
from unittest.mock import ANY, Mock

import pytest
from django.db import models
Expand Down Expand Up @@ -2000,14 +2001,62 @@ class Query(graphene.ObjectType):
assert result.data == expected


def test_connection_should_call_resolver_function():
resolver_mock = Mock(
name="resolver",
return_value=[
Reporter(first_name="Some", last_name="One"),
Reporter(first_name="John", last_name="Doe"),
],
)

class ReporterType(DjangoObjectType):
class Meta:
model = Reporter
fields = "__all__"
interfaces = [Node]

class Query(graphene.ObjectType):
reporters = DjangoConnectionField(ReporterType, resolver=resolver_mock)

schema = graphene.Schema(query=Query)
result = schema.execute(
"""
query {
reporters {
edges {
node {
firstName
lastName
}
}
}
}
"""
)

resolver_mock.assert_called_once_with(None, ANY)
assert not result.errors
assert result.data == {
"reporters": {
"edges": [
{"node": {"firstName": "Some", "lastName": "One"}},
{"node": {"firstName": "John", "lastName": "Doe"}},
],
},
}


def test_should_query_nullable_foreign_key():
class PetType(DjangoObjectType):
class Meta:
model = Pet
fields = "__all__"

class PersonType(DjangoObjectType):
class Meta:
model = Person
fields = "__all__"

class Query(graphene.ObjectType):
pet = graphene.Field(PetType, name=graphene.String(required=True))
Expand All @@ -2022,10 +2071,8 @@ def resolve_person(self, info, name):
schema = graphene.Schema(query=Query)

person = Person.objects.create(name="Jane")
[
Pet.objects.create(name="Stray dog", age=1),
Pet.objects.create(name="Jane's dog", owner=person, age=1),
]
Pet.objects.create(name="Stray dog", age=1)
Pet.objects.create(name="Jane's dog", owner=person, age=1)

query_pet = """
query getPet($name: String!) {
Expand Down Expand Up @@ -2068,6 +2115,7 @@ def test_should_query_nullable_one_to_one_relation_with_custom_resolver():
class FilmType(DjangoObjectType):
class Meta:
model = Film
fields = "__all__"

@classmethod
def get_queryset(cls, queryset, info):
Expand All @@ -2076,6 +2124,7 @@ def get_queryset(cls, queryset, info):
class FilmDetailsType(DjangoObjectType):
class Meta:
model = FilmDetails
fields = "__all__"

@classmethod
def get_queryset(cls, queryset, info):
Expand Down
4 changes: 4 additions & 0 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,7 @@ omit = */tests/*
[tool:pytest]
DJANGO_SETTINGS_MODULE = examples.django_test_settings
addopts = --random-order
filterwarnings =
error
# we can't do anything about the DeprecationWarning about typing.ByteString in graphql
default:'typing\.ByteString' is deprecated:DeprecationWarning:graphql\.pyutils\.is_iterable
Loading