Make Apps.get_model() return Type[Model]

[RJPercival]

According to Django's documentation, get_model() is guaranteed to return the type of the model (or raise an exception).

Related issues

• Fixes Too broad return value for apps.get_model() #304
• Closes Fix apps.get_model() return type #305

[sobolevn]

Hi, @RJPercival! Thanks!

This PR is marked as a draft, is there a reason for that?

[RJPercival]

I wasn't sure whether the fix would be this simple. I've noticed that @yhoiseth tried to fix this back in 2020 but abandoned their PR. There was a suggestion that plugin support would be required.

[sobolevn]

In this case, let's be double sure by adding a new unit test for this feature 🤔

Do you agree?

[Majsvaffla]

I'm not sure of the state of this PR but it seems that only a test case is missing. This is my first go at a test case in this project so I'm not completely sure if this is enough or correct.

-   case: apps_get_model
    main: |
        from django.apps.registry import apps

        model = apps.get_model("myapp", "MyUser")
        reveal_type(model)  # N: Revealed type is "Type[django.db.models.base.Model]"
    installed_apps:
        - myapp
    files:
        -   path: myapp/__init__.py
        -   path: myapp/models.py
            content: |
                from django.db import models
                class MyUser(models.Model):
                    pass

@RJPercival feel free to include this in your PR or I would be happy to open a PR with your change included.

[adamchainz]

I’ve rebased the PR and added your test @Majsvaffla .

To clarify the past issue discussed in #305 : making get_model() return type[Model] prevents code like the below from Django from working unaltered:

Site = django_apps.get_model("sites.Site")
current_site = Site.objects.get_current()

The get_current() function on Site’s SiteManager is a custom attribute. Using type[Any] as the get_model() return type allows the code to pass type-chceking. Using type[Model] will raise an error about the attribute get_current() not existing.

I’m not sure why the tests failed with this change back in #305, and no longer do. I guess the test suite has changed somewhat, perhaps checking Django’s source less strictly? @sobolevn any ideas? Unfortunately #305 is so old we don't have the logs any more.

With a type[Model] return type, users could use type-narrowing:

from django.contrib.sites.models import Site as RealSite
Site = django_apps.get_model("sites.Site")
assert isinstance(Site, RealSite)
current_site = Site.objects.get_current()

…but much of the time, you use get_model() to avoid importing the real model class. In some situations this may be undesirable.

Plugin support, as suggested in the original PR, would need us to dynamically switch the return type of get_model(), where a literal string is provided.

Overall I think a more sensible path forward is:

1. Set the return type to Any (rather than type[Any]). This would allow the problem in Too broad return value for apps.get_model() #304 to be fixed, because the objects attr access would be allowed, but not type-checked.
2. Open an issue for plugin support, which can be worked on at a later date.

This will be the least disruptive IMO.