Fix IndexError crash when using `from_queryset()` of custom Manager subclass #1786

moranabadie · 2023-10-20T15:46:45Z

I have made things!

Related issues

  File "/.venv/lib/python3.11/site-packages/mypy_django_plugin/transformers/managers.py", line 129, in _process_dynamic_method
    ret_type, base_that_has_method.defn.type_vars[0].fullname, manager_instance.args[0]
                                                               ~~~~~~~~~~~~~~~~~~~~~^^^
IndexError: tuple index out of range

Without example I think I can not do more.

Note : base_that_has_method.defn.type_vars cannot be null. since _has_compatible_type_vars already check that.

sobolevn

I think that we need a test case for that :(

moranabadie · 2023-10-21T13:41:15Z

I think that we need a test case for that :(

I found some code that triggers this bug :

The manager has to be called directly NewManager().example() instead of MyModel.objects.example()
The BaseManager has to be typed (not have type vars), like in the following example :

instead of

from .querysets import MyModelQuerySet
from django.db.models.manager import BaseManager
NewManager = BaseManager.from_queryset(MyModelQuerySet)

have :

from .querysets import MyModelQuerySet
from django.db.models.manager import BaseManager
class TypedManager(BaseManager[models.Model]): ...
NewManager = TypedManager.from_queryset(MyModelQuerySet)

TypedManager will not have any type vars (because typed with models.Model).

Sorry for this i did not think about it in the last pull request.

hence I have added this test in this pull requests

moranabadie · 2023-10-21T13:45:09Z

@flaeppe I have added to the test reveal_type(NewManager().example()) # N: Revealed type is "_CTE1" because if the current situation manager_instance.args is empty so we can not get the model.

maybe creating a follow up to manage those cases ? I mean managing the example that I have added to the tests

flaeppe

Even though this is outside of a model, I think it should reveal MyModel since that has been annotated by the queryset used(MyModelQuerySet).

But I'm not at all aware of how the runtime acts in this case and it might deserve its own error if the Django runtime raises calling an unbound manager/queryset method

moranabadie · 2023-10-22T11:50:12Z

Even though this is outside of a model, I think it should reveal MyModel since that has been annotated by the queryset used(MyModelQuerySet).

But I'm not at all aware of how the runtime acts in this case and it might deserve its own error if the Django runtime raises calling an unbound manager/queryset method

OK I have pushed something : either take manager_instance.args, i.e. the model of the generated Manager. Or queryset_info.bases[0].args, if it is not set.

Personnaly I do not realy like this approach because it adds some complexity for not that much.
In this case do you prefer :

To solve the test case bug_1785 coreclty (that resolve "myapp.models.MyModel") with this method
Raise a mypy error (I do not know if it is possible in this ârt of the code)
just return the TypeVar (# N: Revealed type is "_CTE) like before

or something else ?

mypy_django_plugin/transformers/managers.py

intgr · 2023-10-23T15:02:57Z

I renamed the PR title to be hopefully more descriptive. Let me know if I misunderstood the problem or if it's missing something important.

Fix IndexError crash when using from_queryset() of custom Manager subclass

flaeppe

I know this is quite a narrow resolution to something. But I think that's fine as it's a bit hard to know and cover the broader spectrum. Which might require a completely different approach.

intgr · 2023-10-23T15:32:44Z

tests/typecheck/managers/querysets/test_from_queryset.yml

@@ -93,6 +93,43 @@
                NewManager = BaseManager.from_queryset(ModelQuerySet)
                class MyModel(models.Model):
                    objects = NewManager()
+
+-   case: bug_1785


I'd prefer more descriptive testcase names, how about...

Suggested change

- case: bug_1785

- case: from_queryset_custom_manager_subclass

yes this is better, is have pushed the diff

intgr

I tried to break this but couldn't.

meshy · 2024-01-20T14:56:00Z

mypy_django_plugin/transformers/managers.py

+        ):
+            ret_type = _replace_type_var(ret_type, base_that_has_method.defn.type_vars[0].fullname, typed_var[0])
+            args_types = [
+                _replace_type_var(arg_type, base_that_has_method.defn.type_vars[0].fullname, manager_instance.args[0])


Hi! I'm still getting the error on this line. I wonder if this second instance of manager_instance.args was unintentionally missed?

Suggested change

_replace_type_var(arg_type, base_that_has_method.defn.type_vars[0].fullname, manager_instance.args[0])

_replace_type_var(arg_type, base_that_has_method.defn.type_vars[0].fullname, typed_var[0])

I have opened a PR to address this. #1913

Sometimes Mypy crashes on this line with an IndexError. This change builds on the change in typeddjango#1786 by making further use of the safer variable `typed_var`. `typed_var` will equal `manager_instance.args` if `manager_instance.args` is populated, but if it isn't it will have another sensible value we can use instead. Because that value should be populated, this prevents the crash.

sobolevn reviewed Oct 20, 2023

View reviewed changes

moranabadie force-pushed the fix-tuple-index-out-of-range branch from 0759bbd to 01070fc Compare October 21, 2023 13:41

moranabadie force-pushed the fix-tuple-index-out-of-range branch 2 times, most recently from 5985608 to 77e2794 Compare October 21, 2023 13:48

flaeppe reviewed Oct 21, 2023

View reviewed changes

moranabadie force-pushed the fix-tuple-index-out-of-range branch 3 times, most recently from 225a190 to fc92d6e Compare October 22, 2023 11:48

intgr reviewed Oct 22, 2023

View reviewed changes

mypy_django_plugin/transformers/managers.py Outdated Show resolved Hide resolved

moranabadie force-pushed the fix-tuple-index-out-of-range branch from fc92d6e to d2cb998 Compare October 22, 2023 14:50

moranabadie requested a review from flaeppe October 22, 2023 17:12

intgr changed the title ~~fix as manager IndexError: tuple index out of range (#1785)~~ Fix IndexError crash when using from_queryset() of custom Manager subclass Oct 23, 2023

flaeppe approved these changes Oct 23, 2023

View reviewed changes

intgr reviewed Oct 23, 2023

View reviewed changes

intgr approved these changes Oct 23, 2023

View reviewed changes

fix as manager IndexError: tuple index out of range (typeddjango#1785)

d7b5e37

moranabadie force-pushed the fix-tuple-index-out-of-range branch from d2cb998 to d7b5e37 Compare October 23, 2023 16:44

intgr merged commit c313cfd into typeddjango:master Oct 23, 2023
35 checks passed

intgr mentioned this pull request Oct 23, 2023

Next release planning (4.2.6) #1780

Closed

meshy reviewed Jan 20, 2024

View reviewed changes

meshy mentioned this pull request Jan 22, 2024

Fix IndexError for custom queryset managers #1913

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix IndexError crash when using `from_queryset()` of custom Manager subclass #1786

Fix IndexError crash when using `from_queryset()` of custom Manager subclass #1786

moranabadie commented Oct 20, 2023 •

edited by intgr

Loading

sobolevn left a comment

moranabadie commented Oct 21, 2023 •

edited

Loading

moranabadie commented Oct 21, 2023

flaeppe left a comment

moranabadie commented Oct 22, 2023 •

edited

Loading

intgr commented Oct 23, 2023

flaeppe left a comment

intgr Oct 23, 2023

moranabadie Oct 23, 2023

intgr left a comment

meshy Jan 20, 2024

meshy Jan 22, 2024

	- case: bug_1785
	- case: from_queryset_custom_manager_subclass

	_replace_type_var(arg_type, base_that_has_method.defn.type_vars[0].fullname, manager_instance.args[0])
	_replace_type_var(arg_type, base_that_has_method.defn.type_vars[0].fullname, typed_var[0])

Fix IndexError crash when using from_queryset() of custom Manager subclass #1786

Fix IndexError crash when using from_queryset() of custom Manager subclass #1786

Conversation

moranabadie commented Oct 20, 2023 • edited by intgr Loading

I have made things!

Related issues

sobolevn left a comment

Choose a reason for hiding this comment

moranabadie commented Oct 21, 2023 • edited Loading

moranabadie commented Oct 21, 2023

flaeppe left a comment

Choose a reason for hiding this comment

moranabadie commented Oct 22, 2023 • edited Loading

intgr commented Oct 23, 2023

flaeppe left a comment

Choose a reason for hiding this comment

intgr Oct 23, 2023

Choose a reason for hiding this comment

moranabadie Oct 23, 2023

Choose a reason for hiding this comment

intgr left a comment

Choose a reason for hiding this comment

meshy Jan 20, 2024

Choose a reason for hiding this comment

meshy Jan 22, 2024

Choose a reason for hiding this comment

Fix IndexError crash when using `from_queryset()` of custom Manager subclass #1786

Fix IndexError crash when using `from_queryset()` of custom Manager subclass #1786

moranabadie commented Oct 20, 2023 •

edited by intgr

Loading

moranabadie commented Oct 21, 2023 •

edited

Loading

moranabadie commented Oct 22, 2023 •

edited

Loading