Repository: netzkolchose/django-computedfields (language: python)

Issue:
up pulling over multiple multi tables is not stable
Repro from modified docs example:

```python
# wrong order
>>> MultiC.objects.create()
<class 'klaus.models.MultiC'>
<class 'klaus.models.MultiB'> sub-c
<class 'klaus.models.MultiBase'> b
...
>>> active_resolver._querysets_for_update(MultiC,MultiC.objects.filter(pk=4))
OrderedDict([
	(<class 'klaus.models.MultiB'>, 	[<QuerySet [<MultiB: MultiB object (4)>]>, {'comp'}]),
	(<class 'klaus.models.MultiBase'>, 	[<QuerySet [<MultiBase: MultiBase object (4)>]>, {'comp'}])
])


# correct order
>>> MultiC.objects.create()
<class 'klaus.models.MultiC'>
<class 'klaus.models.MultiBase'> b
<class 'klaus.models.MultiB'> sub-c
...
>>> active_resolver._querysets_for_update(MultiC,MultiC.objects.filter(pk=4))
OrderedDict([
	(<class 'klaus.models.MultiBase'>, 	[<QuerySet [<MultiBase: MultiBase object (4)>]>, {'comp'}]),
	(<class 'klaus.models.MultiB'>, 	[<QuerySet [<MultiB: MultiB object (4)>]>, {'comp'}])
])
```

We lose somewhere in the resolver the ordering information, thus the cf `comp` on `MultiC(MultiB(MultiBase))` wrongly get filled from MultiB execution.


Discussion / hints:

Fix patch:
diff --git a/docs/examples.rst b/docs/examples.rst
index a6cc5c6..44afe5a 100644
--- a/docs/examples.rst
+++ b/docs/examples.rst
@@ -458,30 +458,38 @@ updates from different descendent model fields, example:
 
 .. code-block:: python
 
-    class Base(ComputedFieldsModel):
+    class MultiBase(ComputedFieldsModel):
         @computed(models.CharField(max_length=32), depends=[
-            ('a', ['f_on_a']),      # pull custom field from A descendant
-            ('b', ['f_on_b']),      # pull custom field from B descendant
-            ('b.c', ['f_on_c'])     # pull custom field from C descendant
+            ('multia', ['f_on_a']),         # pull custom field from A descendant
+            ('multib', ['f_on_b']),         # pull custom field from B descendant
+            ('multib.multic', ['f_on_c'])   # pull custom field from C descendant
         ])
         def comp(self):
-            # guard access to attributes from submodels
-            if hasattr(self, 'a'):
-                return a.f_on_a
-            if hasattr(self, 'b'):
-                if hasattr(self, 'c'):
-                    return self.b.c.f_on_c
-                return b.f_on_b
+            # since we dont know the actual sub model,
+            # we have to guard the attribute access
+            # important: isinstance check will not work here!
+            if hasattr(self, 'multia'):
+                return self.multia.f_on_a
+            if hasattr(self, 'multib'):
+                if hasattr(self.multib, 'multic'):
+                    return self.multib.multic.f_on_c
+                return self.multib.f_on_b
             return ''
 
-    class A(Base):
+    class MultiA(MultiBase):
         f_on_a = models.CharField(max_length=32, default='a')
-    class B(Base):
+    class MultiB(MultiBase):
         f_on_b = models.CharField(max_length=32, default='b')
-    class C(B):
+    class MultiC(MultiB):
         f_on_c = models.CharField(max_length=32, default='sub-c')
 
-Note that you have to guard the attribute access yourself in the method code as shown above.
+Note that you have to guard the attribute access yourself in the method as shown above.
+Also you cannot rely on the type of `self` with `isinstance`, since the method
+will run late on the model, where the field is defined (`MultiBase` above).
+
+Sidenote: The up-pulling is currently not further optimized in the resolver,
+which leads to a bad update cascade when used across deeper submodels. In the example above
+saving a `MultiC` instance will cascade through updates on `MultiB` to `MultiBase`.
 
 
 Forced Update of Computed Fields


Test patch:
diff --git a/example/test_full/models.py b/example/test_full/models.py
index 667e6d4..edfca1c 100644
--- a/example/test_full/models.py
+++ b/example/test_full/models.py
@@ -934,3 +934,29 @@ class Work(ComputedFieldsModel):
     ])
     def descriptive_assigment(self):
         return '"{}" is assigned to "{}"'.format(self.subject, self.user.fullname)
+
+# issue #93
+class MultiBase(ComputedFieldsModel):
+    @computed(models.CharField(max_length=32), depends=[
+        ('multia', ['f_on_a']),         # pull custom field from A descendant
+        ('multib', ['f_on_b']),         # pull custom field from B descendant
+        ('multib.multic', ['f_on_c'])   # pull custom field from C descendant
+    ])
+    def comp(self):
+        # since we dont know the actual sub model,
+        # we have to guard the attribute access
+        # important: isinstance check will not work here!
+        if hasattr(self, 'multia'):
+            return self.multia.f_on_a
+        if hasattr(self, 'multib'):
+            if hasattr(self.multib, 'multic'):
+                return self.multib.multic.f_on_c
+            return self.multib.f_on_b
+        return ''
+
+class MultiA(MultiBase):
+    f_on_a = models.CharField(max_length=32, default='a')
+class MultiB(MultiBase):
+    f_on_b = models.CharField(max_length=32, default='b')
+class MultiC(MultiB):
+    f_on_c = models.CharField(max_length=32, default='sub-c')
diff --git a/example/test_full/tests/test_multitable.py b/example/test_full/tests/test_multitable.py
index d37bf41..e9f0415 100644
--- a/example/test_full/tests/test_multitable.py
+++ b/example/test_full/tests/test_multitable.py
@@ -1,5 +1,5 @@
 from django.test import TestCase
-from ..models import MtBase, MtDerived, MtRelated, MtDerived2, MtSubDerived
+from ..models import MtBase, MtDerived, MtRelated, MtDerived2, MtSubDerived, MultiBase, MultiA, MultiB, MultiC
 from ..models import ChildModel, ChildModel2, ParentModel, DependsOnParent, DependsOnParentComputed
 from ..models import MtPtrBase, MtPtrDerived
 from computedfields.models import update_dependent
@@ -211,3 +211,39 @@ def test_base_change(self):
         update_dependent(MtPtrBase.objects.all(), update_fields=['basename'])
 
         self.assertEqual(list(MtPtrDerived.objects.all().values_list('basename', flat=True)), ['new value'] * 10)
+
+
+class TestMultiTableUpPullingExample(TestCase):
+    def setUp(self) -> None:
+        self.base = MultiBase.objects.create()
+        self.a = MultiA.objects.create()
+        self.b = MultiB.objects.create()
+        self.c = MultiC.objects.create()
+
+    def test_init(self):
+        self.base.refresh_from_db()
+        self.a.refresh_from_db()
+        self.b.refresh_from_db()
+        self.c.refresh_from_db()
+        self.assertEqual(self.base.comp, '')
+        self.assertEqual(self.a.comp, 'a')
+        self.assertEqual(self.b.comp, 'b')
+        self.assertEqual(self.c.comp, 'sub-c')
+
+    def test_update(self):
+        self.a.f_on_a = 'A'
+        self.a.save()
+        self.a.refresh_from_db()
+        self.assertEqual(self.a.comp, 'A')
+        self.b.f_on_b = 'B'
+        self.b.save(update_fields=['f_on_b'])
+        self.b.refresh_from_db()
+        self.assertEqual(self.b.comp, 'B')
+        self.c.f_on_c = 'C'
+        self.c.save()
+        self.c.refresh_from_db()
+        self.assertEqual(self.c.comp, 'C')
+        self.c.f_on_c = 'CC'
+        self.c.save(update_fields=['f_on_c'])
+        self.c.refresh_from_db()
+        self.assertEqual(self.c.comp, 'CC')