Skip to content

Improve set_rollback() behaviour #6922

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
adamchainz wants to merge 4 commits into
base: main
Choose a base branch
from
+21 −13
Open

Improve set_rollback() behaviour #6922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5e5f559
Improve set_rollback() behaviour
adamchainz Sep 10, 2019
5c65845
Revert check for non_atomic_requests, instead rely again on db.in_ato…
adamchainz Dec 13, 2019
abffc1e
Merge branch 'master' into fix_issue_6921_atomic_requests
auvipy Apr 28, 2025
a6cffb0
Merge branch 'main' into fix_issue_6921_atomic_requests
auvipy Apr 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions rest_framework/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from django import VERSION as DJANGO_VERSION
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 cc_delim_re, 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 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
Copy link
Copy Markdown
Contributor

@lovelydinosaur lovelydinosaur Dec 16, 2019

Choose a reason for hiding this comment

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

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?

Comment on lines +67 to +70
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
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)
Copy link
Copy Markdown
Contributor

@lovelydinosaur lovelydinosaur Dec 12, 2019

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

@adamchainz adamchainz Dec 12, 2019

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
Contributor

@lovelydinosaur lovelydinosaur Dec 12, 2019

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
Contributor

@lovelydinosaur lovelydinosaur Dec 12, 2019

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

@adamchainz adamchainz Dec 12, 2019

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
Contributor

@lovelydinosaur lovelydinosaur Dec 12, 2019

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

@adamchainz adamchainz Dec 12, 2019

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
Contributor

@lovelydinosaur lovelydinosaur Dec 13, 2019

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

@adamchainz adamchainz Dec 13, 2019

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
Contributor

@lovelydinosaur lovelydinosaur Dec 16, 2019

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
Copy link

Copilot AI Apr 2, 2026

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 @@ -229,9 +233,9 @@ def get_exception_handler_context(self):
"""
return {
'view': self,
'args': getattr(self, 'args', ()),
'kwargs': getattr(self, 'kwargs', {}),
'request': getattr(self, 'request', None)
'args': self.args,
'kwargs': self.kwargs,
'request': self.request,
Comment on lines +236 to +238
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

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

def get_view_name(self):
Expand Down
20 changes: 12 additions & 8 deletions tests/test_atomic_requests.py
View file
Open in desktop
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 @@ -94,8 +95,8 @@ def test_generic_exception_delegate_transaction_management(self):
# 1 - begin savepoint
# 2 - insert
# 3 - release savepoint
with transaction.atomic():
self.assertRaises(Exception, self.view, request)
with transaction.atomic(), self.assertRaises(Exception):
self.view(request)
Comment on lines +98 to +99
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
assert not transaction.get_rollback()
assert BasicModel.objects.count() == 1

Expand Down Expand Up @@ -174,12 +175,15 @@ 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())
Loading