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

Add atomic transaction around social_auth creation. #770

Closed

Conversation

doctoryes
Copy link

Since Django 1.6, a transaction.atomic decorator has been available. There's a pattern that's supported in python_social_auth that performs a Django ORM operation and then handles a possible IntegrityError raised by the operation - see here:

https://github.com/omab/python-social-auth/blob/master/social/pipeline/social_auth.py#L40

This pattern is even documented in the Django docs:

https://docs.djangoproject.com/en/1.6/topics/db/transactions/#django.db.transaction.atomic

However, if an IntegrityError is raised without being in a transaction.atomic block (via decorator or context manager), the transaction itself is left in a "broken" state - and any subsequent transaction operations will fail with the following exception:

An error occurred in the current transaction. You can't execute queries until the end of the 'atomic' block.

This condition is not detected in the current unittest suite because the IntegrityError scenario is tested by using a simple raise with no actual DB transaction:

raise IntegrityError()

This PR attempts to fix the broken transaction scenario. I've tested it with our https://github.com/edx/edx-platform test suite and it fixes the broken transaction. As a workaround for our Django 1.8 upgrade, I've monkey-patched edx-platform to fix the issue - that commit can be seen here:

edx/edx-platform@699ae37

The fix in this PR may not be the preferred one - but it does at least fix the issue. I'm happy to provide more information if needed.

@doctoryes
Copy link
Author

@omab FYI - for your review - thanks.

@carsongee
Copy link
Contributor

This is just what I needed, thanks @doctoryes

@doctoryes
Copy link
Author

@carsongee Pleased that it helped you!

max-arnold added a commit to innoteq/python-social-auth that referenced this pull request Jul 14, 2016
@omab omab force-pushed the master branch 2 times, most recently from 1078c07 to de9f179 Compare December 3, 2016 14:23
@omab
Copy link
Owner

omab commented Dec 4, 2016

Closing since this is already in the codebase https://github.com/python-social-auth/social-app-django/blob/master/social_django/storage.py#L112.

Thanks!

@omab omab closed this Dec 4, 2016
opensource521 pushed a commit to opensource521/pythom.auth that referenced this pull request Apr 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants