Merge pull request #21 from kraken-tech/after-commit-transaction-required-setting

Control after-commit behaviour with a setting

Author: meshy

pytest.ini

@@ -48,8 +48,6 @@ addopts =
 markers =
     # See Note [Running after-commit callbacks in tests]
     deprecated_ignore_after_commit_callbacks: require after-commit callbacks to be explicitly captured and called
-    # See Note [After-commit callbacks require a transaction]
-    deprecated_run_after_commit_outside_transaction: relax rule where after-commit callbacks require a transaction
 
 # Ensure passing xfailed (aka "xpass") tests cause the test suite to fail.
 # https://docs.pytest.org/en/latest/skipping.html#strict-parameter

src/django_subatomic/db.py

@@ -6,6 +6,7 @@
 
 import attrs
 from django import db as django_db
+from django.conf import settings
 from django.db import transaction as django_transaction
 
 
@@ -14,8 +15,6 @@
     from typing import NoReturn
 
 
-# See Note [After-commit callbacks require a transaction]
-_REQUIRE_TRANSACTION_FOR_AFTER_COMMIT_CALLBACKS = True
 # See Note [Running after-commit callbacks in tests]
 _RUN_AFTER_COMMIT_CALLBACKS_IN_TESTS = True
 
@@ -323,6 +322,9 @@ class _UnexpectedDanglingTransaction(Exception):
 # We choose to disallow immediate execution because it can be misleading and hide bugs.
 # In particular, it can hide the fact that a transaction is missing or on a different database,
 # which can make code read as though a callback will be deferred when it won't be.
+#
+# To help projects migrate to this behaviour, this requirement can be disabled with
+# the Django setting `SUBATOMIC_AFTER_COMMIT_NEEDS_TRANSACTION`.
 
 
 # Note [Running after-commit callbacks in tests]
@@ -354,7 +356,10 @@ def run_after_commit(
     if using is None:
         using = django_db.DEFAULT_DB_ALIAS
 
-    needs_transaction = _REQUIRE_TRANSACTION_FOR_AFTER_COMMIT_CALLBACKS
+    # See Note [After-commit callbacks require a transaction]
+    needs_transaction = getattr(
+        settings, "SUBATOMIC_AFTER_COMMIT_NEEDS_TRANSACTION", True
+    )
     only_in_testcase_transaction = _innermost_atomic_block_wraps_testcase(using=using)
 
     # Fail if a transaction is required, but none exists.

tests/test_db.py

@@ -7,6 +7,7 @@
 import pytest
 from django import db as django_db
 from django.db import transaction as django_transaction
+from django.test import override_settings
 
 from django_subatomic import db, test
 
@@ -59,32 +60,6 @@ def _parametrize_transaction_testcase(func: Callable[..., None]) -> MarkDecorato
     return parametrize(usefixtures(func))  # type: ignore[no-any-return]
 
 
-@pytest.fixture(autouse=True)
-def _require_transactions_for_after_commit_callbacks(
-    request: pytest.FixtureRequest,
-) -> Generator[None]:
-    """
-    Ensure after-commit callbacks are executed from within a transaction.
-
-    This ensures that we do not accidentally write code which looks like it defers execution,
-    but is actually running immediately.
-
-    This fixture enables that new behaviour for all new tests by default,
-    but offers an escape-hatch for tests which have not yet been fixed.
-
-    TODO: Allow users of this library to do this in their own tests.
-
-    See Note [After-commit callbacks require a transaction].
-    """
-    if "deprecated_run_after_commit_outside_transaction" in request.keywords:
-        with mock.patch.object(
-            db, "_REQUIRE_TRANSACTION_FOR_AFTER_COMMIT_CALLBACKS", new=False
-        ):
-            yield
-    else:
-        yield
-
-
 @pytest.fixture(autouse=True)
 def _prevent_after_commit_callbacks(
     request: pytest.FixtureRequest,
@@ -387,6 +362,10 @@ def test_unclosed_manual_transaction(self, db_name: str) -> None:
 
 
 class TestRunAfterCommit:
+    """
+    Tests for `run_after_commit`.
+    """
+
     @_parametrize_transaction_testcase
     def test_outside_transaction_error(self) -> None:
         """
@@ -403,18 +382,16 @@ def test_outside_transaction_error(self) -> None:
         assert counter.count == 0
 
     @_parametrize_transaction_testcase
-    @pytest.mark.deprecated_run_after_commit_outside_transaction
-    def test_executes_immediately(self) -> None:
+    def test_transaction_check_can_be_disabled(self) -> None:
         """
-        `run_after_commit` executes the callback immediately if there is no transaction...
-
-        ... and if the test is marked with `deprecated_run_after_commit_outside_transaction`.
+        Callbacks run immediately if there is no transaction and `SUBATOMIC_AFTER_COMMIT_NEEDS_TRANSACTION` is False.
 
         See Note [After-commit callbacks require a transaction]
         """
         counter = Counter()
 
-        db.run_after_commit(counter.increment)
+        with override_settings(SUBATOMIC_AFTER_COMMIT_NEEDS_TRANSACTION=False):
+            db.run_after_commit(counter.increment)
 
         assert counter.count == 1
 
@@ -487,29 +464,16 @@ def test_not_executed_if_rolled_back(self) -> None:
 
 
 class TestRunAfterCommitDeprecatedTestBehaviour:
-    @pytest.mark.deprecated_run_after_commit_outside_transaction
-    def test_does_not_raise_error_when_marked(self) -> None:
-        """
-        No error is raised in test marked with `deprecated_run_after_commit_outside_transaction`.
-
-        Note: this would usually fail because we're enqueuing an after-commit callback without a
-        transaction.
-        """
-        try:
-            db.run_after_commit(object)
-        except Exception:  # noqa: BLE001
-            pytest.fail("An error should not have been raised.")
-
     @pytest.mark.deprecated_ignore_after_commit_callbacks
-    @pytest.mark.deprecated_run_after_commit_outside_transaction
+    @override_settings(SUBATOMIC_AFTER_COMMIT_NEEDS_TRANSACTION=False)
     def test_does_not_execute_when_in_testcase_transaction_if_callbacks_disabled(
         self,
     ) -> None:
         """
         `run_after_commit` should not ignore testcase transactions when test marked with `deprecated_ignore_after_commit_callbacks`.
 
-        Note: this test also requires `deprecated_run_after_commit_outside_transaction`,
-        because otherwise an error would be raised when trying to run the callback.
+        We have to disable `SUBATOMIC_AFTER_COMMIT_NEEDS_TRANSACTION`, because
+        otherwise an error would be raised when trying to register the callback.
         """
         counter = Counter()