Opened 8 years ago
Closed 4 years ago
#26977 closed Cleanup/optimization (fixed)
Instantiating an abstract model with a string ForeignKey fails with TypeError: isinstance() arg 2 must be a class, type, or tuple of classes and types
Reported by: | Jonas Trappenberg | Owned by: | Jacob Walls |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.9 |
Severity: | Normal | Keywords: | |
Cc: | Simon Charette | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
Trying to instantiate an abstract model with a foreign key worked without throwing any warnings in Django 1.8. In Django 1.9.8, this code:
class UserProfile(Model): user = models.ForeignKey('auth.user') class Meta(object): app_label = 'core' abstract = True UserProfile()
raises this exception:
Traceback (most recent call last): File "<ipython-input-7-5fa4dfdf1ad9>", line 2, in <module> UserProfile() File ".../.venv/lib/python2.7/site-packages/django/db/models/base.py", line 432, in __init__ val = field.get_default() File ".../.venv/lib/python2.7/site-packages/django/db/models/fields/related.py", line 905, in get_default if isinstance(field_default, self.remote_field.model): TypeError: isinstance() arg 2 must be a class, type, or tuple of classes and types
While the code mentions that abstract models can not be instantiated [1], I couldn't find mention of this in the docs. Abstract models without foreign keys can still be instantiated perfectly fine.
I couldn't find any description of this change in the Release Notes, so not sure if I missed it there or if this is maybe an undocumented backwards-incompatible change.
[1] https://github.com/django/django/blob/026574e03c6b6fd20a45f97b0470afb70e41fda4/django/db/models/base.py#L284
Attachments (1)
Change History (12)
comment:1 by , 8 years ago
Cc: | added |
---|---|
Component: | Uncategorized → Database layer (models, ORM) |
Summary: | TypeError: isinstance() arg 2 must be a class, type, or tuple of classes and types → Instantiating an abstract model with a string ForeignKey fails with TypeError: isinstance() arg 2 must be a class, type, or tuple of classes and types |
Triage Stage: | Unreviewed → Accepted |
Type: | Cleanup/optimization → Bug |
by , 8 years ago
Attachment: | 26977-test.diff added |
---|
comment:2 by , 8 years ago
Component: | Database layer (models, ORM) → Documentation |
---|---|
Needs documentation: | set |
Type: | Bug → Cleanup/optimization |
While it's not explicitly mentioned abstract models are not instantiable in the documentation there's a mention that such models are never meant to be used in isolation.
I would argue that even if it worked in Django < 1.8 it was undefined behavior and this breakage shouldn't be considered as a break of backward compatiblity. Even if we were to fix the get_default()
case abstract models with lazily defined relationships (by passing model names instead of model classes) are now completely unusable as they're not resolved anymore.
Since the issue can be worked around by avoiding lazily defined relationships on abstract models and the reported use case was never meant to be supported I suggest we amend the documentation to be more explicit.
follow-up: 4 comment:3 by , 8 years ago
How about raising a helpful message saying that abstract models can't be instantiated?
comment:4 by , 8 years ago
Replying to timgraham:
How about raising a helpful message saying that abstract models can't be instantiated?
Raising a TypeError
with an helpful message in Django 1.11+ could be useful but I don't think it should be backported at this point.
comment:5 by , 7 years ago
Wouldn't doing unit tests on a abstract model class be an appropriate use case for instantiating an abstract class? If not, what is the recommended approach to unit testing abstract model logic? (This is an actual situation in upgrading a project from 1.8 to 1.9. Unit tests for an abstract model class, used as a mixin, broke due to the change noted in this ticket.)
comment:6 by , 4 years ago
Easy pickings: | set |
---|---|
Has patch: | set |
Owner: | changed from | to
Status: | new → assigned |
The documentation for Abstract base classes does mention that abstract models cannot be instantiated, but it comes in the discussion of the example CommonInfo
. With the feedback provided by the TypeError
raised in this PR, my initial thought is that this is sufficient documentation.
If folks want the prohibition on instantiation moved earlier, up above the example, it could be easily done.
comment:7 by , 4 years ago
Needs documentation: | unset |
---|
comment:8 by , 4 years ago
Thinking more on this, if folks decide to add extra documentation or move documentation (that abstract models should not be instantiated) to a more discoverable place, that is probably something to be considered for backport, if it's worth doing at all. In that case that would be a separate PR/ticket, no? The change in this PR won't be backported.
comment:9 by , 4 years ago
It's stated in docs that abstract models cannot be instantiated, IMO there is no need for an extra clarification.
comment:10 by , 4 years ago
Component: | Documentation → Database layer (models, ORM) |
---|---|
Triage Stage: | Accepted → Ready for checkin |
I'm not sure about the correct resolution, but I bisected the change to 9239f1dda7b94f53d21efb8b5e4d056e24f4e906 and am attaching a reproduction test case for Django's test suite.