Having a model inherit from Generic[T] breaks makemigrations

[Antoine Humeau]

Here is a simple example that can help me explain the issue (and maybe show you why this might be a valid usecase):

import typing
from django.db import models
import stripe
from stripe.stripe_object import StripeObject


StripeClassT = typing.TypeVar('StripeClassT', bound=StripeObject)


class StripeObjectModel(typing.Generic[StripeClassT], models.Model):
    stripe_class: type[StripeClassT]

    id = models.TextField(primary_key=True)

    class Meta:
        abstract = True

    def api_retrieve(self) -> StripeClassT:
        return self.stripe_class.retrieve(self.id)
   

class Customer(StripeObjectModel):
    stripe_class = stripe.Customer


class Source(StripeObjectModel):
    stripe_class = stripe.Source
...

Running makemigrations will result in the following traceback:

Traceback (most recent call last):
  File "...django/core/management/__init__.py", line 419, in execute_from_command_line
    utility.execute()
  File "...django/core/management/__init__.py", line 413, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "...django/core/management/base.py", line 354, in run_from_argv
    self.execute(*args, **cmd_options)
  File "...django/core/management/base.py", line 398, in execute
    output = self.handle(*args, **options)
  File "...django/core/management/base.py", line 89, in wrapped
    res = handle_func(*args, **kwargs)
  File "...django/core/management/commands/makemigrations.py", line 172, in handle
    changes = autodetector.changes(
  File "...django/db/migrations/autodetector.py", line 41, in changes
    changes = self._detect_changes(convert_apps, graph)
  File "...django/db/migrations/autodetector.py", line 127, in _detect_changes
    self.new_apps = self.to_state.apps
  File "...django/utils/functional.py", line 48, in __get__
    res = instance.__dict__[self.name] = self.func(instance)
  File "...django/db/migrations/state.py", line 208, in apps
    return StateApps(self.real_apps, self.models)
  File "...django/db/migrations/state.py", line 270, in __init__
    self.render_multiple([*models.values(), *self.real_models])
  File "...django/db/migrations/state.py", line 305, in render_multiple
    model.render(self)
  File "...django/db/migrations/state.py", line 572, in render
    return type(self.name, bases, body)
  File "...django/db/models/base.py", line 99, in __new__
    new_class = super_new(cls, name, bases, new_attrs, **kwargs)
  File "/usr/lib/python3.9/typing.py", line 1010, in __init_subclass__
    raise TypeError("Cannot inherit from plain Generic")
TypeError: Cannot inherit from plain Generic

I have tracked down the issue to the following chain of events:

• ​ModelState.from_model is called with Customer as argument
• It recursively builds a list of base classes for Customer from Customer.__bases__ and uses it to return a ModelState instance
• This base class list will contain typing.Generic – note: not typing.Generic[StripeClassT] – because it is present in StripeObjectModel.__bases__
• The ModelState instance's ​render method gets called
• An exception is raised in when calling type(self.name, bases, body) because subclassing unparameterized typing.Generic is not allowed

A simple solution might be to filter out typing.Generic from bases in ModelState.from_model.

[Carlton Gibson]

I’m inclined to say we should mark this as needsinfo.

A simple solution might be to filter out typing.Generic from bases in ModelState.from_model.

Possibly. But we very much need to measure several times before cutting here: if we introduce the wrong fix, which is easily imaginable with these typing questions that are still very much in flux, then we’ll be dealing with a potentially worse breaking change down the line.

This base class list will contain typing.Generic – note: not typing.Generic[StripeClassT] – because it is present in StripeObjectModel.bases

First of all I think we need a story for this (from Python?)

Then a concrete analysis of what we need to handle in Django, and how.

In any case I think this needs to go via the DevelopersMailingList before any steps can be agreed, since it touches on the Technical Board’s previous decision to not (currently) support type annotations in Django.

[Adam Johnson]

typing itself provides a workaround here - Generic is only needed for type checking, so use TYPE_CHECKING as in the third pattern documented here: ​https://mypy.readthedocs.io/en/stable/runtime_troubles.html#using-classes-that-are-generic-in-stubs-but-not-at-runtime

For example:

import typing
from django.db import models
import stripe
from stripe.stripe_object import StripeObject


StripeClassT = typing.TypeVar('StripeClassT', bound=StripeObject)

if typing.TYPE_CHECKING:
    class GenericBase(typing.Generic[StripeClassT]):
        pass

else:
    class GenericBase:
        pass

class StripeObjectModel(GenericBase, models.Model):
    ...

I don't think there's anything Django should do here at the moment. Typing is, as Carlton says, very much in flux. Plus Mypy + typing provide clear, if verbose, workarounds for most situations.

[Antoine Humeau]

Indeed the workaround is the better solution.

For future reference, I had to adapt it to:

if TYPE_CHECKING:
    class GenericBase(typing.Generic[StripeClassT]):
        pass
else:
    class GenericBase:
        def __class_getitem__(cls, _):
            return cls


class StripeObjectModel(GenericBase[StripeClassT], models.Model):
    ...

That is because mypy needs the type parameter in the base classes definition to bind it to the class.

Thanks a lot for your answers, I am sorry I should have checked for a workaround more thoroughly before posting.

[Jacob Fredericksen]

I think this should be revisited. Python 3.12 introduces new syntax for generic classes without explicit inheritance from typing.Generic (​https://docs.python.org/3/whatsnew/3.12.html#pep-695-type-parameter-syntax), so having to trick Django by adding verbose TYPE_CHECKING conditionals and explicit Generic inheritance is a bummer. Removing typing.Generic from the list of bases in the migration file seems to work well, though.

[Adam Johnson]

I would be inclined to agree. Have you looked into what it would take to support Generic, Jacob? I would hope the patch is pretty small.

[elonzh]

So will this ticket be reopened?

[Jacob Walls]

I'll venture to reopen at this point. Re: comment:1,

• We do have an updated story from Python, as of PEP 695
• echoing comment:7 the hope is that a small patch is possible
• the decision on annotations was re: django's source, versus here we have a user's valid python class
• 2023 django developers ​survey shows 70% of users use or plan to use type annotations

Happy to continue on forum if I've overstepped, but I think in the intervening time this has crossed over into "Django shouldn't crash when using recent major Python features".

[Jacob Walls]

Cross-posted to ​forum

[Sarah Boyce]

I agree with the decision to reopen the ticket Jacob. Thank you for also raising a forum discussion 👍