[Django] #28263: TestCase breaks for databases that don't support savepoints
Django
---------------------------------------------+------------------------
Reporter: Lokesh Dokara | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------------+------------------------
{{{TestCase}}}s 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}}}.
--
Ticket URL: <https://code.djangoproject.com/ticket/28263>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Lokesh Dokara | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Could you motive this? Are you writing a database backend for a database
that supports transactions but not savepoints?
--
Ticket URL: <https://code.djangoproject.com/ticket/28263#comment:1>
Django
Reporter: Lokesh Dokara | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Lokesh Dokara):
Replying to [comment:1 Tim Graham]:
Yeah, exactly I am writing a backend for [https://www.cockroachlabs.com/
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/28263#comment:2>
Django
Reporter: Lokesh Dokara | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
I'm not sure what the expected behavior is. Can you propose a patch? Maybe
[https://github.com/django/django/blob/cde31daf8815e05b4b86b857b49fb0e31e1f0a38/django/test/testcases.py#L948-L950
connections_support_transactions()] should also consider
`uses_savepoints`?
--
Ticket URL: <https://code.djangoproject.com/ticket/28263#comment:3>
Django
Reporter: Lokesh Dokara | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/28263#comment:4>
Django
Reporter: Lokesh Dokara | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
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 checkpoints in `TestCase.setUpClass()` and
`tearDownClass()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/28263#comment:5>
Django
Reporter: Lokesh Dokara | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Lokesh Dokara):
Replying to [comment:5 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()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/28263#comment:6>
Django
Reporter: Lokesh Dokara | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Héctor Pablos):
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
[https://www.exasol.com/en/ / 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!
--
Ticket URL: <https://code.djangoproject.com/ticket/28263#comment:7>
Django
Reporter: Lokesh Dokara | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28263#comment:8>
Django
Reporter: Lokesh Dokara | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* Attachment "28263.diff" added.
Django
Reporter: Lokesh Dokara | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Hemant Bhanawat):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28263#comment:9>
Django
Reporter: Lokesh Dokara | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28263#comment:10>
Django
Reporter: Lokesh Dokara | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* has_patch: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28263#comment:11>
Django
Reporter: Lokesh Dokara | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28263#comment:12>
Django
Reporter: Lokesh Dokara | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by 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
[https://github.com/pingcap/tidb/issues/6840 doesn't support savepoint], a
real world backend hitting this problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/28263#comment:13>
Django
Reporter: Lokesh Dokara | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.11
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by Mariusz Felisiak):
Replying to [comment:13 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
[https://github.com/pingcap/tidb/issues/6840 doesn't support savepoint], a
real world backend hitting this problem.
Please feel-free to work on this issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/28263#comment:14>
Django
Reporter: Lokesh Dokara | Owner: Tim Graham
Type: Bug | Status: assigned
Component: Testing framework | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* owner: nobody => Tim Graham
* needs_better_patch: 1 => 0
* status: new => assigned
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/28263#comment:15>
Django
Reporter: Lokesh Dokara | Owner: Tim Graham
Component: Testing framework | Version: 1.11
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by GitHub <noreply@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"bf692b2fdcb5e55fafa5d3d38e286407eeef2ef4" bf692b2f]:
{{{#!CommitTicketReference repository=""
revision="bf692b2fdcb5e55fafa5d3d38e286407eeef2ef4"
Fixed #28263 -- Fixed TestCase setup for databases that don't support
savepoints.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28263#comment:16>