diff_against() returns PK wrapper for deleted objs

ddabble · ddabble · commit 0589ac2e6377 · 2024-05-05T22:17:55.000+02:00
...when called with `foreign_keys_are_objs=True`.
This special dataclass (`DeletedObject`) will signal to the user that
the related object was deleted, and provides useful info - i.e. the
object's PK and model - and a verbose `__str__()` implementation.
(An alternative solution of returning `None` for deleted objects, would
introduce ambiguity if the `ForeignKey` field is nullable.)
diff --git a/docs/history_diffing.rst b/docs/history_diffing.rst
@@ -48,6 +48,11 @@ the model modifications.
     ``ForeignKey`` fields.
   - If ``True``: The diff will contain the actual related model objects instead of just
     the primary keys.
+    Deleted related objects (both foreign key objects and many-to-many objects)
+    will be instances of ``DeletedObject``, which only contain a ``model`` field with a
+    reference to the deleted object's model, as well as a ``pk`` field with the value of
+    the deleted object's primary key.
+
     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()``).
@@ -77,7 +82,9 @@ the model modifications.
 
       # Deleting all the polls:
       Poll.objects.all().delete()
-      delta_with_objs = new.diff_against(old, foreign_keys_are_objs=True)  # Will raise `Place.DoesNotExist`
+      delta_with_objs = new.diff_against(old, foreign_keys_are_objs=True)
+      # Printing the changes of `delta_with_objs` will now output:
+      # 'poll' changed from 'Deleted poll (pk=15)' to 'Deleted poll (pk=31)'
 
 
       # --- Effect on many-to-many fields ---
@@ -100,4 +107,4 @@ the model modifications.
       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}]
+      # 'categories' changed from [] to [{'poll': <Poll: what's up?>, 'category': DeletedObject(model=<class 'models.Category'>, pk=63)}]
diff --git a/simple_history/locale/nb/LC_MESSAGES/django.mo b/simple_history/locale/nb/LC_MESSAGES/django.mo
diff --git a/simple_history/locale/nb/LC_MESSAGES/django.po b/simple_history/locale/nb/LC_MESSAGES/django.po
@@ -8,7 +8,7 @@ msgstr ""
 "Project-Id-Version: django-simple-history\n"
 "Report-Msgid-Bugs-To: \n"
 "POT-Creation-Date: 2023-07-09 13:55+0200\n"
-"PO-Revision-Date: 2023-07-09 12:31+0200\n"
+"PO-Revision-Date: 2024-04-11 19:34+0200\n"
 "Last-Translator: Anders <ddabble@github.com>\n"
 "Language-Team: Norwegian Bokmål <placeholder@example.com>\n"
 "Language: nb\n"
@@ -60,6 +60,11 @@ msgstr "Endret"
 msgid "Deleted"
 msgstr "Slettet"
 
+#: simple_history/models.py:1124
+#, python-format
+msgid "Deleted %(type_name)s"
+msgstr "Slettet %(type_name)s"
+
 #: simple_history/templates/simple_history/object_history.html:11
 msgid ""
 "Choose a date from the list below to revert to a previous version of this "
diff --git a/simple_history/models.py b/simple_history/models.py
@@ -4,7 +4,7 @@
 import warnings
 from dataclasses import dataclass
 from functools import partial
-from typing import TYPE_CHECKING, Any, Dict, Iterable, List, Sequence, Union
+from typing import TYPE_CHECKING, Any, Dict, Iterable, List, Sequence, Type, Union
 
 import django
 from django.apps import apps
@@ -961,9 +961,11 @@ def diff_against(
         :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()``).
+               instead of just the PKs; deleted related objects will be instances of
+               ``DeletedObject``.
+               Note that passing ``True`` 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(
@@ -1016,13 +1018,24 @@ 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
-                ):
+                field_meta = self._meta.get_field(field)
+                if foreign_keys_are_objs and isinstance(field_meta, 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)
+                    def get_value(record, foreign_key):
+                        try:
+                            value = getattr(record, field)
+                        # `value` seems to be None (without raising this exception)
+                        # if the object has not been refreshed from the database
+                        except ObjectDoesNotExist:
+                            value = None
+
+                        if value is None:
+                            value = DeletedObject(field_meta.related_model, foreign_key)
+                        return value
+
+                    old_value = get_value(old_history, old_value)
+                    new_value = get_value(self, new_value)
 
                 change = ModelChange(field, old_value, new_value)
                 changes.append(change)
@@ -1069,10 +1082,24 @@ def _get_m2m_field_changes_for_diff(
                     # Set the through fields to their related model objects instead of
                     # the raw PKs from `values()`
                     def rows_with_foreign_key_objs(m2m_qs):
+                        def get_value(obj, through_field):
+                            try:
+                                value = getattr(obj, through_field)
+                            # If the related object has been deleted, `value` seems to
+                            # usually already be None instead of raising this exception
+                            except ObjectDoesNotExist:
+                                value = None
+
+                            if value is None:
+                                meta = m2m_through_model_opts.get_field(through_field)
+                                foreign_key = getattr(obj, meta.attname)
+                                value = DeletedObject(meta.related_model, foreign_key)
+                            return value
+
                         # Replicate the format of the return value of QuerySet.values()
                         return [
                             {
-                                through_field: getattr(through_obj, through_field)
+                                through_field: get_value(through_obj, through_field)
                                 for through_field in through_model_fields
                             }
                             for through_obj in m2m_qs.select_related(*fk_fields)
@@ -1087,11 +1114,41 @@ def rows_with_foreign_key_objs(m2m_qs):
         return changes
 
 
+@dataclass(frozen=True)
+class DeletedObject:
+    model: Type[models.Model]
+    pk: Any
+
+    def __str__(self):
+        deleted_model_str = _("Deleted %(type_name)s") % {
+            "type_name": self.model._meta.verbose_name,
+        }
+        return f"{deleted_model_str} (pk={self.pk})"
+
+
+# Either:
+# - The value of a foreign key field:
+#   - If ``foreign_keys_are_objs=True`` is passed to ``diff_against()``:
+#     Either the related object or ``DeletedObject``.
+#   - Otherwise:
+#     The PK of the related object.
+#
+# - The value of a many-to-many field:
+#   A list of dicts from the through model field names to either:
+#   - If ``foreign_keys_are_objs=True`` is passed to ``diff_against()``:
+#     Either the through model's related objects or ``DeletedObject``.
+#   - Otherwise:
+#     The PK of the through model's related objects.
+#
+# - Any of the other possible values of a model field.
+ModelChangeValue = Union[Any, DeletedObject, List[Dict[str, Union[Any, DeletedObject]]]]
+
+
 @dataclass(frozen=True)
 class ModelChange:
     field: str
-    old: Union[Any, List[Dict[str, Any]]]
-    new: Union[Any, List[Dict[str, Any]]]
+    old: ModelChangeValue
+    new: ModelChangeValue
 
 
 @dataclass(frozen=True)
diff --git a/simple_history/tests/tests/test_models.py b/simple_history/tests/tests/test_models.py
@@ -4,7 +4,6 @@
 import warnings
 from datetime import datetime, timedelta
 
-import django
 from django.apps import apps
 from django.conf import settings
 from django.contrib.auth import get_user_model
@@ -20,6 +19,7 @@
 from simple_history.exceptions import RelatedNameConflictError
 from simple_history.models import (
     SIMPLE_HISTORY_REVERSE_ATTR_NAME,
+    DeletedObject,
     HistoricalRecords,
     ModelChange,
     ModelDelta,
@@ -30,7 +30,6 @@
     pre_create_historical_m2m_records,
     pre_create_historical_record,
 )
-from simple_history.tests.custom_user.models import CustomUser
 from simple_history.tests.tests.utils import (
     database_router_override_settings,
     database_router_override_settings_history_in_diff_db,
@@ -88,7 +87,6 @@
     ModelWithSingleNoDBIndexUnique,
     MultiOneToOne,
     MyOverrideModelNameRegisterMethod1,
-    OverrideModelNameAsCallable,
     OverrideModelNameUsingBaseModel1,
     Person,
     Place,
@@ -734,7 +732,9 @@ def test_history_diff_includes_changed_fields_of_base_model(self):
 
     def test_history_diff_arg__foreign_keys_are_objs__returns_expected_fk_values(self):
         poll1 = Poll.objects.create(question="why?", pub_date=today)
+        poll1_pk = poll1.pk
         poll2 = Poll.objects.create(question="how?", pub_date=tomorrow)
+        poll2_pk = poll2.pk
         choice = Choice.objects.create(poll=poll1, choice="hmm", votes=3)
         choice.poll = poll2
         choice.choice = "idk"
@@ -747,7 +747,7 @@ def test_history_diff_arg__foreign_keys_are_objs__returns_expected_fk_values(sel
             delta = new_record.diff_against(old_record)
         expected_pk_changes = [
             ModelChange("choice", "hmm", "idk"),
-            ModelChange("poll", poll1.pk, poll2.pk),
+            ModelChange("poll", poll1_pk, poll2_pk),
             ModelChange("votes", 3, 0),
         ]
         expected_pk_delta = ModelDelta(
@@ -782,15 +782,27 @@ def test_history_diff_arg__foreign_keys_are_objs__returns_expected_fk_values(sel
         self.assertEqual(delta, expected_pk_delta)
 
         # Test with `foreign_keys_are_objs=True`
-        # (Getting instances of deleted related objects is not possible - unless they
-        # happen to be history-tracked as well, but that's not currently detected)
-        with self.assertRaises(Poll.DoesNotExist):
+        with self.assertNumQueries(2):  # Once for each poll in the new record
             delta = new_record.diff_against(old_record, foreign_keys_are_objs=True)
+        # The model objects should now instead be instances of `DeletedObject`
+        expected_obj_changes = [
+            choice_changes,
+            ModelChange(
+                "poll", DeletedObject(Poll, poll1_pk), DeletedObject(Poll, poll2_pk)
+            ),
+            votes_changes,
+        ]
+        expected_obj_delta = dataclasses.replace(
+            expected_pk_delta, changes=expected_obj_changes
+        )
+        self.assertEqual(delta, expected_obj_delta)
 
     def test_history_diff_arg__foreign_keys_are_objs__returns_expected_m2m_values(self):
         poll = PollWithManyToMany.objects.create(question="why?", pub_date=today)
         place1 = Place.objects.create(name="Here")
+        place1_pk = place1.pk
         place2 = Place.objects.create(name="There")
+        place2_pk = place2.pk
         poll.places.add(place1, place2)
         new_record, old_record = poll.history.all()
 
@@ -801,8 +813,8 @@ def test_history_diff_arg__foreign_keys_are_objs__returns_expected_m2m_values(se
             "places",
             [],
             [
-                {"pollwithmanytomany": poll.pk, "place": place1.pk},
-                {"pollwithmanytomany": poll.pk, "place": place2.pk},
+                {"pollwithmanytomany": poll.pk, "place": place1_pk},
+                {"pollwithmanytomany": poll.pk, "place": place2_pk},
             ],
         )
         expected_pk_delta = ModelDelta(
@@ -840,13 +852,12 @@ def test_history_diff_arg__foreign_keys_are_objs__returns_expected_m2m_values(se
         # Test with `foreign_keys_are_objs=True`
         with self.assertNumQueries(2 * 2):  # Twice for each record
             delta = new_record.diff_against(old_record, foreign_keys_are_objs=True)
-        # (Getting instances of deleted related objects is not possible - unless they
-        # happen to be history-tracked as well, but that's not currently detected)
+        # The model objects should now instead be instances of `DeletedObject`
         expected_obj_change = dataclasses.replace(
             expected_obj_change,
             new=[
-                {"pollwithmanytomany": poll, "place": None},
-                {"pollwithmanytomany": poll, "place": None},
+                {"pollwithmanytomany": poll, "place": DeletedObject(Place, place1_pk)},
+                {"pollwithmanytomany": poll, "place": DeletedObject(Place, place2_pk)},
             ],
         )
         expected_obj_delta = dataclasses.replace(
