Fix crash when constructing an abstract model with recursive relation #1393

zuzelvp · 2023-03-13T23:31:45Z

I have made things!

We have abstract django model with a recursive relation. When we added code using one of its child django db models mypy crashed with the following trace

  /Users/jimmylai/workspace/carta-web/mypy/checkexpr.py(4821)accept()
  /Users/jimmylai/workspace/carta-web/mypy/nodes.py(1891)accept()
  /Users/jimmylai/workspace/carta-web/mypy/checkexpr.py(425)visit_call_expr()
  /Users/jimmylai/workspace/carta-web/mypy/checkexpr.py(545)visit_call_expr_inner()
  /Users/jimmylai/workspace/carta-web/mypy/checkexpr.py(1197)check_call_expr_with_callee_type()
  /Users/jimmylai/workspace/carta-web/mypy/checkexpr.py(1280)check_call()
  /Users/jimmylai/workspace/carta-web/mypy/checkexpr.py(1490)check_callable_call()
  /Users/jimmylai/workspace/carta-web/mypy/checkexpr.py(1020)apply_function_plugin()
  /Users/jimmylai/miniconda3/lib/python3.8/site-packages/mypy_django_plugin/transformers/init_create.py(63)redefine_and_typecheck_model_init()
-> return typecheck_model_method(ctx, django_context, model_cls, "__init__")
  /Users/jimmylai/miniconda3/lib/python3.8/site-packages/mypy_django_plugin/transformers/init_create.py(38)typecheck_model_method()
-> expected_types = django_context.get_expected_types(typechecker_api, model_cls, method=method)
  /Users/jimmylai/miniconda3/lib/python3.8/site-packages/mypy_django_plugin/django/context.py(201)get_expected_types()
-> ) or self.get_field_set_type(api, field, method=method)
  /Users/jimmylai/miniconda3/lib/python3.8/site-packages/mypy_django_plugin/django/context.py(290)get_field_set_type()
-> target_field = field.target_field
  /Users/jimmylai/miniconda3/lib/python3.8/site-packages/django/db/models/fields/related.py(878)target_field()
-> return self.foreign_related_fields[0]
  /Users/jimmylai/miniconda3/lib/python3.8/site-packages/django/db/models/fields/related.py(632)foreign_related_fields()
-> return tuple(rhs_field for lhs_field, rhs_field in self.related_fields if rhs_field)
  /Users/jimmylai/miniconda3/lib/python3.8/site-packages/django/db/models/fields/related.py(619)related_fields()
-> self._related_fields = self.resolve_related_fields()
> /Users/jimmylai/miniconda3/lib/python3.8/site-packages/django/db/models/fields/related.py(604)resolve_related_fields()
-> raise ValueError('Related model %r cannot be resolved' % self.remote_field.model)

After some investigation we tracked the issue to a field.targer_field call with a reflective field on an abstract django model for which django raises a ValueError https://github.com/django/django/blob/main/django/db/models/fields/related.py#L712

Noticed you are also skipping getting the type for the primary key when the django model is abstract so doing something similar for these fields.

Related issues

Did not find any already created

sobolevn

Can you please add a simple test case for that in https://github.com/typeddjango/django-stubs/blob/master/tests/typecheck/models/test_related_fields.yml

Other than that I am fine with the change. Thank you!

zuzelvp · 2023-03-14T20:36:08Z

@sobolevn can you run test suite on #1394 to see if it fails before the fix?

zuzelvp · 2023-03-14T20:38:08Z

Noticed some tests are failing. Investigating

sobolevn · 2023-03-15T10:04:36Z

Yes, sure!

intgr · 2023-04-07T11:07:29Z

Hi! Are you planning to continue this work?

intgr · 2023-04-08T16:02:01Z

Thanks for the effort so far. If I understand correctly, this PR intended to add support for ManyToManyField("self"), ForeginKey("self") for abstract models, which would currently crash our plugin?

If you don't have time to finish this, hopefully someone else can pick it up and finalize it. Maybe @flaeppe is interested, no pressure though. :)

It would be appreciated if you briefly describe the current state of the PR.

zuzelvp · 2023-04-13T23:51:45Z

TODO: add a test that fails without the fix and passes with it and does not break the whole test suite. Got busy at work so had to pause this work.

zuzelvp · 2023-04-13T23:53:36Z

mypy_django_plugin/django/context.py

@@ -195,6 +195,9 @@ def get_field_set_type_from_model_type_info(info: Optional[TypeInfo], field_name
        for field in model_cls._meta.get_fields():
            if isinstance(field, Field):
                field_name = field.attname
+                # Can not determine target_field for recursive relationship when model is abstract
+                if field.related_model == "self" and model_cls._meta.abstract:
+                    continue


This is the main portion of the fix. The rest was trying to get typing to work now that model_cls can be a string.

zuzelvp · 2023-04-13T23:54:37Z

tests/typecheck/models/test_related_fields.yml

@@ -7,6 +7,7 @@
      reveal_type(Model2().test)  # N: Revealed type is "app3.models.Model3_RelatedManager"
      reveal_type(Model1().test2)  # N: Revealed type is "app3.models.Model4_RelatedManager"
      reveal_type(Model2().test2)  # N: Revealed type is "app3.models.Model4_RelatedManager"
+      Model1()


This does not seem to fail without the fix so need a better test #1394

flaeppe · 2023-07-11T21:20:22Z

I rebased, figured out and added a breaking test in fb92aec.

The rebase was a bit messy, gonna try to resolve it. I also realised that we the plugin could yield an error when trying to instantiate an abstract model. As Django raises a TypeError attempting to do that.

flaeppe · 2023-07-11T21:53:08Z

mypy_django_plugin/django/context.py

@@ -362,7 +365,7 @@ def get_field_related_model_cls(self, field: Union["RelatedField[Any, Any]", For
    def _resolve_field_from_parts(
        self, field_parts: Iterable[str], model_cls: Type[Model]
    ) -> Union["Field[Any, Any]", ForeignObjectRel]:
-        currently_observed_model = model_cls
+        currently_observed_model: type[Model] | Literal["self"] = model_cls


I'm not certain the added annotation here is correct, it only introduces a theoretical Literal["self"] while call sites of _resolve_field_from_parts still passes model_cls: Type[Model].

So I'm not sure how currently_observed_model would then end up being a string in this context..

If anything it should be checked whether any of the existing call sites could end up with Type[Model] | Literal["self"]. Unless I'm missing something, none of the existing call sites for _resolve_field_from_parts is part of the provided traceback, which makes me slightly more interested in not changing anything here until we've encountered something being wrong.

Instead of using `field.related_model`

mypy_django_plugin/lib/helpers.py

flaeppe · 2023-07-11T22:03:08Z

@intgr In my eyes, this looks ready now as long as CI doesn't say otherwise. Sorry for being slow to look into this

flaeppe · 2023-07-11T22:20:59Z

tests/typecheck/models/test_abstract.yml

+    first = Concrete.objects.create(parent=None)
+    Concrete.objects.create(parent=first)
+    Recursive.objects.create(parent=None)
+    Recursive(parent=Recursive(parent=None))  # E: Unexpected attribute "parent" for model "Recursive"


For the curious one; it was initialisation of an abstract model(i.e. this line) that raised the error seen in the provided traceback

flaeppe · 2023-08-09T05:10:36Z

Friendly ping @sobolevn and @intgr, would any of you mind having a second look?

intgr

Looks good to me, just one minor tweak request.

Thanks for picking up this fix. And don't hesitate to ping me again if another PR is sitting without reviews.

mypy_django_plugin/lib/helpers.py

Co-authored-by: Marti Raudsepp <[email protected]>

sobolevn reviewed Mar 14, 2023

View reviewed changes

zuzelvp closed this Apr 7, 2023

intgr reopened this Apr 8, 2023

intgr added the help wanted Extra attention is needed label Apr 8, 2023

zuzelvp commented Apr 13, 2023

View reviewed changes

zuzelvp and others added 15 commits July 11, 2023 22:43

500 error when abstract model has a recursive relation

31db97b

Update context.py

b307edf

Update related.pyi

6631dfa

Update related.pyi

f85ceed

Update related.pyi

9863587

Update test_related_fields.yml

dd747e3

Update test_related_fields.yml

8d0300c

Update helpers.py

4cb8652

Update context.py

bc34afb

Update context.py

89359c7

Update reverse_related.pyi

45ec040

Update __init__.pyi

826baea

Update __init__.pyi

e0d25f7

Update test_related_fields.yml

fd92e04

Add breaking test with abstract model having recursive relation

fb92aec

flaeppe force-pushed the zuzelvp-patch-1 branch from 82f0622 to fb92aec Compare July 11, 2023 21:17

flaeppe reviewed Jul 11, 2023

View reviewed changes

Pick up to/target model from field.remote_field.model attribute

05a60ee

Instead of using `field.related_model`

flaeppe reviewed Jul 11, 2023

View reviewed changes

mypy_django_plugin/lib/helpers.py Show resolved Hide resolved

flaeppe mentioned this pull request Jul 11, 2023

Instantiating an abstract model yields no error #1624

Closed

flaeppe reviewed Jul 11, 2023

View reviewed changes

flaeppe requested a review from sobolevn July 12, 2023 12:23

Merge branch 'master' into zuzelvp-patch-1

dd63e76

flaeppe removed the help wanted Extra attention is needed label Aug 9, 2023

Update expected error messages when instantiating abstract model

4d0c0d8

intgr self-assigned this Aug 9, 2023

intgr changed the title ~~500 error when abstract model has a recursive relation~~ Fix crash when constructing an abstract model with recursive relation Aug 9, 2023

intgr approved these changes Aug 9, 2023

View reviewed changes

mypy_django_plugin/lib/helpers.py Show resolved Hide resolved

Update mypy_django_plugin/lib/helpers.py

ab848eb

Co-authored-by: Marti Raudsepp <[email protected]>

flaeppe merged commit 325006c into typeddjango:master Aug 9, 2023
28 checks passed

intgr mentioned this pull request Aug 11, 2023

Restore ability to instantiate, access objects manager, of abstract models #1653

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash when constructing an abstract model with recursive relation #1393

Fix crash when constructing an abstract model with recursive relation #1393

zuzelvp commented Mar 13, 2023 •

edited

Loading

sobolevn left a comment

zuzelvp commented Mar 14, 2023

zuzelvp commented Mar 14, 2023

sobolevn commented Mar 15, 2023

intgr commented Apr 7, 2023

intgr commented Apr 8, 2023 •

edited

Loading

zuzelvp commented Apr 13, 2023

zuzelvp Apr 13, 2023

zuzelvp Apr 13, 2023

flaeppe commented Jul 11, 2023

flaeppe Jul 11, 2023

flaeppe commented Jul 11, 2023

flaeppe Jul 11, 2023

flaeppe commented Aug 9, 2023

intgr left a comment

Fix crash when constructing an abstract model with recursive relation #1393

Fix crash when constructing an abstract model with recursive relation #1393

Conversation

zuzelvp commented Mar 13, 2023 • edited Loading

I have made things!

Related issues

sobolevn left a comment

Choose a reason for hiding this comment

zuzelvp commented Mar 14, 2023

zuzelvp commented Mar 14, 2023

sobolevn commented Mar 15, 2023

intgr commented Apr 7, 2023

intgr commented Apr 8, 2023 • edited Loading

zuzelvp commented Apr 13, 2023

zuzelvp Apr 13, 2023

Choose a reason for hiding this comment

zuzelvp Apr 13, 2023

Choose a reason for hiding this comment

flaeppe commented Jul 11, 2023

flaeppe Jul 11, 2023

Choose a reason for hiding this comment

flaeppe commented Jul 11, 2023

flaeppe Jul 11, 2023

Choose a reason for hiding this comment

flaeppe commented Aug 9, 2023

intgr left a comment

Choose a reason for hiding this comment

zuzelvp commented Mar 13, 2023 •

edited

Loading

intgr commented Apr 8, 2023 •

edited

Loading