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.

Sign up for GitHub

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

Add django.db.models.fields.related.ForeignObject missing methods and attributes #2154

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add django.db.models.fields.related.ForeignObject missing methods and attributes #2154

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3026dc8
Add typing of django.db.models.fields.related.ForeignObject
JaeHyuckSa May 12, 2024
2b6b31a
Merge branch 'master' into improve-related
JaeHyuckSa May 12, 2024
dbd8b3d
Update allowlist_todo.txt
JaeHyuckSa May 12, 2024
c866156
Updated parts that required ClassVar
JaeHyuckSa May 16, 2024
0a91481
Update the __copy__ method
JaeHyuckSa May 16, 2024
33fa5b7
Merge branch 'master' into improve-related
JaeHyuckSa May 16, 2024
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
25 changes: 24 additions & 1 deletion django-stubs/db/models/fields/related.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,14 @@ from uuid import UUID
from django.core import validators # due to weird mypy.stubtest error
from django.db.models.base import Model
from django.db.models.expressions import Combinable, Expression
from django.db.models.fields import NOT_PROVIDED, Field, _AllLimitChoicesTo, _ErrorMessagesMapping, _LimitChoicesTo
from django.db.models.fields import (
NOT_PROVIDED,
Empty,
Field,
_AllLimitChoicesTo,
_ErrorMessagesMapping,
_LimitChoicesTo,
)
from django.db.models.fields.mixins import FieldCacheMixin
from django.db.models.fields.related_descriptors import ForwardManyToOneDescriptor as ForwardManyToOneDescriptor
from django.db.models.fields.related_descriptors import ForwardOneToOneDescriptor as ForwardOneToOneDescriptor
Expand Down Expand Up @@ -87,6 +94,9 @@ class RelatedField(FieldCacheMixin, Field[_ST, _GT]):

class ForeignObject(RelatedField[_ST, _GT]):
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this class has lots of unsolved problems. Let's solve them!

  1. Please, use ClassVar where needed: https://github.com/django/django/blob/a09082a9bec18f8e3ee8c10d473013ec67ffe93b/django/db/models/fields/related.py#L522-L530

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. As you suggested, I have handled all the relevant parts as class variables.

remote_field: ForeignObjectRel
requires_unique_target: bool
related_accessor_class: type[ReverseManyToOneDescriptor]
forward_related_accessor_class: type[ForwardManyToOneDescriptor]
rel_class: type[ForeignObjectRel]
from_fields: Sequence[str]
to_fields: Sequence[str | None] # None occurs in ForeignKey, where to_field defaults to None
Expand Down Expand Up @@ -124,6 +134,7 @@ class ForeignObject(RelatedField[_ST, _GT]):
error_messages: _ErrorMessagesMapping | None = ...,
db_comment: str | None = ...,
) -> None: ...
def __copy__(self) -> Empty: ...
Copy link
Collaborator

@intgr intgr May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this should actually be Self

The Empty class is just an artifact of a weird hack that Django uses, but it modifies the __class__ attribute so that it's no longer actually the Empty class

    def __copy__(self):
        # We need to avoid hitting __reduce__, so define this
        # slightly weird copy construct.
        obj = Empty()
        obj.__class__ = self.__class__
        obj.__dict__ = self.__dict__.copy()
        return obj

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on your explanation, that seems correct. Thank you for pointing that out!

# class access
@overload
def __get__(self, instance: None, owner: Any) -> ForwardManyToOneDescriptor[Self]: ...
Expand All @@ -142,6 +153,18 @@ class ForeignObject(RelatedField[_ST, _GT]):
def local_related_fields(self) -> tuple[Field, ...]: ...
@cached_property
def foreign_related_fields(self) -> tuple[Field, ...]: ...
def get_local_related_value(self, instance: Model) -> tuple[list[Any], ...]: ...
def get_foreign_related_value(self, instance: Model) -> tuple[list[Any], ...]: ...
@staticmethod
def get_instance_value_for_fields(instance: Model, fields: Sequence[Field]) -> tuple[list[Any], ...]: ...
def get_joining_columns(self, reverse_join: bool = False) -> tuple[tuple[str, str], ...]: ...
def get_reverse_joining_columns(self) -> tuple[tuple[str, str], ...]: ...
def get_extra_descriptor_filter(self, instance: Model) -> dict[str, Any]: ...
def get_path_info(self, filtered_relation: FilteredRelation | None = ...) -> list[PathInfo]: ...
def get_reverse_path_info(self, filtered_relation: FilteredRelation | None = ...) -> list[PathInfo]: ...
@classmethod
def get_class_lookups(cls) -> dict[str, Any]: ...
def contribute_to_related_class(self, cls: type[Model], related: RelatedField) -> None: ...
def get_joining_fields(self, reverse_join: bool = False) -> tuple[tuple[Field, Field], ...]: ...
def get_reverse_joining_fields(self) -> tuple[tuple[Field, Field], ...]: ...

Expand Down
53 changes: 1 addition & 52 deletions scripts/stubtest/allowlist_todo.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,28 +290,13 @@ django.contrib.gis.db.models.FileField.formfield
django.contrib.gis.db.models.FilePathField.formfield
django.contrib.gis.db.models.FloatField.formfield
django.contrib.gis.db.models.ForeignKey.__class_getitem__
django.contrib.gis.db.models.ForeignKey.contribute_to_related_class
django.contrib.gis.db.models.ForeignKey.convert_empty_strings
django.contrib.gis.db.models.ForeignKey.descriptor_class
django.contrib.gis.db.models.ForeignKey.formfield
django.contrib.gis.db.models.ForeignKey.get_db_converters
django.contrib.gis.db.models.ForeignObject.__copy__
django.contrib.gis.db.models.ForeignObject.__get__
django.contrib.gis.db.models.ForeignObject.contribute_to_related_class
django.contrib.gis.db.models.ForeignObject.forward_related_accessor_class
django.contrib.gis.db.models.ForeignObject.get_class_lookups
django.contrib.gis.db.models.ForeignObject.get_extra_descriptor_filter
django.contrib.gis.db.models.ForeignObject.get_extra_restriction
django.contrib.gis.db.models.ForeignObject.get_foreign_related_value
django.contrib.gis.db.models.ForeignObject.get_instance_value_for_fields
django.contrib.gis.db.models.ForeignObject.get_joining_columns
django.contrib.gis.db.models.ForeignObject.get_local_related_value
django.contrib.gis.db.models.ForeignObject.get_path_info
django.contrib.gis.db.models.ForeignObject.get_reverse_joining_columns
django.contrib.gis.db.models.ForeignObject.get_reverse_path_info
django.contrib.gis.db.models.ForeignObject.path_infos
django.contrib.gis.db.models.ForeignObject.related_accessor_class
django.contrib.gis.db.models.ForeignObject.requires_unique_target
django.contrib.gis.db.models.ForeignObject.reverse_path_infos
django.contrib.gis.db.models.ForeignObjectRel.__init__
django.contrib.gis.db.models.ForeignObjectRel.empty_strings_allowed
Expand Down Expand Up @@ -357,8 +342,6 @@ django.contrib.gis.db.models.Model.add_to_class
django.contrib.gis.db.models.ObjectDoesNotExist
django.contrib.gis.db.models.OneToOneField.__get__
django.contrib.gis.db.models.OneToOneField.formfield
django.contrib.gis.db.models.OneToOneField.forward_related_accessor_class
django.contrib.gis.db.models.OneToOneField.related_accessor_class
django.contrib.gis.db.models.OneToOneRel.__init__
django.contrib.gis.db.models.OrderBy.as_oracle
django.contrib.gis.db.models.OrderBy.as_sql
Expand Down Expand Up @@ -727,28 +710,13 @@ django.db.models.FileField.formfield
django.db.models.FilePathField.formfield
django.db.models.FloatField.formfield
django.db.models.ForeignKey.__class_getitem__
django.db.models.ForeignKey.contribute_to_related_class
django.db.models.ForeignKey.convert_empty_strings
django.db.models.ForeignKey.descriptor_class
django.db.models.ForeignKey.formfield
django.db.models.ForeignKey.get_db_converters
django.db.models.ForeignObject.__copy__
django.db.models.ForeignObject.__get__
django.db.models.ForeignObject.contribute_to_related_class
django.db.models.ForeignObject.forward_related_accessor_class
django.db.models.ForeignObject.get_class_lookups
django.db.models.ForeignObject.get_extra_descriptor_filter
django.db.models.ForeignObject.get_extra_restriction
django.db.models.ForeignObject.get_foreign_related_value
django.db.models.ForeignObject.get_instance_value_for_fields
django.db.models.ForeignObject.get_joining_columns
django.db.models.ForeignObject.get_local_related_value
django.db.models.ForeignObject.get_path_info
django.db.models.ForeignObject.get_reverse_joining_columns
django.db.models.ForeignObject.get_reverse_path_info
django.db.models.ForeignObject.path_infos
django.db.models.ForeignObject.related_accessor_class
django.db.models.ForeignObject.requires_unique_target
django.db.models.ForeignObject.reverse_path_infos
django.db.models.ForeignObjectRel.__init__
django.db.models.ForeignObjectRel.empty_strings_allowed
Expand Down Expand Up @@ -792,8 +760,6 @@ django.db.models.Model.add_to_class
django.db.models.ObjectDoesNotExist
django.db.models.OneToOneField.__get__
django.db.models.OneToOneField.formfield
django.db.models.OneToOneField.forward_related_accessor_class
django.db.models.OneToOneField.related_accessor_class
django.db.models.OneToOneRel.__init__
django.db.models.OrderBy.as_oracle
django.db.models.OrderBy.as_sql
Expand Down Expand Up @@ -969,28 +935,13 @@ django.db.models.fields.json.KeyTransform.as_sqlite
django.db.models.fields.json.KeyTransformIsNull.as_sqlite
django.db.models.fields.json.KeyTransformNumericLookupMixin.process_rhs
django.db.models.fields.related.ForeignKey.__class_getitem__
django.db.models.fields.related.ForeignKey.contribute_to_related_class
django.db.models.fields.related.ForeignKey.convert_empty_strings
django.db.models.fields.related.ForeignKey.descriptor_class
django.db.models.fields.related.ForeignKey.formfield
django.db.models.fields.related.ForeignKey.get_db_converters
django.db.models.fields.related.ForeignObject.__copy__
django.db.models.fields.related.ForeignObject.__get__
django.db.models.fields.related.ForeignObject.contribute_to_related_class
django.db.models.fields.related.ForeignObject.forward_related_accessor_class
django.db.models.fields.related.ForeignObject.get_class_lookups
django.db.models.fields.related.ForeignObject.get_extra_descriptor_filter
django.db.models.fields.related.ForeignObject.get_extra_restriction
django.db.models.fields.related.ForeignObject.get_foreign_related_value
django.db.models.fields.related.ForeignObject.get_instance_value_for_fields
django.db.models.fields.related.ForeignObject.get_joining_columns
django.db.models.fields.related.ForeignObject.get_local_related_value
django.db.models.fields.related.ForeignObject.get_path_info
django.db.models.fields.related.ForeignObject.get_reverse_joining_columns
django.db.models.fields.related.ForeignObject.get_reverse_path_info
django.db.models.fields.related.ForeignObject.path_infos
django.db.models.fields.related.ForeignObject.related_accessor_class
django.db.models.fields.related.ForeignObject.requires_unique_target
django.db.models.fields.related.ForeignObject.get_extra_restriction
Copy link
Member

Choose a reason for hiding this comment

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

  1. Let's add other missing attributes to ForeignObject

Copy link
Contributor Author

@JaeHyuckSa JaeHyuckSa May 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I have two questions:

  1. I tried to define the get_extra_restriction method, but I encountered an error due to the parameter mismatch with the subclass. I attempted to use type: ignore on the method in the subclass, but it was not allowed. What steps can I take to resolve this issue?
error: not checking stubs due to mypy build errors:
django-stubs/contrib/contenttypes/fields.pyi:90: error: Signature of "get_extra_restriction" incompatible with supertype "ForeignObject"  [override]
django-stubs/contrib/contenttypes/fields.pyi:90: note:      Superclass:
django-stubs/contrib/contenttypes/fields.pyi:90: note:          def get_extra_restriction(self, alias: str, related_alias: str) -> None
django-stubs/contrib/contenttypes/fields.pyi:90: note:      Subclass:
django-stubs/contrib/contenttypes/fields.pyi:90: note:          def get_extra_restriction(self, where_class: Type[WhereNode], alias: Optional[str], remote_alias: str) -> WhereNode
django-stubs/contrib/contenttypes/fields.pyi:92: error: Unused "type: ignore" comment  [unused-ignore]
  1. Should I also add the cached_property method?

django.db.models.fields.related.ForeignObject.reverse_path_infos
django.db.models.fields.related.ForeignObjectRel.__init__
django.db.models.fields.related.ForeignObjectRel.empty_strings_allowed
Expand All @@ -1015,8 +966,6 @@ django.db.models.fields.related.ManyToOneRel.__init__
django.db.models.fields.related.ManyToOneRel.identity
django.db.models.fields.related.OneToOneField.__get__
django.db.models.fields.related.OneToOneField.formfield
django.db.models.fields.related.OneToOneField.forward_related_accessor_class
django.db.models.fields.related.OneToOneField.related_accessor_class
django.db.models.fields.related.OneToOneRel.__init__
django.db.models.fields.related.RelatedField.formfield
django.db.models.fields.related.lazy_related_operation
Expand Down
Loading