Merge pull request #157 from kraken-tech/meshy/ambiguous-after-commit-execution

Ensure after-commit behaviour is explicit in tests

Author: meshy

CHANGELOG.md

@@ -18,6 +18,11 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 - `part_of_a_transaction` now raises an error if unhandled callbacks are detected when it starts.
   This makes it more similar to `transaction`.
   The error can be silenced by setting the `SUBATOMIC_CATCH_UNHANDLED_AFTER_COMMIT_CALLBACKS_IN_TESTS` setting to `False`
+- `run_after_commit` now raises an error in tests if it doesn't know if callbacks should be run.
+  This prevents tests from silently doing the wrong thing (i.e. not running the callbacks).
+  To fix this error, use `transaction` instead of `atomic`
+  (or to explicitly disable callbacks when testing low-level code use `part_of_a_transaction`).
+  The error can be silenced by setting `SUBATOMIC_AFTER_COMMIT_AMBIGUITY_ERROR_IN_TESTS` setting to `False`.
 
 ### Fixed
 - Disallowed nesting of `part_of_a_transaction` to prevent nonsense

docs/reference/settings.md

@@ -15,6 +15,33 @@ transition to this strict behaviour
 by getting it working in tests
 before enabling it in production.
 
+## `SUBATOMIC_AFTER_COMMIT_AMBIGUITY_ERROR_IN_TESTS`
+
+(default: `True`)
+
+When this setting is `True`,
+[`run_after_commit`][django_subatomic.db.run_after_commit] will ensure that it knows whether or not after-commit callbacks should be simulated in tests.
+To avoid silently doing the wrong thing when it is not sure,
+[`run_after_commit`][django_subatomic.db.run_after_commit] will raise `subatomic.db._AmbiguousAfterCommitTestBehaviour`.
+(This setting starts with an underscore because it is not intended to be caught and handled.)
+
+This can happen in tests when [`run_after_commit`][django_subatomic.db.run_after_commit] is called
+inside a Django `atomic` block,
+and that `atomic` is directly nested inside the test suite's transaction.
+Because the test suite would roll back the transaction,
+after-commit callbacks would not normally be run.
+
+Where after-commit callbacks should be run,
+this can be fixed by replacing (or wrapping) the `atomic` block with
+[`subatomic.db.transaction`][django_subatomic.db.transaction]
+(or [`transaction_if_not_already`][django_subatomic.db.transaction_if_not_already] if necessary).
+
+In tests where after-commit callbacks should not be run,
+[`part_of_a_transaction`][django_subatomic.test.part_of_a_transaction] should be used instead.
+
+If this setting is `False`,
+the after-commit callbacks that this check would catch will not be run.
+
 ## `SUBATOMIC_RUN_AFTER_COMMIT_CALLBACKS_IN_TESTS`
 
 (default: `True`)

src/django_subatomic/db.py

@@ -63,6 +63,9 @@ def _transaction(*, using: str | None) -> Generator[None]:
             # the `savepoint` flag is ignored when `durable` is `True`.
             django_transaction.atomic(using=using, durable=True),
         ):
+            connection = django_transaction.get_connection(using=using)
+            atomic_block = connection.atomic_blocks[-1]
+            atomic_block._from_subatomic = True  # noqa: SLF001
             yield
 
     decorator = _transaction(using=using)
@@ -325,6 +328,37 @@ class _MissingRequiredTransaction(Exception):
     database: str
 
 
+@attrs.frozen
+class _AmbiguousAfterCommitTestBehaviour(Exception):
+    """
+    Raised in tests when it's unclear if after-commit callbacks should be run.
+
+    You are calling `run_after_commit` inside an `atomic` block in a test.
+    This `atomic` is inside the test suite's transaction,
+    so after-commit callbacks will not be run
+    (because the test suite will roll back the transaction).
+
+    Subatomic doesn't know if you intended for these callbacks to be run or not,
+    so raises this error to avoid silently doing the wrong thing.
+
+    In production code, or tests where after-commit callbacks should be run,
+    replace (or wrap) the `atomic` block with `subatomic.db.transaction`.
+
+    In tests where after-commit callbacks should not be run,
+    use `subatomic.test.part_of_a_transaction` instead.
+
+    To help your project progressively adopt this check,
+    you can disable this requirement for after-commit callbacks by setting
+    `settings.SUBATOMIC_AFTER_COMMIT_AMBIGUITY_ERROR_IN_TESTS` to `False`.
+
+    See Note [_MissingRequiredTransaction in tests]
+
+    This exception should not be caught, as it indicates a programming error.
+    """
+
+    database: str
+
+
 @attrs.frozen
 class _UnexpectedOpenTransaction(Exception):
     """
@@ -424,18 +458,9 @@ def run_after_commit(
     if using is None:
         using = django_db.DEFAULT_DB_ALIAS
 
-    # 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.
-    # Ignore test-suite transactions when checking for a transaction.
-    # See Note [After-commit callbacks require a transaction]
-    if needs_transaction and not in_transaction(using=using):
-        raise _MissingRequiredTransaction(database=using)
+    _ensure_transaction_is_open(using=using)
 
+    only_in_testcase_transaction = _innermost_atomic_block_wraps_testcase(using=using)
     if (
         # See Note [Running after-commit callbacks in tests]
         getattr(settings, "SUBATOMIC_RUN_AFTER_COMMIT_CALLBACKS_IN_TESTS", True)
@@ -446,6 +471,54 @@ def run_after_commit(
         django_transaction.on_commit(callback, using=using)
 
 
+def _ensure_transaction_is_open(*, using: str) -> None:
+    """
+    Raise an error if transactions are required but missing.
+
+    If there is no transaction open, `_MissingRequiredTransaction` is raised.
+    This can be silenced with the Django setting
+    `SUBATOMIC_AFTER_COMMIT_NEEDS_TRANSACTION`.
+
+    See Note [After-commit callbacks require a transaction]
+
+    When transactions are managed by the test suite,
+    this also ensures after-commit emulation is accounted for by Subatomic.
+    When they are not, `_AmbiguousAfterCommitTestBehaviour` is raised.
+    This can be silenced with the Django setting
+    `SUBATOMIC_AFTER_COMMIT_AMBIGUITY_ERROR_IN_TESTS`.
+    """
+    needs_transaction = getattr(
+        settings, "SUBATOMIC_AFTER_COMMIT_NEEDS_TRANSACTION", True
+    )
+    # Skip checks if they have been disabled.
+    if not needs_transaction:
+        return
+
+    # Fail if we're not in a transaction.
+    if not in_transaction(using=using):
+        raise _MissingRequiredTransaction(database=using)
+
+    ambiguity_error_in_tests = getattr(
+        settings, "SUBATOMIC_AFTER_COMMIT_AMBIGUITY_ERROR_IN_TESTS", True
+    )
+    if not ambiguity_error_in_tests:
+        return
+
+    connection = django_transaction.get_connection(using=using)
+
+    # We expect after-commit callbacks to be handled by Django
+    # if we're not in a test-managed transaction.
+    if not connection.atomic_blocks[0]._from_testcase:  # noqa: SLF001
+        return
+
+    # `_from_testcase` told us that we're in a test-managed transaction.
+    # `in_transaction` told us that we're in a further atomic context.
+    # If Subatomic didn't open that context then we don't know if the
+    # test expects after-commit callbacks to be emulated or not.
+    if not hasattr(connection.atomic_blocks[1], "_from_subatomic"):
+        raise _AmbiguousAfterCommitTestBehaviour(database=using)
+
+
 def _innermost_atomic_block_wraps_testcase(*, using: str | None = None) -> bool:
     """
     Return True if the current innermost atomic block is wrapping a test case.

src/django_subatomic/test.py

@@ -59,6 +59,8 @@ def part_of_a_transaction(using: str | None = None) -> Generator[None]:
             raise _UnhandledCallbacks(tuple(callback for _, callback, _ in callbacks))
 
     with transaction.atomic(using=using, durable=True):
+        atomic_block = connection.atomic_blocks[-1]
+        atomic_block._from_subatomic = True  # noqa: SLF001
         yield
 
     # Throw away any callbacks that were registered during the partial transaction,

tests/test_db.py

@@ -226,7 +226,11 @@ def test_unhandled_callbacks_cause_error(
         counter = Counter()
 
         # Django's `atomic` leaves unhandled after-commit actions on exit.
-        with django_transaction.atomic():
+        with (
+            django_transaction.atomic(),
+            # This setting prevents `run_after_commit` from raising an error when registering the callback.
+            override_settings(SUBATOMIC_AFTER_COMMIT_AMBIGUITY_ERROR_IN_TESTS=False),
+        ):
             db.run_after_commit(counter.increment)
 
         # `transaction` will raise when it finds the unhandled callback.
@@ -250,7 +254,11 @@ def test_unhandled_callbacks_check_can_be_disabled(
         counter = Counter()
 
         # Django's `atomic` leaves unhandled after-commit actions on exit.
-        with django_transaction.atomic():
+        with (
+            django_transaction.atomic(),
+            # This setting prevents `run_after_commit` from raising an error when registering the callback.
+            override_settings(SUBATOMIC_AFTER_COMMIT_AMBIGUITY_ERROR_IN_TESTS=False),
+        ):
             db.run_after_commit(counter.increment)
 
         # Run after-commit callbacks when `transaction` exits,
@@ -587,6 +595,53 @@ def test_outside_transaction_error(self) -> None:
         assert exc.value.database == DEFAULT
         assert counter.count == 0
 
+    def test_ambiguous_after_commit_callback(self) -> None:
+        """
+        `run_after_commit` errors if we're not sure if tests should emulate transactions.
+
+        See Note [After-commit callbacks require a transaction]
+        """
+        counter = Counter()
+
+        with (
+            django_transaction.atomic(),
+            pytest.raises(db._AmbiguousAfterCommitTestBehaviour),  # noqa: SLF001
+        ):
+            db.run_after_commit(counter.increment)
+
+        assert counter.count == 0
+
+    @pytest.mark.django_db(transaction=True)
+    def test_unambiguous_unsimulated_callbacks(self) -> None:
+        """
+        `run_after_commit` doesn't error if in `atomic` in a transaction testcase.
+
+        This is because Django will handle the after-commit callbacks itself.
+        """
+        counter = Counter()
+
+        with django_transaction.atomic():
+            db.run_after_commit(counter.increment)
+
+        assert counter.count == 1
+
+    def test_ambiguous_simulation_requirement_disabled(self) -> None:
+        """
+        Callbacks aren't simulated in tests when ambiguous simulation requirement errors are disabled.
+
+        See Note [After-commit callbacks require a transaction]
+        """
+        counter = Counter()
+
+        with (
+            django_transaction.atomic(),
+            override_settings(SUBATOMIC_AFTER_COMMIT_AMBIGUITY_ERROR_IN_TESTS=False),
+        ):
+            db.run_after_commit(counter.increment)
+
+        # The callback was not run.
+        assert counter.count == 0
+
     @_parametrize_transaction_testcase
     def test_transaction_check_can_be_disabled(self) -> None:
         """