[Django] #33174: Having a model inherit from Generic[T] breaks makemigrations
Django
------------------------------------------+------------------------
Reporter: Antoine Humeau | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 3.2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------+------------------------
Here is a simple example that can help me explain the issue (and maybe
show you why this might be a valid usecase):
{{{
#!python
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:
-
[https://github.com/django/django/blob/4540e976d4e941166fbd2d3f1df1853f7e348740/django/db/migrations/state.py#L396
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
[https://github.com/django/django/blob/4540e976d4e941166fbd2d3f1df1853f7e348740/django/db/migrations/state.py#L551
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/33174>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Antoine Humeau | Owner: nobody
Component: Uncategorized | Version: 3.2
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => needsinfo
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33174#comment:1>
Django
Reporter: Antoine Humeau | Owner: nobody
Component: Migrations | Version: 3.2
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* cc: Adam Johnson (added)
* type: Uncategorized => New feature
* component: Uncategorized => Migrations
--
Ticket URL: <https://code.djangoproject.com/ticket/33174#comment:2>
Django
Reporter: Antoine Humeau | Owner: nobody
Component: Migrations | Version: 3.2
Severity: Normal | Resolution: needsinfo
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33174#comment:3>
Django
Reporter: Antoine Humeau | Owner: nobody
Component: Migrations | Version: 3.2
Severity: Normal | Resolution: needsinfo
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33174#comment:4>
Django
Reporter: Antoine Humeau | Owner: nobody
Component: Migrations | Version: 3.2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* resolution: needsinfo => wontfix
--
Ticket URL: <https://code.djangoproject.com/ticket/33174#comment:5>
Django
Reporter: Antoine Humeau | Owner: nobody
Component: Migrations | Version: 3.2
Severity: Normal | Resolution: wontfix
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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
just fine, though.
--
Ticket URL: <https://code.djangoproject.com/ticket/33174#comment:6>
Django
Reporter: Antoine Humeau | Owner: nobody
Component: Migrations | Version: 3.2
Severity: Normal | Resolution: wontfix
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33174#comment:7>
Django
Component: Migrations | Version: 3.2
Severity: Normal | Resolution: wontfix
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Alexandr Artemyev):
* cc: Alexandr Artemyev (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33174#comment:8>
Django
Component: Migrations | Version: 3.2
Severity: Normal | Resolution: wontfix
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by HarryKane):
* cc: HarryKane (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33174#comment:9>
Django
Component: Migrations | Version: 3.2
Severity: Normal | Resolution: wontfix
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by elonzh):
* cc: elonzh (added)
Comment:
So will this ticket be reopened?
--
Ticket URL: <https://code.djangoproject.com/ticket/33174#comment:10>
Django
Severity: Normal | Resolution:
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* resolution: wontfix =>
* stage: Unreviewed => Accepted
* status: closed => new
Comment:
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 [https://lp.jetbrains.com/django-developer-
survey-2023 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".
--
Ticket URL: <https://code.djangoproject.com/ticket/33174#comment:11>
Django
Component: Migrations | Version: 3.2
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* stage: Accepted => Unreviewed
Comment:
Cross-posted to forum
--
Ticket URL: <https://code.djangoproject.com/ticket/33174#comment:12>
Django
Component: Migrations | Version: 3.2
Severity: Normal | Resolution:
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Sarah Boyce):
* stage: Unreviewed => Accepted
I agree with the decision to reopen the ticket Jacob. Thank you for also
raising a forum discussion 👍
--
Ticket URL: <https://code.djangoproject.com/ticket/33174#comment:13>
Django
Reporter: Antoine Humeau | Owner: Thibaut
| Decombe
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Thibaut Decombe):
* owner: nobody => Thibaut Decombe
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33174#comment:14>
Django
Reporter: Antoine Humeau | Owner: Thibaut
| Decombe
Type: New feature | Status: assigned
Component: Migrations | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by Thibaut Decombe):
--
Ticket URL: <https://code.djangoproject.com/ticket/33174#comment:15>
Django
Reporter: Antoine Humeau | Owner: Thibaut
| Decombe
Type: New feature | Status: assigned
Component: Migrations | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Changes (by Jacob Walls):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33174#comment:16>
Django
Reporter: Antoine Humeau | Owner: Thibaut
| Decombe
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* stage: Accepted => Ready for checkin
* type: New feature => Bug
Comment:
Release note seems unnecessary, it's more a bugfix in the post PEP 695
world.
--
Ticket URL: <https://code.djangoproject.com/ticket/33174#comment:17>
Django
Reporter: Antoine Humeau | Owner: Thibaut
| Decombe
Component: Migrations | Version: 3.2
Severity: Normal | Resolution: fixed
| checkin
Easy pickings: 0 | UI/UX: 0
Changes (by Sarah Boyce <42296566+sarahboyce@…>):
* resolution: => fixed
* status: assigned => closed
Comment:
In [changeset:"825ddda26a14847c30522f4d1112fb506123420d" 825ddda]:
{{{#!CommitTicketReference repository=""
revision="825ddda26a14847c30522f4d1112fb506123420d"
Fixed #33174 -- Fixed migrations crash for model inheriting from
Generic[T].
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33174#comment:18>