-
-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
5e5f559
5c65845
abffc1e
a6cffb0
60a149e
13a4d43
00a29c1
af6b22b
103e596
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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) | ||
|
Comment on lines
66
to
+73
|
||
|
|
||
|
|
||
| def exception_handler(exc, context): | ||
|
|
@@ -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): | ||
|
|
||
There was a problem hiding this comment.
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_blocknot sufficient, since it seems like it'd be the right thing to be doing? What doesconnection.in_atomic_blockreturn when inside anon_atomic_requestdecorated view?Is it possible to isolate the multi-DB fix in this PR from the
_non_atomic_requestschange in the PR, or are they tightly linked?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
connection.in_atomic_blockmeans "is the default DB in a transaction?" not "is any DB in a transaction started byATOMIC_REQUESTS?"This has a number of subtleties.
connection.in_atomic_blockcan returnTruein a view with@non_atomic_requestsif there is an atomic block from another source thanATOMIC_REQUESTS. This is most likely to be from Django'sTestCase, but could also be a custom view decorator or middleware for managing transactions.More information on the specific case I had was:
ATOMIC_REQUESTS = Trueon default DB because they had a number of errors due to not using transactions@non_atomic_requestsbecause they weren't safe for ATOMIC_REQUESTS, and instead manually decorate with@atomicinternallyTestCase, which sets up two atomic blocks around tests.set_rollback()seesconnection.in_atomic_blockisTrue, despite the transaction not coming from ATOMIC_REQUESTS. Tests checking error paths in the views cause attempt to rollback the transaction fromTestCase, whichTestCasealso tries to rollback, which breaks the whole test run due to unbalanced transactions.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.
There was a problem hiding this comment.
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_blockon a per-db basis?There was a problem hiding this comment.
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?...
There was a problem hiding this comment.
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_requestsviews underTestCaseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, walk me through that. Do you mean would be broken for
@non_atomic_requestsviews, or that it would be broken for test cases of@non_atomic_requestsviews?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latter (
TransactionTestCaseshould still work).There was a problem hiding this comment.
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
TransactionTestCaseif required?There was a problem hiding this comment.
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
TestCaseon@non_atomic_requestsviews. I remembered while writing that commit that evenraise Http404counts as an error on DRF ;)There was a problem hiding this comment.
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."