From 96c09ac439985d9548678a08221e86056ec1e703 Mon Sep 17 00:00:00 2001 From: Thomas Leonard <64223923+tcleonard@users.noreply.github.com> Date: Tue, 30 Jan 2024 10:09:18 +0100 Subject: [PATCH] feat!: check django model has a default ordering when used in a relay connection (#1495) Co-authored-by: Thomas Leonard --- examples/starwars/models.py | 3 ++ graphene_django/fields.py | 8 +++++- graphene_django/filter/tests/conftest.py | 3 ++ graphene_django/tests/models.py | 12 ++++++++ graphene_django/tests/test_fields.py | 36 ++++++++++++++++++++++-- graphene_django/tests/test_types.py | 23 +++++++-------- 6 files changed, 69 insertions(+), 16 deletions(-) diff --git a/examples/starwars/models.py b/examples/starwars/models.py index fb76b03..c49206a 100644 --- a/examples/starwars/models.py +++ b/examples/starwars/models.py @@ -24,6 +24,9 @@ class Faction(models.Model): class Ship(models.Model): + class Meta: + ordering = ["pk"] + name = models.CharField(max_length=50) faction = models.ForeignKey(Faction, on_delete=models.CASCADE, related_name="ships") diff --git a/graphene_django/fields.py b/graphene_django/fields.py index 1bbe1f3..a1b9a2c 100644 --- a/graphene_django/fields.py +++ b/graphene_django/fields.py @@ -101,13 +101,19 @@ class DjangoConnectionField(ConnectionField): non_null = True assert issubclass( _type, DjangoObjectType - ), "DjangoConnectionField only accepts DjangoObjectType types" + ), "DjangoConnectionField only accepts DjangoObjectType types as underlying type" assert _type._meta.connection, "The type {} doesn't have a connection".format( _type.__name__ ) connection_type = _type._meta.connection if non_null: return NonNull(connection_type) + # Since Relay Connections require to have a predictible ordering for pagination, + # check on init that the Django model provided has a default ordering declared. + model = connection_type._meta.node._meta.model + assert ( + len(getattr(model._meta, "ordering", [])) > 0 + ), f"Django model {model._meta.app_label}.{model.__name__} has to have a default ordering to be used in a Connection." return connection_type @property diff --git a/graphene_django/filter/tests/conftest.py b/graphene_django/filter/tests/conftest.py index a4097b1..8824042 100644 --- a/graphene_django/filter/tests/conftest.py +++ b/graphene_django/filter/tests/conftest.py @@ -26,6 +26,9 @@ else: class Event(models.Model): + class Meta: + ordering = ["pk"] + name = models.CharField(max_length=50) tags = ArrayField(models.CharField(max_length=50)) tag_ids = ArrayField(models.IntegerField()) diff --git a/graphene_django/tests/models.py b/graphene_django/tests/models.py index ece1bb6..821b370 100644 --- a/graphene_django/tests/models.py +++ b/graphene_django/tests/models.py @@ -5,6 +5,9 @@ CHOICES = ((1, "this"), (2, _("that"))) class Person(models.Model): + class Meta: + ordering = ["pk"] + name = models.CharField(max_length=30) parent = models.ForeignKey( "self", on_delete=models.CASCADE, null=True, blank=True, related_name="children" @@ -12,6 +15,9 @@ class Person(models.Model): class Pet(models.Model): + class Meta: + ordering = ["pk"] + name = models.CharField(max_length=30) age = models.PositiveIntegerField() owner = models.ForeignKey( @@ -31,6 +37,9 @@ class FilmDetails(models.Model): class Film(models.Model): + class Meta: + ordering = ["pk"] + genre = models.CharField( max_length=2, help_text="Genre", @@ -46,6 +55,9 @@ class DoeReporterManager(models.Manager): class Reporter(models.Model): + class Meta: + ordering = ["pk"] + first_name = models.CharField(max_length=30) last_name = models.CharField(max_length=30) email = models.EmailField() diff --git a/graphene_django/tests/test_fields.py b/graphene_django/tests/test_fields.py index 0f5b79a..caaa6dd 100644 --- a/graphene_django/tests/test_fields.py +++ b/graphene_django/tests/test_fields.py @@ -2,11 +2,12 @@ import datetime import re import pytest -from django.db.models import Count, Prefetch +from django.db.models import Count, Model, Prefetch from graphene import List, NonNull, ObjectType, Schema, String +from graphene.relay import Node -from ..fields import DjangoListField +from ..fields import DjangoConnectionField, DjangoListField from ..types import DjangoObjectType from .models import ( Article as ArticleModel, @@ -716,3 +717,34 @@ class TestDjangoListField: r'SELECT .* FROM "tests_film" INNER JOIN "tests_film_reporters" .* LEFT OUTER JOIN "tests_filmdetails"', captured.captured_queries[1]["sql"], ) + + +class TestDjangoConnectionField: + def test_model_ordering_assertion(self): + class Chaos(Model): + class Meta: + app_label = "test" + + class ChaosType(DjangoObjectType): + class Meta: + model = Chaos + interfaces = (Node,) + + class Query(ObjectType): + chaos = DjangoConnectionField(ChaosType) + + with pytest.raises( + TypeError, + match=r"Django model test\.Chaos has to have a default ordering to be used in a Connection\.", + ): + Schema(query=Query) + + def test_only_django_object_types(self): + class Query(ObjectType): + something = DjangoConnectionField(String) + + with pytest.raises( + TypeError, + match="DjangoConnectionField only accepts DjangoObjectType types as underlying type", + ): + Schema(query=Query) diff --git a/graphene_django/tests/test_types.py b/graphene_django/tests/test_types.py index 5c36bb9..72514d2 100644 --- a/graphene_django/tests/test_types.py +++ b/graphene_django/tests/test_types.py @@ -1,3 +1,4 @@ +import warnings from collections import OrderedDict, defaultdict from textwrap import dedent from unittest.mock import patch @@ -399,7 +400,7 @@ def test_django_objecttype_fields_exist_on_model(): with pytest.warns( UserWarning, match=r"Field name .* matches an attribute on Django model .* but it's not a model field", - ) as record: + ): class Reporter2(DjangoObjectType): class Meta: @@ -407,7 +408,8 @@ def test_django_objecttype_fields_exist_on_model(): fields = ["first_name", "some_method", "email"] # Don't warn if selecting a custom field - with pytest.warns(None) as record: + with warnings.catch_warnings(): + warnings.simplefilter("error") class Reporter3(DjangoObjectType): custom_field = String() @@ -416,8 +418,6 @@ def test_django_objecttype_fields_exist_on_model(): model = ReporterModel fields = ["first_name", "custom_field", "email"] - assert len(record) == 0 - @with_local_registry def test_django_objecttype_exclude_fields_exist_on_model(): @@ -445,15 +445,14 @@ def test_django_objecttype_exclude_fields_exist_on_model(): exclude = ["custom_field"] # Don't warn on exclude fields - with pytest.warns(None) as record: + with warnings.catch_warnings(): + warnings.simplefilter("error") class Reporter4(DjangoObjectType): class Meta: model = ReporterModel exclude = ["email", "first_name"] - assert len(record) == 0 - @with_local_registry def test_django_objecttype_neither_fields_nor_exclude(): @@ -467,24 +466,22 @@ def test_django_objecttype_neither_fields_nor_exclude(): class Meta: model = ReporterModel - with pytest.warns(None) as record: + with warnings.catch_warnings(): + warnings.simplefilter("error") class Reporter2(DjangoObjectType): class Meta: model = ReporterModel fields = ["email"] - assert len(record) == 0 - - with pytest.warns(None) as record: + with warnings.catch_warnings(): + warnings.simplefilter("error") class Reporter3(DjangoObjectType): class Meta: model = ReporterModel exclude = ["email"] - assert len(record) == 0 - def custom_enum_name(field): return f"CustomEnum{field.name.title()}"