From d12ea31f88d96ec5710310719f4618e3d8e19cbf Mon Sep 17 00:00:00 2001 From: Thomas Leonard Date: Mon, 29 Jan 2024 16:13:01 +0100 Subject: [PATCH] feat!: check django model has a default ordering when used in a relay connection --- 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 fb76b0395..c49206a4f 100644 --- a/examples/starwars/models.py +++ b/examples/starwars/models.py @@ -24,6 +24,9 @@ def __str__(self): 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 1bbe1f3d2..a1b9a2ccb 100644 --- a/graphene_django/fields.py +++ b/graphene_django/fields.py @@ -101,13 +101,19 @@ def type(self): 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 a4097b183..8824042e9 100644 --- a/graphene_django/filter/tests/conftest.py +++ b/graphene_django/filter/tests/conftest.py @@ -26,6 +26,9 @@ 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 ece1bb6db..821b3701f 100644 --- a/graphene_django/tests/models.py +++ b/graphene_django/tests/models.py @@ -5,6 +5,9 @@ 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 @@ def get_queryset(self): 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 0f5b79a0b..caaa6ddf7 100644 --- a/graphene_django/tests/test_fields.py +++ b/graphene_django/tests/test_fields.py @@ -2,11 +2,12 @@ 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 @@ def resolve_articles(root, info): 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 5c36bb92e..72514d23b 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 @@ class Meta: 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 @@ class Meta: 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 @@ class Meta: 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 @@ class Meta: 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 @@ class Reporter(DjangoObjectType): 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()}"