Rollback the transaction on error if ATOMIC_REQUESTS is set. #2887

ticosax · 2015-04-29T13:14:08Z

based on #1787
fixes #2034

I open the pull request for discussion.

tomchristie · 2015-04-29T13:47:51Z

tests/test_atomic_requests.py

+
+
+class BasicView(APIView):
+    def get(self, request, *args, **kwargs):


It might make the tests slightly more obvious if we use post for this state changing operation.

tomchristie · 2015-04-29T13:51:06Z

This looks really nice! Made a few inline comments as a starting point.
It'd be interesting to know which of the three tests fails in the existing codebase.

tomchristie · 2015-04-29T13:53:35Z

rest_framework/views.py

        return Response(data, status=status.HTTP_404_NOT_FOUND)

    elif isinstance(exc, PermissionDenied):
        msg = _('Permission denied.')
        data = {'detail': six.text_type(msg)}
+
+        set_rollback()


Wondering if it'd make more sense to instead have this behavior once, immediately after the point at which the exception handler is called by the view?

That'd ensure that custom exception handlers don't need to deal with rollbacks explicitly.

Make sense, but since exception_handler is hook-able through settings.EXCEPTION_HANDLER, it allows end-users to control transactional behavior.
Just an idea.

ticosax · 2015-04-29T14:31:09Z

@tomchristie thank you for your feedback.

I bring the changes you suggested.

I would like to squash the commits, so let me know once you think it will be good to go.
And I'll do it before your merge.

ticosax · 2015-04-29T14:33:08Z

@tomchristie To reply your question,
only test_api_exception_rollback_transaction is failing with existing codebase.

Which is, I guess, what we want to achieve.

xordoquy · 2015-06-01T22:29:37Z

@ticosax I'm sorry going to push even more work on you. Would it be possible to merge the current master ?

if `ATOMIC_REQUESTS` is enabled.

ticosax · 2015-06-02T07:20:38Z

Conflict has been resolved.

ticosax · 2015-06-02T07:21:17Z

@xordoquy You plan to merge it for 3.1.3 ?
That would be awesome.

xordoquy · 2015-06-02T07:29:45Z

No promises but working toward it

ticosax · 2015-06-02T07:48:13Z

For what it worth I'm using this patch on my production environment with my own exception_handler with django1.8 and python3.4 since few weeks already.
What would be great, is receiving feedback from users using django versions that doesn't support atomic().
But I'm not sure the audience is big enough. django1.4 or 1.5 are quite old already.
Abandoning support of django1.4 and 1.5 will make this pull request much simpler.
So maybe it worth pushing it back until we drop supports of old django versions ?

xordoquy · 2015-06-02T08:55:39Z

Actually I'm fine with not supporting this for 1.4 or 1.5.
We haven't had issues about this so far so unless someone can invest some time into fixing this, it'll remain as it.

xordoquy · 2015-06-02T08:59:11Z

tox.ini

@@ -6,7 +6,7 @@ envlist =
       {py27,py32,py33,py34}-django{17,18,master}

 [testenv]
-commands = ./runtests.py --fast
+commands = ./runtests.py --fast {posargs}


I wonder what this is for - won't block me for merging.

Allows positional arguments to be passed from running tox <...> on the command line. It's a valid change. Not properly part of this PR, but fine all the same. 😄

@xordoquy
Possible use case: stop tests execution after the first error encountered for a specific tox environment.

$ tox -e py34-django18 -- -x

here -x is an argument intended for py.test

Sure i was just wondering how this related to the pull request
Don't get me wrong i'm fine with it

xordoquy · 2015-06-02T09:33:08Z

Looks good to me as well.
A HUGE thank for this.

Rollback the transaction on error if ATOMIC_REQUESTS is set.

xordoquy · 2015-06-02T09:36:02Z

only test_api_exception_rollback_transaction is failing with existing codebase.
Which is, I guess, what we want to achieve.
Confirmed on both point - I just double checked the test was failing against master.

ticosax · 2015-06-02T09:47:19Z

champagne !

hellysmile · 2015-06-08T04:53:25Z

3.1.3 breaks @transaction.non_atomic_requests at all
#3016

xordoquy · 2015-06-08T21:34:59Z

I just realized that this will only rollback the main database.

marco-silva0000 · 2016-09-20T14:28:42Z

well.. I'm a fool... ty

xordoquy · 2016-09-20T15:31:35Z

@Bakirelived you're just human, it happens to all of us. Though you are ahead of most because you took the time to go further and reproduce the issue.
@ticosax thanks for spotting, I'm not sure I would have noticed.

tomchristie · 2016-09-20T17:26:49Z

Good work all, thanks for taking this through to a resolution.

tomchristie reviewed Apr 29, 2015
View reviewed changes

xordoquy added the Bug label Jun 1, 2015

xordoquy added this to the 3.1.3 Release milestone Jun 1, 2015

ticosax added 5 commits June 2, 2015 09:19

allow to pass arbitrary arguments to py.test

27fd485

Tell default error handler to doom the transaction on error

c2d2417

if `ATOMIC_REQUESTS` is enabled.

Use post instead of get for sanity of use-case.

d1371cc

more assertions make the test more readable

8ad3820

improve wording

34dc98e

ticosax force-pushed the doom-transaction-on-error branch from 92d6a39 to 34dc98e Compare June 2, 2015 07:20

tomchristie changed the title ~~Doom the transaction on error if ATOMIC_REQUESTS is set.~~ Rollback the transaction on error if ATOMIC_REQUESTS is set. Jun 2, 2015

xordoquy reviewed Jun 2, 2015
View reviewed changes

xordoquy added a commit that referenced this pull request Jun 2, 2015

Merge pull request #2887 from ticosax/doom-transaction-on-error

1957679

Rollback the transaction on error if ATOMIC_REQUESTS is set.

xordoquy merged commit 1957679 into encode:master Jun 2, 2015

ticosax deleted the doom-transaction-on-error branch June 2, 2015 09:47

meshy mentioned this pull request Oct 11, 2016

DB transaction rollback on 400, 403 and 404 #4560

Closed

6 tasks

pyup-bot mentioned this pull request Oct 22, 2016

Pin djangorestframework to latest version 3.5.1 wooyek/django-website-pro#16

Merged

pyup-bot mentioned this pull request Oct 30, 2016

Pin djangorestframework to latest version 3.5.1 Cyberbyte-Software/Sensor-Portal#49

Merged

pyup-bot mentioned this pull request Feb 26, 2017

Pin djangorestframework to latest version 3.5.4 maidstone-hackspace/maidstone-hackspace-website#39

Merged

This was referenced Mar 9, 2017

Pin djangorestframework to latest version 3.6.0 getpatchwork/patchwork#87

Closed

Pin djangorestframework to latest version 3.6.1 getpatchwork/patchwork#88

Closed

Pin djangorestframework to latest version 3.6.2 getpatchwork/patchwork#89

Closed

pyup-bot mentioned this pull request Jun 7, 2017

Pin djangorestframework to latest version 3.6.3 srtab/alexandriadocs#39

Closed

pyup-bot mentioned this pull request Sep 19, 2017

Pin djangorestframework to latest version 3.6.4 founders4schools/django-donations#21

Merged

This was referenced Oct 6, 2017

Pin djangorestframework to latest version 3.7.0 founders4schools/django-donations#29

Closed

Pin djangorestframework to latest version 3.7.0 getpatchwork/patchwork#125

Closed

Pin djangorestframework to latest version 3.7.0 javipalanca/ojoalplato#65

Closed

This was referenced Nov 14, 2017

Pin djangorestframework to latest version 3.7.3 adfinis/timed-backend#152

Closed

Pin djangorestframework to latest version 3.7.3 adfinis/timed-backend#168

Closed

This was referenced Dec 10, 2017

Pin djangorestframework to latest version 3.7.3 javipalanca/ojoalplato#141

Closed

Pin djangorestframework to latest version 3.7.3 javipalanca/ojoalplato#195

Closed

This was referenced Mar 2, 2021

support multi db atomic_requests #7739

Merged

support multi db atomic_requests databricks/django-rest-framework#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rollback the transaction on error if ATOMIC_REQUESTS is set. #2887

Rollback the transaction on error if ATOMIC_REQUESTS is set. #2887

ticosax commented Apr 29, 2015

tomchristie Apr 29, 2015

ticosax Apr 29, 2015

tomchristie commented Apr 29, 2015

tomchristie Apr 29, 2015

ticosax Apr 29, 2015

ticosax commented Apr 29, 2015

ticosax commented Apr 29, 2015

xordoquy commented Jun 1, 2015

ticosax commented Jun 2, 2015

ticosax commented Jun 2, 2015

xordoquy commented Jun 2, 2015

ticosax commented Jun 2, 2015

xordoquy commented Jun 2, 2015

xordoquy Jun 2, 2015

tomchristie Jun 2, 2015

ticosax Jun 2, 2015

xordoquy Jun 2, 2015

xordoquy commented Jun 2, 2015

xordoquy commented Jun 2, 2015

ticosax commented Jun 2, 2015

hellysmile commented Jun 8, 2015

xordoquy commented Jun 8, 2015

marco-silva0000 commented Sep 20, 2016

xordoquy commented Sep 20, 2016

tomchristie commented Sep 20, 2016



		class BasicView(APIView):
		def get(self, request, args, *kwargs):

Rollback the transaction on error if ATOMIC_REQUESTS is set. #2887

Rollback the transaction on error if ATOMIC_REQUESTS is set. #2887

Conversation

ticosax commented Apr 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomchristie commented Apr 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ticosax commented Apr 29, 2015

ticosax commented Apr 29, 2015

xordoquy commented Jun 1, 2015

ticosax commented Jun 2, 2015

ticosax commented Jun 2, 2015

xordoquy commented Jun 2, 2015

ticosax commented Jun 2, 2015

xordoquy commented Jun 2, 2015

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

xordoquy commented Jun 2, 2015

xordoquy commented Jun 2, 2015

ticosax commented Jun 2, 2015

hellysmile commented Jun 8, 2015

xordoquy commented Jun 8, 2015

marco-silva0000 commented Sep 20, 2016

xordoquy commented Sep 20, 2016

tomchristie commented Sep 20, 2016