Adding transaction rollback to exception_handler

[jespino]

This PR is to show you the idea, there are other ways to implement it (like, for example, another exception class named RollbackAPIException), anyway I find the interference of APIException capture with the rollback of django transaction middleware on exceptions, a little bit dangerous.

If you find it useful, I can implement with the RollbackAPIException, which is more backward compatible.

[jespino]

Another solution is move it to a middleware (The exception handling, and leave the rollback to the transaction middelware), that's probably more correct on django arquitechture, but harder to modify and configure by the DRF user.

And of course the final version will come with tests and documentation :)

[jespino]

Other thing, the use of transaction api directly have a deprecation warning in django 1.6, this give an extra point to use directly the middleware for easy compatibility between django versions from 1.3 to 1.6 (the supported versions by DRF)

[tomchristie]

Not obvious to me what the implications of this are.
AFAICT process_response should handle this correctly anyway?
https://github.com/django/django/blob/master/django/middleware/transaction.py
I've also no idea what effect this has if TransactionMiddleware isn't installed (which it won't be, by default)

[jespino]

This is a PR to show the idea, but not to merge, I will work more on it if you find it useful.

The transaction middleware only rollback on commit fails or on exception handling, because DRF handle the API Exceptions, thats not rollback the transaction. process_response will commit the changes on any API Exception, this behavior is at least, extrange.

I need to know if you find acceptable to move to a Middleware the API Exception handling, or if you prefer a solution more like the solution exposed in the PR.

Of course i will test it with and without TransactionMiddleware, and with normal and API exceptions.

[tomchristie]

Something like this looks okay to me.

# We've suppressed the exception but still need to rollback any transaction.
if hasattr(transaction, 'set_rollback'):
    # If running in >=1.6 then mark a rollback as required and let Django deal with it.
    transaction.set_rollback(True)  # Django 1.6+
elif transaction.is_managed():
    # If running <=1.5 and TransactionMiddleware is installed then force it's rollback behavior.
    TransactionMiddleware().process_exception(request, None)

Rather than include it in the exception_handler function, we might want to include it here:

https://github.com/jespino/django-rest-framework/blob/17222fd83b6c4151c01a56483bc45e5fef9bd40e/rest_framework/views.py#L380

So that we ensure it only runs if we've suppressed an exception. Keeps user code simpler, as users shouldn't need to worry about if it's something they need to be concerned about in their exception handler.

[jespino]

Sounds good :)

I will do the changes and add some unit tests.

[tomchristie]

Looks like the tests are failing on this as it currently stands.

[jespino]

Yes,the test are failing. I pushed changes but they aren`t ready. I will comment here when the PR is ready.

[tomchristie]

Great, thanks @jespino.

[adamJLev]

+1.
Pretty dangerous that by default the DRF views don't have auto transaction rollback, I basically wrote something similar to the patch for my own codebase

[tomchristie]

@adamJLev if anyone wants to take up getting the patch to pass the tests that'd be appreciated.

[xordoquy]

Shouldn't there be decorators to the views so that they are marked for transactions ?

[dustinfarris]

@xordoquy I think something like this should be included: https://gist.github.com/adamJLev/7e9499ba7e436535fd94 (found on Google, don't know if it works)

but it seems like it might, given this documentation from Django: https://docs.djangoproject.com/en/dev/topics/class-based-views/intro/#decorating-the-class

and if it doesn't, something that does is probably not much different.

[adamJLev]

@dustinfarris @xordoquy Works well, I wrote that and we have been running it in production for months now 👍

[xordoquy]

@tomchristie do we agree DRF should provide the decorator to mark the views as atomic ?

[tomchristie]

I don't think we should provide it as a decorator, but it looks like we probably should provide it.

Perhaps configured by a REST_FRAMEWORK setting?

I'm still slightly confused as to why this isn't considered an issue with non-API views, or say, other packages that provide generic view classes or similar? I'm probably missing something obvious there having not looked at this ticket for a bit, but it'd be tremendous if someone could walk through it.

[xordoquy]

This is probably because exception have a different workflow.
Correct me if I'm wrong but isn't handle_exception called before the dispatch is returned ?
transaction.atomic will commit if the function returns normally and will rollback on uncaught exceptions.
As handle_exception hide the exception I assume the @transaction.atomic() thinks it went fine and doesn't rollbacks.

[tomchristie]

@xordoquy sounds about right, if anyone has suggestions on good wordings / documentation explaining the behaviour briefly and clearly, that'd be helpful.

We might want to consider this for the 2.4 release, right? See #1770.

That's probably already good to get merged into master, but doesn't mean we can't then leave the release to PyPI hanging for a few more days while this is also resolved & some more folks get to test.

[tomchristie]

Milestone set to 2.4, not as a definite, but for consideration prior to release.

[tomchristie]

We ought to put branching code like this in the compat module.

eg introduce a set_rollback(transaction) function there that performs this behavior.

[tomchristie]

• Needs updating to master now that we've merged 2.4.
• Branching behavior needs to move into compat.
• Could do with an integration test.
• Could do with a nice clear example from someone of currently failing behavior (don't mind under what version configuration, but be great to see failure to do this causing a clear and obvious error.)

Either one or both of the last two would be okay - doesn't necessarily need to be both.

[tomchristie]

Note the updates against #1787 - If you're following this thread, you probably want to be on that instead now.