From baff60d05c7099c9f27a7546e290de0f9d85170f Mon Sep 17 00:00:00 2001 From: Charlie Denton Date: Fri, 24 Apr 2026 09:32:49 +0100 Subject: [PATCH 1/4] Extract transaction-enforcement into helper This makes it easier to refactor in a later commit. --- src/django_subatomic/db.py | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/src/django_subatomic/db.py b/src/django_subatomic/db.py index e7bf416..4259e5d 100644 --- a/src/django_subatomic/db.py +++ b/src/django_subatomic/db.py @@ -424,18 +424,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 +437,27 @@ 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] + """ + needs_transaction = getattr( + settings, "SUBATOMIC_AFTER_COMMIT_NEEDS_TRANSACTION", True + ) + + # 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) + + def _innermost_atomic_block_wraps_testcase(*, using: str | None = None) -> bool: """ Return True if the current innermost atomic block is wrapping a test case. From 404ea649cc29dce7d611e80a35e7f7879783b68f Mon Sep 17 00:00:00 2001 From: Charlie Denton Date: Wed, 6 May 2026 17:35:48 +0100 Subject: [PATCH 2/4] Return early if transactions aren't required The logic in this function is about to get bigger, and this early return saves us from repeating the same check on each if-block. --- src/django_subatomic/db.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/django_subatomic/db.py b/src/django_subatomic/db.py index 4259e5d..6822d3b 100644 --- a/src/django_subatomic/db.py +++ b/src/django_subatomic/db.py @@ -450,11 +450,13 @@ def _ensure_transaction_is_open(*, using: str) -> None: needs_transaction = getattr( settings, "SUBATOMIC_AFTER_COMMIT_NEEDS_TRANSACTION", True ) + if not needs_transaction: + return # 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): + if not in_transaction(using=using): raise _MissingRequiredTransaction(database=using) From eb4e6b37f1d32fdd1c1835e0aea291455dbe25b4 Mon Sep 17 00:00:00 2001 From: Charlie Denton Date: Wed, 6 May 2026 18:44:42 +0100 Subject: [PATCH 3/4] Clarify some comments --- src/django_subatomic/db.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/django_subatomic/db.py b/src/django_subatomic/db.py index 6822d3b..7a257df 100644 --- a/src/django_subatomic/db.py +++ b/src/django_subatomic/db.py @@ -450,12 +450,11 @@ def _ensure_transaction_is_open(*, using: str) -> None: needs_transaction = getattr( settings, "SUBATOMIC_AFTER_COMMIT_NEEDS_TRANSACTION", True ) + # Skip checks if they have been disabled. if not needs_transaction: return - # 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] + # Fail if we're not in a transaction. if not in_transaction(using=using): raise _MissingRequiredTransaction(database=using) From 7551249c339940e37609fc0f8a39b6edc09d1caa Mon Sep 17 00:00:00 2001 From: Charlie Denton Date: Wed, 6 May 2026 19:19:46 +0100 Subject: [PATCH 4/4] Ensure after-commit behaviour is explicit in tests Before this change, after-commit callbacks might be silently dropped by the test suite when `run_after_commit` was used inside Django's `atomic`. While this behaviour may be desired in some tests, others might want these callbacks to run. This change ensures developers decide if after-commit callbacks should be run by ensuring the outer atomic context is opened by Subatomic in any test which calls `run_after_commit`. --- CHANGELOG.md | 5 +++ docs/reference/settings.md | 27 ++++++++++++++++ src/django_subatomic/db.py | 60 ++++++++++++++++++++++++++++++++++++ src/django_subatomic/test.py | 2 ++ tests/test_db.py | 59 +++++++++++++++++++++++++++++++++-- 5 files changed, 151 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 60c124b..429de13 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/docs/reference/settings.md b/docs/reference/settings.md index 9240880..61b5a66 100644 --- a/docs/reference/settings.md +++ b/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`) diff --git a/src/django_subatomic/db.py b/src/django_subatomic/db.py index 7a257df..2f3cdcc 100644 --- a/src/django_subatomic/db.py +++ b/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): """ @@ -446,6 +480,12 @@ def _ensure_transaction_is_open(*, using: str) -> None: `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 @@ -458,6 +498,26 @@ def _ensure_transaction_is_open(*, using: str) -> None: 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: """ diff --git a/src/django_subatomic/test.py b/src/django_subatomic/test.py index 3e4ddbc..a487ff0 100644 --- a/src/django_subatomic/test.py +++ b/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, diff --git a/tests/test_db.py b/tests/test_db.py index eb86961..6136f9c 100644 --- a/tests/test_db.py +++ b/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: """