From 74a2e381bf78b0ca6afe7bb0f12019cdcea72491 Mon Sep 17 00:00:00 2001 From: Anders <6058745+ddabble@users.noreply.github.com> Date: Thu, 5 Sep 2024 21:40:51 +0200 Subject: [PATCH 1/8] Deprecated HistoricalRecords.thread `HistoricalRecords.context` was added in favor of it a few years ago, and since it has always been undocumented, it's time to properly deprecate it. 4.0 would have been the natural version to remove it if it were documented as part of the publicly exposed API, but since it's not, 3.10 seems like a reasonable choice. Also removed importing `threading.local` in case `asgiref.local.Local` is not available, as the latter should always be installed now that all our supported Django versions use it as a requirement. --- CHANGES.rst | 11 +++++++ simple_history/models.py | 31 ++++++++++++++----- .../tests/tests/test_deprecation.py | 21 ++++++++++++- 3 files changed, 55 insertions(+), 8 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 9c44563c..3dc996bb 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -4,8 +4,19 @@ Changes Unreleased ---------- +**What's new:** + - Made ``skip_history_when_saving`` work when creating an object - not just when updating an object (gh-1262) + +**Deprecations:** + +- Deprecated the undocumented ``HistoricalRecords.thread`` - use + ``HistoricalRecords.context`` instead. The former attribute will be removed in + version 3.10 (gh-1387) + +**Fixes and improvements:** + - Improved performance of the ``latest_of_each()`` history manager method (gh-1360) 3.7.0 (2024-05-29) diff --git a/simple_history/models.py b/simple_history/models.py index 3ffe42a5..887b42b3 100644 --- a/simple_history/models.py +++ b/simple_history/models.py @@ -4,9 +4,20 @@ import warnings from dataclasses import dataclass from functools import partial -from typing import TYPE_CHECKING, Any, Dict, Iterable, List, Sequence, Type, Union +from typing import ( + TYPE_CHECKING, + Any, + ClassVar, + Dict, + Iterable, + List, + Sequence, + Type, + Union, +) import django +from asgiref.local import Local from django.apps import apps from django.conf import settings from django.contrib import admin @@ -45,11 +56,6 @@ pre_create_historical_record, ) -try: - from asgiref.local import Local as LocalContext -except ImportError: - from threading import local as LocalContext - if TYPE_CHECKING: ModelTypeHint = models.Model else: @@ -83,9 +89,20 @@ def _history_user_setter(historical_instance, user): class HistoricalRecords: DEFAULT_MODEL_NAME_PREFIX = "Historical" - thread = context = LocalContext() # retain thread for backwards compatibility + context: ClassVar = Local() m2m_models = {} + class _DeprecatedThreadDescriptor: + def __get__(self, *args, **kwargs): + warnings.warn( + "Use 'HistoricalRecords.context' instead." + " The 'thread' attribute will be removed in version 3.10.", + DeprecationWarning, + ) + return HistoricalRecords.context + + thread: ClassVar = _DeprecatedThreadDescriptor() + def __init__( self, verbose_name=None, diff --git a/simple_history/tests/tests/test_deprecation.py b/simple_history/tests/tests/test_deprecation.py index 7c8ca999..1364f8d1 100644 --- a/simple_history/tests/tests/test_deprecation.py +++ b/simple_history/tests/tests/test_deprecation.py @@ -1,13 +1,32 @@ import unittest from simple_history import __version__ +from simple_history.models import HistoricalRecords from simple_history.templatetags.simple_history_admin_list import display_list class DeprecationWarningTest(unittest.TestCase): - def test__display_list__warns_deprecation_and_is_yet_to_be_removed(self): + def test__display_list__warns_deprecation(self): with self.assertWarns(DeprecationWarning): display_list({}) # DEV: `display_list()` (and the file `simple_history_admin_list.py`) should be # removed when 3.8 is released self.assertLess(__version__, "3.8") + + def test__HistoricalRecords_thread__warns_deprecation(self): + with self.assertWarns(DeprecationWarning): + context = HistoricalRecords.thread + self.assertIs(context, HistoricalRecords.context) + with self.assertWarns(DeprecationWarning): + context = getattr(HistoricalRecords, "thread") + self.assertIs(context, HistoricalRecords.context) + with self.assertWarns(DeprecationWarning): + context = HistoricalRecords().thread + self.assertIs(context, HistoricalRecords.context) + with self.assertWarns(DeprecationWarning): + context = getattr(HistoricalRecords(), "thread") + self.assertIs(context, HistoricalRecords.context) + + # DEV: `_DeprecatedThreadDescriptor` and the `thread` attribute of + # `HistoricalRecords` should be removed when 3.10 is released + self.assertLess(__version__, "3.10") From b36a280a4c0eea9a467bb4ee66bc40b9c028edae Mon Sep 17 00:00:00 2001 From: Anders <6058745+ddabble@users.noreply.github.com> Date: Fri, 6 Sep 2024 08:41:14 +0200 Subject: [PATCH 2/8] Moved disabling history docs to new section It didn't really fit under the "Querying History" section. --- CHANGES.rst | 2 ++ docs/disabling_history.rst | 51 ++++++++++++++++++++++++++++++++++++++ docs/index.rst | 1 + docs/querying_history.rst | 50 ------------------------------------- 4 files changed, 54 insertions(+), 50 deletions(-) create mode 100644 docs/disabling_history.rst diff --git a/CHANGES.rst b/CHANGES.rst index 3dc996bb..2c7d87bd 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -18,6 +18,8 @@ Unreleased **Fixes and improvements:** - Improved performance of the ``latest_of_each()`` history manager method (gh-1360) +- Moved the "Save without creating historical records" subsection of "Querying History" + in the docs to a new section: "Disable Creating Historical Records" (gh-1387) 3.7.0 (2024-05-29) ------------------ diff --git a/docs/disabling_history.rst b/docs/disabling_history.rst new file mode 100644 index 00000000..9fc288a1 --- /dev/null +++ b/docs/disabling_history.rst @@ -0,0 +1,51 @@ +Disable Creating Historical Records +=================================== + +Save without creating historical records +---------------------------------------- + +If you want to save model objects without triggering the creation of any historical +records, you can do the following: + +.. code-block:: python + + poll.skip_history_when_saving = True + poll.save() + # We recommend deleting the attribute afterward + del poll.skip_history_when_saving + +This also works when creating an object, but only when calling ``save()``: + +.. code-block:: python + + # Note that `Poll.objects.create()` is not called + poll = Poll(question="Why?") + poll.skip_history_when_saving = True + poll.save() + del poll.skip_history_when_saving + +.. note:: + Historical records will always be created when calling the ``create()`` manager method. + +Alternatively, call the ``save_without_historical_record()`` method on each object +instead of ``save()``. +This method is automatically added to a model when registering it for history-tracking +(i.e. defining a ``HistoricalRecords`` manager field on the model), +and it looks like this: + +.. code-block:: python + + def save_without_historical_record(self, *args, **kwargs): + self.skip_history_when_saving = True + try: + ret = self.save(*args, **kwargs) + finally: + del self.skip_history_when_saving + return ret + +Or disable the creation of historical records for *all* models +by adding the following line to your settings: + +.. code-block:: python + + SIMPLE_HISTORY_ENABLED = False diff --git a/docs/index.rst b/docs/index.rst index 2276ba9f..d135d8ae 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -66,6 +66,7 @@ Documentation admin historical_model user_tracking + disabling_history signals history_diffing multiple_dbs diff --git a/docs/querying_history.rst b/docs/querying_history.rst index bdf33971..d7ef113d 100644 --- a/docs/querying_history.rst +++ b/docs/querying_history.rst @@ -175,56 +175,6 @@ model history. -Save without creating historical records ----------------------------------------- - -If you want to save model objects without triggering the creation of any historical -records, you can do the following: - -.. code-block:: python - - poll.skip_history_when_saving = True - poll.save() - # We recommend deleting the attribute afterward - del poll.skip_history_when_saving - -This also works when creating an object, but only when calling ``save()``: - -.. code-block:: python - - # Note that `Poll.objects.create()` is not called - poll = Poll(question="Why?") - poll.skip_history_when_saving = True - poll.save() - del poll.skip_history_when_saving - -.. note:: - Historical records will always be created when calling the ``create()`` manager method. - -Alternatively, call the ``save_without_historical_record()`` method on each object -instead of ``save()``. -This method is automatically added to a model when registering it for history-tracking -(i.e. defining a ``HistoricalRecords`` manager field on the model), -and it looks like this: - -.. code-block:: python - - def save_without_historical_record(self, *args, **kwargs): - self.skip_history_when_saving = True - try: - ret = self.save(*args, **kwargs) - finally: - del self.skip_history_when_saving - return ret - -Or disable the creation of historical records for *all* models -by adding the following line to your settings: - -.. code-block:: python - - SIMPLE_HISTORY_ENABLED = False - - Filtering data using a relationship to the model ------------------------------------------------ From e2894b54009298c924c5e70656492a085c404529 Mon Sep 17 00:00:00 2001 From: Anders <6058745+ddabble@users.noreply.github.com> Date: Fri, 6 Sep 2024 08:56:52 +0200 Subject: [PATCH 3/8] Added delete_without_historical_record() --- CHANGES.rst | 2 + docs/disabling_history.rst | 37 +++++++++---------- simple_history/models.py | 27 ++++++++++++-- simple_history/tests/tests/test_models.py | 45 +++++++++++++++-------- 4 files changed, 71 insertions(+), 40 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 2c7d87bd..c6b8f94d 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -8,6 +8,8 @@ Unreleased - Made ``skip_history_when_saving`` work when creating an object - not just when updating an object (gh-1262) +- Added ``delete_without_historical_record()`` to all history-tracked model objects, + which complements ``save_without_historical_record()`` (gh-1387) **Deprecations:** diff --git a/docs/disabling_history.rst b/docs/disabling_history.rst index 9fc288a1..1e3c16f2 100644 --- a/docs/disabling_history.rst +++ b/docs/disabling_history.rst @@ -1,16 +1,26 @@ Disable Creating Historical Records =================================== -Save without creating historical records ----------------------------------------- +``save_without_historical_record()`` and ``delete_without_historical_record()`` +------------------------------------------------------------------------------- -If you want to save model objects without triggering the creation of any historical -records, you can do the following: +These methods are automatically added to a model when registering it for history-tracking +(i.e. defining a ``HistoricalRecords`` manager on the model), +and can be called instead of ``save()`` and ``delete()``, respectively. + +Setting the ``skip_history_when_saving`` attribute +-------------------------------------------------- + +If you want to save or delete model objects without triggering the creation of any +historical records, you can do the following: .. code-block:: python poll.skip_history_when_saving = True + # It applies both when saving... poll.save() + # ...and when deleting + poll.delete() # We recommend deleting the attribute afterward del poll.skip_history_when_saving @@ -27,23 +37,10 @@ This also works when creating an object, but only when calling ``save()``: .. note:: Historical records will always be created when calling the ``create()`` manager method. -Alternatively, call the ``save_without_historical_record()`` method on each object -instead of ``save()``. -This method is automatically added to a model when registering it for history-tracking -(i.e. defining a ``HistoricalRecords`` manager field on the model), -and it looks like this: - -.. code-block:: python - - def save_without_historical_record(self, *args, **kwargs): - self.skip_history_when_saving = True - try: - ret = self.save(*args, **kwargs) - finally: - del self.skip_history_when_saving - return ret +The ``SIMPLE_HISTORY_ENABLED`` setting +-------------------------------------- -Or disable the creation of historical records for *all* models +Disable the creation of historical records for *all* models by adding the following line to your settings: .. code-block:: python diff --git a/simple_history/models.py b/simple_history/models.py index 887b42b3..ac053598 100644 --- a/simple_history/models.py +++ b/simple_history/models.py @@ -195,7 +195,7 @@ def contribute_to_class(self, cls, name): warnings.warn(msg, UserWarning) def add_extra_methods(self, cls): - def save_without_historical_record(self, *args, **kwargs): + def save_without_historical_record(self: models.Model, *args, **kwargs): """ Save the model instance without creating a historical record. @@ -208,7 +208,21 @@ def save_without_historical_record(self, *args, **kwargs): del self.skip_history_when_saving return ret - setattr(cls, "save_without_historical_record", save_without_historical_record) + def delete_without_historical_record(self: models.Model, *args, **kwargs): + """ + Delete the model instance without creating a historical record. + + Make sure you know what you're doing before using this method. + """ + self.skip_history_when_saving = True + try: + ret = self.delete(*args, **kwargs) + finally: + del self.skip_history_when_saving + return ret + + cls.save_without_historical_record = save_without_historical_record + cls.delete_without_historical_record = delete_without_historical_record def finalize(self, sender, **kwargs): inherited = False @@ -665,7 +679,9 @@ def get_meta_options(self, model): ) return meta_fields - def post_save(self, instance, created, using=None, **kwargs): + def post_save( + self, instance: models.Model, created: bool, using: str = None, **kwargs + ): if not getattr(settings, "SIMPLE_HISTORY_ENABLED", True): return if hasattr(instance, "skip_history_when_saving"): @@ -674,9 +690,12 @@ def post_save(self, instance, created, using=None, **kwargs): if not kwargs.get("raw", False): self.create_historical_record(instance, created and "+" or "~", using=using) - def post_delete(self, instance, using=None, **kwargs): + def post_delete(self, instance: models.Model, using: str = None, **kwargs): if not getattr(settings, "SIMPLE_HISTORY_ENABLED", True): return + if hasattr(instance, "skip_history_when_saving"): + return + if self.cascade_delete_history: manager = getattr(instance, self.manager_name) manager.using(using).all().delete() diff --git a/simple_history/tests/tests/test_models.py b/simple_history/tests/tests/test_models.py index d70ca3ce..c9daea9e 100644 --- a/simple_history/tests/tests/test_models.py +++ b/simple_history/tests/tests/test_models.py @@ -262,7 +262,14 @@ def test_cascade_delete_history(self): self.assertEqual(len(thames.history.all()), 1) self.assertEqual(len(nile.history.all()), 0) - def test_save_without_historical_record(self): + def test_registered_model_has_extra_methods(self): + model = ExternalModelSpecifiedWithAppParam.objects.create( + name="registered model" + ) + self.assertTrue(hasattr(model, "save_without_historical_record")) + self.assertTrue(hasattr(model, "delete_without_historical_record")) + + def test__save_without_historical_record__creates_no_records(self): pizza_place = Restaurant.objects.create(name="Pizza Place", rating=3) pizza_place.rating = 4 pizza_place.save_without_historical_record() @@ -290,6 +297,26 @@ def test_save_without_historical_record(self): }, ) + def test__delete_without_historical_record__creates_no_records(self): + self.assertEqual(Restaurant.objects.count(), 0) + pizza_place = Restaurant.objects.create(name="Pizza Place", rating=3) + self.assertEqual(Restaurant.objects.count(), 1) + pizza_place.rating = 4 + pizza_place.delete_without_historical_record() + self.assertEqual(Restaurant.objects.count(), 0) + + (create_record,) = Restaurant.updates.all() + self.assertRecordValues( + create_record, + Restaurant, + { + "name": "Pizza Place", + "rating": 3, + "id": pizza_place.id, + "history_type": "+", + }, + ) + @override_settings(SIMPLE_HISTORY_ENABLED=False) def test_save_with_disabled_history(self): anthony = Person.objects.create(name="Anthony Gillard") @@ -299,12 +326,6 @@ def test_save_with_disabled_history(self): anthony.delete() self.assertEqual(Person.history.count(), 0) - def test_save_without_historical_record_for_registered_model(self): - model = ExternalModelSpecifiedWithAppParam.objects.create( - name="registered model" - ) - self.assertTrue(hasattr(model, "save_without_historical_record")) - def test_save_raises_exception(self): anthony = Person(name="Anthony Gillard") with self.assertRaises(RuntimeError): @@ -2142,7 +2163,7 @@ def setUp(self): self.poll = self.model.objects.create(question="what's up?", pub_date=today) -class ManyToManyTest(TestCase): +class ManyToManyTest(HistoricalTestCase): def setUp(self): self.model = PollWithManyToMany self.history_model = self.model.history.model @@ -2154,14 +2175,6 @@ def setUp(self): def assertDatetimesEqual(self, time1, time2): self.assertAlmostEqual(time1, time2, delta=timedelta(seconds=2)) - def assertRecordValues(self, record, klass, values_dict): - for key, value in values_dict.items(): - self.assertEqual(getattr(record, key), value) - self.assertEqual(record.history_object.__class__, klass) - for key, value in values_dict.items(): - if key not in ["history_type", "history_change_reason"]: - self.assertEqual(getattr(record.history_object, key), value) - def test_create(self): # There should be 1 history record for our poll, the create from setUp self.assertEqual(self.poll.history.all().count(), 1) From c2576e3514049fb950551773af7189d9e18976d5 Mon Sep 17 00:00:00 2001 From: Anders <6058745+ddabble@users.noreply.github.com> Date: Sat, 7 Sep 2024 15:53:30 +0200 Subject: [PATCH 4/8] Changed the order of some util functions --- simple_history/tests/tests/test_utils.py | 43 ++++++++++++------------ simple_history/utils.py | 24 ++++++------- 2 files changed, 34 insertions(+), 33 deletions(-) diff --git a/simple_history/tests/tests/test_utils.py b/simple_history/tests/tests/test_utils.py index 7db701d9..e5290e9d 100644 --- a/simple_history/tests/tests/test_utils.py +++ b/simple_history/tests/tests/test_utils.py @@ -10,7 +10,17 @@ from django.utils import timezone from simple_history.exceptions import AlternativeManagerError, NotHistoricalModelError -from simple_history.tests.models import ( +from simple_history.utils import ( + bulk_create_with_history, + bulk_update_with_history, + get_history_manager_for_model, + get_history_model_for_model, + get_m2m_field_name, + get_m2m_reverse_field_name, + update_change_reason, +) + +from ..models import ( BulkCreateManyToManyModel, Document, Place, @@ -28,19 +38,21 @@ PollWithUniqueQuestion, Street, ) -from simple_history.utils import ( - bulk_create_with_history, - bulk_update_with_history, - get_history_manager_for_model, - get_history_model_for_model, - get_m2m_field_name, - get_m2m_reverse_field_name, - update_change_reason, -) User = get_user_model() +class UpdateChangeReasonTestCase(TestCase): + def test_update_change_reason_with_excluded_fields(self): + poll = PollWithExcludeFields( + question="what's up?", pub_date=timezone.now(), place="The Pub" + ) + poll.save() + update_change_reason(poll, "Test change reason.") + most_recent = poll.history.order_by("-history_date").first() + self.assertEqual(most_recent.history_change_reason, "Test change reason.") + + class GetM2MFieldNamesTestCase(unittest.TestCase): def test__get_m2m_field_name__returns_expected_value(self): def field_names(model): @@ -629,14 +641,3 @@ def test_bulk_manager_with_custom_model_attributes(self): PollWithHistoricalSessionAttr.history.filter(session="co-op").count(), 5, ) - - -class UpdateChangeReasonTestCase(TestCase): - def test_update_change_reason_with_excluded_fields(self): - poll = PollWithExcludeFields( - question="what's up?", pub_date=timezone.now(), place="The Pub" - ) - poll.save() - update_change_reason(poll, "Test change reason.") - most_recent = poll.history.order_by("-history_date").first() - self.assertEqual(most_recent.history_change_reason, "Test change reason.") diff --git a/simple_history/utils.py b/simple_history/utils.py index a5bafeaf..fbe037f1 100644 --- a/simple_history/utils.py +++ b/simple_history/utils.py @@ -5,6 +5,13 @@ from simple_history.exceptions import AlternativeManagerError, NotHistoricalModelError +def get_change_reason_from_object(obj): + if hasattr(obj, "_change_reason"): + return getattr(obj, "_change_reason") + + return None + + def update_change_reason(instance, reason): attrs = {} model = type(instance) @@ -35,6 +42,11 @@ def get_history_manager_for_model(model): return getattr(model, manager_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_history_manager_from_history(history_instance): """ Return the history manager, based on an existing history instance. @@ -45,11 +57,6 @@ def get_history_manager_from_history(history_instance): ) -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): @@ -233,10 +240,3 @@ def bulk_update_with_history( custom_historical_attrs=custom_historical_attrs, ) return rows_updated - - -def get_change_reason_from_object(obj): - if hasattr(obj, "_change_reason"): - return getattr(obj, "_change_reason") - - return None From 23c37dda6c1d70d934ac53bb2f1ef4469a890f9e Mon Sep 17 00:00:00 2001 From: Anders <6058745+ddabble@users.noreply.github.com> Date: Sat, 7 Sep 2024 15:43:57 +0200 Subject: [PATCH 5/8] Code cleanup of some util functions Incl. removing `HistoryManager.get_super_queryset()` and renaming the following `utils` functions: * get_history_manager_from_history -> get_historical_records_of_instance * get_app_model_primary_key_name -> get_pk_name Also added some tests for some of the changed util functions. --- CHANGES.rst | 7 + simple_history/manager.py | 16 +- simple_history/models.py | 4 +- simple_history/tests/tests/test_utils.py | 211 +++++++++++++++++++++++ simple_history/tests/tests/utils.py | 15 +- simple_history/utils.py | 53 +++--- 6 files changed, 266 insertions(+), 40 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index c6b8f94d..193de381 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -11,6 +11,13 @@ Unreleased - Added ``delete_without_historical_record()`` to all history-tracked model objects, which complements ``save_without_historical_record()`` (gh-1387) +**Breaking changes:** + +- Removed ``HistoryManager.get_super_queryset()`` (gh-1387) +- Renamed the ``utils`` functions ``get_history_manager_from_history()`` + to ``get_historical_records_of_instance()`` and ``get_app_model_primary_key_name()`` + to ``get_pk_name()`` (gh-1387) + **Deprecations:** - Deprecated the undocumented ``HistoricalRecords.thread`` - use diff --git a/simple_history/manager.py b/simple_history/manager.py index 2d9bda65..03f53034 100644 --- a/simple_history/manager.py +++ b/simple_history/manager.py @@ -3,10 +3,7 @@ from django.db.models import Exists, OuterRef, Q, QuerySet from django.utils import timezone -from simple_history.utils import ( - get_app_model_primary_key_name, - get_change_reason_from_object, -) +from . import utils # when converting a historical record to an instance, this attribute is added # to the instance so that code can reverse the instance to its historical record @@ -118,16 +115,13 @@ def __init__(self, model, instance=None): self.model = model self.instance = instance - def get_super_queryset(self): - return super().get_queryset() - def get_queryset(self): - qs = self.get_super_queryset() + qs = super().get_queryset() if self.instance is None: return qs - key_name = get_app_model_primary_key_name(self.instance) - return self.get_super_queryset().filter(**{key_name: self.instance.pk}) + pk_name = utils.get_pk_name(self.instance._meta.model) + return qs.filter(**{pk_name: self.instance.pk}) def most_recent(self): """ @@ -241,7 +235,7 @@ def bulk_history_create( instance, "_history_date", default_date or timezone.now() ), history_user=history_user, - history_change_reason=get_change_reason_from_object(instance) + history_change_reason=utils.get_change_reason_from_object(instance) or default_change_reason, history_type=history_type, **{ diff --git a/simple_history/models.py b/simple_history/models.py index ac053598..27ca59b8 100644 --- a/simple_history/models.py +++ b/simple_history/models.py @@ -567,7 +567,7 @@ def get_next_record(self): """ Get the next history record for the instance. `None` if last. """ - history = utils.get_history_manager_from_history(self) + history = utils.get_historical_records_of_instance(self) return ( history.filter(history_date__gt=self.history_date) .order_by("history_date") @@ -578,7 +578,7 @@ def get_prev_record(self): """ Get the previous history record for the instance. `None` if first. """ - history = utils.get_history_manager_from_history(self) + history = utils.get_historical_records_of_instance(self) return ( history.filter(history_date__lt=self.history_date) .order_by("history_date") diff --git a/simple_history/tests/tests/test_utils.py b/simple_history/tests/tests/test_utils.py index e5290e9d..e7adac03 100644 --- a/simple_history/tests/tests/test_utils.py +++ b/simple_history/tests/tests/test_utils.py @@ -1,42 +1,88 @@ import unittest +from dataclasses import dataclass from datetime import datetime +from typing import Optional, Type from unittest import skipUnless from unittest.mock import Mock, patch import django from django.contrib.auth import get_user_model from django.db import IntegrityError, transaction +from django.db.models import Model from django.test import TestCase, TransactionTestCase, override_settings from django.utils import timezone from simple_history.exceptions import AlternativeManagerError, NotHistoricalModelError +from simple_history.manager import HistoryManager +from simple_history.models import HistoricalChanges from simple_history.utils import ( bulk_create_with_history, bulk_update_with_history, + get_historical_records_of_instance, get_history_manager_for_model, get_history_model_for_model, get_m2m_field_name, get_m2m_reverse_field_name, + get_pk_name, update_change_reason, ) +from ..external import models as external from ..models import ( + AbstractBase, + AbstractModelCallable1, + BaseModel, + Book, BulkCreateManyToManyModel, + Choice, + ConcreteAttr, + ConcreteExternal, + ConcreteUtil, + Contact, + ContactRegister, + CustomManagerNameModel, Document, + ExternalModelSpecifiedWithAppParam, + ExternalModelWithAppLabel, + FirstLevelInheritedModel, + HardbackBook, + HistoricalBook, + HistoricalPoll, + HistoricalPollInfo, + InheritTracking1, + ModelWithHistoryInDifferentApp, + ModelWithHistoryUsingBaseModelDb, + OverrideModelNameAsCallable, + OverrideModelNameRegisterMethod1, + OverrideModelNameUsingBaseModel1, Place, Poll, PollChildBookWithManyToMany, PollChildRestaurantWithManyToMany, + PollInfo, + PollParentWithManyToMany, PollWithAlternativeManager, + PollWithCustomManager, + PollWithExcludedFKField, PollWithExcludeFields, PollWithHistoricalSessionAttr, PollWithManyToMany, PollWithManyToManyCustomHistoryID, PollWithManyToManyWithIPAddress, + PollWithQuerySetCustomizations, PollWithSelfManyToMany, PollWithSeveralManyToMany, PollWithUniqueQuestion, + Profile, + Restaurant, Street, + TestHistoricParticipanToHistoricOrganization, + TestParticipantToHistoricOrganization, + TrackedAbstractBaseA, + TrackedConcreteBase, + TrackedWithAbstractBase, + TrackedWithConcreteBase, + Voter, ) User = get_user_model() @@ -53,6 +99,171 @@ def test_update_change_reason_with_excluded_fields(self): self.assertEqual(most_recent.history_change_reason, "Test change reason.") +@dataclass +class HistoryTrackedModelTestInfo: + model: Type[Model] + history_manager_name: Optional[str] + + def __init__( + self, + model: Type[Model], + history_manager_name: Optional[str] = "history", + ): + self.model = model + self.history_manager_name = history_manager_name + + +class GetHistoryManagerAndModelHelpersTestCase(TestCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + + H = HistoryTrackedModelTestInfo + cls.history_tracked_models = [ + H(Choice), + H(ConcreteAttr), + H(ConcreteExternal), + H(ConcreteUtil), + H(Contact), + H(ContactRegister), + H(CustomManagerNameModel, "log"), + H(ExternalModelSpecifiedWithAppParam, "histories"), + H(ExternalModelWithAppLabel), + H(InheritTracking1), + H(ModelWithHistoryInDifferentApp), + H(ModelWithHistoryUsingBaseModelDb), + H(OverrideModelNameAsCallable), + H(OverrideModelNameRegisterMethod1), + H(OverrideModelNameUsingBaseModel1), + H(Poll), + H(PollChildBookWithManyToMany), + H(PollWithAlternativeManager), + H(PollWithCustomManager), + H(PollWithExcludedFKField), + H(PollWithHistoricalSessionAttr), + H(PollWithManyToMany), + H(PollWithManyToManyCustomHistoryID), + H(PollWithManyToManyWithIPAddress), + H(PollWithQuerySetCustomizations), + H(PollWithSelfManyToMany), + H(Restaurant, "updates"), + H(TestHistoricParticipanToHistoricOrganization), + H(TrackedConcreteBase), + H(TrackedWithAbstractBase), + H(TrackedWithConcreteBase), + H(Voter), + H(external.ExternalModel), + H(external.ExternalModelRegistered, "histories"), + H(external.Poll), + ] + cls.models_without_history_manager = [ + H(AbstractBase, None), + H(AbstractModelCallable1, None), + H(BaseModel, None), + H(FirstLevelInheritedModel, None), + H(HardbackBook, None), + H(Place, None), + H(PollParentWithManyToMany, None), + H(Profile, None), + H(TestParticipantToHistoricOrganization, None), + H(TrackedAbstractBaseA, None), + ] + + def test__get_history_manager_for_model(self): + """Test that ``get_history_manager_for_model()`` returns the expected value + for various models.""" + + def assert_history_manager(history_manager, info: HistoryTrackedModelTestInfo): + expected_manager = getattr(info.model, info.history_manager_name) + expected_historical_model = expected_manager.model + historical_model = history_manager.model + # Can't compare the managers directly, as the history manager classes are + # dynamically created through `HistoryDescriptor` + self.assertIsInstance(history_manager, HistoryManager) + self.assertIsInstance(expected_manager, HistoryManager) + self.assertTrue(issubclass(historical_model, HistoricalChanges)) + self.assertEqual(historical_model.instance_type, info.model) + self.assertEqual(historical_model, expected_historical_model) + + for model_info in self.history_tracked_models: + with self.subTest(model_info=model_info): + model = model_info.model + manager = get_history_manager_for_model(model) + assert_history_manager(manager, model_info) + + for model_info in self.models_without_history_manager: + with self.subTest(model_info=model_info): + model = model_info.model + with self.assertRaises(NotHistoricalModelError): + get_history_manager_for_model(model) + + def test__get_history_model_for_model(self): + """Test that ``get_history_model_for_model()`` returns the expected value + for various models.""" + for model_info in self.history_tracked_models: + with self.subTest(model_info=model_info): + model = model_info.model + historical_model = get_history_model_for_model(model) + self.assertTrue(issubclass(historical_model, HistoricalChanges)) + self.assertEqual(historical_model.instance_type, model) + + for model_info in self.models_without_history_manager: + with self.subTest(model_info=model_info): + model = model_info.model + with self.assertRaises(NotHistoricalModelError): + get_history_model_for_model(model) + + def test__get_pk_name(self): + """Test that ``get_pk_name()`` returns the expected value for various models.""" + self.assertEqual(get_pk_name(Poll), "id") + self.assertEqual(get_pk_name(PollInfo), "poll_id") + self.assertEqual(get_pk_name(Book), "isbn") + + self.assertEqual(get_pk_name(HistoricalPoll), "history_id") + self.assertEqual(get_pk_name(HistoricalPollInfo), "history_id") + self.assertEqual(get_pk_name(HistoricalBook), "history_id") + + +class GetHistoricalRecordsOfInstanceTestCase(TestCase): + def test__get_historical_records_of_instance(self): + """Test that ``get_historical_records_of_instance()`` returns the expected + queryset for history-tracked model instances.""" + poll1 = Poll.objects.create(pub_date=timezone.now()) + poll1_history = poll1.history.all() + (record1_1,) = poll1_history + self.assertQuerySetEqual( + get_historical_records_of_instance(record1_1), + poll1_history, + ) + + poll2 = Poll.objects.create(pub_date=timezone.now()) + poll2.question = "?" + poll2.save() + poll2_history = poll2.history.all() + (record2_2, record2_1) = poll2_history + self.assertQuerySetEqual( + get_historical_records_of_instance(record2_1), + poll2_history, + ) + self.assertQuerySetEqual( + get_historical_records_of_instance(record2_2), + poll2_history, + ) + + poll3 = Poll.objects.create(id=123, pub_date=timezone.now()) + poll3.delete() + poll3_history = Poll.history.filter(id=123) + (record3_2, record3_1) = poll3_history + self.assertQuerySetEqual( + get_historical_records_of_instance(record3_1), + poll3_history, + ) + self.assertQuerySetEqual( + get_historical_records_of_instance(record3_2), + poll3_history, + ) + + class GetM2MFieldNamesTestCase(unittest.TestCase): def test__get_m2m_field_name__returns_expected_value(self): def field_names(model): diff --git a/simple_history/tests/tests/utils.py b/simple_history/tests/tests/utils.py index ae6fe949..3700b95f 100644 --- a/simple_history/tests/tests/utils.py +++ b/simple_history/tests/tests/utils.py @@ -26,13 +26,14 @@ def assertRecordValues(self, record, klass: Type[Model], values_dict: dict): :param klass: The type of the history-tracked class of ``record``. :param values_dict: Field names of ``record`` mapped to their expected values. """ - for key, value in values_dict.items(): - self.assertEqual(getattr(record, key), value) - - self.assertEqual(record.history_object.__class__, klass) - for key, value in values_dict.items(): - if key not in ("history_type", "history_change_reason"): - self.assertEqual(getattr(record.history_object, key), value) + for field_name, expected_value in values_dict.items(): + self.assertEqual(getattr(record, field_name), expected_value) + + history_object = record.history_object + self.assertEqual(history_object.__class__, klass) + for field_name, expected_value in values_dict.items(): + if field_name not in ("history_type", "history_change_reason"): + self.assertEqual(getattr(history_object, field_name), expected_value) class TestDbRouter: diff --git a/simple_history/utils.py b/simple_history/utils.py index fbe037f1..f28c2a3a 100644 --- a/simple_history/utils.py +++ b/simple_history/utils.py @@ -1,18 +1,21 @@ +from typing import TYPE_CHECKING, Optional, Type + from django.db import transaction -from django.db.models import Case, ForeignKey, ManyToManyField, Q, When +from django.db.models import Case, ForeignKey, ManyToManyField, Model, Q, When from django.forms.models import model_to_dict -from simple_history.exceptions import AlternativeManagerError, NotHistoricalModelError +from .exceptions import AlternativeManagerError, NotHistoricalModelError +if TYPE_CHECKING: + from .manager import HistoricalQuerySet, HistoryManager + from .models import HistoricalChanges -def get_change_reason_from_object(obj): - if hasattr(obj, "_change_reason"): - return getattr(obj, "_change_reason") - return None +def get_change_reason_from_object(obj: Model) -> Optional[str]: + return getattr(obj, "_change_reason", None) -def update_change_reason(instance, reason): +def update_change_reason(instance: Model, reason: Optional[str]) -> None: attrs = {} model = type(instance) manager = instance if instance.pk is not None else model @@ -33,8 +36,12 @@ def update_change_reason(instance, reason): record.save() -def get_history_manager_for_model(model): - """Return the history manager for a given app model.""" +def get_history_manager_for_model(model: Type[Model]) -> "HistoryManager": + """Return the history manager for ``model``. + + :raise NotHistoricalModelError: If the model has not been registered to track + history. + """ try: manager_name = model._meta.simple_history_manager_attribute except AttributeError: @@ -42,25 +49,31 @@ def get_history_manager_for_model(model): return getattr(model, manager_name) -def get_history_model_for_model(model): - """Return the history model for a given app model.""" +def get_history_model_for_model(model: Type[Model]) -> Type["HistoricalChanges"]: + """Return the history model for ``model``. + + :raise NotHistoricalModelError: If the model has not been registered to track + history. + """ return get_history_manager_for_model(model).model -def get_history_manager_from_history(history_instance): +def get_historical_records_of_instance( + historical_record: "HistoricalChanges", +) -> "HistoricalQuerySet": """ - Return the history manager, based on an existing history instance. + Return a queryset with all the historical records of the same instance as + ``historical_record`` is for. """ - 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)} - ) + pk_name = get_pk_name(historical_record.instance_type) + manager = get_history_manager_for_model(historical_record.instance_type) + return manager.filter(**{pk_name: getattr(historical_record, pk_name)}) -def get_app_model_primary_key_name(model): - """Return the primary key name for a given app model.""" +def get_pk_name(model: Type[Model]) -> str: + """Return the primary key name for ``model``.""" if isinstance(model._meta.pk, ForeignKey): - return model._meta.pk.name + "_id" + return f"{model._meta.pk.name}_id" return model._meta.pk.name From 78286f6f5b691d932c89e56e3e7f5dbbc5608047 Mon Sep 17 00:00:00 2001 From: Anders <6058745+ddabble@users.noreply.github.com> Date: Sun, 8 Sep 2024 00:12:41 +0200 Subject: [PATCH 6/8] Made some helper utils support models and instances Their code already works with passing an instance instead of a model, so might as well make it official. --- CHANGES.rst | 3 + simple_history/tests/tests/test_utils.py | 74 ++++++++++++++++++------ simple_history/utils.py | 24 +++++--- 3 files changed, 75 insertions(+), 26 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 193de381..0b567c17 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -29,6 +29,9 @@ Unreleased - Improved performance of the ``latest_of_each()`` history manager method (gh-1360) - Moved the "Save without creating historical records" subsection of "Querying History" in the docs to a new section: "Disable Creating Historical Records" (gh-1387) +- The ``utils`` functions ``get_history_manager_for_model()`` and + ``get_history_model_for_model()`` now explicitly support being passed model instances + instead of just model types (gh-1387) 3.7.0 (2024-05-29) ------------------ diff --git a/simple_history/tests/tests/test_utils.py b/simple_history/tests/tests/test_utils.py index e7adac03..002e74c9 100644 --- a/simple_history/tests/tests/test_utils.py +++ b/simple_history/tests/tests/test_utils.py @@ -77,6 +77,7 @@ Restaurant, Street, TestHistoricParticipanToHistoricOrganization, + TestOrganizationWithHistory, TestParticipantToHistoricOrganization, TrackedAbstractBaseA, TrackedConcreteBase, @@ -103,14 +104,17 @@ def test_update_change_reason_with_excluded_fields(self): class HistoryTrackedModelTestInfo: model: Type[Model] history_manager_name: Optional[str] + init_kwargs: dict def __init__( self, model: Type[Model], history_manager_name: Optional[str] = "history", + **init_kwargs, ): self.model = model self.history_manager_name = history_manager_name + self.init_kwargs = init_kwargs class GetHistoryManagerAndModelHelpersTestCase(TestCase): @@ -118,9 +122,19 @@ class GetHistoryManagerAndModelHelpersTestCase(TestCase): def setUpClass(cls): super().setUpClass() + user = User.objects.create(username="user") + poll_kwargs = {"pub_date": timezone.now()} + poll = Poll.objects.create(**poll_kwargs) + choice_kwargs = {"poll": poll, "votes": 0} + choice = Choice.objects.create(**choice_kwargs) + place = Place.objects.create() + org_kwarg = { + "organization": TestOrganizationWithHistory.objects.create(), + } + H = HistoryTrackedModelTestInfo cls.history_tracked_models = [ - H(Choice), + H(Choice, **choice_kwargs), H(ConcreteAttr), H(ConcreteExternal), H(ConcreteUtil), @@ -135,23 +149,23 @@ def setUpClass(cls): H(OverrideModelNameAsCallable), H(OverrideModelNameRegisterMethod1), H(OverrideModelNameUsingBaseModel1), - H(Poll), - H(PollChildBookWithManyToMany), - H(PollWithAlternativeManager), - H(PollWithCustomManager), - H(PollWithExcludedFKField), + H(Poll, **poll_kwargs), + H(PollChildBookWithManyToMany, **poll_kwargs), + H(PollWithAlternativeManager, **poll_kwargs), + H(PollWithCustomManager, **poll_kwargs), + H(PollWithExcludedFKField, place=place, **poll_kwargs), H(PollWithHistoricalSessionAttr), - H(PollWithManyToMany), - H(PollWithManyToManyCustomHistoryID), - H(PollWithManyToManyWithIPAddress), - H(PollWithQuerySetCustomizations), + H(PollWithManyToMany, **poll_kwargs), + H(PollWithManyToManyCustomHistoryID, **poll_kwargs), + H(PollWithManyToManyWithIPAddress, **poll_kwargs), + H(PollWithQuerySetCustomizations, **poll_kwargs), H(PollWithSelfManyToMany), - H(Restaurant, "updates"), - H(TestHistoricParticipanToHistoricOrganization), + H(Restaurant, "updates", rating=0), + H(TestHistoricParticipanToHistoricOrganization, **org_kwarg), H(TrackedConcreteBase), H(TrackedWithAbstractBase), H(TrackedWithConcreteBase), - H(Voter), + H(Voter, user=user, choice=choice), H(external.ExternalModel), H(external.ExternalModelRegistered, "histories"), H(external.Poll), @@ -161,11 +175,11 @@ def setUpClass(cls): H(AbstractModelCallable1, None), H(BaseModel, None), H(FirstLevelInheritedModel, None), - H(HardbackBook, None), + H(HardbackBook, None, isbn="123", price=0), H(Place, None), - H(PollParentWithManyToMany, None), - H(Profile, None), - H(TestParticipantToHistoricOrganization, None), + H(PollParentWithManyToMany, None, **poll_kwargs), + H(Profile, None, date_of_birth=timezone.now().date()), + H(TestParticipantToHistoricOrganization, None, **org_kwarg), H(TrackedAbstractBaseA, None), ] @@ -191,12 +205,25 @@ def assert_history_manager(history_manager, info: HistoryTrackedModelTestInfo): manager = get_history_manager_for_model(model) assert_history_manager(manager, model_info) + # Passing a model instance should also work + instance = model(**model_info.init_kwargs) + instance.save() + manager = get_history_manager_for_model(instance) + assert_history_manager(manager, model_info) + for model_info in self.models_without_history_manager: with self.subTest(model_info=model_info): model = model_info.model with self.assertRaises(NotHistoricalModelError): get_history_manager_for_model(model) + # The same error should be raised if passing a model instance + if not model._meta.abstract: + instance = model(**model_info.init_kwargs) + instance.save() + with self.assertRaises(NotHistoricalModelError): + get_history_manager_for_model(instance) + def test__get_history_model_for_model(self): """Test that ``get_history_model_for_model()`` returns the expected value for various models.""" @@ -207,12 +234,25 @@ def test__get_history_model_for_model(self): self.assertTrue(issubclass(historical_model, HistoricalChanges)) self.assertEqual(historical_model.instance_type, model) + # Passing a model instance should also work + instance = model(**model_info.init_kwargs) + instance.save() + historical_model_from_instance = get_history_model_for_model(instance) + self.assertEqual(historical_model_from_instance, historical_model) + for model_info in self.models_without_history_manager: with self.subTest(model_info=model_info): model = model_info.model with self.assertRaises(NotHistoricalModelError): get_history_model_for_model(model) + # The same error should be raised if passing a model instance + if not model._meta.abstract: + instance = model(**model_info.init_kwargs) + instance.save() + with self.assertRaises(NotHistoricalModelError): + get_history_model_for_model(instance) + def test__get_pk_name(self): """Test that ``get_pk_name()`` returns the expected value for various models.""" self.assertEqual(get_pk_name(Poll), "id") diff --git a/simple_history/utils.py b/simple_history/utils.py index f28c2a3a..fe272ff6 100644 --- a/simple_history/utils.py +++ b/simple_history/utils.py @@ -1,4 +1,4 @@ -from typing import TYPE_CHECKING, Optional, Type +from typing import TYPE_CHECKING, Optional, Type, Union from django.db import transaction from django.db.models import Case, ForeignKey, ManyToManyField, Model, Q, When @@ -36,26 +36,32 @@ def update_change_reason(instance: Model, reason: Optional[str]) -> None: record.save() -def get_history_manager_for_model(model: Type[Model]) -> "HistoryManager": - """Return the history manager for ``model``. +def get_history_manager_for_model( + model_or_instance: Union[Type[Model], Model] +) -> "HistoryManager": + """Return the history manager for ``model_or_instance``. :raise NotHistoricalModelError: If the model has not been registered to track history. """ try: - manager_name = model._meta.simple_history_manager_attribute + manager_name = model_or_instance._meta.simple_history_manager_attribute except AttributeError: - raise NotHistoricalModelError(f"Cannot find a historical model for {model}.") - return getattr(model, manager_name) + raise NotHistoricalModelError( + f"Cannot find a historical model for {model_or_instance}." + ) + return getattr(model_or_instance, manager_name) -def get_history_model_for_model(model: Type[Model]) -> Type["HistoricalChanges"]: - """Return the history model for ``model``. +def get_history_model_for_model( + model_or_instance: Union[Type[Model], Model] +) -> Type["HistoricalChanges"]: + """Return the history model for ``model_or_instance``. :raise NotHistoricalModelError: If the model has not been registered to track history. """ - return get_history_manager_for_model(model).model + return get_history_manager_for_model(model_or_instance).model def get_historical_records_of_instance( From 8ea956c4067f166642336a32c2c478a55dc14c56 Mon Sep 17 00:00:00 2001 From: Anders <6058745+ddabble@users.noreply.github.com> Date: Sun, 8 Sep 2024 01:33:12 +0200 Subject: [PATCH 7/8] Added a disable_history() context manager This replaces setting the `skip_history_when_saving` attribute on a model instance, which has been deprecated (and replaced with `disable_history()` or the utils mentioned below where possible), as well as having been removed from the docs and tests. (`HistoricalRecords.post_delete()` does not generate deprecation warnings on it, since support for `skip_history_when_saving` while deleting objects is part of the next, unreleased version.) The context manager was inspired by https://github.com/jazzband/django-simple-history/issues/642#issuecomment-922110914 Also added a couple useful utils related to `disable_history()`: `is_history_disabled()` and a `DisableHistoryInfo` dataclass - see their docstrings in `utils.py`. --- CHANGES.rst | 6 + docs/disabling_history.rst | 48 +- simple_history/manager.py | 9 +- simple_history/models.py | 51 +- simple_history/tests/models.py | 5 +- .../tests/tests/test_deprecation.py | 27 +- simple_history/tests/tests/test_manager.py | 43 ++ simple_history/tests/tests/test_models.py | 43 +- simple_history/tests/tests/test_utils.py | 495 +++++++++++++++++- simple_history/tests/tests/utils.py | 28 +- simple_history/utils.py | 165 +++++- 11 files changed, 815 insertions(+), 105 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 0b567c17..5c3ed6da 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -10,6 +10,9 @@ Unreleased updating an object (gh-1262) - Added ``delete_without_historical_record()`` to all history-tracked model objects, which complements ``save_without_historical_record()`` (gh-1387) +- Added a ``disable_history()`` context manager, which disables history record creation + while it's active; see usage in the docs under "Disable Creating Historical Records" + (gh-1387) **Breaking changes:** @@ -23,6 +26,9 @@ Unreleased - Deprecated the undocumented ``HistoricalRecords.thread`` - use ``HistoricalRecords.context`` instead. The former attribute will be removed in version 3.10 (gh-1387) +- Deprecated ``skip_history_when_saving`` in favor of the newly added + ``disable_history()`` context manager. The former attribute will be removed in + version 4.0 (gh-1387) **Fixes and improvements:** diff --git a/docs/disabling_history.rst b/docs/disabling_history.rst index 1e3c16f2..adafd9df 100644 --- a/docs/disabling_history.rst +++ b/docs/disabling_history.rst @@ -8,34 +8,40 @@ These methods are automatically added to a model when registering it for history (i.e. defining a ``HistoricalRecords`` manager on the model), and can be called instead of ``save()`` and ``delete()``, respectively. -Setting the ``skip_history_when_saving`` attribute --------------------------------------------------- +Using the ``disable_history()`` context manager +----------------------------------------------- -If you want to save or delete model objects without triggering the creation of any -historical records, you can do the following: +``disable_history()`` has three ways of being called: -.. code-block:: python - - poll.skip_history_when_saving = True - # It applies both when saving... - poll.save() - # ...and when deleting - poll.delete() - # We recommend deleting the attribute afterward - del poll.skip_history_when_saving +#. With no arguments: This will disable all historical record creation + (as if the ``SIMPLE_HISTORY_ENABLED`` setting was set to ``False``; see below) + within the context manager's ``with`` block. +#. With ``only_for_model``: Only disable history creation for the provided model type. +#. With ``instance_predicate``: Only disable history creation for model instances passing + this predicate. -This also works when creating an object, but only when calling ``save()``: +See some examples below: .. code-block:: python - # Note that `Poll.objects.create()` is not called - poll = Poll(question="Why?") - poll.skip_history_when_saving = True - poll.save() - del poll.skip_history_when_saving + from simple_history.utils import disable_history + + # No historical records are created + with disable_history(): + User.objects.create(...) + Poll.objects.create(...) + + # A historical record is only created for the poll + with disable_history(only_for_model=User): + User.objects.create(...) + Poll.objects.create(...) -.. note:: - Historical records will always be created when calling the ``create()`` manager method. + # A historical record is created for the second poll, but not for the first poll + # (remember to check the instance type in the passed function if you expect + # historical records of more than one model to be created inside the `with` block) + with disable_history(instance_predicate=lambda poll: "ignore" in poll.question): + Poll.objects.create(question="ignore this") + Poll.objects.create(question="what's up?") The ``SIMPLE_HISTORY_ENABLED`` setting -------------------------------------- diff --git a/simple_history/manager.py b/simple_history/manager.py index 03f53034..66b2cb08 100644 --- a/simple_history/manager.py +++ b/simple_history/manager.py @@ -1,4 +1,3 @@ -from django.conf import settings from django.db import models from django.db.models import Exists, OuterRef, Q, QuerySet from django.utils import timezone @@ -216,8 +215,9 @@ def bulk_history_create( If called by bulk_update_with_history, use the update boolean and save the history_type accordingly. """ - if not getattr(settings, "SIMPLE_HISTORY_ENABLED", True): - return + info = utils.DisableHistoryInfo.get() + if info.disabled_globally: + return [] history_type = "+" if update: @@ -225,6 +225,9 @@ def bulk_history_create( historical_instances = [] for instance in objs: + if info.disabled_for(instance): + continue + history_user = getattr( instance, "_history_user", diff --git a/simple_history/models.py b/simple_history/models.py index 27ca59b8..d612b00b 100644 --- a/simple_history/models.py +++ b/simple_history/models.py @@ -7,6 +7,7 @@ from typing import ( TYPE_CHECKING, Any, + Callable, ClassVar, Dict, Iterable, @@ -195,18 +196,22 @@ def contribute_to_class(self, cls, name): warnings.warn(msg, UserWarning) def add_extra_methods(self, cls): + def get_instance_predicate( + self: models.Model, + ) -> Callable[[models.Model], bool]: + def predicate(instance: models.Model) -> bool: + return instance == self + + return predicate + def save_without_historical_record(self: models.Model, *args, **kwargs): """ Save the model instance without creating a historical record. Make sure you know what you're doing before using this method. """ - self.skip_history_when_saving = True - try: - ret = self.save(*args, **kwargs) - finally: - del self.skip_history_when_saving - return ret + with utils.disable_history(instance_predicate=get_instance_predicate(self)): + return self.save(*args, **kwargs) def delete_without_historical_record(self: models.Model, *args, **kwargs): """ @@ -214,12 +219,8 @@ def delete_without_historical_record(self: models.Model, *args, **kwargs): Make sure you know what you're doing before using this method. """ - self.skip_history_when_saving = True - try: - ret = self.delete(*args, **kwargs) - finally: - del self.skip_history_when_saving - return ret + with utils.disable_history(instance_predicate=get_instance_predicate(self)): + return self.delete(*args, **kwargs) cls.save_without_historical_record = save_without_historical_record cls.delete_without_historical_record = delete_without_historical_record @@ -682,18 +683,22 @@ def get_meta_options(self, model): def post_save( self, instance: models.Model, created: bool, using: str = None, **kwargs ): - if not getattr(settings, "SIMPLE_HISTORY_ENABLED", True): - return if hasattr(instance, "skip_history_when_saving"): + warnings.warn( + "Setting 'skip_history_when_saving' has been deprecated in favor of the" + " 'disable_history()' context manager." + " Support for the former attribute will be removed in version 4.0.", + DeprecationWarning, + ) + return + if utils.is_history_disabled(instance): return if not kwargs.get("raw", False): self.create_historical_record(instance, created and "+" or "~", using=using) def post_delete(self, instance: models.Model, using: str = None, **kwargs): - if not getattr(settings, "SIMPLE_HISTORY_ENABLED", True): - return - if hasattr(instance, "skip_history_when_saving"): + if utils.is_history_disabled(instance): return if self.cascade_delete_history: @@ -709,10 +714,16 @@ def get_change_reason_for_object(self, instance, history_type, using): """ return utils.get_change_reason_from_object(instance) - def m2m_changed(self, instance, action, attr, pk_set, reverse, **_): - if not getattr(settings, "SIMPLE_HISTORY_ENABLED", True): - return + def m2m_changed(self, instance: models.Model, action: str, **kwargs): if hasattr(instance, "skip_history_when_saving"): + warnings.warn( + "Setting 'skip_history_when_saving' has been deprecated in favor of the" + " 'disable_history()' context manager." + " Support for the former attribute will be removed in version 4.0.", + DeprecationWarning, + ) + return + if utils.is_history_disabled(instance): return if action in ("post_add", "post_remove", "post_clear"): diff --git a/simple_history/tests/models.py b/simple_history/tests/models.py index f35b5cf6..87e840c7 100644 --- a/simple_history/tests/models.py +++ b/simple_history/tests/models.py @@ -5,10 +5,9 @@ from django.conf import settings from django.db import models from django.db.models.deletion import CASCADE -from django.db.models.fields.related import ForeignKey from django.urls import reverse -from simple_history import register +from simple_history import register, utils from simple_history.manager import HistoricalQuerySet, HistoryManager from simple_history.models import HistoricalRecords, HistoricForeignKey @@ -355,7 +354,7 @@ class Person(models.Model): history = HistoricalRecords() def save(self, *args, **kwargs): - if hasattr(self, "skip_history_when_saving"): + if utils.DisableHistoryInfo.get().disabled_for(self): raise RuntimeError("error while saving") else: super().save(*args, **kwargs) diff --git a/simple_history/tests/tests/test_deprecation.py b/simple_history/tests/tests/test_deprecation.py index 1364f8d1..b3f327c0 100644 --- a/simple_history/tests/tests/test_deprecation.py +++ b/simple_history/tests/tests/test_deprecation.py @@ -1,11 +1,14 @@ -import unittest +from django.test import TestCase +from django.utils import timezone from simple_history import __version__ from simple_history.models import HistoricalRecords from simple_history.templatetags.simple_history_admin_list import display_list +from ..models import Place, PollWithManyToMany -class DeprecationWarningTest(unittest.TestCase): + +class DeprecationWarningTest(TestCase): def test__display_list__warns_deprecation(self): with self.assertWarns(DeprecationWarning): display_list({}) @@ -30,3 +33,23 @@ def test__HistoricalRecords_thread__warns_deprecation(self): # DEV: `_DeprecatedThreadDescriptor` and the `thread` attribute of # `HistoricalRecords` should be removed when 3.10 is released self.assertLess(__version__, "3.10") + + def test__skip_history_when_saving__warns_deprecation(self): + place = Place.objects.create(name="Here") + + poll = PollWithManyToMany(question="why?", pub_date=timezone.now()) + poll.skip_history_when_saving = True + with self.assertWarns(DeprecationWarning): + poll.save() + poll.question = "how?" + with self.assertWarns(DeprecationWarning): + poll.save() + with self.assertWarns(DeprecationWarning): + poll.places.add(place) + self.assertEqual(PollWithManyToMany.history.count(), 0) + self.assertEqual(poll.history.count(), 0) + + # DEV: The `if` statements checking for `skip_history_when_saving` (in the + # `post_save()` and `m2m_changed()` methods of `HistoricalRecords`) + # should be removed when 4.0 is released + self.assertLess(__version__, "4.0") diff --git a/simple_history/tests/tests/test_manager.py b/simple_history/tests/tests/test_manager.py index acb9e025..dbbefcd0 100644 --- a/simple_history/tests/tests/test_manager.py +++ b/simple_history/tests/tests/test_manager.py @@ -6,6 +6,7 @@ from django.test import TestCase, override_settings, skipUnlessDBFeature from simple_history.manager import SIMPLE_HISTORY_REVERSE_ATTR_NAME +from simple_history.utils import disable_history from ..models import Choice, Document, Poll, RankedDocument from .utils import HistoricalTestCase @@ -289,6 +290,23 @@ def test_simple_bulk_history_create_without_history_enabled(self): Poll.history.bulk_history_create(self.data) self.assertEqual(Poll.history.count(), 0) + def test_simple_bulk_history_create_with__disable_history(self): + with disable_history(): + Poll.history.bulk_history_create(self.data) + self.assertEqual(Poll.history.count(), 0) + + def test_simple_bulk_history_create_with__disable_history__only_for_model(self): + with disable_history(only_for_model=Poll): + Poll.history.bulk_history_create(self.data) + self.assertEqual(Poll.history.count(), 0) + + def test_simple_bulk_history_create_with__disable_history__instance_predicate(self): + with disable_history(instance_predicate=lambda poll: poll.id == 2): + Poll.history.bulk_history_create(self.data) + self.assertEqual(Poll.history.count(), 3) + historical_poll_ids = sorted(record.id for record in Poll.history.all()) + self.assertListEqual(historical_poll_ids, [1, 3, 4]) + def test_bulk_history_create_with_change_reason(self): for poll in self.data: poll._change_reason = "reason" @@ -412,6 +430,31 @@ def test_simple_bulk_history_create(self): self.assertEqual(created, []) self.assertEqual(Poll.history.count(), 4) + @override_settings(SIMPLE_HISTORY_ENABLED=False) + def test_simple_bulk_history_update_without_history_enabled(self): + Poll.history.bulk_history_create(self.data, update=True) + self.assertEqual(Poll.history.count(), 0) + + def test_simple_bulk_history_update_with__disable_history(self): + with disable_history(): + Poll.history.bulk_history_create(self.data, update=True) + self.assertEqual(Poll.history.count(), 0) + + def test_simple_bulk_history_update_with__disable_history__only_for_model(self): + with disable_history(only_for_model=Poll): + Poll.history.bulk_history_create(self.data, update=True) + self.assertEqual(Poll.history.count(), 0) + + def test_simple_bulk_history_update_with__disable_history__instance_predicate(self): + with disable_history(instance_predicate=lambda poll: poll.id == 2): + Poll.history.bulk_history_create(self.data, update=True) + self.assertEqual(Poll.history.count(), 3) + historical_poll_ids = sorted(record.id for record in Poll.history.all()) + self.assertListEqual(historical_poll_ids, [1, 3, 4]) + self.assertTrue( + all(record.history_type == "~" for record in Poll.history.all()) + ) + def test_bulk_history_create_with_change_reason(self): for poll in self.data: poll._change_reason = "reason" diff --git a/simple_history/tests/tests/test_models.py b/simple_history/tests/tests/test_models.py index c9daea9e..b9ffb8fa 100644 --- a/simple_history/tests/tests/test_models.py +++ b/simple_history/tests/tests/test_models.py @@ -318,7 +318,7 @@ def test__delete_without_historical_record__creates_no_records(self): ) @override_settings(SIMPLE_HISTORY_ENABLED=False) - def test_save_with_disabled_history(self): + def test_saving_without_history_enabled_creates_no_records(self): anthony = Person.objects.create(name="Anthony Gillard") anthony.name = "something else" anthony.save() @@ -330,7 +330,6 @@ def test_save_raises_exception(self): anthony = Person(name="Anthony Gillard") with self.assertRaises(RuntimeError): anthony.save_without_historical_record() - self.assertFalse(hasattr(anthony, "skip_history_when_saving")) self.assertEqual(Person.history.count(), 0) anthony.save() self.assertEqual(Person.history.count(), 1) @@ -2430,45 +2429,9 @@ def test_m2m_relation(self): self.assertEqual(self.poll.history.all()[0].places.count(), 0) self.assertEqual(poll_2.history.all()[0].places.count(), 2) - def test_skip_history_when_updating_an_object(self): - skip_poll = PollWithManyToMany.objects.create( - question="skip history?", pub_date=today - ) - self.assertEqual(skip_poll.history.all().count(), 1) - self.assertEqual(skip_poll.history.all()[0].places.count(), 0) - - skip_poll.skip_history_when_saving = True - - skip_poll.question = "huh?" - skip_poll.save() - skip_poll.places.add(self.place) - - self.assertEqual(skip_poll.history.all().count(), 1) - self.assertEqual(skip_poll.history.all()[0].places.count(), 0) - - del skip_poll.skip_history_when_saving - place_2 = Place.objects.create(name="Place 2") - - skip_poll.places.add(place_2) - - self.assertEqual(skip_poll.history.all().count(), 2) - self.assertEqual(skip_poll.history.all()[0].places.count(), 2) - - def test_skip_history_when_creating_an_object(self): - initial_poll_count = PollWithManyToMany.objects.count() - - skip_poll = PollWithManyToMany(question="skip history?", pub_date=today) - skip_poll.skip_history_when_saving = True - skip_poll.save() - skip_poll.places.add(self.place) - - self.assertEqual(skip_poll.history.count(), 0) - self.assertEqual(PollWithManyToMany.objects.count(), initial_poll_count + 1) - self.assertEqual(skip_poll.places.count(), 1) - @override_settings(SIMPLE_HISTORY_ENABLED=False) - def test_saving_with_disabled_history_doesnt_create_records(self): - # 1 from `setUp()` + def test_saving_without_history_enabled_creates_no_records(self): + # 1 record from `setUp()` self.assertEqual(PollWithManyToMany.history.count(), 1) poll = PollWithManyToMany.objects.create( diff --git a/simple_history/tests/tests/test_utils.py b/simple_history/tests/tests/test_utils.py index 002e74c9..9ff334fa 100644 --- a/simple_history/tests/tests/test_utils.py +++ b/simple_history/tests/tests/test_utils.py @@ -1,9 +1,11 @@ import unittest +from collections.abc import Callable from dataclasses import dataclass from datetime import datetime -from typing import Optional, Type +from enum import Enum, auto +from typing import Final, List, Optional, Type, Union from unittest import skipUnless -from unittest.mock import Mock, patch +from unittest.mock import ANY, Mock, patch import django from django.contrib.auth import get_user_model @@ -14,16 +16,20 @@ from simple_history.exceptions import AlternativeManagerError, NotHistoricalModelError from simple_history.manager import HistoryManager -from simple_history.models import HistoricalChanges +from simple_history.models import HistoricalChanges, HistoricalRecords from simple_history.utils import ( + DisableHistoryInfo, + _StoredDisableHistoryInfo, bulk_create_with_history, bulk_update_with_history, + disable_history, get_historical_records_of_instance, get_history_manager_for_model, get_history_model_for_model, get_m2m_field_name, get_m2m_reverse_field_name, get_pk_name, + is_history_disabled, update_change_reason, ) @@ -85,10 +91,461 @@ TrackedWithConcreteBase, Voter, ) +from .utils import HistoricalTestCase User = get_user_model() +class DisableHistoryTestCase(HistoricalTestCase): + """Tests related to the ``disable_history()`` context manager.""" + + def test_disable_history_info(self): + """Test that the various utilities for checking the current info on how + historical record creation is disabled, return the expected values. + This includes ``DisableHistoryInfo`` and ``is_history_disabled()``, as well as + the ``_StoredDisableHistoryInfo`` stored through ``HistoricalRecords.context``. + """ + + class DisableHistoryMode(Enum): + NOT_DISABLED = auto() + GLOBALLY = auto() + PREDICATE = auto() + + poll1 = Poll.objects.create(question="question?", pub_date=timezone.now()) + poll2 = Poll.objects.create(question="ignore this", pub_date=timezone.now()) + + def assert_disable_history_info( + mode: DisableHistoryMode, predicate_target: Union[Type[Poll], Poll] = None + ): + # Check the stored info + attr_name = _StoredDisableHistoryInfo.LOCAL_STORAGE_ATTR_NAME + info = getattr(HistoricalRecords.context, attr_name, None) + if mode is DisableHistoryMode.NOT_DISABLED: + self.assertIsNone(info) + elif mode is DisableHistoryMode.GLOBALLY: + self.assertEqual( + info, _StoredDisableHistoryInfo(instance_predicate=None) + ) + elif mode is DisableHistoryMode.PREDICATE: + self.assertEqual( + info, _StoredDisableHistoryInfo(instance_predicate=ANY) + ) + self.assertIsInstance(info.instance_predicate, Callable) + + # Check `DisableHistoryInfo` + info = DisableHistoryInfo.get() + self.assertEqual(info.not_disabled, mode == DisableHistoryMode.NOT_DISABLED) + self.assertEqual( + info.disabled_globally, mode == DisableHistoryMode.GLOBALLY + ) + self.assertEqual( + info.disabled_for(poll1), + predicate_target is Poll or predicate_target == poll1, + ) + self.assertEqual( + info.disabled_for(poll2), + predicate_target is Poll or predicate_target == poll2, + ) + + # Check `is_history_disabled()` + self.assertEqual(is_history_disabled(), mode == DisableHistoryMode.GLOBALLY) + self.assertEqual( + is_history_disabled(poll1), + mode == DisableHistoryMode.GLOBALLY + or predicate_target is Poll + or predicate_target == poll1, + ) + self.assertEqual( + is_history_disabled(poll2), + mode == DisableHistoryMode.GLOBALLY + or predicate_target is Poll + or predicate_target == poll2, + ) + + assert_disable_history_info(DisableHistoryMode.NOT_DISABLED) + + with disable_history(): + assert_disable_history_info(DisableHistoryMode.GLOBALLY) + + assert_disable_history_info(DisableHistoryMode.NOT_DISABLED) + + with disable_history(only_for_model=Poll): + assert_disable_history_info(DisableHistoryMode.PREDICATE, Poll) + + assert_disable_history_info(DisableHistoryMode.NOT_DISABLED) + + with disable_history(instance_predicate=lambda poll: "ignore" in poll.question): + assert_disable_history_info(DisableHistoryMode.PREDICATE, poll2) + + assert_disable_history_info(DisableHistoryMode.NOT_DISABLED) + + @staticmethod + def _test_disable_poll_history(**kwargs): + """Create, update and delete some ``Poll`` instances outside and inside + the context manager.""" + last_pk = 0 + + def manipulate_poll( + poll: Poll = None, *, create=False, update=False, delete=False + ) -> Poll: + if create: + nonlocal last_pk + last_pk += 1 + poll = Poll.objects.create( + pk=last_pk, question=f"qUESTION {last_pk}?", pub_date=timezone.now() + ) + if update: + poll.question = f"Question {poll.pk}!" + poll.save() + if delete: + poll.delete() + return poll + + poll1 = manipulate_poll(create=True, update=True, delete=True) # noqa: F841 + poll2 = manipulate_poll(create=True, update=True) + poll3 = manipulate_poll(create=True) + + with disable_history(**kwargs): + manipulate_poll(poll2, delete=True) + manipulate_poll(poll3, update=True) + poll4 = manipulate_poll(create=True, update=True, delete=True) # noqa: F841 + poll5 = manipulate_poll(create=True, update=True) + poll6 = manipulate_poll(create=True) + + manipulate_poll(poll5, delete=True) + manipulate_poll(poll6, update=True) + poll7 = manipulate_poll(create=True, update=True, delete=True) # noqa: F841 + + expected_poll_records_before_disable: Final = [ + {"id": 1, "question": "qUESTION 1?", "history_type": "+"}, + {"id": 1, "question": "Question 1!", "history_type": "~"}, + {"id": 1, "question": "Question 1!", "history_type": "-"}, + {"id": 2, "question": "qUESTION 2?", "history_type": "+"}, + {"id": 2, "question": "Question 2!", "history_type": "~"}, + {"id": 3, "question": "qUESTION 3?", "history_type": "+"}, + ] + expected_poll_records_during_disable: Final = [ + {"id": 2, "question": "Question 2!", "history_type": "-"}, + {"id": 3, "question": "Question 3!", "history_type": "~"}, + {"id": 4, "question": "qUESTION 4?", "history_type": "+"}, + {"id": 4, "question": "Question 4!", "history_type": "~"}, + {"id": 4, "question": "Question 4!", "history_type": "-"}, + {"id": 5, "question": "qUESTION 5?", "history_type": "+"}, + {"id": 5, "question": "Question 5!", "history_type": "~"}, + {"id": 6, "question": "qUESTION 6?", "history_type": "+"}, + ] + expected_poll_records_after_disable: Final = [ + {"id": 5, "question": "Question 5!", "history_type": "-"}, + {"id": 6, "question": "Question 6!", "history_type": "~"}, + {"id": 7, "question": "qUESTION 7?", "history_type": "+"}, + {"id": 7, "question": "Question 7!", "history_type": "~"}, + {"id": 7, "question": "Question 7!", "history_type": "-"}, + ] + + @staticmethod + def _test_disable_poll_with_m2m_history(**kwargs): + """Create some ``PollWithManyToMany`` instances and add, remove, set and clear + their ``Place`` relations outside and inside the context manager.""" + last_pk = 0 + place1 = Place.objects.create(pk=1, name="1") + place2 = Place.objects.create(pk=2, name="2") + + def manipulate_places( + poll=None, *, add=False, remove=False, set=False, clear=False + ) -> PollWithManyToMany: + if not poll: + nonlocal last_pk + last_pk += 1 + poll = PollWithManyToMany.objects.create( + pk=last_pk, question=f"{last_pk}?", pub_date=timezone.now() + ) + if add: + poll.places.add(place1) + if remove: + poll.places.remove(place1) + if set: + poll.places.set([place2]) + if clear: + poll.places.clear() + return poll + + poll1 = manipulate_places( # noqa: F841 + add=True, remove=True, set=True, clear=True + ) + poll2 = manipulate_places(add=True, remove=True, set=True) + poll3 = manipulate_places(add=True, remove=True) + poll4 = manipulate_places(add=True) + + with disable_history(**kwargs): + manipulate_places(poll2, clear=True) + manipulate_places(poll3, set=True, clear=True) + manipulate_places(poll4, remove=True, set=True, clear=True) + poll5 = manipulate_places( # noqa: F841 + add=True, remove=True, set=True, clear=True + ) + poll6 = manipulate_places(add=True, remove=True, set=True) + poll7 = manipulate_places(add=True, remove=True) + poll8 = manipulate_places(add=True) + + manipulate_places(poll6, clear=True) + manipulate_places(poll7, set=True, clear=True) + manipulate_places(poll8, remove=True, set=True, clear=True) + poll9 = manipulate_places( # noqa: F841 + add=True, remove=True, set=True, clear=True + ) + + expected_poll_with_m2m_records_before_disable: Final = [ + {"id": 1, "question": "1?", "history_type": "+", "places": []}, + {"id": 1, "question": "1?", "history_type": "~", "places": [Place(pk=1)]}, + {"id": 1, "question": "1?", "history_type": "~", "places": []}, + {"id": 1, "question": "1?", "history_type": "~", "places": [Place(pk=2)]}, + {"id": 1, "question": "1?", "history_type": "~", "places": []}, + {"id": 2, "question": "2?", "history_type": "+", "places": []}, + {"id": 2, "question": "2?", "history_type": "~", "places": [Place(pk=1)]}, + {"id": 2, "question": "2?", "history_type": "~", "places": []}, + {"id": 2, "question": "2?", "history_type": "~", "places": [Place(pk=2)]}, + {"id": 3, "question": "3?", "history_type": "+", "places": []}, + {"id": 3, "question": "3?", "history_type": "~", "places": [Place(pk=1)]}, + {"id": 3, "question": "3?", "history_type": "~", "places": []}, + {"id": 4, "question": "4?", "history_type": "+", "places": []}, + {"id": 4, "question": "4?", "history_type": "~", "places": [Place(pk=1)]}, + ] + expected_poll_with_m2m_records_during_disable: Final = [ + {"id": 2, "question": "2?", "history_type": "~", "places": []}, + {"id": 3, "question": "3?", "history_type": "~", "places": [Place(pk=2)]}, + {"id": 3, "question": "3?", "history_type": "~", "places": []}, + {"id": 4, "question": "4?", "history_type": "~", "places": []}, + {"id": 4, "question": "4?", "history_type": "~", "places": [Place(pk=2)]}, + {"id": 4, "question": "4?", "history_type": "~", "places": []}, + {"id": 5, "question": "5?", "history_type": "+", "places": []}, + {"id": 5, "question": "5?", "history_type": "~", "places": [Place(pk=1)]}, + {"id": 5, "question": "5?", "history_type": "~", "places": []}, + {"id": 5, "question": "5?", "history_type": "~", "places": [Place(pk=2)]}, + {"id": 5, "question": "5?", "history_type": "~", "places": []}, + {"id": 6, "question": "6?", "history_type": "+", "places": []}, + {"id": 6, "question": "6?", "history_type": "~", "places": [Place(pk=1)]}, + {"id": 6, "question": "6?", "history_type": "~", "places": []}, + {"id": 6, "question": "6?", "history_type": "~", "places": [Place(pk=2)]}, + {"id": 7, "question": "7?", "history_type": "+", "places": []}, + {"id": 7, "question": "7?", "history_type": "~", "places": [Place(pk=1)]}, + {"id": 7, "question": "7?", "history_type": "~", "places": []}, + {"id": 8, "question": "8?", "history_type": "+", "places": []}, + {"id": 8, "question": "8?", "history_type": "~", "places": [Place(pk=1)]}, + ] + expected_poll_with_m2m_records_after_disable: Final = [ + {"id": 6, "question": "6?", "history_type": "~", "places": []}, + {"id": 7, "question": "7?", "history_type": "~", "places": [Place(pk=2)]}, + {"id": 7, "question": "7?", "history_type": "~", "places": []}, + {"id": 8, "question": "8?", "history_type": "~", "places": []}, + {"id": 8, "question": "8?", "history_type": "~", "places": [Place(pk=2)]}, + {"id": 8, "question": "8?", "history_type": "~", "places": []}, + {"id": 9, "question": "9?", "history_type": "+", "places": []}, + {"id": 9, "question": "9?", "history_type": "~", "places": [Place(pk=1)]}, + {"id": 9, "question": "9?", "history_type": "~", "places": []}, + {"id": 9, "question": "9?", "history_type": "~", "places": [Place(pk=2)]}, + {"id": 9, "question": "9?", "history_type": "~", "places": []}, + ] + + def test__disable_history__with_no_args(self): + """Test that no historical records are created inside the context manager with + no arguments (i.e. history is globally disabled).""" + # Test with `Poll` instances + self._test_disable_poll_history() + expected_records = [ + *self.expected_poll_records_before_disable, + *self.expected_poll_records_after_disable, + ] + self.assert_all_records_of_model_equal(Poll, expected_records) + + # Test with `PollWithManyToMany` instances + self._test_disable_poll_with_m2m_history() + expected_records = [ + *self.expected_poll_with_m2m_records_before_disable, + *self.expected_poll_with_m2m_records_after_disable, + ] + self.assert_all_records_of_model_equal(PollWithManyToMany, expected_records) + + def test__disable_history__with__only_for_model__poll(self): + """Test that no historical records are created for ``Poll`` instances inside + the context manager with ``only_for_model=Poll`` as argument.""" + # Test with `Poll` instances + self._test_disable_poll_history(only_for_model=Poll) + expected_records = [ + *self.expected_poll_records_before_disable, + *self.expected_poll_records_after_disable, + ] + self.assert_all_records_of_model_equal(Poll, expected_records) + + # Test with `PollWithManyToMany` instances + self._test_disable_poll_with_m2m_history(only_for_model=Poll) + expected_records = [ + *self.expected_poll_with_m2m_records_before_disable, + *self.expected_poll_with_m2m_records_during_disable, + *self.expected_poll_with_m2m_records_after_disable, + ] + self.assert_all_records_of_model_equal(PollWithManyToMany, expected_records) + + def test__disable_history__with__only_for_model__poll_with_m2m(self): + """Test that no historical records are created for ``PollWithManyToMany`` + instances inside the context manager with ``only_for_model=PollWithManyToMany`` + as argument.""" + # Test with `Poll` instances + self._test_disable_poll_history(only_for_model=PollWithManyToMany) + expected_records = [ + *self.expected_poll_records_before_disable, + *self.expected_poll_records_during_disable, + *self.expected_poll_records_after_disable, + ] + self.assert_all_records_of_model_equal(Poll, expected_records) + + # Test with `PollWithManyToMany` instances + self._test_disable_poll_with_m2m_history(only_for_model=PollWithManyToMany) + expected_records = [ + *self.expected_poll_with_m2m_records_before_disable, + *self.expected_poll_with_m2m_records_after_disable, + ] + self.assert_all_records_of_model_equal(PollWithManyToMany, expected_records) + + def test__disable_history__with__instance_predicate(self): + """Test that no historical records are created inside the context manager, for + model instances that match the provided ``instance_predicate`` argument.""" + # Test with `Poll` instances + self._test_disable_poll_history(instance_predicate=lambda poll: poll.pk == 4) + expected_records = [ + *self.expected_poll_records_before_disable, + *filter( + lambda poll_dict: poll_dict["id"] != 4, + self.expected_poll_records_during_disable, + ), + *self.expected_poll_records_after_disable, + ] + self.assert_all_records_of_model_equal(Poll, expected_records) + + # Test with `PollWithManyToMany` instances + self._test_disable_poll_with_m2m_history( + instance_predicate=lambda poll: poll.pk == 5 + ) + expected_records = [ + *self.expected_poll_with_m2m_records_before_disable, + *filter( + lambda poll_dict: poll_dict["id"] != 5, + self.expected_poll_with_m2m_records_during_disable, + ), + *self.expected_poll_with_m2m_records_after_disable, + ] + self.assert_all_records_of_model_equal(PollWithManyToMany, expected_records) + + def test__disable_history__for_queryset_delete(self): + """Test that no historical records are created inside the context manager when + deleting objects using the ``delete()`` queryset method.""" + Poll.objects.create(pk=1, question="delete me", pub_date=timezone.now()) + Poll.objects.create(pk=2, question="keep me", pub_date=timezone.now()) + Poll.objects.create(pk=3, question="keep me", pub_date=timezone.now()) + Poll.objects.create(pk=4, question="delete me", pub_date=timezone.now()) + + with disable_history(): + Poll.objects.filter(question__startswith="delete").delete() + + expected_records = [ + {"id": 1, "question": "delete me", "history_type": "+"}, + {"id": 2, "question": "keep me", "history_type": "+"}, + {"id": 3, "question": "keep me", "history_type": "+"}, + {"id": 4, "question": "delete me", "history_type": "+"}, + ] + self.assert_all_records_of_model_equal(Poll, expected_records) + + Poll.objects.all().delete() + expected_records += [ + # Django reverses the order before sending the `post_delete` signals + # while bulk-deleting + {"id": 3, "question": "keep me", "history_type": "-"}, + {"id": 2, "question": "keep me", "history_type": "-"}, + ] + self.assert_all_records_of_model_equal(Poll, expected_records) + + def test__disable_history__for_foreign_key_cascade_delete(self): + """Test that no historical records are created inside the context manager when + indirectly deleting objects through a foreign key relationship with + ``on_delete=CASCADE``.""" + poll1 = Poll.objects.create(pk=1, pub_date=timezone.now()) + poll2 = Poll.objects.create(pk=2, pub_date=timezone.now()) + Choice.objects.create(pk=11, poll=poll1, votes=0) + Choice.objects.create(pk=12, poll=poll1, votes=0) + Choice.objects.create(pk=21, poll=poll2, votes=0) + Choice.objects.create(pk=22, poll=poll2, votes=0) + + with disable_history(): + poll1.delete() + + expected_records = [ + {"id": 11, "poll_id": 1, "history_type": "+"}, + {"id": 12, "poll_id": 1, "history_type": "+"}, + {"id": 21, "poll_id": 2, "history_type": "+"}, + {"id": 22, "poll_id": 2, "history_type": "+"}, + ] + self.assert_all_records_of_model_equal(Choice, expected_records) + + poll2.delete() + expected_records += [ + # Django reverses the order before sending the `post_delete` signals + # while bulk-deleting + {"id": 22, "poll_id": 2, "history_type": "-"}, + {"id": 21, "poll_id": 2, "history_type": "-"}, + ] + self.assert_all_records_of_model_equal(Choice, expected_records) + + def assert_all_records_of_model_equal( + self, model: Type[Model], expected_records: List[dict] + ): + records = model.history.all() + self.assertEqual(len(records), len(expected_records)) + for record, expected_record in zip(reversed(records), expected_records): + with self.subTest(record=record, expected_record=expected_record): + self.assertRecordValues(record, model, expected_record) + + def test_providing_illegal_arguments_fails(self): + """Test that providing various illegal arguments and argument combinations + fails.""" + + def predicate(_): + return True + + # Providing both arguments should fail + with self.assertRaises(ValueError): + with disable_history(only_for_model=Poll, instance_predicate=predicate): + pass + # Providing the arguments individually should not fail + with disable_history(only_for_model=Poll): + pass + with disable_history(instance_predicate=predicate): + pass + + # Passing non-history-tracked models should fail + with self.assertRaises(NotHistoricalModelError): + with disable_history(only_for_model=Place): + pass + # Passing non-history-tracked model instances should fail + place = Place.objects.create() + with self.assertRaises(NotHistoricalModelError): + is_history_disabled(place) + + def test_nesting_fails(self): + """Test that nesting ``disable_history()`` contexts fails.""" + # Nesting (twice or more) should fail + with self.assertRaises(AssertionError): + with disable_history(): + with disable_history(): + pass + with self.assertRaises(AssertionError): + with disable_history(): + with disable_history(): + with disable_history(): + pass + # No nesting should not fail + with disable_history(): + pass + + class UpdateChangeReasonTestCase(TestCase): def test_update_change_reason_with_excluded_fields(self): poll = PollWithExcludeFields( @@ -412,8 +869,17 @@ def test_bulk_create_history(self): self.assertEqual(Poll.history.count(), 5) @override_settings(SIMPLE_HISTORY_ENABLED=False) - def test_bulk_create_history_with_disabled_setting(self): - bulk_create_with_history(self.data, Poll) + def test_bulk_create_history_without_history_enabled(self): + with self.assertNumQueries(1): + bulk_create_with_history(self.data, Poll) + + self.assertEqual(Poll.objects.count(), 5) + self.assertEqual(Poll.history.count(), 0) + + def test_bulk_create_history_with__disable_history(self): + with self.assertNumQueries(1): + with disable_history(only_for_model=Poll): + bulk_create_with_history(self.data, Poll) self.assertEqual(Poll.objects.count(), 5) self.assertEqual(Poll.history.count(), 0) @@ -667,13 +1133,20 @@ def test_bulk_update_history(self): @override_settings(SIMPLE_HISTORY_ENABLED=False) def test_bulk_update_history_without_history_enabled(self): + # 5 records from `setUp()` self.assertEqual(Poll.history.count(), 5) - # because setup called with enabled settings - bulk_update_with_history( - self.data, - Poll, - fields=["question"], - ) + bulk_update_with_history(self.data, Poll, fields=["question"]) + + self.assertEqual(Poll.objects.count(), 5) + self.assertEqual(Poll.objects.get(id=4).question, "Updated question") + self.assertEqual(Poll.history.count(), 5) + self.assertEqual(Poll.history.filter(history_type="~").count(), 0) + + def test_bulk_update_history_with__disable_history(self): + # 5 records from `setUp()` + self.assertEqual(Poll.history.count(), 5) + with disable_history(only_for_model=Poll): + bulk_update_with_history(self.data, Poll, fields=["question"]) self.assertEqual(Poll.objects.count(), 5) self.assertEqual(Poll.objects.get(id=4).question, "Updated question") diff --git a/simple_history/tests/tests/utils.py b/simple_history/tests/tests/utils.py index 3700b95f..314956bc 100644 --- a/simple_history/tests/tests/utils.py +++ b/simple_history/tests/tests/utils.py @@ -1,10 +1,12 @@ from enum import Enum -from typing import Type +from typing import List, Type from django.conf import settings -from django.db.models import Model +from django.db.models import Manager, Model from django.test import TestCase +from simple_history.utils import get_m2m_reverse_field_name + request_middleware = "simple_history.middleware.HistoryRequestMiddleware" OTHER_DB_NAME = "other" @@ -26,15 +28,33 @@ def assertRecordValues(self, record, klass: Type[Model], values_dict: dict): :param klass: The type of the history-tracked class of ``record``. :param values_dict: Field names of ``record`` mapped to their expected values. """ + values_dict_copy = values_dict.copy() for field_name, expected_value in values_dict.items(): - self.assertEqual(getattr(record, field_name), expected_value) + value = getattr(record, field_name) + if isinstance(value, Manager): + # Assuming that the value being a manager means that it's an M2M field + self._assert_m2m_record(record, field_name, expected_value) + # Remove the field, as `history_object` (used below) doesn't currently + # support historical M2M queryset values + values_dict_copy.pop(field_name) + else: + self.assertEqual(value, expected_value) history_object = record.history_object self.assertEqual(history_object.__class__, klass) - for field_name, expected_value in values_dict.items(): + for field_name, expected_value in values_dict_copy.items(): if field_name not in ("history_type", "history_change_reason"): self.assertEqual(getattr(history_object, field_name), expected_value) + def _assert_m2m_record(self, record, field_name: str, expected_value: List[Model]): + value = getattr(record, field_name) + field = record.instance_type._meta.get_field(field_name) + reverse_field_name = get_m2m_reverse_field_name(field) + self.assertListEqual( + [getattr(m2m_record, reverse_field_name) for m2m_record in value.all()], + expected_value, + ) + class TestDbRouter: def db_for_read(self, model, **hints): diff --git a/simple_history/utils.py b/simple_history/utils.py index fe272ff6..b43c2f9f 100644 --- a/simple_history/utils.py +++ b/simple_history/utils.py @@ -1,5 +1,10 @@ -from typing import TYPE_CHECKING, Optional, Type, Union +import sys +from contextlib import contextmanager +from dataclasses import dataclass +from typing import TYPE_CHECKING, Callable, ClassVar, Iterator, Optional, Type, Union +from asgiref.local import Local +from django.conf import settings from django.db import transaction from django.db.models import Case, ForeignKey, ManyToManyField, Model, Q, When from django.forms.models import model_to_dict @@ -11,6 +16,164 @@ from .models import HistoricalChanges +@contextmanager +def disable_history( + *, + only_for_model: Type[Model] = None, + instance_predicate: Callable[[Model], bool] = None, +) -> Iterator[None]: + """ + Disable creating historical records while this context manager is active. + + Note: ``only_for_model`` and ``instance_predicate`` cannot both be provided. + + :param only_for_model: Only disable history creation for this model type. + :param instance_predicate: Only disable history creation for model instances passing + this predicate. + """ + assert ( # nosec + _StoredDisableHistoryInfo.get() is None + ), "Nesting 'disable_history()' contexts is undefined behavior" + + if only_for_model: + # Raise an error if it's not a history-tracked model + get_history_manager_for_model(only_for_model) + if instance_predicate: + raise ValueError( + "'only_for_model' and 'instance_predicate' cannot both be provided" + ) + else: + + def instance_predicate(instance: Model): + return isinstance(instance, only_for_model) + + info = _StoredDisableHistoryInfo(instance_predicate) + info.set() + try: + yield + finally: + info.delete() + + +@dataclass(frozen=True) +class _StoredDisableHistoryInfo: + """ + Data related to how historical record creation is disabled, stored in + ``HistoricalRecords.context`` through the ``disable_history()`` context manager. + """ + + LOCAL_STORAGE_ATTR_NAME: ClassVar = "disable_history_info" + + instance_predicate: Callable[[Model], bool] = None + + def set(self) -> None: + setattr(self._get_storage(), self.LOCAL_STORAGE_ATTR_NAME, self) + + @classmethod + def get(cls) -> Optional["_StoredDisableHistoryInfo"]: + """ + A return value of ``None`` means that the ``disable_history()`` context manager + is not active. + """ + return getattr(cls._get_storage(), cls.LOCAL_STORAGE_ATTR_NAME, None) + + @classmethod + def delete(cls) -> None: + delattr(cls._get_storage(), cls.LOCAL_STORAGE_ATTR_NAME) + + @staticmethod + def _get_storage() -> Local: + from .models import HistoricalRecords # Avoids circular importing + + return HistoricalRecords.context + + +@dataclass( + frozen=True, + # DEV: Replace this with just `kw_only=True` when the minimum required + # Python version is 3.10 + **({"kw_only": True} if sys.version_info >= (3, 10) else {}), +) +class DisableHistoryInfo: + """ + Provides info on *how* historical record creation is disabled. + + Create a new instance through ``get()`` for updated info. + (The ``__init__()`` method is intended for internal use.) + """ + + _disabled_globally: bool + _instance_predicate: Optional[Callable[[Model], bool]] + + @property + def not_disabled(self) -> bool: + """ + A value of ``True`` means that historical record creation is not disabled + in any way. + If ``False``, check ``disabled_globally`` and ``disabled_for()``. + """ + return not self._disabled_globally and not self._instance_predicate + + @property + def disabled_globally(self) -> bool: + """ + Whether historical record creation is disabled due to + the ``SIMPLE_HISTORY_ENABLED`` setting or the ``disable_history()`` context + manager being active. + """ + return self._disabled_globally + + def disabled_for(self, instance: Model) -> bool: + """ + Returns whether history record creation is disabled for the provided instance + specifically. + Remember to also check ``disabled_globally``! + """ + return bool(self._instance_predicate) and self._instance_predicate(instance) + + @classmethod + def get(cls) -> "DisableHistoryInfo": + """ + Returns an instance of this class. + + Note that this method must be called again every time you want updated info. + """ + stored_info = _StoredDisableHistoryInfo.get() + context_manager_active = bool(stored_info) + instance_predicate = ( + stored_info.instance_predicate if context_manager_active else None + ) + + disabled_globally = not getattr(settings, "SIMPLE_HISTORY_ENABLED", True) or ( + context_manager_active and not instance_predicate + ) + return cls( + _disabled_globally=disabled_globally, + _instance_predicate=instance_predicate, + ) + + +def is_history_disabled(instance: Model = None) -> bool: + """ + Returns whether creating historical records is disabled. + + :param instance: If *not* provided, will return whether history is disabled + globally. Otherwise, will return whether history is disabled for the provided + instance (either globally or due to the arguments passed to + the ``disable_history()`` context manager). + """ + if instance: + # Raise an error if it's not a history-tracked model instance + get_history_manager_for_model(instance) + + info = DisableHistoryInfo.get() + if info.disabled_globally: + return True + if instance and info.disabled_for(instance): + return True + return False + + def get_change_reason_from_object(obj: Model) -> Optional[str]: return getattr(obj, "_change_reason", None) From 7755c1e496a4dfeb2e60f3da8546123552048631 Mon Sep 17 00:00:00 2001 From: Anders <6058745+ddabble@users.noreply.github.com> Date: Sun, 8 Sep 2024 15:07:50 +0200 Subject: [PATCH 8/8] Documented overriding create_historical_record() ...as a more fine-grained alternative to the other ways of disabling historical record creation. --- docs/disabling_history.rst | 23 +++++++++++++++++++++++ simple_history/models.py | 4 +++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/docs/disabling_history.rst b/docs/disabling_history.rst index adafd9df..e2938948 100644 --- a/docs/disabling_history.rst +++ b/docs/disabling_history.rst @@ -43,6 +43,29 @@ See some examples below: Poll.objects.create(question="ignore this") Poll.objects.create(question="what's up?") +Overriding ``create_historical_record()`` +----------------------------------------- + +For even more fine-grained control, you can subclass ``HistoricalRecords`` and override +its ``create_historical_record()`` method, for example like this: + +.. code-block:: python + + class CustomHistoricalRecords(HistoricalRecords): + def create_historical_record( + self, instance: models.Model, history_type: str, *args, **kwargs + ) -> None: + # Don't create records for "ignore" polls that are being deleted + if "ignore" in poll.question and history_type == "-": + return + + super().create_historical_record(instance, history_type, *args, **kwargs) + + + class Poll(models.Model): + # ... + history = CustomHistoricalRecords() + The ``SIMPLE_HISTORY_ENABLED`` setting -------------------------------------- diff --git a/simple_history/models.py b/simple_history/models.py index d612b00b..1c590a29 100644 --- a/simple_history/models.py +++ b/simple_history/models.py @@ -768,7 +768,9 @@ def create_historical_record_m2ms(self, history_instance, instance): field=field, ) - def create_historical_record(self, instance, history_type, using=None): + def create_historical_record( + self, instance: models.Model, history_type: str, using: str = None + ) -> None: using = using if self.use_base_model_db else None history_date = getattr(instance, "_history_date", timezone.now()) history_user = self.get_history_user(instance)