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 e7bf416..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): """ @@ -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. 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: """