[Django] #26552: `TransactionTestCase.serialized_rollback` fails to restore objects due to ordering constraints
Django
ordering constraints
---------------------------------------------+------------------------
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.9
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 |
---------------------------------------------+------------------------
I hit this problem in a fairly complex projet and haven't had the time to
write a minimal reproduction case. I think it can be understood just by
inspecting the code so I'm going to describe it while I have it in mind.
----
Setting `serialized_rollback = True` on a `TransactionTestCase` triggers
[https://docs.djangoproject.com/en/1.9/topics/testing/overview/#rollback-
emulation rollback emulation]. In practice, for each database:
- `BaseDatabaseCreation.create_test_db` calls
`connection._test_serialized_contents =
connection.creation.serialize_db_to_string()`
- `TransactionTestCase._fixture_setup` calls
`connection.creation.deserialize_db_from_string(connection._test_serialized_contents)`
(The actual code isn't written that way; it's equivalent but the symmetry
is less visible.)
----
`serialize_db_to_string` orders models with
`serializers.sort_dependencies` and serializes them. The sorting algorithm
only deals with natural keys. It doesn't do anything to order models
referenced by foreign keys before models containing said foreign keys.
That wouldn't be possible in general because circular foreign keys are
allowed.
`deserialize_db_from_string` deserializes and saves models without
wrapping in a transaction. This can result in integrity errors if an
instance containing a foreign key is saved before the instance it
references. I'm suggesting to fix it as follows:
{{{
diff --git a/django/db/backends/base/creation.py
b/django/db/backends/base/creation.py
index bca8376..7bed2be 100644
--- a/django/db/backends/base/creation.py
+++ b/django/db/backends/base/creation.py
@@ -4,7 +4,7 @@ import time
from django.apps import apps
from django.conf import settings
from django.core import serializers
-from django.db import router
+from django.db import router, transaction
from django.utils.six import StringIO
from django.utils.six.moves import input
@@ -128,8 +128,9 @@ class BaseDatabaseCreation(object):
the serialize_db_to_string method.
"""
data = StringIO(data)
- for obj in serializers.deserialize("json", data,
using=self.connection.alias):
- obj.save()
+ with transaction.atomic(using=self.connection.alias):
+ for obj in serializers.deserialize("json", data,
using=self.connection.alias):
+ obj.save()
def _get_database_display_str(self, verbosity, database_name):
"""
}}}
----
Note that `loaddata` doesn't have this problem because it wraps everything
in a transaction:
{{{
def handle(self, *fixture_labels, **options):
# ...
with transaction.atomic(using=self.using):
self.loaddata(fixture_labels)
# ...
}}}
This suggest that the transaction was just forgotten in the implementation
of `deserialize_db_from_string`.
----
It should be possible to write a deterministic test for this bug because
the order in which `serialize_db_to_string` serializes models depends on
the app registry, and the app registry uses `OrderedDict` to store apps
and models in a deterministic order.
--
Ticket URL: <https://code.djangoproject.com/ticket/26552>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
ordering constraints
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* has_patch: 0 => 1
* needs_tests: 0 => 1
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/26552#comment:1>
Django
ordering constraints
Reporter: Aymeric Augustin | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.9
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Fabio Caritas Barrionuevo da Luz):
* cc: Fabio Caritas Barrionuevo da Luz (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/26552#comment:2>
Django
ordering constraints
Reporter: Aymeric Augustin | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.9
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthijs Kooijman):
I've run into a problem related to this one (just reported as #31051), so
I ended up looking into this problem as well. The original report still
seems accurate to me, with the proposed solution valid.
I've been working on a fix and (most of the work), testcase for this
problem. I'll do some more testing and provide a proper PR for this issue
and #31051 soon. The testcase is not ideal yet (testing the testing
framework is tricky), but I'll expand on that in the PR.
Furthermore, I noticed that `loaddata` does not just wrap everything in a
transaction, it also explicitly disables constraint checks inside the
transaction:
{{{
with connection.constraint_checks_disabled():
self.objs_with_deferred_fields = []
for fixture_label in fixture_labels:
self.load_label(fixture_label)
for obj in self.objs_with_deferred_fields:
obj.save_deferred_fields(using=self.using)
# Since we disabled constraint checks, we must manually check for
# any invalid keys that might have been added
table_names = [model._meta.db_table for model in self.models]
try:
connection.check_constraints(table_names=table_names)
except Exception as e:
e.args = ("Problem installing fixtures: %s" % e,)
raise
}}}
I had a closer look at how this works (since I understood that a
transaction already implicitly disables constraint checks) and it turns
out that MySQL/InnoDB is an exception and does *not* defer constraint
checks to the end of the transaction, but instead needs extra handling (so
`constraint_checks_disabled()` is a no-op on most database backends). See
#3615.
--
Ticket URL: <https://code.djangoproject.com/ticket/26552#comment:3>
Django
ordering constraints
Reporter: Aymeric Augustin | Owner: Matthijs
| Kooijman
Type: Bug | Status: assigned
Component: Testing framework | Version: 1.9
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Matthijs Kooijman):
* status: new => assigned
* owner: nobody => Matthijs Kooijman
* cc: Matthijs Kooijman (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/26552#comment:4>
Django
ordering constraints
Reporter: Aymeric Augustin | Owner: Matthijs
| Kooijman
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Matthijs Kooijman):
* has_patch: 1 => 0
* needs_tests: 1 => 0
Comment:
A PR is available at https://github.com/django/django/pull/12166. It is
still marked as draft, since there are still some issues with running the
tests, but review of the code, additions to the discussion in the PR and
maybe ideas on how to fix the remaining testsuite issues are welcome
already.
--
Ticket URL: <https://code.djangoproject.com/ticket/26552#comment:5>
Django
ordering constraints
Reporter: Aymeric Augustin | Owner: Matthijs
| Kooijman
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Matthijs Kooijman):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/26552#comment:6>
Django
ordering constraints
Reporter: Aymeric Augustin | Owner: Matthijs
| Kooijman
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* needs_better_patch: 0 => 1
* version: 1.9 => master
Comment:
Marking as `Patch needs improvement`, because it's a draft PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/26552#comment:7>
Django
ordering constraints
Reporter: Aymeric Augustin | Owner: Matthijs
| Kooijman
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Matthijs Kooijman):
* needs_better_patch: 1 => 0
Comment:
The issues with the testcases have been resolved, so the patch is ready
for review.
--
Ticket URL: <https://code.djangoproject.com/ticket/26552#comment:8>
Django
ordering constraints
Reporter: Aymeric Augustin | Owner: Matthijs
| Kooijman
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* stage: Accepted => Ready for checkin
Comment:
[https://github.com/django/django/pull/12453 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/26552#comment:9>
Django
ordering constraints
Reporter: Aymeric Augustin | Owner: Matthijs
| Kooijman
Component: Testing framework | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"98f23a8af0be7e87535426c5c83058e2682bfdf8" 98f23a8]:
{{{
#!CommitTicketReference repository=""
revision="98f23a8af0be7e87535426c5c83058e2682bfdf8"
Fixed #26552 -- Deferred constraint checks when reloading the database
with data for tests.
deserialize_db_from_string() loads the full serialized database
contents, which might contain forward references and cycles. That
caused IntegrityError because constraints were checked immediately.
Now, it loads data in a transaction with constraint checks deferred
until the end of the transaction.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26552#comment:10>
Django
ordering constraints
Reporter: Aymeric Augustin | Owner: Matthijs
| Kooijman
Type: Bug | Status: closed
Component: Testing framework | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"12e6f573adbf825c74042f15b70600341220674b" 12e6f57]:
{{{
#!CommitTicketReference repository=""
revision="12e6f573adbf825c74042f15b70600341220674b"
Refs #26552 -- Added test for reloading the database with self-referential
objects.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26552#comment:11>
Django
ordering constraints
Reporter: Aymeric Augustin | Owner: Matthijs
| Kooijman
Type: Bug | Status: closed
Component: Testing framework | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by GitHub <noreply@…>):
In [changeset:"eeab63e57e797c41fa42ffe804927c5f77a0d739" eeab63e5]:
{{{
#!CommitTicketReference repository=""
revision="eeab63e57e797c41fa42ffe804927c5f77a0d739"
Refs #26552 -- Made reloading the database for tests check only loaded
tables constraints.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26552#comment:12>