Skip to content
10 changes: 7 additions & 3 deletions rest_framework/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"""
from django.conf import settings
from django.core.exceptions import PermissionDenied
from django.db import connections, models
from django.db import connections, models, transaction
from django.http import Http404
from django.http.response import HttpResponseBase
from django.utils.cache import patch_vary_headers
Expand Down Expand Up @@ -64,9 +64,13 @@ def get_view_description(view, html=False):


def set_rollback():
# 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.
for db in connections.all(initialized_only=True):
if db.settings_dict['ATOMIC_REQUESTS'] and db.in_atomic_block:
db.set_rollback(True)
transaction.set_rollback(True, using=db.alias)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latter (TransactionTestCase should still work).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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."

Comment on lines 66 to +73

Copilot AI Apr 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.


def exception_handler(exc, context):
Expand Down Expand Up @@ -230,7 +234,7 @@ def get_exception_handler_context(self):
'view': self,
'args': getattr(self, 'args', ()),
'kwargs': getattr(self, 'kwargs', {}),
'request': getattr(self, 'request', None)
'request': getattr(self, 'request', None),
}

def get_view_name(self):
Expand Down
19 changes: 12 additions & 7 deletions tests/test_atomic_requests.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,12 @@ def dispatch(self, *args, **kwargs):
return super().dispatch(*args, **kwargs)

def get(self, request, *args, **kwargs):
BasicModel.objects.all()
list(BasicModel.objects.all())
raise Http404


urlpatterns = (
path('non-atomic-exception', NonAtomicAPIExceptionView.as_view()),
path('', NonAtomicAPIExceptionView.as_view()),
)

Expand Down Expand Up @@ -95,7 +96,8 @@ def test_generic_exception_delegate_transaction_management(self):
# 2 - insert
# 3 - release savepoint
with transaction.atomic():
self.assertRaises(Exception, self.view, request)
with self.assertRaises(Exception):
self.view(request)
assert not transaction.get_rollback()
assert BasicModel.objects.count() == 1

Expand Down Expand Up @@ -174,15 +176,18 @@ 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

def test_api_exception_rollback_transaction_non_atomic_view(self):
response = self.client.get('/')
response = self.client.get('/non-atomic-exception')

# without checking connection.in_atomic_block view raises 500
# due attempt to rollback without transaction
# without check for db.in_atomic_block, would raise 500 due to attempt
# to rollback without transaction
assert response.status_code == status.HTTP_404_NOT_FOUND
# Check we can still perform DB queries
list(BasicModel.objects.all())


@unittest.skipUnless(
Expand Down