Opened 7 years ago

Closed 5 weeks ago

#28263 closed Bug (fixed)

TestCase breaks for databases that don't support savepoints

Reported by: Lokesh Dokara Owned by: Tim Graham
Component: Testing framework Version: 1.11
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

TestCases with more than one test fail with

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

when uses_savepoints = False and supports_transactions = True.

To reproduce this issue set uses_savepoints = False for any of the backends other than MySQL with MyISAM as MyISAM engine doesn't transactions as well. This setting can be set in any of the following files according to the database used.

  • django/db/backends/postgresql/features.py
  • django/db/backends/mysql/features.py
  • django/db/backends/sqlite3/features.py

I feel this case is not handled in the implementation of TestCase.

Attachments (1)

28263.diff (2.9 KB ) - added by Tim Graham 4 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 by Tim Graham, 7 years ago

Could you motive this? Are you writing a database backend for a database that supports transactions but not savepoints?

in reply to:  1 comment:2 by Lokesh Dokara, 7 years ago

Replying to Tim Graham:

Yeah, exactly I am writing a backend for CockroachDB which supports transactions but does not have full support for savepoints. So I am currently unable to properly test this backend because of this issue.

Could you motive this? Are you writing a database backend for a database that supports transactions but not savepoints?

comment:3 by Tim Graham, 7 years ago

I'm not sure what the expected behavior is. Can you propose a patch? Maybe connections_support_transactions() should also consider uses_savepoints?

comment:4 by Tim Graham, 7 years ago

Triage Stage: UnreviewedAccepted

comment:5 by Simon Charette, 7 years ago

Since da9fe5c717c179a9e881a40efd3bfe36a21bf4a6 (#20392) TestCase subclasses execution is wrapped in a transaction per-db on class setup and each method is wrapped in a save point per-db.

In your case I think you'd want to disable the transaction wrapping at the class level so test method get wrapped in a transaction instead of save points. I think you should be able to achieve that by also checking that all connections support save points in TestCase.setUpClass() and tearDownClass().

Last edited 7 years ago by Simon Charette (previous) (diff)

in reply to:  5 comment:6 by Lokesh Dokara, 7 years ago

Replying to Simon Charette:
I think we might also need to modify _should_reload_connections(), _fixture_setup(), _fixture_teardown() to achieve that.

In your case I think you'd want to disable the transaction wrapping at the class level so test method get wrapped in a transaction instead of save points. I think you should be able to achieve that by also checking that all connections support save points in TestCase.setUpClass() and tearDownClass().

comment:7 by Héctor Pablos, 4 years ago

Bringing this up again because I'm facing the same issue Lokesh Dokara was facing in django 2.2.7. In my case I'm developing a database backend for / ExaSol.

I also met the problem while executing some tests, and I managed to overcome it by extending django.test.testcases.TestCase and overwriting the _enter_atomics and _rollback_atomics methods so no transactions are started or rolled back for databases with uses_savepoints = False and supports_transactions = True, but that means I have to be careful not to modify the data, as it won't be rolled back. Using TransactionTestCase, which would not create any transactions, is not an option for me, as I can't truncate other databases the tests depend on.

The underlying issue is not only in the tests, every database backend with uses_savepoints = False and supports_transactions = True will raise exceptions in the following scenario:

- Init parent transaction (django.db.transaction.Atomic.__enter__)
    * Not in an atomic block, so: 
       connection.commit_on_exit = True
       connection.needs_rollback = False
       connection.set_autocommit(False, force_begin_transaction_with_broken_autocommit=True)
       connection.in_atomic_block = True

    - Init child transaction 1 (django.db.transaction.Atomic.__enter__)
        * In an atomic block, so:
           call to connection.savepoint(), as connection.features.uses_savepoints == False, returns None
           connection.savepoint_ids.append(None)

    - Do some database operation that raises an exception and marks the connection as needs_rollback = True

    - Exit child transaction 1 (django.db.transaction.Atomic.__exit__)
           * sid = connection.savepoint_ids.pop(), so
           connection.savepoint_ids == []
           sid == None
           Connection is inside an atomic block and sid is None so connection.needs_rollback is set to True again and nothing else is done.

    - Init child transaction 2 (django.db.transaction.Atomic.__enter__)
        * Same as init child transaction 1

    - Execute a query (django.db.backends.utils.CursorWrapper._execute)
          We get to django.db.backends.utils.CursorWrapper._execute, where self.db.validate_no_broken_transaction() is executed.
          This function finds connection.needs_rollback == True, because it was set when exiting the child transaction 1, and
          it raises the aforementioned TransactionManagementError

I'm afraid I'm not really sure how to fix this, but it's not something just affecting tests. The django.db.transaction.Atomic class doesn't seem to be able to handle the databases without savepoints portrayed here.

The normal flow would be to just do nothing when any child transaction is started or exited in these databases, and leave it up to the topmost transaction to commit or rollback at the end and set autocommit again to True, but of course that would mean that users trying to create subtransactions and roll back only those changes would not be able to do so, and all the changes would be rolled back silently.

Let me know if this sounds like something reasonable and I could try to do a PR myself, I'm just afraid I don't have the necessary whole context to do so.

Cheers!

Last edited 4 years ago by Héctor Pablos (previous) (diff)

comment:8 by Tim Graham, 4 years ago

I'm attaching the patch that I used to fix this issue in a Django fork for CockroachDB. The most recent release, CockroachDB 20.1, adds support for savepoints, so this issue is no longer relevant there.

by Tim Graham, 4 years ago

Attachment: 28263.diff added

comment:9 by Hemant Bhanawat, 3 years ago

Any update on this issue? This looks like more of a Atomic class issue rather than a test issue. I encountered this while developing a backend for Yugabyte which supports transaction but not savepoint.

comment:10 by Tim Graham, 3 years ago

I think my patch would be suitable for inclusion if you want to polish it. If you believe the problem is elsewhere, feel free to propose a patch.

comment:11 by Simon Charette, 3 years ago

Has patch: set

Also pretty sure Tim's patch is correct; it restores the pre-da9fe5c717c179a9e881a40efd3bfe36a21bf4a6 behaviour when savepoints are not available but transactions are which is something #20392 didn't account for since no supported backend fell in this category at the time.

comment:12 by Mariusz Felisiak, 3 years ago

Needs tests: set
Patch needs improvement: set

Tests raise django.db.utils.IntegrityError with attached patch. Moreover I can reproduce the issue with changed feature flags and PostgreSQL backend but it works fine with SQLite backend. I'm puzzled.

comment:13 by Xiang Zhang, 2 years ago

I'd like to re-activate this ticket since this causes a problem to add a backend for TiDB. TiDB is a NewSQL database who supports transactions but doesn't support savepoint, a real world backend hitting this problem.

in reply to:  13 comment:14 by Mariusz Felisiak, 2 years ago

Replying to Xiang Zhang:

I'd like to re-activate this ticket since this causes a problem to add a backend for TiDB. TiDB is a NewSQL database who supports transactions but doesn't support savepoint, a real world backend hitting this problem.

Please feel-free to work on this issue.

comment:15 by Simon Charette, 5 weeks ago

Needs tests: unset
Owner: changed from nobody to Tim Graham
Patch needs improvement: unset
Status: newassigned
Last edited 5 weeks ago by Tim Graham (previous) (diff)

comment:16 by GitHub <noreply@…>, 5 weeks ago

Resolution: fixed
Status: assignedclosed

In bf692b2f:

Fixed #28263 -- Fixed TestCase setup for databases that don't support savepoints.

Note: See TracTickets for help on using tickets.
Back to Top