Optimise next prev record (#791)

* Optimise `next_record` and `prev_record` Previously they called `.instance`, which was unnecessary as only the `_meta` was needed. By using the class itself, the need for an extra query is removed in cases where fields are excluded. * Add query assertions to other cases to reduce query creep * Update test names so they run, and fix them Methods are only run as test cases if they start with `test_` * Add self to `AUTHORS.rst` * Remove unnecessary Q object * Add changelog entry for #791 * Ensure history is still filtered by the correct model This means history for another model doesn't appear incorrectly. Co-authored-by: Jim King <[email protected]>
jazzband · Sep 16, 2021 · e3065b9 · e3065b9
1 parent c5943f4
commit e3065b9
Show file tree

Hide file tree

Showing 6 changed files with 76 additions and 27 deletions.
diff --git a/AUTHORS.rst b/AUTHORS.rst
@@ -51,6 +51,7 @@ Authors
 - Hernan Esteves (`sevetseh28 <https://github.com/sevetseh28>`_)
 - Hielke Walinga (`hwalinga <https://github.com/hwalinga>`_)
 - Jack Cushman (`jcushman <https://github.com/jcushman>`_)
+- Jake Howard (`RealOrangeOne <https://github.com/realorangeone>`_)
 - James Muranga (`jamesmura <https://github.com/jamesmura>`_)
 - James Pulec
 - Jesse Shapiro

diff --git a/CHANGES.rst b/CHANGES.rst
@@ -4,6 +4,7 @@ Changes
 Unreleased
 ----------
 
+- Fixed ``prev_record`` and ``next_record`` performance when using ``excluded_fields`` (gh-791)
 - Fixed `update_change_reason` in pk (gh-806)
 - Fixed bug where serializer of djangorestframework crashed if used with ``OrderingFilter`` (gh-821)
 - Fixed `make format` so it works by using tox (gh-859)

diff --git a/simple_history/manager.py b/simple_history/manager.py
@@ -2,7 +2,10 @@
 from django.db.models import OuterRef, Subquery
 from django.utils import timezone
 
-from simple_history.utils import get_change_reason_from_object
+from simple_history.utils import (
+    get_app_model_primary_key_name,
+    get_change_reason_from_object,
+)
 
 
 class HistoryDescriptor:
@@ -29,10 +32,7 @@ def get_queryset(self):
         if self.instance is None:
             return qs
 
-        if isinstance(self.instance._meta.pk, models.ForeignKey):
-            key_name = self.instance._meta.pk.name + "_id"
-        else:
-            key_name = self.instance._meta.pk.name
+        key_name = get_app_model_primary_key_name(self.instance)
         return self.get_super_queryset().filter(**{key_name: self.instance.pk})
 
     def most_recent(self):

diff --git a/simple_history/models.py b/simple_history/models.py
@@ -9,7 +9,7 @@
 from django.contrib.auth import get_user_model
 from django.core.exceptions import ObjectDoesNotExist
 from django.db import models
-from django.db.models import ManyToManyField, Q
+from django.db.models import ManyToManyField
 from django.db.models.fields.proxy import OrderWrt
 from django.forms.models import model_to_dict
 from django.urls import reverse
@@ -399,9 +399,9 @@ def get_next_record(self):
             """
             Get the next history record for the instance. `None` if last.
             """
-            history = utils.get_history_manager_for_model(self.instance)
+            history = utils.get_history_manager_from_history(self)
             return (
-                history.filter(Q(history_date__gt=self.history_date))
+                history.filter(history_date__gt=self.history_date)
                 .order_by("history_date")
                 .first()
             )
@@ -410,9 +410,9 @@ def get_prev_record(self):
             """
             Get the previous history record for the instance. `None` if first.
             """
-            history = utils.get_history_manager_for_model(self.instance)
+            history = utils.get_history_manager_from_history(self)
             return (
-                history.filter(Q(history_date__lt=self.history_date))
+                history.filter(history_date__lt=self.history_date)
                 .order_by("history_date")
                 .last()
             )

diff --git a/simple_history/tests/tests/test_models.py b/simple_history/tests/tests/test_models.py
@@ -690,9 +690,12 @@ def test_get_prev_record(self):
         third_record = self.poll.history.filter(question="eh?").get()
         fourth_record = self.poll.history.filter(question="one more?").get()
 
-        self.assertRecordsMatch(second_record.prev_record, first_record)
-        self.assertRecordsMatch(third_record.prev_record, second_record)
-        self.assertRecordsMatch(fourth_record.prev_record, third_record)
+        with self.assertNumQueries(1):
+            self.assertRecordsMatch(second_record.prev_record, first_record)
+        with self.assertNumQueries(1):
+            self.assertRecordsMatch(third_record.prev_record, second_record)
+        with self.assertNumQueries(1):
+            self.assertRecordsMatch(fourth_record.prev_record, third_record)
 
     def test_get_prev_record_none_if_only(self):
         self.assertEqual(self.poll.history.count(), 1)
@@ -705,14 +708,26 @@ def test_get_prev_record_none_if_earliest(self):
         first_record = self.poll.history.filter(question="what's up?").get()
         self.assertIsNone(first_record.prev_record)
 
-    def get_prev_record_with_custom_manager_name(self):
-        instance = CustomManagerNameModel(name="Test name 1")
-        instance.save()
+    def test_get_prev_record_with_custom_manager_name(self):
+        instance = CustomManagerNameModel.objects.create(name="Test name 1")
         instance.name = "Test name 2"
-        first_record = instance.log.filter(name="Test name").get()
+        instance.save()
+        first_record = instance.log.filter(name="Test name 1").get()
         second_record = instance.log.filter(name="Test name 2").get()
 
-        self.assertRecordsMatch(second_record.prev_record, first_record)
+        self.assertEqual(second_record.prev_record, first_record)
+
+    def test_get_prev_record_with_excluded_field(self):
+        instance = PollWithExcludeFields.objects.create(
+            question="what's up?", pub_date=today
+        )
+        instance.question = "ask questions?"
+        instance.save()
+        first_record = instance.history.filter(question="what's up?").get()
+        second_record = instance.history.filter(question="ask questions?").get()
+
+        with self.assertNumQueries(1):
+            self.assertRecordsMatch(second_record.prev_record, first_record)
 
     def test_get_next_record(self):
         self.poll.question = "ask questions?"
@@ -727,9 +742,12 @@ def test_get_next_record(self):
         fourth_record = self.poll.history.filter(question="one more?").get()
         self.assertIsNone(fourth_record.next_record)
 
-        self.assertRecordsMatch(first_record.next_record, second_record)
-        self.assertRecordsMatch(second_record.next_record, third_record)
-        self.assertRecordsMatch(third_record.next_record, fourth_record)
+        with self.assertNumQueries(1):
+            self.assertRecordsMatch(first_record.next_record, second_record)
+        with self.assertNumQueries(1):
+            self.assertRecordsMatch(second_record.next_record, third_record)
+        with self.assertNumQueries(1):
+            self.assertRecordsMatch(third_record.next_record, fourth_record)
 
     def test_get_next_record_none_if_only(self):
         self.assertEqual(self.poll.history.count(), 1)
@@ -742,14 +760,26 @@ def test_get_next_record_none_if_most_recent(self):
         recent_record = self.poll.history.filter(question="ask questions?").get()
         self.assertIsNone(recent_record.next_record)
 
-    def get_next_record_with_custom_manager_name(self):
-        instance = CustomManagerNameModel(name="Test name 1")
-        instance.save()
+    def test_get_next_record_with_custom_manager_name(self):
+        instance = CustomManagerNameModel.objects.create(name="Test name 1")
         instance.name = "Test name 2"
-        first_record = instance.log.filter(name="Test name").get()
+        instance.save()
+        first_record = instance.log.filter(name="Test name 1").get()
         second_record = instance.log.filter(name="Test name 2").get()
 
-        self.assertRecordsMatch(first_record.next_record, second_record)
+        self.assertEqual(first_record.next_record, second_record)
+
+    def test_get_next_record_with_excluded_field(self):
+        instance = PollWithExcludeFields.objects.create(
+            question="what's up?", pub_date=today
+        )
+        instance.question = "ask questions?"
+        instance.save()
+        first_record = instance.history.filter(question="what's up?").get()
+        second_record = instance.history.filter(question="ask questions?").get()
+
+        with self.assertNumQueries(1):
+            self.assertRecordsMatch(first_record.next_record, second_record)
 
 
 class CreateHistoryModelTests(unittest.TestCase):

diff --git a/simple_history/utils.py b/simple_history/utils.py
@@ -2,7 +2,7 @@
 
 import django
 from django.db import transaction
-from django.db.models import ManyToManyField
+from django.db.models import ForeignKey, ManyToManyField
 from django.forms.models import model_to_dict
 
 from simple_history.exceptions import AlternativeManagerError, NotHistoricalModelError
@@ -40,11 +40,28 @@ def get_history_manager_for_model(model):
     return getattr(model, manager_name)
 
 
+def get_history_manager_from_history(history_instance):
+    """
+    Return the history manager, based on an existing history instance.
+    """
+    key_name = get_app_model_primary_key_name(history_instance.instance_type)
+    return get_history_manager_for_model(history_instance.instance_type).filter(
+        **{key_name: getattr(history_instance, key_name)}
+    )
+
+
 def get_history_model_for_model(model):
     """Return the history model for a given app model."""
     return get_history_manager_for_model(model).model
 
 
+def get_app_model_primary_key_name(model):
+    """Return the primary key name for a given app model."""
+    if isinstance(model._meta.pk, ForeignKey):
+        return model._meta.pk.name + "_id"
+    return model._meta.pk.name
+
+
 def bulk_create_with_history(
     objs,
     model,