Improve set_rollback() behaviour

[adamchainz]

trafficstars

Description

Fixes #6921.

Added tests that fail before and pass afterwards.

Remove the check for connection.in_atomic_block to determine if the current request is under a transaction.atomic from ATOMIC_REQUESTS. Instead, duplicate the method that Django itself uses in BaseHandler.

This requires fetching the actual view function from as_view(), as seen by the URL resolver / BaseHandler. Since this requires request, I've also changed the accesses in get_exception_handler_context to be direct attribute accesses rather than getattr(). It seems the getattr defaults not accessible since self.request, self.args, and self.kwargs are always set in dispatch() before handle_exception() can ever be called. This is useful since request is always needed for the new set_rollback logic.

This also fixes a bug with multi-DB compatibility - previously set_rollback would only be called on the default DB when there are multiple DB's.

[adamchainz]

So there are a bunch of test failures, all:

AttributeError: 'NoneType' object has no attribute 'func'

This is all due to use of RequestFactory, which doesn't set resolver_match on its requests as it bypasses URL resolution. In a real world Django app, URL resolution doesn't get bypassed like this.

This could be fixed by moving all those tests to use the test Client, which does set resolver_match, but it's a lot of work.

[auvipy]

I can help with updating the tests, can you share your suggested changes for the tests? I can come up with a PR

[rpkilby]

This could be fixed by moving all those tests to use the test Client, which does set resolver_match, but it's a lot of work.

An off the cuff reaction, but I don't think we'd want to go this route. Ignoring the headache of updating the test suite, wouldn't this imply that users are unable to test their views with the request factory? Users would have to use the test client instead, right?

Would it be sufficient to do a best effort to get the non_atomic_requests flag? e.g., from this:

    view_func = request.resolver_match.func
    non_atomic_requests = getattr(view_func, '_non_atomic_requests', set())

to this (and document the deviation from the BaseHandler):

    try:
        non_atomic_requests = request.resolver_match.func._non_atomic_requests
    except AttributeError:
        non_atomic_requests = set()

[adamchainz]

An off the cuff reaction, but I don't think we'd want to go this route. Ignoring the headache of updating the test suite, wouldn't this imply that users are unable to test their views with the request factory? Users would have to use the test client instead, right?

You're right, it's probably not a good idea.

Would it be sufficient to do a best effort to get the non_atomic_requests flag?

I guess so - I've updated the PR accordingly.

[lovelydinosaur]

I'd like to release 3.11, and I'm not clear enough about this one yet, so going to drop this from the milestone.

If we absolutely need to we can always include it on a point release so long as we make sure to handle the potential signature change on the exception handler in a backwards compat way.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.