Skip to content
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

Jump to bottom

Improve set_rollback() behaviour #6922

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Improve set_rollback() behaviour #6922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 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
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 11 additions & 7 deletions rest_framework/views.py
View file
Open in desktop
Expand Up @@ -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
Expand Down Expand Up @@ -63,9 +63,13 @@ def get_view_description(view, html=False):


def set_rollback():
atomic_requests = connection.settings_dict.get('ATOMIC_REQUESTS', False)
if atomic_requests and connection.in_atomic_block:
transaction.set_rollback(True)
# 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
Member

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?

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



def exception_handler(exc, context):
Expand Down Expand Up @@ -223,9 +227,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,
rpkilby marked this conversation as resolved.
Show resolved Hide resolved
}

def get_view_name(self):
Expand Down
21 changes: 12 additions & 9 deletions tests/test_atomic_requests.py
View file
Open in desktop
Expand Up @@ -39,12 +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 = (
url(r'^$', NonAtomicAPIExceptionView.as_view()),
url(r'^non-atomic-exception$', NonAtomicAPIExceptionView.as_view()),
)


Expand Down Expand Up @@ -94,8 +94,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

Expand Down Expand Up @@ -139,12 +139,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())