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.

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

Handle ATOMIC_REQUESTS correctly. #2034

Closed
tomchristie opened this issue Nov 6, 2014 · 15 comments · Fixed by #2887, #7739 or bitdotioinc/django-rest-framework#1
Closed

Handle ATOMIC_REQUESTS correctly. #2034

tomchristie opened this issue Nov 6, 2014 · 15 comments · Fixed by #2887, #7739 or bitdotioinc/django-rest-framework#1

Comments

@tomchristie
Copy link
Member

tomchristie commented Nov 6, 2014

We should include some documentation around sensible ways to deal with atomicity in transactions, and also deal properly with ATOMIC_REQUESTS correctly. This needs both documentation, and probably also enhancements for handling ATOMIC_REQUESTS = True, which is complicated by the fact that we use a custom exception handler.

PR #1787 was an attempt at this but doesn't take the right approach, unsure how to resolve currently.

@alvarocavalcanti
Copy link

alvarocavalcanti commented Apr 15, 2015

@tomchristie this question may seem dull but this point isn't clear to me: Does it mean that DRF do not support ATOMIC_REQUESTS = True? Because we're just trying to use it in our project and it does not seem to be working. I even posted a question on StackOverflow.

@xordoquy
Copy link
Collaborator

xordoquy commented Apr 15, 2015

@alvarocavalcanti This is confirmed. I'll try to see if I can get a test case for this.

Edit: Longer answer is that ATOMIC_REQUESTS will fail if you have a custom exception handler in DRF and your exception is a APIException one. Pure Python exception will not break the ATOMIC_REQUESTS.

@xordoquy
Copy link
Collaborator

xordoquy commented Apr 15, 2015

@tomchristie can you be more specific about why #1787 doesn't take the right approach ?

@tomchristie
Copy link
Member Author

tomchristie commented Apr 17, 2015

Well, stupidly I don't recall - so let's put it another way around.
We need to be able to write tests that verify any proposed fix.
Until and unless we've done that we shouldn't accept something. (including #1787)

@xordoquy
Copy link
Collaborator

xordoquy commented Apr 17, 2015

@tomchristie fair enough

@tomchristie
Copy link
Member Author

tomchristie commented Apr 17, 2015

(Or if not tests, then at least a process by which we can verify the proposed fix.)

@xordoquy xordoquy added this to the 3.1.3 Release milestone Jun 3, 2015
@kakulukia
Copy link

kakulukia commented Apr 15, 2018

its seems there is still an error with ATOMIC_REQUESTS, because the transaction rollback is only done on APIExceptions rather than any exception that might happen during processing the request.

Is that really intended by design? Because in Django itself any exception will trigger a rollback.

@tomchristie
Copy link
Member Author

tomchristie commented Apr 15, 2018

Any other handling gets dealt with by the Django stack. We only need to special case APIException because we’re returning responses from the view.

@xordoquy
Copy link
Collaborator

xordoquy commented Apr 15, 2018

Thinking about it, we could call the rollback on handle_exception which calls the exception handler. That would secure the current non obvious behavior.

@kakulukia
Copy link

kakulukia commented Apr 16, 2018

Well, i wanted to report that bug, because Django didnt care about catching my exception as claimed and DRF was only handling APIExceptions. So im going to figure out why. Could someone point out the Django part that should trigger the rollback? That might speed up my investigations.
A general rollback on handle_exception would be nice tho!

@kakulukia
Copy link

kakulukia commented Apr 16, 2018

Oh damn .. forgive me! I was setting ATOMIC_REQUESTS only in the base settings and forgot about my dev settings where its was overwritten with a default False. Setup correctly it works!

@bramski
Copy link

bramski commented Feb 14, 2019

This is still broken. If I use @transaction.atomic on a view and raise APIException my transaction is not rolled back. That is a huge gotcha and unexpected behavior in my opinion.

@xordoquy
Copy link
Collaborator

xordoquy commented Feb 14, 2019

At this point, a failing test case would be required to figure out whether or not it's broken

@tomchristie
Copy link
Member Author

tomchristie commented Feb 14, 2019

There's no indication that there's an issue.

A quick sanity check looks exactly correct: https://github.com/encode/django-rest-framework/blob/master/rest_framework/views.py#L67-103 and we've not had a bunch of users raising this, which I'd strongly expect.

@bramski Perhaps you've some custom exception handling or something, or there's some other integration issue that we could do a better job of documenting?

As @xordoquy say, if you believe there's an issue we'd need to have a simple reproducible example we can work against, or something more compelling than "this is broken" to demonstrate that it needs looking into.

@jmo-qap
Copy link
Contributor

jmo-qap commented Mar 2, 2021

It is possible that @bramski was using @transaction.atomic on the non-default DB. #7739 may help fully resolve #2034.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment