Circularly dependent fixtures fail due to transaction committing on each object save

[Michael Angeletti]

When using manage.py loaddata to load a fixture that contains 2 objects that refer to each other circularly via foreign keys you'll encounter an integrity error. This is caused by the use of transaction.commit_on_success_unless_managed in the loaddata management command combined with the use of it in Model.save_base, which after each save leads to a transaction commit.

When I modified the loaddata management command to use transaction.atomic in lieu of transaction.commit_on_success_unless_managed the issue no longer occurred. I realize that isn't the solution (for backwards compatibility reasons), but it might help in debugging. Also, see the printed stack frames and debugging values in ​this post (full thread ​here).

This affects at least Django 1.6a1. A similar exception was encountered with Django 1.5.1, though something was subtly different about the issue in Django 1.5.1 (wasn't provided a stack trace to verify it was the same issue).

[anonymous]

I have the same problem in Django 1.6a1, but fixtures were loading without any problem in 1.5.1 and before. Something commits transactions for every object inserted, instead of committing once for all.

[anonymous]

And, as stated, transaction.atomic fixes the loading of fixtures.

[Michael Angeletti]

Replying to anonymous:

I have the same problem in Django 1.6a1, but fixtures were loading without any problem in 1.5.1 and before. Something commits transactions for every object inserted, instead of committing once for all.

I actually ran into the problem with 1.5.1 as well, but I didn't take the time to figure out whether the cause was exactly the same (the error was from a different field, which threw me off).

[Aymeric Augustin]

This ticket is a follow-up to this mailing list discussion: ​https://groups.google.com/forum/#!topic/django-developers/2_xR_PKeubs

First, I tried reproducing the issue with the models in the first post of the thread, but they don't even validate:

fixtures_regress.collection: Accessor for field 'main_thing' clashes with field 'Thing.collection'. Add a related_name argument to the definition for 'main_thing'.
fixtures_regress.collection: Reverse query name for field 'main_thing' clashes with field 'Thing.collection'. Add a related_name argument to the definition for 'main_thing'.

Then, I tried with the models and fixture provided in the third post of the thread, and I cannot reproduce the issue. I'm attaching the diff corresponding to that test.

If someone could provide a *reproducible* way to trigger the problem, in the form of a patch adding a failing test case to Django's test suite, it would really help.

[Aymeric Augustin]

While I failed to reproduce the issue, I can see how d04964e70d5c6277e79874ba8950e23c224b323b would cause it.

If you're encountering the problem, can you apply this patch to Django and tell me if it helps:

diff --git a/django/core/management/commands/loaddata.py b/django/core/management/commands/loaddata.py
index 6856e85..f3f68f4 100644
--- a/django/core/management/commands/loaddata.py
+++ b/django/core/management/commands/loaddata.py
@@ -51,7 +51,7 @@ class Command(BaseCommand):
 
         self.verbosity = int(options.get('verbosity'))
 
-        with transaction.commit_on_success_unless_managed(using=self.using):
+        with transaction.atomic(using=self.using):
             self.loaddata(fixture_labels)
 
         # Close the DB connection -- unless we're still in a transaction. This

Strictly speaking, it's backwards-incompatible for users who have pre_save or post_save signals that invoke transaction.autocommit/commit_on_success/commit_manually, because the deprecated transaction management APIs cannot be invoked inside transaction.atomic.

Now, I'm not sure it's the right level to fix this bug. It's possible that commit_on_success_unless_managed itself is the source of the problem.

[Aymeric Augustin]

After reviewing the implementation of the new transaction management, I think this issue is specific to loaddata, and that it should use transaction.atomic as shown above.

commit_on_success was used to begin a transaction, and hopefully nothing else would commit it, but that wasn't guaranteed at all. That was a bug, and it was revealed by my changes.

(I'm setting the "has patch" flag, that's referring to the diff shown in the previous comment.)

[Aymeric Augustin]

After re-reading the original thread I finally found out the cause. commit_on_success_unless_managed didn't ensure that the database connection was established before testing the autocommit status. When running loaddata, the connection isn't opened yet, and commit_on_success_unless_managed mistakenly believes it's already in a transaction.

Scratch everything I've written above.

I'm going to introduce a getter to test the autocommit status rather than access the autocommit flag directly. For consistency I'll do the same for the rollback flag.

This is hard to test, because the problem only happens when there isn't a connection to the database...

[Aymeric Augustin <aymeric.augustin@…>]

In dd9c6bc359a799fcbed647055b596239956a472a:

Introduced getters for connection.autocommit and .needs_rollback.

They ensure that the attributes aren't accessed in conditions where they
don't contain a valid value.

Fixed #20666.

[Aymeric Augustin <aymeric.augustin@…>]

In 02976a46c926880e78eb64424a9c8aa28d8be48a:

[1.6.x] Introduced getters for connection.autocommit and .needs_rollback.

They ensure that the attributes aren't accessed in conditions where they
don't contain a valid value.

Fixed #20666.

Backport of dd9c6bc359a799fcbed647055b596239956a472a from master.