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

Fix set_rollback on @transaction.non_atomic_requests. #3016

Merged
merged 3 commits into from Jun 8, 2015

Conversation

hellysmile
Copy link
Contributor

@hellysmile hellysmile commented Jun 8, 2015

set_rollback()

checks only ATOMIC_REQUEST value, but it can be turned off per view by

@transaction.non_atomic_requests

Attempt to rollback without atomic block raises

raise TransactionManagementError("The rollback flag doesn't work outside of an 'atomic' block.")

https://github.com/django/django/blob/06dc6759d85c6c24b599de07cea47387d3dc2cf9/django/db/backends/base/base.py#L361

Really critical, due http404 for every @transaction.non_atomic_requests

Review on Reviewable

@xordoquy
Copy link
Collaborator

xordoquy commented Jun 8, 2015

Thanks for the pull request.
Will try to get it reviewed, merged and released today

@xordoquy
Copy link
Collaborator

xordoquy commented Jun 8, 2015

The provided test is already working against the current master (without your fix being included).
I didn't investigate yet.

@xordoquy xordoquy added the Bug label Jun 8, 2015
@xordoquy xordoquy added this to the 3.1.4 Release milestone Jun 8, 2015
@hellysmile
Copy link
Contributor Author

hellysmile commented Jun 8, 2015

@xordoquy
Hmm, looks really strange

Here is runnig app
https://pull3016.herokuapp.com/

With this code
https://github.com/hellysmile/pull3016/blob/master/pull3016/urls.py#L28

@hellysmile
Copy link
Contributor Author

hellysmile commented Jun 8, 2015

Hey, factory.{HTTP_METHOD} do not triggers Django internal BaseHandler.get_response, which wraps views in transitions via BaseHandler.make_view_atomic.

In addition, django.test.TestCase wraps all database connection in atomic manually, django.test.TransactionTestCase must be used in this case.

@xordoquy can You try now without checking connection.in_atomic_block, it will definitely fails.

It was really hard to figure out this Django test internals...

@xordoquy
Copy link
Collaborator

xordoquy commented Jun 8, 2015

@hellysmile ace !

xordoquy added a commit that referenced this pull request Jun 8, 2015
Fix set_rollback on @transaction.non_atomic_requests.
@xordoquy xordoquy merged commit b06f944 into encode:master Jun 8, 2015
1 check passed
@xordoquy
Copy link
Collaborator

xordoquy commented Jun 8, 2015

Thanks for the PR !

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.

None yet

3 participants