Only break circular dependencies at a soft dependency (#91)

* Failing test for #84

* Add clarifying comments / assertions for missing resolution in _add_to_operation_order

As per #84 (comment)

* Ensure that circular dependencies are only broken on soft dependencies

Fixes #84

tests/tests/test_import.py

@@ -1512,6 +1512,87 @@ def test_omitting_references_in_m2m_relations(self):
         # pages, but not the missing and not-to-be-imported page id=31
         self.assertEqual(set(salad_dressing_page.related_pages.all()), set([oil_page, vinegar_page]))
 
+    def test_import_with_soft_dependency_on_grandchild(self):
+        # https://github.com/wagtail/wagtail-transfer/issues/84 -
+        # if there is a dependency loop with multiple hard dependencies and one soft dependency,
+        # the soft dependency should be the one to be broken
+        data = """{
+            "ids_for_import": [
+                ["wagtailcore.page", 10],
+                ["wagtailcore.page", 11],
+                ["wagtailcore.page", 12]
+            ],
+            "mappings": [
+                ["wagtailcore.page", 10, "10101010-0000-0000-0000-000000000000"],
+                ["wagtailcore.page", 11, "11111111-0000-0000-0000-000000000000"],
+                ["wagtailcore.page", 12, "12121212-0000-0000-0000-000000000000"]
+            ],
+            "objects": [
+                {
+                    "model": "tests.pagewithrichtext",
+                    "pk": 10,
+                    "parent_id": 1,
+                    "fields": {
+                        "title": "002 Level 1 page",
+                        "show_in_menus": false,
+                        "live": true,
+                        "slug": "level-1-page",
+                        "body": "<p>link to <a id=\\"12\\" linktype=\\"page\\">level 3</a></p>",
+                        "comments": []
+                    }
+                },
+                {
+                    "model": "tests.pagewithrichtext",
+                    "pk": 11,
+                    "parent_id": 10,
+                    "fields": {
+                        "title": "000 Level 2 page",
+                        "show_in_menus": false,
+                        "live": true,
+                        "slug": "level-2-page",
+                        "body": "<p>level 2</p>",
+                        "comments": []
+                    }
+                },
+                {
+                    "model": "tests.pagewithrichtext",
+                    "pk": 12,
+                    "parent_id": 11,
+                    "fields": {
+                        "title": "001 Level 3 page",
+                        "show_in_menus": false,
+                        "live": true,
+                        "slug": "level-3-page",
+                        "body": "<p>level 3</p>",
+                        "comments": []
+                    }
+                }
+            ]
+        }"""
+
+        importer = ImportPlanner(root_page_source_pk=10, destination_parent_id=2)
+        importer.add_json(data)
+        # importer.run() will build a running order by iterating over the self.operations set.
+        # Since the ordering of that set is non-deterministic, it may arrive at an ordering that
+        # works by chance (i.e. at the point that it recognises the circular dependency, it is
+        # looking at the soft dependency, which happens to be the correct one to break).
+        # To prevent that, we'll hack importer.operations into a list, so that when importer.run()
+        # iterates over it, it gets back a known 'worst case' ordering as defined by the page
+        # titles.
+        importer.operations = list(importer.operations)
+        importer.operations.sort(key=lambda op: op.object_data['fields']['title'])
+
+        importer.run()
+
+        # all pages should be imported
+        self.assertTrue(PageWithRichText.objects.filter(slug="level-1-page").exists())
+        self.assertTrue(PageWithRichText.objects.filter(slug="level-2-page").exists())
+        self.assertTrue(PageWithRichText.objects.filter(slug="level-3-page").exists())
+
+        # link from homepage has to be broken
+        page = PageWithRichText.objects.get(slug="level-1-page")
+        self.assertEqual(page.body, '<p>link to level 3</p>')
+
     @mock.patch('requests.get')
     def test_import_custom_file_field(self, get):
         get.return_value.status_code = 200

wagtail_transfer/operations.py

@@ -32,6 +32,10 @@
 ]
 
 
+class CircularDependencyException(Exception):
+    pass
+
+
 class Objective:
     """
     An objective identifies an individual database object that we want to exist on the destination
@@ -533,21 +537,47 @@ def _add_to_operation_order(self, operation, operation_order, path):
             try:
                 resolution = self.resolutions[(dep_model, dep_source_id)]
             except KeyError:
-                # At this point this should only happen for soft dependencies, as we should have
-                # stripped out unsatisfiable hard dependencies via _check_satisfiable
+                # There is no resolution for this dependency - for example, it's a rich text link
+                # to a page outside of the subtree being imported (and NO_FOLLOW_MODELS tells us
+                # not to recursively import it).
+
+                # If everything is working properly, this should be a case we already encountered
+                # during task / objective solving and logged in failed_creations.
+                assert (dep_model, dep_source_id) in self.failed_creations
+
+                # Also, it should be a soft dependency, since we've eliminated unsatisfiable hard
+                # hard dependencies during _check_satisfiable.
                 assert not dep_is_hard
 
-                # So, given that this is a soft dependency, carry on regardless
+                # Since this is a soft dependency, we can (and must!) leave it unsatisfied.
+                # Abandon this dependency and move on to the next in the list
                 continue
 
             if resolution is None:
                 # dependency is already satisfied with no further action
                 continue
             elif resolution in path:
-                # we have a circular dependency; we have to break it somewhere, so break it here
-                continue
+                # The resolution for this dependency is an operation that's currently under
+                # consideration, so we have a circular dependency. This will be one that we can
+                # resolve by breaking a soft dependency - a circular dependency consisting of
+                # only hard dependencies would have been caught by _check_satisfiable. So, raise
+                # an exception to be propagated back up the chain until we're back to a caller that
+                # can handle it gracefully - namely, a soft dependency that can be left
+                # unsatisfied.
+                raise CircularDependencyException()
             else:
-                self._add_to_operation_order(resolution, operation_order, path + [resolution])
+                try:
+                    # recursively add the operation that we're depending on here
+                    self._add_to_operation_order(resolution, operation_order, path + [resolution])
+                except CircularDependencyException:
+                    if dep_is_hard:
+                        # we can't resolve the circular dependency by breaking the chain here,
+                        # so propagate it to the next level up
+                        raise
+                    else:
+                        # this is a soft dependency, and we can break the chain by leaving this
+                        # unsatisfied. Abandon this dependency and move on to the next in the list
+                        continue
 
         operation_order.append(operation)