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

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

Merged
merged 5 commits into from Jun 2, 2015

Conversation

ticosax
Copy link
Contributor

@ticosax ticosax commented Apr 29, 2015

based on #1787
fixes #2034

I open the pull request for discussion.



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

@tomchristie tomchristie Apr 29, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ticosax ticosax Apr 29, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point

@tomchristie
Copy link
Member

tomchristie commented Apr 29, 2015

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.

return Response(data, status=status.HTTP_404_NOT_FOUND)

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

set_rollback()
Copy link
Member

@tomchristie tomchristie Apr 29, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ticosax ticosax Apr 29, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

ticosax commented Apr 29, 2015

@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
Copy link
Contributor Author

ticosax commented Apr 29, 2015

@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 xordoquy added the Bug label Jun 1, 2015
@xordoquy xordoquy added this to the 3.1.3 Release milestone Jun 1, 2015
@xordoquy
Copy link
Collaborator

xordoquy commented Jun 1, 2015

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

@ticosax ticosax force-pushed the doom-transaction-on-error branch from 92d6a39 to 34dc98e Compare Jun 2, 2015
@ticosax
Copy link
Contributor Author

ticosax commented Jun 2, 2015

Conflict has been resolved.

@ticosax
Copy link
Contributor Author

ticosax commented Jun 2, 2015

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

@xordoquy
Copy link
Collaborator

xordoquy commented Jun 2, 2015

No promises but working toward it

@ticosax
Copy link
Contributor Author

ticosax commented Jun 2, 2015

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 ?

@tomchristie 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
Copy link
Collaborator

xordoquy commented Jun 2, 2015

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.

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

[testenv]
commands = ./runtests.py --fast
commands = ./runtests.py --fast {posargs}
Copy link
Collaborator

@xordoquy xordoquy Jun 2, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@tomchristie tomchristie Jun 2, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ticosax ticosax Jun 2, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Collaborator

@xordoquy xordoquy Jun 2, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@xordoquy
Copy link
Collaborator

xordoquy commented Jun 2, 2015

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

xordoquy added a commit that referenced this issue Jun 2, 2015
Rollback the transaction on error if ATOMIC_REQUESTS is set.
@xordoquy xordoquy merged commit 1957679 into encode:master Jun 2, 2015
1 check passed
@xordoquy
Copy link
Collaborator

xordoquy commented Jun 2, 2015

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
Copy link
Contributor Author

ticosax commented Jun 2, 2015

champagne !

@ticosax ticosax deleted the doom-transaction-on-error branch Jun 2, 2015
@hellysmile
Copy link
Contributor

hellysmile commented Jun 8, 2015

3.1.3 breaks @transaction.non_atomic_requests at all
#3016

@xordoquy
Copy link
Collaborator

xordoquy commented Jun 8, 2015

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

@marco-silva0000
Copy link

marco-silva0000 commented Sep 20, 2016

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

@xordoquy
Copy link
Collaborator

xordoquy commented Sep 20, 2016

@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
Copy link
Member

tomchristie commented Sep 20, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants