`TransactionTestCase.serialized_rollback` fails to restore objects due to ordering constraints

[Aymeric Augustin]

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 ​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.

[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.

[Matthijs Kooijman]

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.

[Mariusz Felisiak]

Marking as Patch needs improvement, because it's a draft PR.

[Matthijs Kooijman]

The issues with the testcases have been resolved, so the patch is ready for review.

[Mariusz Felisiak]

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In 98f23a8:

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.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 12e6f57:

Refs #26552 -- Added test for reloading the database with self-referential objects.

[GitHub <noreply@…>]

In eeab63e5:

Refs #26552 -- Made reloading the database for tests check only loaded tables constraints.