`disable_history()` context manager #1387

ddabble · 2024-09-10T21:17:58Z

Description

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" (8ea956c).

Also:

Added delete_without_historical_record() to all history-tracked model objects, which complements save_without_historical_record() (e2894b5)

Breaking changes:

Removed HistoryManager.get_super_queryset() (23c37dd)
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() (23c37dd)

Deprecations:

Deprecated the undocumented HistoricalRecords.thread - use HistoricalRecords.context instead. The former attribute will be removed in version 3.10 (74a2e38)
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 (8ea956c)

Fixes and improvements:

Moved the "Save without creating historical records" subsection of "Querying History" in the docs to a new section: "Disable Creating Historical Records" (b36a280)
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 (78286f6)

Other changes:

Documented overriding create_historical_record() (7755c1e)

Related Issue

Motivation and Context

Having an easier and more universal way of disabling the creation of historical records in various contexts.

How Has This Been Tested?

See the added tests.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have run the pre-commit run command to format and lint.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have added my name and/or github handle to AUTHORS.rst
I have added my change to CHANGES.rst
All new and existing tests passed.

`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.

It didn't really fit under the "Querying History" section.

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.

Their code already works with passing an instance instead of a model, so might as well make it official.

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 #642 (comment) Also added a couple useful utils related to `disable_history()`: `is_history_disabled()` and a `DisableHistoryInfo` dataclass - see their docstrings in `utils.py`.

...as a more fine-grained alternative to the other ways of disabling historical record creation.

tim-schilling · 2024-09-10T23:50:30Z

@ddabble I'm pretty swamped through the first week of October right now if you can wait until then.

ddabble · 2024-09-11T05:03:57Z

Sure, no rush 😊

tim-schilling

I feel somewhat strongly about the API naming for the disable_history() arguments as well as the DisableHistoryInfo class naming. Most of everything else is suggestions or questions for clarification.

Great work as always 💪. It's always fun and educational to review your code!

tim-schilling · 2024-09-10T23:42:25Z

simple_history/utils.py

+    assert (  # nosec
+        _StoredDisableHistoryInfo.get() is None
+    ), "Nesting 'disable_history()' contexts is undefined behavior"


I think this is better off as a RuntimeError since asserts can be disabled.

tim-schilling · 2024-10-13T03:04:10Z

docs/disabling_history.rst

+#. With ``instance_predicate``: Only disable history creation for model instances passing
+   this predicate.


This argument name doesn't seem very beginner friendly. The best thing I can come up with is should_disable_callback. Or maybe should_disable?

We should also document what arguments this function must take.

tim-schilling · 2024-10-13T03:06:40Z

docs/disabling_history.rst

+#. 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.


Perhaps this is better as models to match the signature of isinstance? This would allow people to disable multiple models in a single context manager and it shouldn't be too much additional complexity on our end.

tim-schilling · 2024-10-13T03:08:19Z

docs/disabling_history.rst

+Disable the creation of historical records for *all* models
+by adding the following line to your settings:


Do we want to indicate this defaults to True?

Oh, this can be ignored. I realized that it's just moving.

tim-schilling · 2024-10-13T03:10:33Z

simple_history/manager.py

        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)


Suggested change

pk_name = utils.get_pk_name(self.instance._meta.model)

pk_name = utils.get_pk_name(self.model)

Does that work just as well?

tim-schilling · 2024-10-13T03:28:51Z

simple_history/tests/tests/utils.py

+            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]):


might this be better named as _assert_m2m_relation? Or _assert_m2m_queryset? I don't mean to bikeshed, but as it currently stands, it makes it sounds like it's comparing a single value. However, it's really fetching all the related objects to compare. Having the name imply that it'll do a DB hit will be useful for the future.

tim-schilling · 2024-10-13T03:30:08Z

simple_history/utils.py

+    :param instance_predicate: Only disable history creation for model instances passing
+        this predicate.


Similar to our documentation, we should include the expected signature here.

tim-schilling · 2024-10-13T03:34:34Z

simple_history/utils.py

+            raise ValueError(
+                "'only_for_model' and 'instance_predicate' cannot both be provided"
+            )


I'm curious why you decided on this. I'm generally fine with it, but we could AND them together.

tim-schilling · 2024-10-13T03:43:23Z

simple_history/utils.py

+    #      Python version is 3.10
+    **({"kw_only": True} if sys.version_info >= (3, 10) else {}),
+)
+class DisableHistoryInfo:


I didn't have an issue with this before, but looking at the first method of not_disabled and I'm not sure this is how things should be done. My concern is that this control is solely focused on history being disabled. However, that also means it's focused on history being enabled.

So maybe a name like HistoryControl, HistoryState or HistoryInstrumentation? Then we can change not_disabled to enabled 😁

tim-schilling · 2024-10-13T03:45:09Z

simple_history/utils.py

+        return not self._disabled_globally and not self._instance_predicate
+
+    @property
+    def disabled_globally(self) -> bool:


nitpick: There is a bit of a precedence set in the API for a is_ prefix on these types of boolean property checks.

ddabble added 8 commits September 10, 2024 23:07

Moved disabling history docs to new section

b36a280

It didn't really fit under the "Querying History" section.

Added delete_without_historical_record()

e2894b5

Changed the order of some util functions

c2576e3

Made some helper utils support models and instances

78286f6

Their code already works with passing an instance instead of a model, so might as well make it official.

Documented overriding create_historical_record()

7755c1e

...as a more fine-grained alternative to the other ways of disabling historical record creation.

ddabble requested a review from tim-schilling September 10, 2024 21:17

tim-schilling requested changes Oct 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`disable_history()` context manager #1387

`disable_history()` context manager #1387

ddabble commented Sep 10, 2024

tim-schilling commented Sep 10, 2024

ddabble commented Sep 11, 2024

tim-schilling left a comment

tim-schilling Sep 10, 2024

tim-schilling Oct 13, 2024

tim-schilling Oct 13, 2024

tim-schilling Oct 13, 2024

tim-schilling Oct 13, 2024

tim-schilling Oct 13, 2024

tim-schilling Oct 13, 2024

tim-schilling Oct 13, 2024

tim-schilling Oct 13, 2024

tim-schilling Oct 13, 2024

tim-schilling Oct 13, 2024

		#. With ``instance_predicate``: Only disable history creation for model instances passing
		this predicate.

		Disable the creation of historical records for all models
		by adding the following line to your settings:

	pk_name = utils.get_pk_name(self.instance._meta.model)
	pk_name = utils.get_pk_name(self.model)

		:param instance_predicate: Only disable history creation for model instances passing
		this predicate.

disable_history() context manager #1387

Are you sure you want to change the base?

disable_history() context manager #1387

Conversation

ddabble commented Sep 10, 2024

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

tim-schilling commented Sep 10, 2024

ddabble commented Sep 11, 2024

tim-schilling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`disable_history()` context manager #1387

`disable_history()` context manager #1387