Fixed #14226 -- Dependency calculation for complex M2M relations.

`sort_dependencies` incorrectly interpreted 'complex' M2M relations
(with explicit through models) as dependencies for a model. This caused
circular complex M2M relations to be unserializable by dumpdata.

Thanks to aneil for the report and outofculture for initial tests.

django/core/management/commands/dumpdata.py

@@ -189,17 +189,21 @@ def sort_dependencies(app_list):
             else:
                 deps = []
 
-            # Now add a dependency for any FK or M2M relation with
+            # Now add a dependency for any FK relation with a model that
-            # a model that defines a natural key
+            # defines a natural key
             for field in model._meta.fields:
                 if hasattr(field.rel, 'to'):
                     rel_model = field.rel.to
                     if hasattr(rel_model, 'natural_key') and rel_model != model:
                         deps.append(rel_model)
+            # Also add a dependency for any simple M2M relation with a model
+            # that defines a natural key.  M2M relations with explicit through
+            # models don't count as dependencies.
             for field in model._meta.many_to_many:
-                rel_model = field.rel.to
+                if field.rel.through._meta.auto_created:
-                if hasattr(rel_model, 'natural_key') and rel_model != model:
+                    rel_model = field.rel.to
-                    deps.append(rel_model)
+                    if hasattr(rel_model, 'natural_key') and rel_model != model:
+                        deps.append(rel_model)
             model_dependencies.append((model, deps))
 
     model_dependencies.reverse()

tests/fixtures_regress/models.py

@@ -235,3 +235,97 @@ class ExternalDependency(models.Model):
 # Model for regression test of #11101
 class Thingy(models.Model):
     name = models.CharField(max_length=255)
+
+
+@python_2_unicode_compatible
+class BaseNKModel(models.Model):
+    """
+    Base model with a natural_key and a manager with `get_by_natural_key`
+    """
+    data = models.CharField(max_length=20, unique=True)
+    objects = NKManager()
+
+    class Meta:
+        abstract = True
+
+    def __str__(self):
+        return self.data
+
+    def natural_key(self):
+        return (self.data,)
+
+
+class M2MSimpleA(BaseNKModel):
+    b_set = models.ManyToManyField("M2MSimpleB")
+
+
+class M2MSimpleB(BaseNKModel):
+    pass
+
+
+class M2MSimpleCircularA(BaseNKModel):
+    b_set = models.ManyToManyField("M2MSimpleCircularB")
+
+
+class M2MSimpleCircularB(BaseNKModel):
+    a_set = models.ManyToManyField("M2MSimpleCircularA")
+
+
+class M2MComplexA(BaseNKModel):
+    b_set = models.ManyToManyField("M2MComplexB", through="M2MThroughAB")
+
+
+class M2MComplexB(BaseNKModel):
+    pass
+
+
+class M2MThroughAB(BaseNKModel):
+    a = models.ForeignKey(M2MComplexA)
+    b = models.ForeignKey(M2MComplexB)
+
+
+class M2MComplexCircular1A(BaseNKModel):
+    b_set = models.ManyToManyField("M2MComplexCircular1B",
+                                   through="M2MCircular1ThroughAB")
+
+
+class M2MComplexCircular1B(BaseNKModel):
+    c_set = models.ManyToManyField("M2MComplexCircular1C",
+                                   through="M2MCircular1ThroughBC")
+
+
+class M2MComplexCircular1C(BaseNKModel):
+    a_set = models.ManyToManyField("M2MComplexCircular1A",
+                                   through="M2MCircular1ThroughCA")
+
+
+class M2MCircular1ThroughAB(BaseNKModel):
+    a = models.ForeignKey(M2MComplexCircular1A)
+    b = models.ForeignKey(M2MComplexCircular1B)
+
+
+class M2MCircular1ThroughBC(BaseNKModel):
+    b = models.ForeignKey(M2MComplexCircular1B)
+    c = models.ForeignKey(M2MComplexCircular1C)
+
+
+class M2MCircular1ThroughCA(BaseNKModel):
+    c = models.ForeignKey(M2MComplexCircular1C)
+    a = models.ForeignKey(M2MComplexCircular1A)
+
+
+class M2MComplexCircular2A(BaseNKModel):
+    b_set = models.ManyToManyField("M2MComplexCircular2B",
+                                   through="M2MCircular2ThroughAB")
+
+
+class M2MComplexCircular2B(BaseNKModel):
+    def natural_key(self):
+        return (self.data,)
+    # Fake the dependency for a circularity
+    natural_key.dependencies = ["fixtures_regress.M2MComplexCircular2A"]
+
+
+class M2MCircular2ThroughAB(BaseNKModel):
+    a = models.ForeignKey(M2MComplexCircular2A)
+    b = models.ForeignKey(M2MComplexCircular2B)

tests/fixtures_regress/tests.py

@@ -7,6 +7,7 @@ import os
 import re
 import warnings
 
+from django.core import serializers
 from django.core.serializers.base import DeserializationError
 from django.core import management
 from django.core.management.base import CommandError
@@ -23,7 +24,12 @@ from django.utils.six import PY3, StringIO
 
 from .models import (Animal, Stuff, Absolute, Parent, Child, Article, Widget,
     Store, Person, Book, NKChild, RefToNKChild, Circle1, Circle2, Circle3,
-    ExternalDependency, Thingy)
+    ExternalDependency, Thingy,
+    M2MSimpleA, M2MSimpleB, M2MSimpleCircularA, M2MSimpleCircularB,
+    M2MComplexA, M2MComplexB, M2MThroughAB, M2MComplexCircular1A,
+    M2MComplexCircular1B, M2MComplexCircular1C, M2MCircular1ThroughAB,
+    M2MCircular1ThroughBC, M2MCircular1ThroughCA, M2MComplexCircular2A,
+    M2MComplexCircular2B, M2MCircular2ThroughAB)
 
 _cur_dir = os.path.dirname(os.path.abspath(upath(__file__)))
 
@@ -702,6 +708,123 @@ class NaturalKeyFixtureTests(TestCase):
         )
 
 
+class M2MNaturalKeyFixtureTests(TestCase):
+    """Tests for ticket #14426."""
+
+    def test_dependency_sorting_m2m_simple(self):
+        """
+        M2M relations without explicit through models SHOULD count as dependencies
+
+        Regression test for bugs that could be caused by flawed fixes to
+        #14226, namely if M2M checks are removed from sort_dependencies
+        altogether.
+        """
+        sorted_deps = sort_dependencies(
+            [('fixtures_regress', [M2MSimpleA, M2MSimpleB])]
+        )
+        self.assertEqual(sorted_deps, [M2MSimpleB, M2MSimpleA])
+
+    def test_dependency_sorting_m2m_simple_circular(self):
+        """
+        Resolving circular M2M relations without explicit through models should
+        fail loudly
+        """
+        self.assertRaisesMessage(
+            CommandError,
+            "Can't resolve dependencies for fixtures_regress.M2MSimpleCircularA, "
+            "fixtures_regress.M2MSimpleCircularB in serialized app list.",
+            sort_dependencies,
+            [('fixtures_regress', [M2MSimpleCircularA, M2MSimpleCircularB])]
+        )
+
+    def test_dependency_sorting_m2m_complex(self):
+        """
+        M2M relations with explicit through models should NOT count as
+        dependencies.  The through model itself will have dependencies, though.
+        """
+        sorted_deps = sort_dependencies(
+            [('fixtures_regress', [M2MComplexA, M2MComplexB, M2MThroughAB])]
+        )
+        # Order between M2MComplexA and M2MComplexB doesn't matter. The through
+        # model has dependencies to them though, so it should come last.
+        self.assertEqual(sorted_deps[-1], M2MThroughAB)
+
+    def test_dependency_sorting_m2m_complex_circular_1(self):
+        """
+        Circular M2M relations with explicit through models should be serializable
+        """
+        A, B, C, AtoB, BtoC, CtoA = (M2MComplexCircular1A, M2MComplexCircular1B,
+                                     M2MComplexCircular1C, M2MCircular1ThroughAB,
+                                     M2MCircular1ThroughBC, M2MCircular1ThroughCA)
+        try:
+            sorted_deps = sort_dependencies(
+                [('fixtures_regress', [A, B, C, AtoB, BtoC, CtoA])]
+            )
+        except CommandError:
+            self.fail("Serialization dependency solving algorithm isn't "
+                      "capable of handling circular M2M setups with "
+                      "intermediate models.")
+
+        # The dependency sorting should not result in an error, and the
+        # through model should have dependencies to the other models and as
+        # such come last in the list.
+        self.assertEqual(sorted(sorted_deps[:3]), sorted([A, B, C]))
+        self.assertEqual(sorted(sorted_deps[3:]), sorted([AtoB, BtoC, CtoA]))
+
+    def test_dependency_sorting_m2m_complex_circular_2(self):
+        """
+        Circular M2M relations with explicit through models should be serializable
+        This test tests the circularity with explicit natural_key.dependencies
+        """
+        try:
+            sorted_deps = sort_dependencies(
+                [('fixtures_regress', [
+                    M2MComplexCircular2A,
+                    M2MComplexCircular2B,
+                    M2MCircular2ThroughAB]
+                  )
+                ]
+            )
+        except CommandError:
+            self.fail("Serialization dependency solving algorithm isn't "
+                      "capable of handling circular M2M setups with "
+                      "intermediate models plus natural key dependency hints.")
+        self.assertEqual(sorted(sorted_deps[:2]), sorted([M2MComplexCircular2A, M2MComplexCircular2B]))
+        self.assertEqual(sorted_deps[2:], [M2MCircular2ThroughAB])
+
+    def test_dump_and_load_m2m_simple(self):
+        """
+        Test serializing and deserializing back models with simple M2M relations
+        """
+        a = M2MSimpleA.objects.create(data="a")
+        b1 = M2MSimpleB.objects.create(data="b1")
+        b2 = M2MSimpleB.objects.create(data="b2")
+        a.b_set.add(b1)
+        a.b_set.add(b2)
+
+        stdout = StringIO()
+        management.call_command(
+            'dumpdata',
+            'fixtures_regress.M2MSimpleA',
+            'fixtures_regress.M2MSimpleB',
+            use_natural_foreign_keys=True,
+            stdout=stdout
+        )
+
+        for model in [M2MSimpleA, M2MSimpleB]:
+            model.objects.all().delete()
+
+        objects = serializers.deserialize("json", stdout.getvalue())
+        for obj in objects:
+            obj.save()
+
+        new_a = M2MSimpleA.objects.get_by_natural_key("a")
+        self.assertQuerysetEqual(new_a.b_set.all(), [
+            "<M2MSimpleB: b1>",
+            "<M2MSimpleB: b2>"
+        ], ordered=False)
+
+
 class TestTicket11101(TransactionTestCase):
 
     available_apps = [