Fix ForeignKey queryset filters on un-swapped models #1495

UnknownPlatypus · 2023-05-12T16:54:46Z

I have made things!

Followup PR to #1436.
I found a reproducible exemple, the first commit should make test fail.
The second should fix it.

Related issues

Closes 'self'-referencing relationship causes crash #626

UnknownPlatypus · 2023-05-12T19:29:22Z

Marking as draft for tonight, will check the typing issue later this weekend

sobolevn · 2023-05-24T08:21:17Z

mypy_django_plugin/django/context.py

@@ -364,13 +366,19 @@ def _resolve_field_from_parts(

            field = currently_observed_model._meta.get_field(field_part)
            if isinstance(field, RelatedField):
-                currently_observed_model = field.related_model
+                related_model = self.get_field_related_model_cls(field)


The code duplication is quite high. Can you please refactor it to be a method / function?

wasn't a big fan of that too, I can do that yes 👍

I was also considering actually raising LookupError (or re-raising a custom UnregisteredModelError or similar) in get_field_related_model_cls and handle them in places we actually return mypy types. (return AnyType(TypeOfAny.from_error))

Currently this is quite cumbersome to pass None values all the way up the intermediary functions/methods.
I'm not sure this is really better thought and maybe the intent is less clear?

UnknownPlatypus · 2023-06-16T09:32:39Z

Sorry, couldn't get back to this earlier, got caught up with other things.

I tried the approach of raising an exception for unregistered model in get_field_related_model_cls, this lead to some nice simplifications but I'm still not sure this is the best way to deal with this.

The diff is a bit bigger because of that, let me know if this seems reasonable to you.
I'll be more available in the following days to take any feedback

EDIT: hum, test are passing locally, will investigate further.
Edit2: I had django 3 instead of django 4 installed, hence the test passing locally

sobolevn

Looks like a solid solution! Thank you!

intgr · 2023-06-16T15:10:07Z

FWIW, I only have little experience working on plugin code, but I had the same feeling: checking for and passing None values up the stack felt unwieldy. I prefer the exception approach. 👍

typeddjango#1495 updated `get_field_related_model_cls` to raise UnregisteredModelError rather than returning `None` for failure paths. However, None can still be returned if the initial retrieval of `related_model_cls` returns None. This patch adds a check to see if the initial retrieval has got a `None` and then raises the appropriate error rather than letting `None` be returned.

typeddjango#1495 updated `get_field_related_model_cls` to raise `UnregisteredModelError` rather than returning `None` for failure paths. However, None can still be returned if the initial retrieval of `related_model_cls` returns None. This patch adds a check to see if the initial retrieval has got a `None` and then raises the appropriate error rather than letting `None` be returned.

#1495 updated `get_field_related_model_cls` to raise `UnregisteredModelError` rather than returning `None` for failure paths. However, None can still be returned if the initial retrieval of `related_model_cls` returns None. This patch adds a check to see if the initial retrieval has got a `None` and then raises the appropriate error rather than letting `None` be returned.

UnknownPlatypus marked this pull request as draft May 12, 2023 19:28

UnknownPlatypus marked this pull request as ready for review May 24, 2023 08:13

sobolevn reviewed May 24, 2023

View reviewed changes

UnknownPlatypus added 6 commits June 16, 2023 10:18

Add failing test

d63f5d5

Fix related_model lookups for un-swapped models

d5ad93e

Handle possible unregistered models

e933c5e

Add get_models_foreign_key helper

f6457d1

Refactor get_field_related_model_cls to raise UnregisteredModelError

1ce442c

Remove unreachable statements

dd5cf7b

UnknownPlatypus force-pushed the fix-issue-626 branch from 1290dc5 to dd5cf7b Compare June 16, 2023 09:26

Add missing check

add62ff

sobolevn approved these changes Jun 16, 2023

View reviewed changes

sobolevn merged commit e778561 into typeddjango:master Jun 16, 2023

UnknownPlatypus deleted the fix-issue-626 branch July 1, 2023 22:43

SingingTree mentioned this pull request Feb 18, 2024

AttributeError: 'NoneType' object has no attribute '__module__'. Did you mean: '__reduce__'? #1953

Closed

SingingTree mentioned this pull request Feb 18, 2024

Avoid returning None from get_field_related_model_cls #1956

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ForeignKey queryset filters on un-swapped models #1495

Fix ForeignKey queryset filters on un-swapped models #1495

UnknownPlatypus commented May 12, 2023 •

edited

Loading

UnknownPlatypus commented May 12, 2023

sobolevn May 24, 2023

UnknownPlatypus May 24, 2023

UnknownPlatypus May 24, 2023

UnknownPlatypus commented Jun 16, 2023 •

edited

Loading

sobolevn left a comment

intgr commented Jun 16, 2023

Fix ForeignKey queryset filters on un-swapped models #1495

Fix ForeignKey queryset filters on un-swapped models #1495

Conversation

UnknownPlatypus commented May 12, 2023 • edited Loading

I have made things!

Related issues

UnknownPlatypus commented May 12, 2023

sobolevn May 24, 2023

Choose a reason for hiding this comment

UnknownPlatypus May 24, 2023

Choose a reason for hiding this comment

UnknownPlatypus May 24, 2023

Choose a reason for hiding this comment

UnknownPlatypus commented Jun 16, 2023 • edited Loading

sobolevn left a comment

Choose a reason for hiding this comment

intgr commented Jun 16, 2023

UnknownPlatypus commented May 12, 2023 •

edited

Loading

UnknownPlatypus commented Jun 16, 2023 •

edited

Loading