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

Mark for transaction rollback after APIException occurs. #1787

Closed
wants to merge 1 commit into from

Conversation

tomchristie
Copy link
Member

Superseeds #1204.

Could do with either an integration test, or a nice clear example from someone of currently failing behaviour with steps to replicate.

@tomchristie tomchristie added this to the 2.4 Release milestone Aug 21, 2014
@tomchristie
Copy link
Member Author

Okay this'd need the @transaction.atomic decorator around dispatch. Also tried this approach, which looks nicer...

# initial bits of our dispatch method
try:
     with transaction.atomic():
        # main body of our dispatch method
except Exception as exc:
    response = self.handle_exception(exc)

(And the following in compat...)

if not hasattr(transaction, 'atomic'):
    transaction.atomic = transaction.commit_on_success

However, fails with AssertionError: 4 != 2 : 4 queries executed, 2 expected and similar errors.

@tomchristie
Copy link
Member Author

Actually note that the two additional queries are the savepoint and release.

QUERY = 'SAVEPOINT "s140735288746368_x1"' - PARAMS = ()

...and...

QUERY = 'RELEASE SAVEPOINT "s140735288746368_x1"' - PARAMS = ()

@tomchristie
Copy link
Member Author

I'm actually not convinced this is exactly the right thing to do in any case.

It does look to me like we're probably not respecting ATOMIC_REQUESTS = True as things stand, since we're handling the exceptions, but otherwise forcibly turning on atomic transactions for all APIViews doesn't seem correct. It's not the default for regular django views, and I'm not convinced it should be the default for us.

@tomchristie
Copy link
Member Author

To be more specific, not respecting ATOMIC_REQUESTS = True if an APIException is raised, although it's not even clear if that's problematic or not.

@tomchristie
Copy link
Member Author

Dropping the 2.4 milestone from this - what the desired behavior actually should be is not as clear cut as first assumed.

@Xinkai
Copy link

Xinkai commented Oct 17, 2014

According to https://docs.djangoproject.com/en/dev/releases/1.6/#savepoints-and-assertnumqueries, assertNumQueries needs adjust to work with ATOMIC_REQUESTS.

I monkey-patched django.test.testcases._AssertNumQueriesContext so that it compares num + 2 with captured query count. All tests pass.

So I am thinking maybe we can provide another settings.py inside the tests folder, which enables atomic requests. Also monkey patch the test tools so that assertNumQueries always passes when atom requests is enabled, since the non-transaction-related query count shouldn't change.

@tomchristie
Copy link
Member Author

Closing in favour of issue #2034 as I don't think this PR takes the right approach. Needs more consideration.

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

Successfully merging this pull request may close these issues.

2 participants