Added foreign_keys_are_objs arg to diff_against()

This makes it easier to get the related objects from the diff objects,
and facilitates prefetching the related objects when listing the diffs
- which is needed for the upcoming changes to the admin history page
adding a "Changes" column.

The returned `ModelDelta` fields are now alphabetically ordered, to
prevent flakiness of one of the added tests.

The `old` and `new` M2M lists of `ModelChange` are now also sorted,
which will prevent inconsistent ordering of the `Place` objects in
`AdminSiteTest.test_history_list_contains_diff_changes_for_m2m_fields()`
(will be added as part of the previously mentioned upcoming admin
history changes).

Lastly, cleaned up some `utils` imports in `models.py`.

Author: ddabble

CHANGES.rst

@@ -17,6 +17,11 @@ Unreleased
   ``history_list_display`` by default, and made the latter into an actual field (gh-1128)
 - ``ModelDelta`` and ``ModelChange`` (in ``simple_history.models``) are now immutable
   dataclasses; their signatures remain unchanged (gh-1128)
+- ``ModelDelta``'s ``changes`` and ``changed_fields`` are now sorted alphabetically by
+  field name. Also, if ``ModelChange`` is for an M2M field, its ``old`` and ``new``
+  lists are sorted by the related object. This should help prevent flaky tests. (gh-1128)
+- ``diff_against()`` has a new keyword argument, ``foreign_keys_are_objs``;
+  see usage in the docs under "History Diffing" (gh-1128)
 
 3.5.0 (2024-02-19)
 ------------------

docs/historical_model.rst

@@ -554,7 +554,6 @@ You will see the many to many changes when diffing between two historical record
     informal = Category.objects.create(name="informal questions")
     official = Category.objects.create(name="official questions")
     p = Poll.objects.create(question="what's up?")
-    p.save()
     p.categories.add(informal, official)
     p.categories.remove(informal)
 

docs/history_diffing.rst

@@ -1,24 +1,103 @@
 History Diffing
 ===============
 
-When you have two instances of the same ``HistoricalRecord`` (such as the ``HistoricalPoll`` example above),
-you can perform diffs to see what changed. This will result in a ``ModelDelta`` containing the following properties:
+When you have two instances of the same historical model
+(such as the ``HistoricalPoll`` example above),
+you can perform a diff using the ``diff_against()`` method to see what changed.
+This will return a ``ModelDelta`` object with the following attributes:
 
-1. A list with each field changed between the two historical records
-2. A list with the names of all fields that incurred changes from one record to the other
-3. the old and new records.
+- ``old_record`` and ``new_record``: The old and new history records
+- ``changed_fields``: A list of the names of all fields that were changed between
+  ``old_record`` and ``new_record``, in alphabetical order
+- ``changes``: A list of ``ModelChange`` objects - one for each field in
+  ``changed_fields``, in the same order.
+  These objects have the following attributes:
 
-This may be useful when you want to construct timelines and need to get only the model modifications.
+  - ``field``: The name of the changed field
+    (this name is equal to the corresponding field in ``changed_fields``)
+  - ``old`` and ``new``: The old and new values of the changed field
+
+    - For many-to-many fields, these values will be lists of dicts from the through
+      model field names to the primary keys of the through model's related objects.
+      The lists are sorted by the value of the many-to-many related object.
+
+This may be useful when you want to construct timelines and need to get only
+the model modifications.
 
 .. code-block:: python
 
-    p = Poll.objects.create(question="what's up?")
-    p.question = "what's up, man?"
-    p.save()
+    poll = Poll.objects.create(question="what's up?")
+    poll.question = "what's up, man?"
+    poll.save()
 
-    new_record, old_record = p.history.all()
+    new_record, old_record = poll.history.all()
     delta = new_record.diff_against(old_record)
     for change in delta.changes:
-        print("{} changed from {} to {}".format(change.field, change.old, change.new))
+        print(f"'{change.field}' changed from '{change.old}' to '{change.new}'")
+
+    # Output:
+    # 'question' changed from 'what's up?' to 'what's up, man?'
+
+``diff_against()`` also accepts the following additional arguments:
+
+- ``excluded_fields`` and ``included_fields``: These can be used to either explicitly
+  exclude or include fields from being diffed, respectively.
+- ``foreign_keys_are_objs``:
+
+  - If ``False`` (default): The diff will only contain the raw primary keys of any
+    ``ForeignKey`` fields.
+  - If ``True``: The diff will contain the actual related model objects instead of just
+    the primary keys.
+    Note that this will add extra database queries for each related field that's been
+    changed - as long as the related objects have not been prefetched
+    (using e.g. ``select_related()``).
+
+  A couple examples showing the difference:
+
+  .. code-block:: python
+
+      # --- Effect on foreign key fields ---
+
+      whats_up = Poll.objects.create(pk=15, name="what's up?")
+      still_around = Poll.objects.create(pk=31, name="still around?")
+
+      choice = Choice.objects.create(poll=whats_up)
+      choice.poll = still_around
+      choice.save()
+
+      new, old = choice.history.all()
+
+      default_delta = new.diff_against(old)
+      # Printing the changes of `default_delta` will output:
+      # 'poll' changed from '15' to '31'
+
+      delta_with_objs = new.diff_against(old, foreign_keys_are_objs=True)
+      # Printing the changes of `delta_with_objs` will output:
+      # 'poll' changed from 'what's up?' to 'still around?'
+
+      # Deleting all the polls:
+      Poll.objects.all().delete()
+      delta_with_objs = new.diff_against(old, foreign_keys_are_objs=True)  # Will raise `Place.DoesNotExist`
+
+
+      # --- Effect on many-to-many fields ---
+
+      informal = Category.objects.create(pk=63, name="informal questions")
+      whats_up.categories.add(informal)
+
+      new = whats_up.history.latest()
+      old = new.prev_record
+
+      default_delta = new.diff_against(old)
+      # Printing the changes of `default_delta` will output:
+      # 'categories' changed from [] to [{'poll': 15, 'category': 63}]
+
+      delta_with_objs = new.diff_against(old, foreign_keys_are_objs=True)
+      # Printing the changes of `delta_with_objs` will output:
+      # 'categories' changed from [] to [{'poll': <Poll: what's up?>, 'category': <Category: informal questions>}]
 
-``diff_against`` also accepts 2 arguments ``excluded_fields`` and ``included_fields`` to either explicitly include or exclude fields from being diffed.
+      # Deleting all the categories:
+      Category.objects.all().delete()
+      delta_with_objs = new.diff_against(old, foreign_keys_are_objs=True)
+      # Printing the changes of `delta_with_objs` will now output:
+      # 'categories' changed from [] to [{'poll': <Poll: what's up?>, 'category': None}]

simple_history/models.py

@@ -4,7 +4,7 @@
 import warnings
 from dataclasses import dataclass
 from functools import partial
-from typing import Any, Dict, Iterable, List, Sequence, Union
+from typing import TYPE_CHECKING, Any, Dict, Iterable, List, Sequence, Union
 
 import django
 from django.apps import apps
@@ -31,9 +31,7 @@
 from django.utils.text import format_lazy
 from django.utils.translation import gettext_lazy as _
 
-from simple_history import utils
-
-from . import exceptions
+from . import exceptions, utils
 from .manager import (
     SIMPLE_HISTORY_REVERSE_ATTR_NAME,
     HistoricalQuerySet,
@@ -46,13 +44,17 @@
     pre_create_historical_m2m_records,
     pre_create_historical_record,
 )
-from .utils import get_change_reason_from_object
 
 try:
     from asgiref.local import Local as LocalContext
 except ImportError:
     from threading import local as LocalContext
 
+if TYPE_CHECKING:
+    ModelTypeHint = models.Model
+else:
+    ModelTypeHint = object
+
 registered_models = {}
 
 
@@ -668,7 +670,7 @@ def get_change_reason_for_object(self, instance, history_type, using):
         Get change reason for object.
         Customize this method to automatically fill change reason from context.
         """
-        return get_change_reason_from_object(instance)
+        return utils.get_change_reason_from_object(instance)
 
     def m2m_changed(self, instance, action, attr, pk_set, reverse, **_):
         if hasattr(instance, "skip_history_when_saving"):
@@ -941,8 +943,28 @@ def __get__(self, instance, owner):
         return self.model(**values)
 
 
-class HistoricalChanges:
-    def diff_against(self, old_history, excluded_fields=None, included_fields=None):
+class HistoricalChanges(ModelTypeHint):
+    def diff_against(
+        self,
+        old_history: "HistoricalChanges",
+        excluded_fields: Iterable[str] = None,
+        included_fields: Iterable[str] = None,
+        *,
+        foreign_keys_are_objs=False,
+    ) -> "ModelDelta":
+        """
+        :param old_history:
+        :param excluded_fields: The names of fields to exclude from diffing.
+               This takes precedence over ``included_fields``.
+        :param included_fields: The names of the only fields to include when diffing.
+               If not provided, all history-tracked fields will be included.
+        :param foreign_keys_are_objs: If ``False``, the returned diff will only contain
+               the raw PKs of any ``ForeignKey`` fields.
+               If ``True``, the diff will contain the actual related model objects
+               instead of just the PKs.
+               The latter case will necessarily query the database if the related
+               objects have not been prefetched (using e.g. ``select_related()``).
+        """
         if not isinstance(old_history, type(self)):
             raise TypeError(
                 "unsupported type(s) for diffing:"
@@ -965,16 +987,23 @@ def diff_against(self, old_history, excluded_fields=None, included_fields=None):
         m2m_fields = set(included_m2m_fields).difference(excluded_fields)
 
         changes = [
-            *self._get_field_changes_for_diff(old_history, fields),
-            *self._get_m2m_field_changes_for_diff(old_history, m2m_fields),
+            *self._get_field_changes_for_diff(
+                old_history, fields, foreign_keys_are_objs
+            ),
+            *self._get_m2m_field_changes_for_diff(
+                old_history, m2m_fields, foreign_keys_are_objs
+            ),
         ]
+        # Sort by field (attribute) name, to ensure a consistent order
+        changes.sort(key=lambda change: change.field)
         changed_fields = [change.field for change in changes]
         return ModelDelta(changes, changed_fields, old_history, self)
 
     def _get_field_changes_for_diff(
         self,
         old_history: "HistoricalChanges",
         fields: Iterable[str],
+        foreign_keys_are_objs: bool,
     ) -> List["ModelChange"]:
         """Helper method for ``diff_against()``."""
         changes = []
@@ -987,6 +1016,14 @@ def _get_field_changes_for_diff(
             new_value = new_values[field]
 
             if old_value != new_value:
+                if foreign_keys_are_objs and isinstance(
+                    self._meta.get_field(field), ForeignKey
+                ):
+                    # Set the fields to their related model objects instead of
+                    # the raw PKs from `model_to_dict()`
+                    old_value = getattr(old_history, field)
+                    new_value = getattr(self, field)
+
                 change = ModelChange(field, old_value, new_value)
                 changes.append(change)
 
@@ -996,14 +1033,18 @@ def _get_m2m_field_changes_for_diff(
         self,
         old_history: "HistoricalChanges",
         m2m_fields: Iterable[str],
+        foreign_keys_are_objs: bool,
     ) -> List["ModelChange"]:
         """Helper method for ``diff_against()``."""
         changes = []
 
         for field in m2m_fields:
-            old_m2m_manager = getattr(old_history, field)
-            new_m2m_manager = getattr(self, field)
-            m2m_through_model_opts = new_m2m_manager.model._meta
+            original_field_meta = self.instance_type._meta.get_field(field)
+            reverse_field_name = utils.get_m2m_reverse_field_name(original_field_meta)
+            # Sort the M2M rows by the related object, to ensure a consistent order
+            old_m2m_qs = getattr(old_history, field).order_by(reverse_field_name)
+            new_m2m_qs = getattr(self, field).order_by(reverse_field_name)
+            m2m_through_model_opts = new_m2m_qs.model._meta
 
             # Create a list of field names to compare against.
             # The list is generated without the PK of the intermediate (through)
@@ -1014,10 +1055,32 @@ def _get_m2m_field_changes_for_diff(
                 for f in m2m_through_model_opts.fields
                 if f.editable and f.name not in ["id", "m2m_history_id", "history"]
             ]
-            old_rows = list(old_m2m_manager.values(*through_model_fields))
-            new_rows = list(new_m2m_manager.values(*through_model_fields))
+            old_rows = list(old_m2m_qs.values(*through_model_fields))
+            new_rows = list(new_m2m_qs.values(*through_model_fields))
 
             if old_rows != new_rows:
+                if foreign_keys_are_objs:
+                    fk_fields = [
+                        f
+                        for f in through_model_fields
+                        if isinstance(m2m_through_model_opts.get_field(f), ForeignKey)
+                    ]
+
+                    # Set the through fields to their related model objects instead of
+                    # the raw PKs from `values()`
+                    def rows_with_foreign_key_objs(m2m_qs):
+                        # Replicate the format of the return value of QuerySet.values()
+                        return [
+                            {
+                                through_field: getattr(through_obj, through_field)
+                                for through_field in through_model_fields
+                            }
+                            for through_obj in m2m_qs.select_related(*fk_fields)
+                        ]
+
+                    old_rows = rows_with_foreign_key_objs(old_m2m_qs)
+                    new_rows = rows_with_foreign_key_objs(new_m2m_qs)
+
                 change = ModelChange(field, old_rows, new_rows)
                 changes.append(change)