Improve set_rollback() behaviour by adamchainz · Pull Request #6922 · encode/django-rest-framework

adamchainz · 2019-09-10T11:29:22Z

Description

Added tests that fail before and pass afterwards.

Remove the check for connection.in_atomic_block to determine if the current request is under a transaction.atomic from ATOMIC_REQUESTS. Instead, duplicate the method that Django itself uses in BaseHandler.

This requires fetching the actual view function from as_view(), as seen by the URL resolver / BaseHandler. Since this requires request, I've also changed the accesses in get_exception_handler_context to be direct attribute accesses rather than getattr(). It seems the getattr defaults not accessible since self.request, self.args, and self.kwargs are always set in dispatch() before handle_exception() can ever be called. This is useful since request is always needed for the new set_rollback logic.

This also fixes a bug with multi-DB compatibility - previously set_rollback would only be called on the default DB when there are multiple DB's.

adamchainz · 2019-09-10T11:31:28Z

So there are a bunch of test failures, all:

AttributeError: 'NoneType' object has no attribute 'func'

This is all due to use of RequestFactory, which doesn't set resolver_match on its requests as it bypasses URL resolution. In a real world Django app, URL resolution doesn't get bypassed like this.

This could be fixed by moving all those tests to use the test Client, which does set resolver_match, but it's a lot of work.

auvipy · 2019-09-23T15:20:52Z

I can help with updating the tests, can you share your suggested changes for the tests? I can come up with a PR

rpkilby · 2019-09-23T21:41:22Z

This could be fixed by moving all those tests to use the test Client, which does set resolver_match, but it's a lot of work.

An off the cuff reaction, but I don't think we'd want to go this route. Ignoring the headache of updating the test suite, wouldn't this imply that users are unable to test their views with the request factory? Users would have to use the test client instead, right?

Would it be sufficient to do a best effort to get the non_atomic_requests flag? e.g., from this:

    view_func = request.resolver_match.func
    non_atomic_requests = getattr(view_func, '_non_atomic_requests', set())

to this (and document the deviation from the BaseHandler):

    try:
        non_atomic_requests = request.resolver_match.func._non_atomic_requests
    except AttributeError:
        non_atomic_requests = set()

adamchainz · 2019-09-24T21:58:08Z

An off the cuff reaction, but I don't think we'd want to go this route. Ignoring the headache of updating the test suite, wouldn't this imply that users are unable to test their views with the request factory? Users would have to use the test client instead, right?

You're right, it's probably not a good idea.

Would it be sufficient to do a best effort to get the non_atomic_requests flag?

I guess so - I've updated the PR accordingly.

rpkilby

Thanks for the update. Added to the 3.11 milestone.

rest_framework/views.py

tests/test_atomic_requests.py

lovelydinosaur · 2019-12-12T14:39:40Z

rest_framework/views.py

+
+    for db in connections.all():
+        if db.settings_dict['ATOMIC_REQUESTS'] and db.alias not in non_atomic_requests:
+            transaction.set_rollback(True, using=db.alias)


I'm a bit wary of this. Why is connection.in_atomic_block not sufficient, since it seems like it'd be the right thing to be doing? What does connection.in_atomic_block return when inside a non_atomic_request decorated view?

Is it possible to isolate the multi-DB fix in this PR from the _non_atomic_requests change in the PR, or are they tightly linked?

Why is connection.in_atomic_block not sufficient, since it seems like it'd be the right thing to be doing? What does connection.in_atomic_block return when inside a non_atomic_request decorated view?

connection.in_atomic_block means "is the default DB in a transaction?" not "is any DB in a transaction started by ATOMIC_REQUESTS?"

This has a number of subtleties. connection.in_atomic_block can return True in a view with @non_atomic_requests if there is an atomic block from another source than ATOMIC_REQUESTS. This is most likely to be from Django's TestCase, but could also be a custom view decorator or middleware for managing transactions.

More information on the specific case I had was:

Move a client's app to ATOMIC_REQUESTS = True on default DB because they had a number of errors due to not using transactions

Wrap some views with @non_atomic_requests because they weren't safe for ATOMIC_REQUESTS, and instead manually decorate with @atomic internally

Have unit tests for those views using django's TestCase, which sets up two atomic blocks around tests.

Those unit tests crash. DRF's set_rollback() sees connection.in_atomic_block is True, despite the transaction not coming from ATOMIC_REQUESTS. Tests checking error paths in the views cause attempt to rollback the transaction from TestCase, which TestCase also tries to rollback, which breaks the whole test run due to unbalanced transactions.

Is it possible to isolate the multi-DB fix in this PR from the _non_atomic_requests change in the PR, or are they tightly linked?

The multi-DB fix comes "for free" because set_rollback() here now copies what Django does in BaseHandler. I think moving away from this is riskier than trying to make this patch focussed only on the single DB case.

I have monkey patched the implementation in this PR into my client's app to fix things there and there have been no issues for 3 months now.

Gotcha. Is there any more graceful way we can do connection.in_atomic_block on a per-db basis?

Eg, is something along these lines possible instead?...

for db in connections.all(): if db.settings_dict['ATOMIC_REQUESTS'] and db.in_atomic_block: transaction.set_rollback(True, using=db.alias)

Yes that should work but it wouldn't fix my bug with testing the @non_atomic_requests views under TestCase

it wouldn't fix my bug with testing the @non_atomic_requests views under TestCase

Sorry, walk me through that. Do you mean would be broken for @non_atomic_requests views, or that it would be broken for test cases of @non_atomic_requests views?

The latter (TransactionTestCase should still work).

Okey dokes. So would we be able to update the PR to use the style in the comment above, and switch any test cases to TransactionTestCase if required?

Done.

However this doesn't fix #6921. I will still need that patch in place for my client because their tests use TestCase on @non_atomic_requests views. I remembered while writing that commit that even raise Http404 counts as an error on DRF ;)

"I will still need that patch in place for my client because their tests use TestCase on @non_atomic_requests views."

Okay, that seems to fall within documented expected behavior...

https://docs.djangoproject.com/en/2.2/topics/testing/tools/#django.test.TransactionTestCase "Django’s TestCase class is a more commonly used subclass of TransactionTestCase that makes use of database transaction facilities to speed up the process of resetting the database to a known state at the beginning of each test. A consequence of this, however, is that some database behaviors cannot be tested within a Django TestCase class. For instance, you cannot test that a block of code is executing within a transaction, as is required when using select_for_update(). In those cases, you should use TransactionTestCase."

lovelydinosaur · 2019-12-12T14:44:53Z

I'd like to release 3.11, and I'm not clear enough about this one yet, so going to drop this from the milestone.

If we absolutely need to we can always include it on a point release so long as we make sure to handle the potential signature change on the exception handler in a backwards compat way.

\## Description Fixes encode#6921. Added tests that fail before and pass afterwards. Remove the check for `connection.in_atomic_block` to determine if the current request is under a `transaction.atomic` from `ATOMIC_REQUESTS`. Instead, duplicate the method that Django itself uses [in BaseHandler](https://github.com/django/django/blob/964dd4f4f208722d8993a35c1ff047d353cea1ea/django/core/handlers/base.py#L64). This requires fetching the actual view function from `as_view()`, as seen by the URL resolver / BaseHandler. Since this requires `request`, I've also changed the accesses in `get_exception_handler_context` to be direct attribute accesses rather than `getattr()`. It seems the `getattr` defaults not accessible since `self.request`, `self.args`, and `self.kwargs` are always set in `dispatch()` before `handle_exception()` can ever be called. This is useful since `request` is always needed for the new `set_rollback` logic.

…mic_block

lovelydinosaur · 2019-12-16T13:27:07Z

rest_framework/views.py

+    # Rollback all connections that have ATOMIC_REQUESTS set, if it looks their
+    # @atomic for the request was started
+    # Note this check in_atomic_block may be a false positive due to
+    # transactions started another way, e.g. through testing with TestCase


Perhaps we ought to be a bit more specific here and link to the https://docs.djangoproject.com/en/2.2/topics/testing/tools/#django.test.TransactionTestCase docs?

stale · 2022-06-19T18:00:43Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

auvipy

OK Adam, can you please come back to it and make it merge able again? And perhaps from where anyone should retake it?

stale · 2025-04-27T19:31:43Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

auvipy · 2025-04-28T03:45:54Z

I was able to fix the merge errors, somehow. and the tests seems still passing. but will left this for both of you to decide if we should move it with it or not

stale · 2025-06-27T06:34:56Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

ulgens · 2025-06-27T09:26:19Z

I think this is still relevant and shouldn't be marked as stale.

Copilot

Pull request overview

This PR aims to correct DRF’s rollback behavior when handling exceptions under ATOMIC_REQUESTS, including improving multi-database behavior and adding regression coverage for non-atomic views.

Changes:

Update set_rollback() to apply rollback per database alias (multi-DB) and adjust transaction API usage.
Add/adjust atomic/non-atomic request tests and URL routing used by those tests.
Change exception handler context to use direct self.args/self.kwargs/self.request attribute access.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
`rest_framework/views.py`	Adjusts rollback logic and modifies exception handler context attribute access.
`tests/test_atomic_requests.py`	Updates atomic/non-atomic transaction tests and URL patterns to exercise rollback behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-02T14:27:05Z

tests/test_atomic_requests.py

+            with transaction.atomic(), self.assertRaises(Exception):
+                self.view(request)


The assert not transaction.get_rollback() line is unreachable because self.view(request) raises inside the combined with transaction.atomic(), self.assertRaises(Exception): context manager. As written, this test no longer verifies that rollback is not set when the exception is caught inside the atomic block. Consider nesting the context managers (or separating them) so the rollback assertion runs after the exception is handled, while still inside the atomic block.

Suggested change

with transaction.atomic(), self.assertRaises(Exception):

self.view(request)

with transaction.atomic():

with self.assertRaises(Exception):

self.view(request)

Copilot · 2026-04-02T14:27:05Z

rest_framework/views.py

 def set_rollback():
+    # Rollback all connections that have ATOMIC_REQUESTS set, if it looks their
+    # @atomic for the request was started
+    # Note this check in_atomic_block may be a false positive due to
+    # transactions started another way, e.g. through testing with TestCase
    for db in connections.all():
        if db.settings_dict['ATOMIC_REQUESTS'] and db.in_atomic_block:
-            db.set_rollback(True)
+            transaction.set_rollback(True, using=db.alias)


set_rollback() still gates rollback purely on ATOMIC_REQUESTS + db.in_atomic_block. This is the behavior called out in #6921 as inaccurate: in_atomic_block can be true due to an outer transaction (e.g. TestCase wrapping) even when the resolved view is marked @transaction.non_atomic_requests, and in that case DRF should not mark the transaction for rollback. To address the issue, set_rollback likely needs access to the resolved view callable (e.g. via request.resolver_match.func) and to replicate Django's make_view_atomic/non_atomic_requests checks per-DB alias before calling transaction.set_rollback(..., using=db.alias).

Copilot · 2026-04-02T14:27:05Z

tests/test_atomic_requests.py

 @override_settings(ROOT_URLCONF='tests.test_atomic_requests')
 class NonAtomicDBTransactionAPIExceptionTests(TransactionTestCase):
    def setUp(self):
        connections.databases['default']['ATOMIC_REQUESTS'] = True

-    def tearDown(self):
-        connections.databases['default']['ATOMIC_REQUESTS'] = False
+        @self.addCleanup
+        def restore_atomic_requests():
+            connections.databases['default']['ATOMIC_REQUESTS'] = False


This regression test uses TransactionTestCase, which does not wrap each test in an outer transaction.atomic(). The false-positive behavior described in #6921 occurs specifically when connection.in_atomic_block is true for reasons other than ATOMIC_REQUESTS (e.g. Django TestCase’s per-test atomic wrapper). Consider switching this test to TestCase (or otherwise ensuring an outer atomic is active during the request) so it actually fails under the pre-fix behavior and guards against the reported scenario.

Copilot · 2026-04-02T14:27:06Z

rest_framework/views.py

+            'args': self.args,
+            'kwargs': self.kwargs,
+            'request': self.request,


Switching get_exception_handler_context() to direct attribute access can raise AttributeError and mask the original exception in some edge cases (e.g., if an exception occurs before self.request is set in dispatch(), or if a subclass/third-party code calls this helper outside the normal dispatch() flow). Keeping the previous getattr(..., default) pattern (or initializing self.request/self.args/self.kwargs earlier) would preserve the prior, more robust behavior while still allowing request to be present in the common path.

Suggested change

'args': self.args,

'kwargs': self.kwargs,

'request': self.request,

'args': getattr(self, 'args', ()),

'kwargs': getattr(self, 'kwargs', {}),

'request': getattr(self, 'request', None),

Copilot · 2026-04-02T14:27:06Z

rest_framework/views.py

+    # Rollback all connections that have ATOMIC_REQUESTS set, if it looks their
+    # @atomic for the request was started
+    # Note this check in_atomic_block may be a false positive due to
+    # transactions started another way, e.g. through testing with TestCase


The comment has a couple of grammar issues that make it harder to parse (e.g., “if it looks their @atomic…” / missing “for” in “Note this check …”). Consider rewording for clarity (e.g., “if it looks like an ATOMIC_REQUESTS atomic block was started for the request”).

Suggested change

# Rollback all connections that have ATOMIC_REQUESTS set, if it looks their

# @atomic for the request was started

# Note this check in_atomic_block may be a false positive due to

# transactions started another way, e.g. through testing with TestCase

# Rollback all connections that have ATOMIC_REQUESTS set, if it looks like

# the @atomic block for the request was started.

# Note that this in_atomic_block check may be a false positive due to

# transactions started in other ways, e.g. when testing with TestCase.

adamchainz force-pushed the fix_issue_6921_atomic_requests branch from 7059fdc to 13b4c47 Compare September 24, 2019 21:58

auvipy approved these changes Sep 25, 2019

View reviewed changes

rpkilby added this to the 3.11 Release milestone Sep 25, 2019

rpkilby approved these changes Sep 25, 2019

View reviewed changes

rpkilby reviewed Sep 25, 2019

View reviewed changes

rest_framework/views.py Show resolved Hide resolved

xordoquy suggested changes Sep 25, 2019

View reviewed changes

rest_framework/views.py Show resolved Hide resolved

rpkilby reviewed Sep 25, 2019

View reviewed changes

tests/test_atomic_requests.py Outdated Show resolved Hide resolved

rpkilby previously approved these changes Dec 11, 2019

View reviewed changes

rpkilby requested a review from xordoquy December 11, 2019 14:48

lovelydinosaur reviewed Dec 12, 2019

View reviewed changes

lovelydinosaur removed this from the 3.11 Release milestone Dec 12, 2019

adamchainz added 2 commits December 13, 2019 18:27

Revert check for non_atomic_requests, instead rely again on db.in_ato…

5c65845

…mic_block

adamchainz force-pushed the fix_issue_6921_atomic_requests branch from 9405aa3 to 5c65845 Compare December 13, 2019 20:53

lovelydinosaur reviewed Dec 16, 2019

View reviewed changes

stale bot added the stale label Jun 19, 2022

auvipy requested changes Nov 24, 2022

View reviewed changes

stale bot removed the stale label Nov 24, 2022

adamchainz dismissed rpkilby’s stale review via 5c65845 May 30, 2024 15:42

stale bot added the stale label Apr 27, 2025

auvipy removed the stale label Apr 28, 2025

Merge branch 'master' into fix_issue_6921_atomic_requests

abffc1e

stale bot added the stale label Jun 27, 2025

stale bot closed this Oct 19, 2025

auvipy reopened this Oct 19, 2025

stale bot removed the stale label Oct 19, 2025

browniebroke mentioned this pull request Oct 20, 2025

set_rollback should only rollback initialized connections #9599

Open

Merge branch 'main' into fix_issue_6921_atomic_requests

a6cffb0

auvipy requested review from Copilot and removed request for xordoquy April 2, 2026 14:20

Copilot started reviewing on behalf of auvipy April 2, 2026 14:21 View session

Copilot AI reviewed Apr 2, 2026

View reviewed changes

		with transaction.atomic(), self.assertRaises(Exception):
		self.view(request)

Uh oh!

Conversation

adamchainz commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

adamchainz commented Sep 10, 2019

Uh oh!

auvipy commented Sep 23, 2019

Uh oh!

rpkilby commented Sep 23, 2019

Uh oh!

adamchainz commented Sep 24, 2019

Uh oh!

rpkilby left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lovelydinosaur commented Dec 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Jun 19, 2022

Uh oh!

auvipy left a comment • 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!

stale bot commented Apr 27, 2025

Uh oh!

auvipy commented Apr 28, 2025

Uh oh!

stale bot commented Jun 27, 2025

Uh oh!

ulgens commented Jun 27, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

adamchainz commented Sep 10, 2019 •

edited

Loading

auvipy left a comment •

edited

Loading