feat(db): raise when entering transaction if testcase has pending on_… #87

HamaBarhamou · 2025-10-08T16:45:28Z

Add a guard that raises when entering db.transaction()/transaction_if_not_already() if the testcase transaction already has pending on_commit callbacks (opt-out via setting); closes #31.

…commit (kraken-tech#31) Add guard that raises on entering db.transaction() (and transaction_if_not_already() when it opens) if the testcase transaction already has pending on_commit callbacks. Introduce _PendingTestcaseAfterCommitCallbacks and _error_if_pending_testcase_on_commit_callbacks; integrate with transaction() and transaction_if_not_already(). Add opt-out setting SUBATOMIC_RAISE_IF_PENDING_TESTCASE_ON_COMMIT_ON_ENTER (default: True). Fix transaction_if_not_already(): preserve semantics (no new atomic if already in one) and pass savepoint when innermost atomic wraps testcase. Tests: add tests/test_on_commit_guard.py covering raise + opt-out. Docs: update settings reference; CHANGELOG updated. Closes kraken-tech#31

samueljsb · 2025-10-08T18:57:47Z

Thanks so much for getting involved! 😁

I think @meshy has a plan to solve this one slightly differently. It might be worth discussing what the right approach would be on the issue before moving forward with this implementation.

LilyFirefly

Thanks for the fix! If we go with this solution, I have some suggestions:

LilyFirefly · 2025-10-09T08:17:52Z

src/django_subatomic/db.py

+    with _error_if_pending_testcase_on_commit_callbacks(using=using):
+        with _execute_on_commit_callbacks_in_tests(using), django_transaction.atomic(
+            using=using, durable=True
+        ):
+            yield


We prefer this style for nested context managers:

Suggested change

with _error_if_pending_testcase_on_commit_callbacks(using=using):

with _execute_on_commit_callbacks_in_tests(using), django_transaction.atomic(

using=using, durable=True

):

yield

with (

_error_if_pending_testcase_on_commit_callbacks(using=using),

_execute_on_commit_callbacks_in_tests(using),

django_transaction.atomic(using=using, durable=True),

):

yield

LilyFirefly · 2025-10-09T08:32:26Z

src/django_subatomic/db.py

+        if pending:  # non vide => des callbacks ont été enregistrés avant
+            raise _PendingTestcaseAfterCommitCallbacks(
+                pending=len(pending),
+                database=getattr(connection, "alias", None),


A connection will always have alias set.

Suggested change

database=getattr(connection, "alias", None),

database=connection.alias,

LilyFirefly · 2025-10-09T08:34:08Z

src/django_subatomic/db.py

+
+    if _innermost_atomic_block_wraps_testcase(using=using):
+        connection = django_transaction.get_connection(using=using)
+        pending = getattr(connection, "run_on_commit", None)


connection should always have a run_on_commit attribute.

Suggested change

pending = getattr(connection, "run_on_commit", None)

pending = connection.run_on_commit

LilyFirefly · 2025-10-09T08:41:55Z

src/django_subatomic/db.py

+    if in_transaction(using=using):
+        # Already in a transaction; don't open another.
        yield
+    else:
+        # If the innermost atomic block comes from the testcase, create a SAVEPOINT.
+        savepoint = _innermost_atomic_block_wraps_testcase(using=using)
+        with _error_if_pending_testcase_on_commit_callbacks(using=using):
+            with _execute_on_commit_callbacks_in_tests(using), django_transaction.atomic(
+                using=using, savepoint=savepoint
+            ):
+                yield


I don't think we want the in_transaction here:

Suggested change

if in_transaction(using=using):

# Already in a transaction; don't open another.

yield

else:

# If the innermost atomic block comes from the testcase, create a SAVEPOINT.

savepoint = _innermost_atomic_block_wraps_testcase(using=using)

with _error_if_pending_testcase_on_commit_callbacks(using=using):

with _execute_on_commit_callbacks_in_tests(using), django_transaction.atomic(

using=using, savepoint=savepoint

):

yield

savepoint = _innermost_atomic_block_wraps_testcase(using=using)

with (

_error_if_pending_testcase_on_commit_callbacks(using=using),

_execute_on_commit_callbacks_in_tests(using),

django_transaction.atomic(using=using, savepoint=savepoint),

):

yield

LilyFirefly · 2025-10-09T08:43:30Z

tests/test_on_commit_guard.py

+        dj_tx.on_commit(lambda: None)
+        with self.assertRaises(_PendingTestcaseAfterCommitCallbacks):
+            with db.transaction():
+                pass


I think it makes sense to assert that the on_commit callback also doesn't run in this case:

Suggested change

dj_tx.on_commit(lambda: None)

with self.assertRaises(_PendingTestcaseAfterCommitCallbacks):

with db.transaction():

pass

calls = []

dj_tx.on_commit(lambda: calls.append("ok"))

with self.assertRaises(_PendingTestcaseAfterCommitCallbacks):

with db.transaction():

pass

self.assertEqual(calls, [])

LilyFirefly · 2025-10-09T08:44:29Z

tests/test_on_commit_guard.py

+        dj_tx.on_commit(lambda: None)
+        with self.assertRaises(_PendingTestcaseAfterCommitCallbacks):
+            with db.transaction_if_not_already():
+                pass


Suggested change

dj_tx.on_commit(lambda: None)

with self.assertRaises(_PendingTestcaseAfterCommitCallbacks):

with db.transaction_if_not_already():

pass

calls = []

dj_tx.on_commit(lambda: calls.append("ok"))

with self.assertRaises(_PendingTestcaseAfterCommitCallbacks):

with db.transaction_if_not_already():

pass

self.assertEqual(calls, [])

LilyFirefly · 2025-10-09T08:45:56Z

src/django_subatomic/db.py

+@attrs.frozen
+class _PendingTestcaseAfterCommitCallbacks(Exception):
+    pending: int
+    database: str | None = None


Suggested change

database: str | None = None

database: str

HamaBarhamou · 2025-10-09T09:26:28Z

hank you @LilyFirefly @samueljsb for your feedback; I will take it into account and, as already mentioned, let's first agree on the direction to take before moving forward.

cc @meshy

meshy · 2025-10-09T15:38:20Z

Hi @HamaBarhamou. Thank you for taking the time to address this issue. In hindsight I realise that I should have assigned myself to #31 when I started working on it, so that other people such as yourself didn't consider it "open". My apologies: I'll make an effort to "claim" tickets in future to prevent this kind of duplicated work.

I have now committed and pushed my (draft) implementation to #93.

HamaBarhamou · 2025-10-09T16:34:57Z

Hi @HamaBarhamou. Thank you for taking the time to address this issue. In hindsight I realise that I should have assigned myself to #31 when I started working on it, so that other people such as yourself didn't consider it "open". My apologies: I'll make an effort to "claim" tickets in future to prevent this kind of duplicated work.

I have now committed and pushed my (draft) implementation to #93.

@meshy OK, thanks for the feedback. No problem, it's my pleasure to contribute. I think we can close this draft and focus on yours.

HamaBarhamou requested a review from a team as a code owner October 8, 2025 16:45

LilyFirefly reviewed Oct 9, 2025

View reviewed changes

meshy mentioned this pull request Oct 9, 2025

Raise error when encountering unhandled callbacks #93

Draft

1 task

HamaBarhamou closed this Oct 9, 2025

	database=getattr(connection, "alias", None),
	database=connection.alias,

	pending = getattr(connection, "run_on_commit", None)
	pending = connection.run_on_commit

feat(db): raise when entering transaction if testcase has pending on_… #87

feat(db): raise when entering transaction if testcase has pending on_… #87

Uh oh!

Conversation

HamaBarhamou commented Oct 8, 2025

Uh oh!

samueljsb commented Oct 8, 2025

Uh oh!

LilyFirefly left a comment

Choose a reason for hiding this comment

Uh oh!

LilyFirefly Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

LilyFirefly Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

LilyFirefly Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

LilyFirefly Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

LilyFirefly Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LilyFirefly Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

LilyFirefly Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

HamaBarhamou commented Oct 9, 2025

Uh oh!

meshy commented Oct 9, 2025

Uh oh!

HamaBarhamou commented Oct 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LilyFirefly Oct 9, 2025 •

edited

Loading