Improve set_rollback() behaviour #6922

adamchainz · 2019-09-10T11:29:22Z

Description

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.

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.

This also fixes a bug with multi-DB compatibility - previously set_rollback would only be called on the default DB when there are multiple DB's.

adamchainz · 2019-09-10T11:31:28Z

So there are a bunch of test failures, all:

AttributeError: 'NoneType' object has no attribute 'func'

This is all due to use of RequestFactory, which doesn't set resolver_match on its requests as it bypasses URL resolution. In a real world Django app, URL resolution doesn't get bypassed like this.

This could be fixed by moving all those tests to use the test Client, which does set resolver_match, but it's a lot of work.

auvipy · 2019-09-23T15:20:52Z

I can help with updating the tests, can you share your suggested changes for the tests? I can come up with a PR

rpkilby · 2019-09-23T21:41:22Z

This could be fixed by moving all those tests to use the test Client, which does set resolver_match, but it's a lot of work.

An off the cuff reaction, but I don't think we'd want to go this route. Ignoring the headache of updating the test suite, wouldn't this imply that users are unable to test their views with the request factory? Users would have to use the test client instead, right?

Would it be sufficient to do a best effort to get the non_atomic_requests flag? e.g., from this:

    view_func = request.resolver_match.func
    non_atomic_requests = getattr(view_func, '_non_atomic_requests', set())

to this (and document the deviation from the BaseHandler):

    try:
        non_atomic_requests = request.resolver_match.func._non_atomic_requests
    except AttributeError:
        non_atomic_requests = set()

adamchainz · 2019-09-24T21:58:08Z

An off the cuff reaction, but I don't think we'd want to go this route. Ignoring the headache of updating the test suite, wouldn't this imply that users are unable to test their views with the request factory? Users would have to use the test client instead, right?

You're right, it's probably not a good idea.

Would it be sufficient to do a best effort to get the non_atomic_requests flag?

I guess so - I've updated the PR accordingly.

rpkilby

Thanks for the update. Added to the 3.11 milestone.

rest_framework/views.py

tests/test_atomic_requests.py

tomchristie · 2019-12-12T14:39:40Z

rest_framework/views.py

+
+    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)


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?

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.

Gotcha. Is there any more graceful way we can do connection.in_atomic_block on a per-db basis?

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)

Yes that should work but it wouldn't fix my bug with testing the @non_atomic_requests views under TestCase

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?

The latter (TransactionTestCase should still work).

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?

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 ;)

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

tomchristie · 2019-12-12T14:44:53Z

I'd like to release 3.11, and I'm not clear enough about this one yet, so going to drop this from the milestone.

If we absolutely need to we can always include it on a point release so long as we make sure to handle the potential signature change on the exception handler in a backwards compat way.

\## 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.

…mic_block

tomchristie · 2019-12-16T13:27:07Z

rest_framework/views.py

+    # 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


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?

stale · 2022-06-19T18:00:43Z

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.

auvipy

OK Adam, can you please come back to it and make it merge able again? And perhaps from where anyone should retake it?

adamchainz force-pushed the fix_issue_6921_atomic_requests branch from 7059fdc to 13b4c47 Compare September 24, 2019 21:58

auvipy approved these changes Sep 25, 2019

View reviewed changes

rpkilby added this to the 3.11 Release milestone Sep 25, 2019

rpkilby approved these changes Sep 25, 2019

View reviewed changes

rpkilby reviewed Sep 25, 2019

View reviewed changes

rest_framework/views.py Show resolved Hide resolved

xordoquy requested changes Sep 25, 2019

View reviewed changes

rest_framework/views.py Show resolved Hide resolved

rpkilby reviewed Sep 25, 2019

View reviewed changes

tests/test_atomic_requests.py Outdated Show resolved Hide resolved

rpkilby approved these changes Dec 11, 2019

View reviewed changes

rpkilby requested a review from xordoquy December 11, 2019 14:48

tomchristie reviewed Dec 12, 2019

View reviewed changes

tomchristie removed this from the 3.11 Release milestone Dec 12, 2019

adamchainz added 2 commits December 13, 2019 18:27

Revert check for non_atomic_requests, instead rely again on db.in_ato…

5c65845

…mic_block

adamchainz force-pushed the fix_issue_6921_atomic_requests branch from 9405aa3 to 5c65845 Compare December 13, 2019 20:53

tomchristie reviewed Dec 16, 2019

View reviewed changes

stale bot added the stale label Jun 19, 2022

auvipy requested changes Nov 24, 2022

View reviewed changes

stale bot removed the stale label Nov 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve set_rollback() behaviour #6922

Improve set_rollback() behaviour #6922

adamchainz commented Sep 10, 2019 •

edited

adamchainz commented Sep 10, 2019

auvipy commented Sep 23, 2019

rpkilby commented Sep 23, 2019

adamchainz commented Sep 24, 2019

rpkilby left a comment

tomchristie Dec 12, 2019

adamchainz Dec 12, 2019

tomchristie Dec 12, 2019

tomchristie Dec 12, 2019

adamchainz Dec 12, 2019

tomchristie Dec 12, 2019

adamchainz Dec 12, 2019

tomchristie Dec 13, 2019

adamchainz Dec 13, 2019

tomchristie Dec 16, 2019

tomchristie commented Dec 12, 2019

tomchristie Dec 16, 2019

stale bot commented Jun 19, 2022

auvipy left a comment •

edited

Improve set_rollback() behaviour #6922

Are you sure you want to change the base?

Improve set_rollback() behaviour #6922

Conversation

adamchainz commented Sep 10, 2019 • edited

Description

adamchainz commented Sep 10, 2019

auvipy commented Sep 23, 2019

rpkilby commented Sep 23, 2019

adamchainz commented Sep 24, 2019

rpkilby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomchristie commented Dec 12, 2019

Choose a reason for hiding this comment

stale bot commented Jun 19, 2022

auvipy left a comment • edited

Choose a reason for hiding this comment

adamchainz commented Sep 10, 2019 •

edited

auvipy left a comment •

edited