From e43fae86f1e70ce517c518210171f09d9984286b Mon Sep 17 00:00:00 2001 From: emilecaron Date: Thu, 25 Jun 2015 15:37:15 +0000 Subject: [PATCH 1/9] reproduce RuntimeError --- tests/queryset/queryset.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/queryset/queryset.py b/tests/queryset/queryset.py index 4f00e1c6..1e279744 100644 --- a/tests/queryset/queryset.py +++ b/tests/queryset/queryset.py @@ -1413,6 +1413,20 @@ class QuerySetTest(unittest.TestCase): self.Person.objects(name='Test User').delete() self.assertEqual(1, BlogPost.objects.count()) + def test_reverse_delete_rule_cascade_cycle(self): + """Ensure reference cascading doesn't loop if reference graph isn't + a tree + """ + class Category(Document): + reference = ReferenceField('self', reverse_delete_rule=CASCADE) + + base = Category().save() + other = Category(reference=base).save() + base.reference = other + base.save() + + self.assertEqual(2, base.delete()) + def test_reverse_delete_rule_cascade_self_referencing(self): """Ensure self-referencing CASCADE deletes do not result in infinite loop From 1e3d2df9e7430593b73d4515553612cd737f6c8d Mon Sep 17 00:00:00 2001 From: emilecaron Date: Thu, 25 Jun 2015 15:40:12 +0000 Subject: [PATCH 2/9] fix illogicality --- mongoengine/queryset/base.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/mongoengine/queryset/base.py b/mongoengine/queryset/base.py index 50b2ee19..21d20dae 100644 --- a/mongoengine/queryset/base.py +++ b/mongoengine/queryset/base.py @@ -404,8 +404,7 @@ class BaseQuerySet(object): if rule == CASCADE: ref_q = document_cls.objects(**{field_name + '__in': self}) ref_q_count = ref_q.count() - if (doc != document_cls and ref_q_count > 0 or - (doc == document_cls and ref_q_count > 0)): + if ref_q_count > 0: ref_q.delete(write_concern=write_concern) elif rule == NULLIFY: document_cls.objects(**{field_name + '__in': self}).update( From 02f61c323df31e6966e93ebd18c64ad9da7650ae Mon Sep 17 00:00:00 2001 From: emilecaron Date: Thu, 25 Jun 2015 18:26:52 +0000 Subject: [PATCH 3/9] update test --- tests/queryset/queryset.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/queryset/queryset.py b/tests/queryset/queryset.py index 1e279744..050b2d03 100644 --- a/tests/queryset/queryset.py +++ b/tests/queryset/queryset.py @@ -1417,15 +1417,18 @@ class QuerySetTest(unittest.TestCase): """Ensure reference cascading doesn't loop if reference graph isn't a tree """ - class Category(Document): + class Dummy(Document): reference = ReferenceField('self', reverse_delete_rule=CASCADE) - base = Category().save() - other = Category(reference=base).save() + base = Dummy().save() + other = Dummy(reference=base).save() base.reference = other base.save() - self.assertEqual(2, base.delete()) + base.delete() + + self.assertRaises(DoesNotExist, base.reload) + self.assertRaises(DoesNotExist, other.reload) def test_reverse_delete_rule_cascade_self_referencing(self): """Ensure self-referencing CASCADE deletes do not result in infinite From 646baddce45512e01367346a07195c6a30a613b3 Mon Sep 17 00:00:00 2001 From: emilecaron Date: Thu, 25 Jun 2015 18:27:22 +0000 Subject: [PATCH 4/9] fix cascade delete cycle issuue --- mongoengine/queryset/base.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/mongoengine/queryset/base.py b/mongoengine/queryset/base.py index 21d20dae..df628c35 100644 --- a/mongoengine/queryset/base.py +++ b/mongoengine/queryset/base.py @@ -346,7 +346,7 @@ class BaseQuerySet(object): return 0 return self._cursor.count(with_limit_and_skip=with_limit_and_skip) - def delete(self, write_concern=None, _from_doc_delete=False): + def delete(self, write_concern=None, _from_doc_delete=False, cascade_refs=None): """Delete the documents matched by the query. :param write_concern: Extra keyword arguments are passed down which @@ -363,6 +363,11 @@ class BaseQuerySet(object): queryset = self.clone() doc = queryset._document + cascade_refs = set() if cascade_refs is None else cascade_refs + if doc in cascade_refs: + return 0 + cascade_refs.add(doc) + if write_concern is None: write_concern = {} @@ -405,7 +410,7 @@ class BaseQuerySet(object): ref_q = document_cls.objects(**{field_name + '__in': self}) ref_q_count = ref_q.count() if ref_q_count > 0: - ref_q.delete(write_concern=write_concern) + ref_q.delete(write_concern=write_concern, cascade_refs=cascade_refs) elif rule == NULLIFY: document_cls.objects(**{field_name + '__in': self}).update( write_concern=write_concern, **{'unset__%s' % field_name: 1}) From b691a56d5113b135c5ca077fc0c19aab4e9e26de Mon Sep 17 00:00:00 2001 From: emilecaron Date: Fri, 26 Jun 2015 08:52:30 +0000 Subject: [PATCH 5/9] late set instanciation --- mongoengine/queryset/base.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mongoengine/queryset/base.py b/mongoengine/queryset/base.py index df628c35..317e47bc 100644 --- a/mongoengine/queryset/base.py +++ b/mongoengine/queryset/base.py @@ -363,10 +363,8 @@ class BaseQuerySet(object): queryset = self.clone() doc = queryset._document - cascade_refs = set() if cascade_refs is None else cascade_refs - if doc in cascade_refs: + if cascade_refs and doc in cascade_refs: return 0 - cascade_refs.add(doc) if write_concern is None: write_concern = {} @@ -410,6 +408,8 @@ class BaseQuerySet(object): ref_q = document_cls.objects(**{field_name + '__in': self}) ref_q_count = ref_q.count() if ref_q_count > 0: + cascade_refs = set() if cascade_refs is None else cascade_refs + cascade_refs.add(doc) ref_q.delete(write_concern=write_concern, cascade_refs=cascade_refs) elif rule == NULLIFY: document_cls.objects(**{field_name + '__in': self}).update( From 9b7fe9ac312abab6f35664d8c35df53d313b1c80 Mon Sep 17 00:00:00 2001 From: emilecaron Date: Fri, 26 Jun 2015 09:31:07 +0000 Subject: [PATCH 6/9] restore broken behavior --- mongoengine/queryset/base.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/mongoengine/queryset/base.py b/mongoengine/queryset/base.py index 317e47bc..496e0566 100644 --- a/mongoengine/queryset/base.py +++ b/mongoengine/queryset/base.py @@ -409,7 +409,8 @@ class BaseQuerySet(object): ref_q_count = ref_q.count() if ref_q_count > 0: cascade_refs = set() if cascade_refs is None else cascade_refs - cascade_refs.add(doc) + if doc != document_cls: + cascade_refs.add(doc) ref_q.delete(write_concern=write_concern, cascade_refs=cascade_refs) elif rule == NULLIFY: document_cls.objects(**{field_name + '__in': self}).update( From 56a2e07dc21ec8438510dd9ff5e9dbb0c079b474 Mon Sep 17 00:00:00 2001 From: emilecaron Date: Fri, 26 Jun 2015 10:45:07 +0000 Subject: [PATCH 7/9] always store docs in cascade_refs --- mongoengine/queryset/base.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/mongoengine/queryset/base.py b/mongoengine/queryset/base.py index 496e0566..8db8351f 100644 --- a/mongoengine/queryset/base.py +++ b/mongoengine/queryset/base.py @@ -363,9 +363,6 @@ class BaseQuerySet(object): queryset = self.clone() doc = queryset._document - if cascade_refs and doc in cascade_refs: - return 0 - if write_concern is None: write_concern = {} @@ -405,12 +402,12 @@ class BaseQuerySet(object): continue rule = doc._meta['delete_rules'][rule_entry] if rule == CASCADE: - ref_q = document_cls.objects(**{field_name + '__in': self}) + cascade_refs = set() if cascade_refs is None else cascade_refs + for ref in queryset: + cascade_refs.add(ref.id) + ref_q = document_cls.objects(**{field_name + '__in': self, 'id__nin': cascade_refs}) ref_q_count = ref_q.count() if ref_q_count > 0: - cascade_refs = set() if cascade_refs is None else cascade_refs - if doc != document_cls: - cascade_refs.add(doc) ref_q.delete(write_concern=write_concern, cascade_refs=cascade_refs) elif rule == NULLIFY: document_cls.objects(**{field_name + '__in': self}).update( From 4525eb457b576aabb975445b5ef542519ea68b29 Mon Sep 17 00:00:00 2001 From: emilecaron Date: Fri, 26 Jun 2015 14:22:39 +0200 Subject: [PATCH 8/9] update changelog --- AUTHORS | 2 +- docs/changelog.rst | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/AUTHORS b/AUTHORS index 29993c84..b0400f4d 100644 --- a/AUTHORS +++ b/AUTHORS @@ -226,4 +226,4 @@ that much better: * Emmanuel Leblond (https://github.com/touilleMan) * Breeze.Kay (https://github.com/9nix00) * Vicki Donchenko (https://github.com/kivistein) - + * Emile Caron (https://github.com/emilecaron) diff --git a/docs/changelog.rst b/docs/changelog.rst index d35551ce..1fba7985 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -34,6 +34,7 @@ Changes in 0.10.0 - Allow dynamic lookup for more than two parts. #882 - Added support for min_distance on geo queries. #831 - Allow to add custom metadata to fields #705 +- Fix infinite recursion with CASCADE delete rules under specific conditions. #1046 Changes in 0.9.0 ================ From 4dc158589c1eab2313a905deff8a97a989cce558 Mon Sep 17 00:00:00 2001 From: Matthieu Rigal Date: Fri, 26 Jun 2015 17:58:53 +0200 Subject: [PATCH 9/9] Moved change to right place and added fancier test --- docs/changelog.rst | 2 +- tests/queryset/queryset.py | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 1fba7985..de47000d 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -4,6 +4,7 @@ Changelog Changes in 0.10.1 - DEV ======================= +- Fix infinite recursion with CASCADE delete rules under specific conditions. #1046 Changes in 0.10.0 ================= @@ -34,7 +35,6 @@ Changes in 0.10.0 - Allow dynamic lookup for more than two parts. #882 - Added support for min_distance on geo queries. #831 - Allow to add custom metadata to fields #705 -- Fix infinite recursion with CASCADE delete rules under specific conditions. #1046 Changes in 0.9.0 ================ diff --git a/tests/queryset/queryset.py b/tests/queryset/queryset.py index 050b2d03..9f4cd9b9 100644 --- a/tests/queryset/queryset.py +++ b/tests/queryset/queryset.py @@ -1430,6 +1430,30 @@ class QuerySetTest(unittest.TestCase): self.assertRaises(DoesNotExist, base.reload) self.assertRaises(DoesNotExist, other.reload) + def test_reverse_delete_rule_cascade_complex_cycle(self): + """Ensure reference cascading doesn't loop if reference graph isn't + a tree + """ + class Category(Document): + name = StringField() + + class Dummy(Document): + reference = ReferenceField('self', reverse_delete_rule=CASCADE) + cat = ReferenceField(Category, reverse_delete_rule=CASCADE) + + cat = Category(name='cat').save() + base = Dummy(cat=cat).save() + other = Dummy(reference=base).save() + other2 = Dummy(reference=other).save() + base.reference = other + base.save() + + cat.delete() + + self.assertRaises(DoesNotExist, base.reload) + self.assertRaises(DoesNotExist, other.reload) + self.assertRaises(DoesNotExist, other2.reload) + def test_reverse_delete_rule_cascade_self_referencing(self): """Ensure self-referencing CASCADE deletes do not result in infinite loop