From 5e5f559b4d336866939560e0b30084b7df53b280 Mon Sep 17 00:00:00 2001 From: Adam Johnson Date: Tue, 10 Sep 2019 11:21:20 +0100 Subject: [PATCH 1/6] Improve set_rollback() behaviour \## Description Fixes #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. --- rest_framework/views.py | 27 ++++++++++++++++-------- tests/test_atomic_requests.py | 39 +++++++++++++++++++++++++++-------- 2 files changed, 48 insertions(+), 18 deletions(-) diff --git a/rest_framework/views.py b/rest_framework/views.py index 69db053d64..cadcd2a644 100644 --- a/rest_framework/views.py +++ b/rest_framework/views.py @@ -3,7 +3,7 @@ """ from django.conf import settings from django.core.exceptions import PermissionDenied -from django.db import connection, models, transaction +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 @@ -62,10 +62,19 @@ def get_view_description(view, html=False): return description -def set_rollback(): - atomic_requests = connection.settings_dict.get('ATOMIC_REQUESTS', False) - if atomic_requests and connection.in_atomic_block: - transaction.set_rollback(True) +def set_rollback(request): + # We need the actual view func returned by the URL resolver which gets used + # by Django's BaseHandler to determine `non_atomic_requests`. Be cautious + # when fetching it though as it won't be set when views are tested with + # requessts from a RequestFactory. + try: + non_atomic_requests = request.resolver_match.func._non_atomic_requests + except AttributeError: + non_atomic_requests = set() + + 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) def exception_handler(exc, context): @@ -95,7 +104,7 @@ def exception_handler(exc, context): else: data = {'detail': exc.detail} - set_rollback() + set_rollback(context['request']) return Response(data, status=exc.status_code, headers=headers) return None @@ -223,9 +232,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, } def get_view_name(self): diff --git a/tests/test_atomic_requests.py b/tests/test_atomic_requests.py index de04d2c069..c158169dbc 100644 --- a/tests/test_atomic_requests.py +++ b/tests/test_atomic_requests.py @@ -3,7 +3,7 @@ from django.conf.urls import url from django.db import connection, connections, transaction from django.http import Http404 -from django.test import TestCase, TransactionTestCase, override_settings +from django.test import TestCase, override_settings from rest_framework import status from rest_framework.exceptions import APIException @@ -39,12 +39,24 @@ 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 + + +class UrlDecoratedNonAtomicAPIExceptionView(APIView): + def get(self, request, *args, **kwargs): + list(BasicModel.objects.all()) raise Http404 urlpatterns = ( - url(r'^$', NonAtomicAPIExceptionView.as_view()), + url(r'^non-atomic-exception$', NonAtomicAPIExceptionView.as_view()), + url( + r'^url-decorated-non-atomic-exception$', + transaction.non_atomic_requests( + UrlDecoratedNonAtomicAPIExceptionView.as_view() + ), + ), ) @@ -94,8 +106,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) assert not transaction.get_rollback() assert BasicModel.objects.count() == 1 @@ -135,7 +147,7 @@ def test_api_exception_rollback_transaction(self): "'atomic' requires transactions and savepoints." ) @override_settings(ROOT_URLCONF='tests.test_atomic_requests') -class NonAtomicDBTransactionAPIExceptionTests(TransactionTestCase): +class NonAtomicDBTransactionAPIExceptionTests(TestCase): def setUp(self): connections.databases['default']['ATOMIC_REQUESTS'] = True @@ -143,8 +155,17 @@ def tearDown(self): 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 assert response.status_code == status.HTTP_404_NOT_FOUND + assert not transaction.get_rollback() + # Check we can still perform DB queries + list(BasicModel.objects.all()) + + def test_api_exception_rollback_transaction_url_decorated_non_atomic_view(self): + response = self.client.get('/url-decorated-non-atomic-exception') + + assert response.status_code == status.HTTP_404_NOT_FOUND + assert not transaction.get_rollback() + # Check we can still perform DB queries + list(BasicModel.objects.all()) From 5c65845b158b112ac44fe8d35b4aed230ddb741a Mon Sep 17 00:00:00 2001 From: Adam Johnson Date: Fri, 13 Dec 2019 20:51:54 +0000 Subject: [PATCH 2/6] Revert check for non_atomic_requests, instead rely again on db.in_atomic_block --- rest_framework/views.py | 19 +++++++------------ tests/test_atomic_requests.py | 32 +++++++------------------------- 2 files changed, 14 insertions(+), 37 deletions(-) diff --git a/rest_framework/views.py b/rest_framework/views.py index cadcd2a644..9120079d07 100644 --- a/rest_framework/views.py +++ b/rest_framework/views.py @@ -62,18 +62,13 @@ def get_view_description(view, html=False): return description -def set_rollback(request): - # We need the actual view func returned by the URL resolver which gets used - # by Django's BaseHandler to determine `non_atomic_requests`. Be cautious - # when fetching it though as it won't be set when views are tested with - # requessts from a RequestFactory. - try: - non_atomic_requests = request.resolver_match.func._non_atomic_requests - except AttributeError: - non_atomic_requests = set() - +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.alias not in non_atomic_requests: + if db.settings_dict['ATOMIC_REQUESTS'] and db.in_atomic_block: transaction.set_rollback(True, using=db.alias) @@ -104,7 +99,7 @@ def exception_handler(exc, context): else: data = {'detail': exc.detail} - set_rollback(context['request']) + set_rollback() return Response(data, status=exc.status_code, headers=headers) return None diff --git a/tests/test_atomic_requests.py b/tests/test_atomic_requests.py index c158169dbc..e6969bdfa7 100644 --- a/tests/test_atomic_requests.py +++ b/tests/test_atomic_requests.py @@ -3,7 +3,7 @@ from django.conf.urls import url from django.db import connection, connections, transaction from django.http import Http404 -from django.test import TestCase, override_settings +from django.test import TestCase, TransactionTestCase, override_settings from rest_framework import status from rest_framework.exceptions import APIException @@ -43,20 +43,8 @@ def get(self, request, *args, **kwargs): raise Http404 -class UrlDecoratedNonAtomicAPIExceptionView(APIView): - def get(self, request, *args, **kwargs): - list(BasicModel.objects.all()) - raise Http404 - - urlpatterns = ( url(r'^non-atomic-exception$', NonAtomicAPIExceptionView.as_view()), - url( - r'^url-decorated-non-atomic-exception$', - transaction.non_atomic_requests( - UrlDecoratedNonAtomicAPIExceptionView.as_view() - ), - ), ) @@ -147,25 +135,19 @@ def test_api_exception_rollback_transaction(self): "'atomic' requires transactions and savepoints." ) @override_settings(ROOT_URLCONF='tests.test_atomic_requests') -class NonAtomicDBTransactionAPIExceptionTests(TestCase): +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('/non-atomic-exception') + # 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 - assert not transaction.get_rollback() - # Check we can still perform DB queries - list(BasicModel.objects.all()) - - def test_api_exception_rollback_transaction_url_decorated_non_atomic_view(self): - response = self.client.get('/url-decorated-non-atomic-exception') - - assert response.status_code == status.HTTP_404_NOT_FOUND - assert not transaction.get_rollback() # Check we can still perform DB queries list(BasicModel.objects.all()) From 60a149ee233dc96b6625a1bac3963ebb70359576 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Asif=20Saif=20Uddin=20=7B=22Auvi=22=3A=22=E0=A6=85?= =?UTF-8?q?=E0=A6=AD=E0=A6=BF=22=7D?= Date: Wed, 5 Aug 2026 22:43:05 +0600 Subject: [PATCH 3/6] Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- rest_framework/views.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rest_framework/views.py b/rest_framework/views.py index b2b0bff5d7..22fdc3eb82 100644 --- a/rest_framework/views.py +++ b/rest_framework/views.py @@ -64,10 +64,10 @@ 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 + # 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(): if db.settings_dict['ATOMIC_REQUESTS'] and db.in_atomic_block: transaction.set_rollback(True, using=db.alias) From 13a4d437c1547f722ce60bde2080eb5743a13bde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Asif=20Saif=20Uddin=20=7B=22Auvi=22=3A=22=E0=A6=85?= =?UTF-8?q?=E0=A6=AD=E0=A6=BF=22=7D?= Date: Wed, 5 Aug 2026 22:43:22 +0600 Subject: [PATCH 4/6] Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- rest_framework/views.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rest_framework/views.py b/rest_framework/views.py index 22fdc3eb82..661ae431ae 100644 --- a/rest_framework/views.py +++ b/rest_framework/views.py @@ -233,9 +233,9 @@ def get_exception_handler_context(self): """ return { 'view': self, - 'args': self.args, - 'kwargs': self.kwargs, - 'request': self.request, + 'args': getattr(self, 'args', ()), + 'kwargs': getattr(self, 'kwargs', {}), + 'request': getattr(self, 'request', None), } def get_view_name(self): From 00a29c18388af86789199d14b8907f6f137d6081 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Asif=20Saif=20Uddin=20=7B=22Auvi=22=3A=22=E0=A6=85?= =?UTF-8?q?=E0=A6=AD=E0=A6=BF=22=7D?= Date: Wed, 5 Aug 2026 22:43:34 +0600 Subject: [PATCH 5/6] Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- tests/test_atomic_requests.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/test_atomic_requests.py b/tests/test_atomic_requests.py index 208ab80690..68ec364def 100644 --- a/tests/test_atomic_requests.py +++ b/tests/test_atomic_requests.py @@ -95,8 +95,9 @@ 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(): + with self.assertRaises(Exception): + self.view(request) assert not transaction.get_rollback() assert BasicModel.objects.count() == 1 From 377f61b369b86ec1fc9b10a9a1c1fcf6ad113f0e Mon Sep 17 00:00:00 2001 From: Majid Date: Wed, 16 Sep 2026 13:37:47 +0330 Subject: [PATCH 6/6] Fix set_rollback for non_atomic_requests views set_rollback() now mirrors Django's BaseHandler.make_view_atomic() logic: when a request is provided, it reads the resolved view's _non_atomic_requests attribute and skips databases that were explicitly excluded from ATOMIC_REQUESTS. This prevents DRF from marking outer transactions (e.g. Django's TestCase wrapper) for rollback when an APIException is raised from a @non_atomic_requests view. The previous signature set_rollback() is deprecated and will emit a RemovedInDRF321Warning. It falls back to the old ATOMIC_REQUESTS + in_atomic_block behavior. Refs #6921, #6922. --- rest_framework/deprecation.py | 4 ++ rest_framework/views.py | 52 +++++++++++++++++++----- tests/test_atomic_requests.py | 76 ++++++++++++++++++++++++++++++++++- 3 files changed, 121 insertions(+), 11 deletions(-) diff --git a/rest_framework/deprecation.py b/rest_framework/deprecation.py index ce2be3a2ec..0189f11545 100644 --- a/rest_framework/deprecation.py +++ b/rest_framework/deprecation.py @@ -21,6 +21,10 @@ class RemovedInDRF320Warning(PendingDeprecationWarning): pass +class RemovedInDRF321Warning(PendingDeprecationWarning): + pass + + # Aliases that always track the current release cycle, so that projects can # filter on them without editing their configuration on every DRF release. RemovedInNextDRFVersionWarning = RemovedInDRF319Warning diff --git a/rest_framework/views.py b/rest_framework/views.py index c29ccdfc62..72d6eeb301 100644 --- a/rest_framework/views.py +++ b/rest_framework/views.py @@ -1,9 +1,11 @@ """ Provides an APIView class that is the base of all views in REST framework. """ +import warnings + from django.conf import settings from django.core.exceptions import PermissionDenied -from django.db import connections, models, transaction +from django.db import connections, models from django.http import Http404 from django.http.response import HttpResponseBase from django.utils.cache import patch_vary_headers @@ -13,6 +15,7 @@ from rest_framework import exceptions, status from rest_framework.compat import split_header_value +from rest_framework.deprecation import RemovedInDRF321Warning from rest_framework.request import Request from rest_framework.response import Response from rest_framework.schemas import DefaultSchema @@ -63,14 +66,45 @@ def get_view_description(view, html=False): return description -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. +def set_rollback(request=None): + """ + Mark databases for rollback that were atomically wrapped by + ATOMIC_REQUESTS. + + If `request` is provided, we mirror Django's + ``BaseHandler.make_view_atomic`` logic and skip databases where the + resolved view is decorated with ``@transaction.non_atomic_requests``. + This prevents DRF from accidentally marking outer transactions (for + example, the one Django's ``TestCase`` wraps each test in) for + rollback. + + Passing ``request`` is recommended. Calling without it is deprecated. + """ + if request is None: + warnings.warn( + "Calling `set_rollback()` without a `request` argument is " + "deprecated. `set_rollback(request)` will be required in a " + "future release.", + RemovedInDRF321Warning, + stacklevel=2, + ) + non_atomic_requests = None + else: + # Best-effort: RequestFactory may not set `resolver_match`. + view_func = getattr( + getattr(request, 'resolver_match', None), 'func', None + ) + non_atomic_requests = getattr(view_func, '_non_atomic_requests', None) + for db in connections.all(initialized_only=True): - if db.settings_dict['ATOMIC_REQUESTS'] and db.in_atomic_block: - transaction.set_rollback(True, using=db.alias) + if not db.settings_dict['ATOMIC_REQUESTS']: + continue + if not db.in_atomic_block: + continue + # Mirror Django's make_view_atomic. + if non_atomic_requests is not None and db.alias in non_atomic_requests: + continue + db.set_rollback(True) def exception_handler(exc, context): @@ -100,7 +134,7 @@ def exception_handler(exc, context): else: data = {'detail': exc.detail} - set_rollback() + set_rollback(context.get('request')) return Response(data, status=exc.status_code, headers=headers) return None diff --git a/tests/test_atomic_requests.py b/tests/test_atomic_requests.py index e433bd799b..b0dd04a7ef 100644 --- a/tests/test_atomic_requests.py +++ b/tests/test_atomic_requests.py @@ -1,4 +1,5 @@ import unittest +import warnings from django.db import connection, connections, transaction from django.http import Http404 @@ -6,6 +7,7 @@ from django.urls import path from rest_framework import status +from rest_framework.deprecation import RemovedInDRF321Warning from rest_framework.exceptions import APIException from rest_framework.response import Response from rest_framework.test import APIRequestFactory @@ -202,8 +204,9 @@ def tearDown(self): connections.databases['default']['ATOMIC_REQUESTS'] = False def test_marks_initialized_atomic_connection_for_rollback(self): + request = factory.post('/') with transaction.atomic(): - set_rollback() + set_rollback(request) assert transaction.get_rollback() @@ -238,7 +241,8 @@ def tearDown(self): super().tearDown() def test_does_not_initialize_unused_connections(self): - set_rollback() + request = factory.post('/') + set_rollback(request) assert not hasattr(connections._connections, 'secondary') @@ -257,3 +261,71 @@ def test_api_exception_leaves_unused_connection_uninitialized(self): response = self.view(request) assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR assert not hasattr(connections._connections, 'secondary') + + +@override_settings(ROOT_URLCONF='tests.test_atomic_requests') +class NonAtomicViewUnderTestCaseTests(TestCase): + """ + Regression test for #6921. + + A view decorated with ``@transaction.non_atomic_requests`` must not + poison the outer transaction that Django's ``TestCase`` wraps each + test in. Before the fix, ``set_rollback`` would see + ``connection.in_atomic_block == True`` and mark the outer transaction + for rollback, breaking any subsequent queries in the test. + """ + def setUp(self): + connections.databases['default']['ATOMIC_REQUESTS'] = True + + def tearDown(self): + connections.databases['default']['ATOMIC_REQUESTS'] = False + + def test_non_atomic_view_does_not_mark_outer_transaction_for_rollback(self): + # TestCase has already opened an atomic block around this test. + assert transaction.get_rollback() is False + + # Sending the request through the test client populates + # ``request.resolver_match``, so ``set_rollback`` can see the + # view's ``_non_atomic_requests`` attribute and skip the database. + response = self.client.get('/') + assert response.status_code == status.HTTP_404_NOT_FOUND + + # The outer transaction must remain usable. Without the fix, + # ``set_rollback`` would have marked it for rollback and the + # following query would raise. + assert transaction.get_rollback() is False + BasicModel.objects.create() + + +class SetRollbackDeprecationTests(TestCase): + """ + Calling ``set_rollback()`` without a ``request`` argument should emit + a ``RemovedInDRF321Warning``. + """ + def test_calling_without_request_emits_warning(self): + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + set_rollback() + assert len(caught) == 1 + assert issubclass(caught[0].category, RemovedInDRF321Warning) + assert "without a `request` argument is deprecated" in str(caught[0].message) + + +class SetRollbackWithoutResolverMatchTests(TestCase): + """ + ``APIRequestFactory`` produces requests without a ``resolver_match``. + ``set_rollback`` must fall back to the previous + ``ATOMIC_REQUESTS + in_atomic_block`` behavior in that case. + """ + def setUp(self): + connections.databases['default']['ATOMIC_REQUESTS'] = True + + def tearDown(self): + connections.databases['default']['ATOMIC_REQUESTS'] = False + + def test_falls_back_when_resolver_match_missing(self): + request = factory.post('/') + assert getattr(request, 'resolver_match', None) is None + with transaction.atomic(): + set_rollback(request) + assert transaction.get_rollback()