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

.annotate(...) does not seem to work with reverse-related foreign key lookupu #2382

Open
asottile-sentry opened this issue Sep 24, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@asottile-sentry
Copy link

Bug report

wrote a little testcase -- will see if I can figure out a quick fix for it

- case: test_annotate_reverse_related
  main: |
    from myapp.models import MB
    from django.db.models import Count

    o = MB.objects.annotate(Count("mas")).get()
    reveal_type(o.mas__count)  # N: Revealed type is "builtins.int"
  installed_apps:
    - myapp
  files:
    - path: myapp/__init__.py
    - path: myapp/models.py
      content: |
        from django.db import models

        class MA(models.Model):
            mb = models.ForeignKey("MB", related_name="mas", on_delete=models.CASCADE)

        class MB(models.Model): pass

What's wrong

_______________________________________ test_annotate_reverse_related _______________________________________
/Users/asottile/workspace/sentry-forked-django-stubs/tests/typecheck/managers/querysets/test_annotate.yml:405: 
E   pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: 
E   Actual:
E     main:5: error: "MB" has no attribute "mas__count"  [attr-defined] (diff)
E     main:5: note: Revealed type is "Any"          (diff)
E   Expected:
E     main:5: note: Revealed type is "builtins.int" (diff)
E   Alignment of first line difference:
E     E: main:5: note: Revealed type is "builtins.int"
E     A: main:5: error: "MB" has no attribute "mas__count"  [attr-defined]
E                ^

How is that should be

should resolve correctly

System information

  • OS:
  • python version: 3.12.6
  • django version: 5.1.1
  • mypy version: 1.11.
  • django-stubs version: 5.1.0
  • django-stubs-ext version: 5.1.0
@asottile-sentry asottile-sentry added the bug Something isn't working label Sep 24, 2024
@asottile-sentry
Copy link
Author

ah I see from reading the code that it just doesn't support non-named arguments. there's a few tests which are passing just due to luck it seems (since they also use non-named arguments assuming they'll produce a WithAnnotations)

what's a little weird is this seems to have worked before django-stubs 5.1.0 -- but I suspect that was due to the old implementation being buggy

@asottile-sentry
Copy link
Author

actually just one test:

qs = User.objects.annotate(Count('id'))

@sterliakov
Copy link
Contributor

Yes, the most recent implementation is less forgiving for .annotate(), but positional arguments are indeed missed.

if kwargs:
# For now, we don't try to resolve the output_field of the field would be, but use Any.
# NOTE: It's important that we use 'special_form' for 'Any' as otherwise we can
# get stuck with mypy interpreting an overload ambiguity towards the
# overloaded 'Field.__get__' method when its 'model' argument gets matched. This
# is because the model argument gets matched with a model subclass that is
# parametrized with a type that contains the 'Any' below and then mypy runs in
# to a (false?) ambiguity, due to 'Any', and can't decide what overload/return
# type to select
#
# Example:
# class MyModel(models.Model):
# field = models.CharField()
#
# # Our plugin auto generates the following subclass
# class MyModel_WithAnnotations(MyModel, django_stubs_ext.Annotations[_Annotations]):
# ...
# # Assume
# x = MyModel.objects.annotate(foo=F("id")).get()
# reveal_type(x) # MyModel_WithAnnotations[TypedDict({"foo": Any})]
# # Then, on an attribute access of 'field' like
# reveal_type(x.field)
#
# Now 'CharField.__get__', which is overloaded, is passed 'x' as the 'model'
# argument and mypy consider it ambiguous to decide which overload method to
# select due to the 'Any' in 'TypedDict({"foo": Any})'. But if we specify the
# 'Any' as 'TypeOfAny.special_form' mypy doesn't consider the model instance to
# contain 'Any' and the ambiguity goes away.
field_types = {name: AnyType(TypeOfAny.special_form) for name, _ in kwargs.items()}

Since the alias resolution logic is fairly non-trivial, we have several options here:

  • fail loudly ("positional aliases are not supported")
  • fail silently, allow any extra attributes
  • duplicate runtime Django logic (Aggregate.default_alias), which is just f"{args[0]}__{func.name.lower()}" and is only supported when there is exactly one argument to a function.

The last options appears to be most promising: it also enables an extra safety check (detect a runtime error if a more complex aggregate is passed as a positional arg to .annotate()).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

2 participants