feat!: check django model has a default ordering when used in a relay connection (#1495)

Co-authored-by: Thomas Leonard <[email protected]>

Author: tcleonard

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")
 

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

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())

graphene_django/tests/models.py

@@ -5,13 +5,19 @@
 
 
 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"
     )
 
 
 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()

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)

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,15 +400,16 @@ 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:
                 model = ReporterModel
                 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()}"