django-ordered-model · shuckc · Mar 14, 2023 · Mar 11, 2023 · Mar 11, 2023 · Mar 13, 2023
diff --git a/CHANGES.md b/CHANGES.md
@@ -4,6 +4,13 @@ Change log
 Unreleased
 ----------
 
+3.7.2 - 2023-03-14
+----------
+- Fix a performance regression (unnecessary queries) in the WRT change detection (#286)
+- Add a Check that `order_with_respect_to` specifies only ForeignKey fields
+- Add a Check that our subclasses of ModelManager and QuerySet are used (#286) 
+
+
 3.7.1 - 2023-03-06
 ----------
 

diff --git a/README.md b/README.md
@@ -211,6 +211,8 @@ class GroupedItem(OrderedModel):
 
 Here items are put into groups that have some general information used by its items, but the ordering of the items is independent of the group the item is in.
 
+In all cases `order_with_respect_to` must specify a `ForeignKey` field on the model, or a Django Check `E002`, `E005` or `E006` error will be raised with further help.  
+
 When you want ordering on the baseclass instead of subclasses in an ordered list of objects of various classes, specify the full module path of the base class:
 
 ```python
@@ -242,9 +244,9 @@ When your model your extends `OrderedModel`, it inherits a custom `ModelManager`
 * `above(index)`,
 * `below(index)`
 
-If your `Model` uses a custom `ModelManager` (such as `ItemManager` below) please have it extend `OrderedModelManager`.
+If your `Model` uses a custom `ModelManager` (such as `ItemManager` below) please have it extend `OrderedModelManager`, or else Django Check `E003` will be raised.
 
-If your `ModelManager` returns a custom `QuerySet` (such as `ItemQuerySet` below) please have it extend `OrderedModelQuerySet`.
+If your `ModelManager` returns a custom `QuerySet` (such as `ItemQuerySet` below) please have it extend `OrderedModelQuerySet`, or Django Check `E004` will be raised.
 
 ```python
 from ordered_model.models import OrderedModel, OrderedModelManager, OrderedModelQuerySet

diff --git a/ordered_model/admin.py b/ordered_model/admin.py
@@ -209,17 +209,13 @@ def move_up_down_links(self, obj):
 
         # Find the fields which refer to the parent model of this inline, and
         # use one of them if they aren't None.
-        order_with_respect_to = obj._wrt_map() or []
-        fields = [
-            str(value.pk)
-            for value in order_with_respect_to.values()
-            if (
-                type(value) == self.parent_model
-                or issubclass(self.parent_model, type(value))
-            )
-            and value is not None
-            and value.pk is not None
-        ]
+        fields = []
+        for value in obj._get_related_objects():
+            # Note 'a class is considered a subclass of itself' pydocs
+            if issubclass(self.parent_model, type(value)):
+                if value is not None and value.pk is not None:
+                    fields.append(str(value.pk))
+
         order_obj_name = fields[0] if len(fields) > 0 else None
 
         model_info = self._get_model_info()

diff --git a/ordered_model/models.py b/ordered_model/models.py
@@ -1,9 +1,10 @@
 from functools import partial, reduce
 
 from django.core import checks
-from django.core.exceptions import ObjectDoesNotExist
+from django.core.exceptions import ObjectDoesNotExist, FieldDoesNotExist
 from django.db import models<
10000
/span>
 from django.db.models import Max, Min, F
+from django.db.models.fields.related import ForeignKey
 from django.db.models.constants import LOOKUP_SEP
 from django.utils.module_loading import import_string
 from django.utils.translation import gettext_lazy as _
@@ -127,10 +128,19 @@ def __init__(self, *args, **kwargs):
 
     def _wrt_map(self):
         d = {}
-        for a in self.get_order_with_respect_to():
-            d[a] = get_lookup_value(self, a)
+        for order_wrt_name in self.get_order_with_respect_to():
+            # we know order_wrt_name is a ForeignKey, so use a cheaper _id lookup
+            field_path = order_wrt_name + "_id"
+            d[order_wrt_name] = get_lookup_value(self, field_path)
         return d
 
+    def _get_related_objects(self):
+        # slow path, for use in the admin which requires the objects
+        # expected to generate extra queries
+        return [
+            get_lookup_value(self, name) for name in self.get_order_with_respect_to()
+        ]
+
     @classmethod
     def get_order_with_respect_to(cls):
         if type(cls.order_with_respect_to) is tuple:
@@ -140,7 +150,7 @@ def get_order_with_respect_to(cls):
         elif cls.order_with_respect_to is None:
             return tuple()
         else:
-            raise AssertionError("Invalid value for model.order_with_respect_to")
+            raise ValueError("Invalid value for model.order_with_respect_to")
 
     def _validate_ordering_reference(self, ref):
         if self._wrt_map() != ref._wrt_map():
@@ -312,22 +322,71 @@ def check(cls, **kwargs):
         ordering = getattr(cls._meta, "ordering", None)
         if ordering is None or len(ordering) < 1:
             errors.append(
-                checks.Warning(
+                checks.Error(
                     "OrderedModelBase subclass needs Meta.ordering specified.",
                     hint="If you have overwritten Meta, try inheriting with Meta(OrderedModel.Meta).",
                     obj=str(cls.__qualname__),
-                    id="ordered_model.W001",
+                    id="ordered_model.E001",
                 )
             )
         owrt = getattr(cls, "order_with_respect_to")
         if not (type(owrt) is tuple or type(owrt) is str or owrt is None):
             errors.append(
                 checks.Error(
-                    "OrderedModelBase subclass order_with_respect_to value invalid. Expected tuple, str or None",
+                    "OrderedModelBase subclass order_with_respect_to value invalid. Expected tuple, str or None.",
                     obj=str(cls.__qualname__),
-                    id="ordered_model.E001",
+                    id="ordered_model.E002",
                 )
             )
+        if not issubclass(cls.objects.__class__, OrderedModelManager):
+            errors.append(
+                checks.Error(
+                    "OrderedModelBase subclass has a ModelManager that does not inherit from OrderedModelManager.",
+                    obj=str(cls.__qualname__),
+                    id="ordered_model.E003",
+                )
+            )
+        if not issubclass(cls.objects.none().__class__, OrderedModelQuerySet):
+            errors.append(
+                checks.Error(
+                    "OrderedModelBase subclass ModelManager did not return a QuerySet inheriting from OrderedModelQuerySet.",
+                    obj=str(cls.__qualname__),
+                    id="ordered_model.E004",
+                )
+            )
+
+        # each field may be an FK, or recursively an FK ref to an FK
+        try:
+            for wrt_field in cls.get_order_with_respect_to():
+                mc = cls
+                for p in wrt_field.split(LOOKUP_SEP):
+                    try:
+                        f = mc._meta.get_field(p)
+                        if not isinstance(f, ForeignKey):
+                            errors.append(
+                                checks.Error(
+                                    "OrderedModel order_with_respect_to specifies field '{0}' (within '{1}') which is not a ForeignKey. This is unsupported.".format(
+                                        p, wrt_field
+                                    ),
+                                    obj=str(cls.__qualname__),
+                                    id="ordered_model.E005",
+                                )
+                            )
+                            break
+                        mc = f.remote_field.model
+                    except FieldDoesNotExist:
+                        errors.append(
+                            checks.Error(
+                                "OrderedModel order_with_respect_to specifies field '{0}' (within '{1}') which does not exist.".format(
+                                    p, wrt_field
+                                ),
+                                obj=str(cls.__qualname__),
+                                id="ordered_model.E006",
+                            )
+                        )
+        except ValueError:
+            # already handled by type checks for E002
+            pass
         return errors
 
 

diff --git a/tests/tests.py b/tests/tests.py
@@ -4,6 +4,7 @@
 from django.contrib.auth.models import User
 from django.core.management import call_command
 from django.core import checks
+from django.db import models
 from django.db.models.signals import post_delete
 from django.dispatch import Signal
 from django.utils.timezone import now
@@ -16,8 +17,9 @@
 from rest_framework.test import APIRequestFactory, APITestCase
 from rest_framework import status
 from tests.drf import ItemViewSet, router
+from tests.utils import assertNumQueries
 
-from ordered_model.models import OrderedModel
+from ordered_model.models import OrderedModel, OrderedModelManager, OrderedModelQuerySet
 from ordered_model.signals import on_ordered_model_delete
 
 from tests.models import (
@@ -164,6 +166,9 @@ def setUp(self):
         self.q1_a2 = q1.answers.create(user=u0)
         self.q2_a2 = q2.answers.create(user=u0)
 
+        with assertNumQueries(self, 1):
+            Answer.objects.get(id=self.q1_a1.id)
+
     def test_saved_order(self):
         self.assertSequenceEqual(
             Answer.objects.values_list("pk", "order"),
@@ -303,9 +308,14 @@ def setUp(self):
         self.u1_a2 = q1.answers.create(user=self.u1)
         self.u1_a3 = q1.answers.create(user=self.u1)
 
+        with assertNumQueries(self, 1):
+            Answer.objects.get(id=self.u0_a1.id)
+
     def test_reorder_when_field_value_changed(self):
         self.u0_a2.user = self.u1
-        self.u0_a2.save()
+        with assertNumQueries(self, 3):
+            self.u0_a2.save()
+
         self.assertSequenceEqual(
             Answer.objects.values_list("pk", "order"),
             [
@@ -685,7 +695,9 @@ def setUp(self):
         )  # tomatoe, mozarella, mushrooms, ham
         # Now put the toppings on the pizza
         self.p1_t1 = PizzaToppingsThroughModel(pizza=self.p1, topping=self.t1)
-        self.p1_t1.save()
+        with assertNumQueries(self, 2):
+            self.p1_t1.save()
+
         self.p1_t2 = PizzaToppingsThroughModel(pizza=self.p1, topping=self.t2)
         self.p1_t2.save()
         self.p1_t3 = PizzaToppingsThroughModel(pizza=self.p1, topping=self.t3)
@@ -1269,11 +1281,11 @@ class Meta:
         self.assertEqual(
             checks.run_checks(app_configs=self.apps.get_app_configs()),
             [
-                checks.Warning(
+                checks.Error(
                     msg="OrderedModelBase subclass needs Meta.ordering specified.",
                     hint="If you have overwritten Meta, try inheriting with Meta(OrderedModel.Meta).",
                     obj="ChecksTest.test_no_inherited_ordering.<locals>.TestModel",
-                    id="ordered_model.W001",
+                    id="ordered_model.E001",
                 )
             ],
         )
@@ -1293,6 +1305,95 @@ class Meta(OrderedModel.Meta):
 
         self.assertEqual(checks.run_checks(app_configs=self.apps.get_app_configs()), [])
 
+    def test_bad_owrt(self):
+        class TestModel(OrderedModel):
+            order_with_respect_to = 7
+
+        self.assertEqual(
+            checks.run_checks(app_configs=self.apps.get_app_configs()),
+            [
+                checks.Error(
+                    msg="OrderedModelBase subclass order_with_respect_to value invalid. Expected tuple, str or None.",
+                    obj="ChecksTest.test_bad_owrt.<locals>.TestModel",
+                    id="ordered_model.E002",
+                )
+            ],
+        )
+
+    def test_bad_manager(self):
+        class BadModelManager(models.Manager.from_queryset(OrderedModelQuerySet)):
+            pass
+
+        class TestModel(OrderedModel):
+            objects = BadModelManager()
+
+        self.assertEqual(
+            checks.run_checks(app_configs=self.apps.get_app_configs()),
+            [
+                checks.Error(
+                    msg="OrderedModelBase subclass has a ModelManager that does not inherit from OrderedModelManager.",
+                    obj="ChecksTest.test_bad_manager.<locals>.TestModel",
+                    id="ordered_model.E003",
+                )
+            ],
+        )
+
+    def test_bad_queryset(self):
+        # I've swapped the inheritance order here so that the models.QuerySet is returned
+        class BadQSModelManager(
+            models.Manager.from_queryset(models.QuerySet), OrderedModelManager
+        ):
+            pass
+
+        class TestModel(OrderedModel):
+            objects = BadQSModelManager()
+
+        self.assertEqual(
+            checks.run_checks(app_configs=self.apps.get_app_configs()),
+            [
+                checks.Error(
+                    msg="OrderedModelBase subclass ModelManager did not return a QuerySet inheriting from OrderedModelQuerySet.",
+                    obj="ChecksTest.test_bad_queryset.<locals>.TestModel",
+                    id="ordered_model.E004",
+                )
+            ],
+        )
+
+    def test_owrt_not_foreign_key(self):
+        class TestModel(OrderedModel):
+            name = models.CharField(max_length=100)
+            order_with_respect_to = "name"
+
+        self.assertEqual(
+            checks.run_checks(app_configs=self.apps.get_app_configs()),
+            [
+                checks.Error(
+                    msg="OrderedModel order_with_respect_to specifies field 'name' (within 'name') which is not a ForeignKey. This is unsupported.",
+                    obj="ChecksTest.test_owrt_not_foreign_key.<locals>.TestModel",
+                    id="ordered_model.E005",
+                )
+            ],
+        )
+
+    def test_owrt_not_immediate_foreign_key(self):
+        class TestTargetModel(OrderedModel):
+            name = models.CharField(max_length=100)
+
+        class TestModel(OrderedModel):
+            target = models.ForeignKey(to=TestTargetModel, on_delete=models.CASCADE)
+            order_with_respect_to = "target__name"
+
+        self.assertEqual(
+            checks.run_checks(app_configs=self.apps.get_app_configs()),
+            [
+                checks.Error(
+                    msg="OrderedModel order_with_respect_to specifies field 'name' (within 'target__name') which is not a ForeignKey. This is unsupported.",
+                    obj="ChecksTest.test_owrt_not_immediate_foreign_key.<locals>.TestModel",
+                    id="ordered_model.E005",
+                )
+            ],
+        )
+
 
 class TestCascadedDelete(TestCase):
     def test_that_model_when_deleted_by_cascade_still_maintains_ordering(self):

diff --git a/tests/utils.py b/tests/utils.py
@@ -0,0 +1,41 @@
+# Query count helpers, copied from django source
+# Added here for compatibility (django<3.1)
+from django.db import DEFAULT_DB_ALIAS, connections
+from django.test.utils import CaptureQueriesContext
+
+
+class _AssertNumQueriesContext(CaptureQueriesContext):
+    def __init__(self, test_case, num, connection):
+        self.test_case = test_case
+        self.num = num
+        super().__init__(connection)
+
+    def __exit__(self, exc_type, exc_value, traceback):
+        super().__exit__(exc_type, exc_value, traceback)
+        if exc_type is not None:
+            return
+        executed = len(self)
+        self.test_case.assertEqual(
+            executed,
+            self.num,
+            "%d queries executed, %d expected\nCaptured queries were:\n%s"
+            % (
+                executed,
+                self.num,
+                "\n".join(
+                    "%d. %s" % (i, query["sql"])
+                    for i, query in enumerate(self.captured_queries, start=1)
+                ),
+            ),
+        )
+
+
+def assertNumQueries(self, num, func=None, *args, using=DEFAULT_DB_ALIAS, **kwargs):
+    conn = connections[using]
+
+    context = _AssertNumQueriesContext(self, num, conn)
+    if func is None:
+        return context
+
+    with context:
+        func(*args, **kwargs)