-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
set_rollback's atomic check is inaccurate #6921
Comments
adamchainz
changed the title
set_rollback() isn't multi-DB aware
set_rollback's atomic check is inaccurate
Sep 10, 2019
adamchainz
added a commit
to adamchainz/django-rest-framework
that referenced
this issue
Sep 10, 2019
\## Description Fixes encode#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.
adamchainz
added a commit
to adamchainz/django-rest-framework
that referenced
this issue
Sep 24, 2019
\## Description Fixes encode#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.
adamchainz
added a commit
to adamchainz/django-rest-framework
that referenced
this issue
Dec 13, 2019
\## Description Fixes encode#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.
I am seeing this on DRF 3.12 When using a |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Checklist
master
branch of Django REST framework.Steps to reproduce
Wrap a view that errors with
@non_atomic_requests
, but use a transaction around it somehow. The easiest way is to test it from a DjangoTestCase
, which uses transactions around each test. Another way would be to have a custom middleware that does a transaction a different way.Expected behavior
rest_framework.set_rollback
should not calltransaction.set_rollback
because the view is declared as non-atomic.Actual behavior
rest_framework.set_rollback
callstransaction.set_rollback
. Its check for "is this view non-atomic" checksconnection.in_atomic_block
which as indicated above can be true for a number of reasons.The text was updated successfully, but these errors were encountered: