typeddjango / django-stubs

PEP-484 stubs for Django
MIT License
1.55k stars 432 forks source link

reverse relations are not recognized by mypy #2272

Closed AlexandrVasilchuk closed 1 month ago

AlexandrVasilchuk commented 1 month ago

Bug report

Description

I am facing an issue with django-stubs where reverse relations are not recognized by mypy. Despite trying multiple threads and solutions, including those in the following discussions and issues:

Discussion 2192 Issue 150

The problem persists.

Actual Behavior

mypy raises errors indicating that the reverse relation field is not recognized. Specifically, it cannot resolve the keyword 'proposals' into the field.

Expected Behavior

mypy should recognize the reverse relations between models and not raise errors when these relations are used.

Steps to Reproduce

Define models with ManyToManyField and reverse relations. Run mypy to check for typing errors.

Снимок экрана 2024-07-24 в 15 43 03 Снимок экрана 2024-07-24 в 15 43 28 Снимок экрана 2024-07-24 в 15 44 04

System information


How can I make mypy recognize the reverse relation between these models? It is also important to mention that the models implement standard objects.

Thank you for your assistance.

flaeppe commented 1 month ago

I've tried to investigate this further, but I'm having trouble trying to figure out if your provided snippets are a repro case or not. Could you please fill in the missing pieces so that we can check if we have a repro case here?

From your images:

I'm not saying that you necessarily need to include all model fields, a runnable minimal breaking case is fine. If possible, of course.

AlexandrVasilchuk commented 1 month ago

@flaeppe

Thank You and Apologies

First of all, thank you for the quick response to my previous issue. I apologize for not providing the complete code necessary to reproduce the problem in my initial post. Below, I have included a more comprehensive example.

Reproduction Code

In my attempt to reproduce the issue, I wrote the following code:

@track_history
class FakeDocument(TemplateForDocumentAndFolder):
    name = models.TextField(verbose_name="Название документа")
    file = models.FileField(max_length=4096, upload_to="documents", verbose_name="Файл")

class FakeFolder(models.Model):
    name = models.CharField()
    documents = models.ManyToManyField(FakeDocument, related_name="folder")

class FakePCR(models.Model):
    name = models.CharField(
        max_length=500,
        verbose_name="Название",
        default="",
    )
    folders = models.ForeignKey(
        FakeFolder,
        verbose_name="Папка с документами",
        blank=True,
        null=True,
        related_name="pcrs",
        on_delete=models.PROTECT,
    )

class FakeProject(models.Model):
    name = models.CharField()

class FakeProposal(models.Model):
    name = models.TextField("Название", null=True, blank=True)
    project = models.ForeignKey(
        FakeProject,
        verbose_name="Проект",
        on_delete=models.PROTECT,
        null=True,
        blank=True,
    )
    pcrs = models.ManyToManyField(FakePCR, verbose_name="ЗНИ/ЕЗИ", related_name="proposals", blank=False)

def case_to_reproduce():
    document: FakeFolder = FakeFolder.objects.get()
    document.pcrs.filter(proposals__project__is_null=True)

However, this code does not reproduce the error. To reproduce the error, it is necessary to move the FakeProposal model into a separate file (I created a new folder in the current directory, and init.py is present in the folder, if that matters).

image


Discoveries and Workarounds

While trying to reproduce the issue, I discovered that if I import the FakeProposal model into this code, the mypy error disappears (presumably due to scanning), but this results in legitimate linter warnings about unused imports. image

Here's another example of how the error can be reproduced. Leaving the code as it was, add the following lines:

    pcr: FakePCR = FakePCR.objects.get()
    pcr.proposals.all()  # "FakePCR" has no attribute "proposals"Mypyattr-defined

image

This will also trigger the error. In our project, we have found a way to resolve this by using assert hasattr(obj, "reverse-relation-name"). This is an ugly workaround that we don't like, but it is less annoying than a red mypy error. image

Thank you for your help in resolving this issue.

P.S. unfortunately, the model import solution does not help me in the main project code, although it works correctly on this mini-case perhaps this has something to do with the fact that in the main project, many of the models used are distributed across different applications

cuu508 commented 1 month ago

Minimal repro case:

from django.db import models

class Author(models.Model):
    name = models.CharField(max_length=100, blank=True)

    def featured_books(self) -> list[Book]:
        return list(self.book_set.filter(featured=True))

class Book(models.Model):
    name = models.CharField(max_length=100, blank=True)
    featured = models.BooleanField(default=False)
    authors = models.ManyToManyField(Author)

(and here's a complete Django project: https://github.com/cuu508/django-stubs-2272)

$ mypy --no-incremental --strict demo
demo/books/models.py:10: error: Cannot resolve keyword 'featured' into field. Choices are: author, author_id, book, book_id, id  [misc]
Found 1 error in 1 file (checked 12 source files)

Interestingly, I get this error with django-stubs 5.0.3.

With 5.0.2:

$ pip install django-stubs==5.0.2
...
$ mypy --no-incremental --strict demo
Success: no issues found in 12 source files
flaeppe commented 1 month ago

[...] To reproduce the error, it is necessary to move the FakeProposal model into a separate file (I created a new folder in the current directory, and init.py is present in the folder, if that matters).

I can reproduce the problem this way too, but that means that the model isn't registered by Django. And if it isn't registered by Django, django-stubs won't pick it up either. Models needs to go into models.py: https://docs.djangoproject.com/en/5.0/topics/db/models/#using-models. And the problem goes away when importing because it's then pulled into models.py and gets registered and picked up by Django.

I'm afraid that https://github.com/typeddjango/django-stubs/issues/2272#issuecomment-2252260327 isn't a repro case.


I can reproduce from https://github.com/typeddjango/django-stubs/issues/2272#issuecomment-2254502136 though. Let me check it out

flaeppe commented 1 month ago

Bisected the problem in https://github.com/typeddjango/django-stubs/issues/2272#issuecomment-2254502136 to #2275.

There's a fix for that in #2283. But I think all of that is unrelated to the original issue reported here.

cuu508 commented 1 month ago

Thanks for the quick fix @flaeppe!

Testing django-stubs 5.0.4 with my project, I hit another issue which perhaps requires a similar fix:

from __future__ import annotations

from django.db import models

class Author(models.Model):
    name = models.CharField(max_length=100, blank=True)

    def featured_names(self) -> list[str]:
        return list(self.book_set.values_list("name", flat=True))

class Book(models.Model):
    name = models.CharField(max_length=100, blank=True)
    featured = models.BooleanField(default=False)
    authors = models.ManyToManyField(Author)
$ mypy --no-incremental --strict demo
demo/books/models.py:10: error: Cannot resolve keyword 'name' into field. Choices are: author, author_id, book, book_id, id  [misc]
Found 1 error in 1 file (checked 13 source files)

I pushed the repro case to the same project but different branch here: https://github.com/cuu508/django-stubs-2272/tree/values_list

flaeppe commented 1 month ago

I added a fix for https://github.com/typeddjango/django-stubs/issues/2272#issuecomment-2255065461 in #2288

cuu508 commented 1 month ago

Awesome, just tested this change, and fixes the issue I saw in my project.

HappyCthulhu commented 1 month ago

Hi. I have access to the code that @AlexandrVasilchuk mentioned. I looked into it. Here are the conclusions:

You are right. This problem is hard to reproduce. In general, reverse relations support works in this project. They don't work with one specific model - Proposal. The error appears everywhere there is code trying to get a reverse relation from a model related to Proposal.

The proposals app is included in INSTALLED_APPS of settings.py. We have a ViewSet that takes Proposal.objects.all() as the value of the queryset attribute. Also, when I'm running this code in the django shell:

>>> from django.apps import apps
>>> applications = [app_config for app_config in apps.get_app_configs()]

the proposals app is among the applications. So, it's loaded.

I managed to solve this issue when I added this ready() function in ProposalsConfig (apps.py), which imports the Proposal model:

from django.apps import AppConfig

class ProposalsConfig(AppConfig):
    default_auto_field = "django.db.models.BigAutoField"
    name = "proposals"

    def ready(self) -> None:
        from proposals.models.proposal import Proposal  # noqa: F401

After I added this code, mypy stopped giving me an error.

Do you have any thoughts about this behavior? As far as I understand, the Proposal model isn't imported when type-checking happens. But how is that possible?

flaeppe commented 1 month ago

Is the Proposal model imported into proposals/models/__init__.py?

HappyCthulhu commented 1 month ago

Is the Proposal model imported into proposals/models/__init__.py?

No. I added it for test. Mypy stopped giving me error. Its not mandatory to do this with every model, right? Didnt see this recomendation for django. Also, every other model are not imported to __init__.py file of their subsystem. And they work nicelly with django-stubs

flaeppe commented 1 month ago

AFAIK Django requires every model to be registered to be discoverable via a models module inside an app

I'm going to close now since that fixed the problem