Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 19 commits into from
Aug 9, 2023
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions django-stubs/db/models/fields/__init__.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import uuid
from collections.abc import Callable, Iterable, Sequence
from datetime import date, time, timedelta
from datetime import datetime as real_datetime
from typing import Any, ClassVar, Generic, Protocol, TypeVar, overload
from typing import Any, ClassVar, Generic, Literal, Protocol, TypeVar, overload

from django.core import validators # due to weird mypy.stubtest error
from django.core.checks import CheckMessage
Expand Down Expand Up @@ -122,7 +122,7 @@ class Field(RegisterLookupMixin, Generic[_ST, _GT]):
primary_key: bool
remote_field: ForeignObjectRel | None
is_relation: bool
related_model: type[Model] | None
related_model: type[Model] | Literal["self"] | None
one_to_many: bool | None
one_to_one: bool | None
many_to_many: bool | None
Expand Down
2 changes: 1 addition & 1 deletion django-stubs/db/models/fields/related.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class RelatedField(FieldCacheMixin, Field[_ST, _GT]):
rel_class: type[ForeignObjectRel]
swappable: bool
@property
def related_model(self) -> type[Model]: ... # type: ignore
def related_model(self) -> type[Model] | Literal["self"]: ... # type: ignore
def get_forward_related_filter(self, obj: Model) -> dict[str, int | UUID]: ...
def get_reverse_related_filter(self, obj: Model) -> Q: ...
@property
Expand Down
2 changes: 1 addition & 1 deletion django-stubs/db/models/fields/reverse_related.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class ForeignObjectRel(FieldCacheMixin):
@property
def target_field(self) -> AutoField: ...
@property
def related_model(self) -> type[Model]: ...
def related_model(self) -> type[Model] | Literal["self"]: ...
@property
def many_to_many(self) -> bool: ...
@property
Expand Down
3 changes: 3 additions & 0 deletions mypy_django_plugin/django/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Try to retrieve set type from a model's TypeInfo object and fallback to retrieving it manually
# from django-stubs own declaration. This is to align with the setter types declared for
# assignment.
Expand Down
2 changes: 1 addition & 1 deletion mypy_django_plugin/lib/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ def get_private_descriptor_type(type_info: TypeInfo, private_field_name: str, is

def get_field_lookup_exact_type(api: TypeChecker, field: "Field[Any, Any]") -> MypyType:
if isinstance(field, (RelatedField, ForeignObjectRel)):
lookup_type_class = field.related_model
lookup_type_class = field.remote_field.model
flaeppe marked this conversation as resolved.
Show resolved Hide resolved
rel_model_info = lookup_class_typeinfo(api, lookup_type_class)
if rel_model_info is None:
return AnyType(TypeOfAny.from_error)
Expand Down
25 changes: 25 additions & 0 deletions tests/typecheck/models/test_abstract.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,28 @@

class MyModel(BaseModel):
field = models.IntegerField()

- case: test_can_instantiate_with_recursive_relation_on_abstract_model
main: |
from myapp.models import Concrete, Recursive
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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Concrete(parent=Concrete(parent=None))
installed_apps:
- myapp
files:
- path: myapp/__init__.py
- path: myapp/models.py
content: |
from django.db import models

class Recursive(models.Model):
parent = models.ForeignKey("self", null=True, on_delete=models.CASCADE)

class Meta:
abstract = True

class Concrete(Recursive):
...