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)

26977-test.diff (1.5 KB ) - added by Tim Graham 8 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 by Tim Graham, 8 years ago

Cc: Simon Charette added
Component: UncategorizedDatabase layer (models, ORM)
Summary: TypeError: isinstance() arg 2 must be a class, type, or tuple of classes and typesInstantiating 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: UnreviewedAccepted
Type: Cleanup/optimizationBug

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.

by Tim Graham, 8 years ago

Attachment: 26977-test.diff added

comment:2 by Simon Charette, 8 years ago

Component: Database layer (models, ORM)Documentation
Needs documentation: set
Type: BugCleanup/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.

comment:3 by Tim Graham, 8 years ago

How about raising a helpful message saying that abstract models can't be instantiated?

in reply to:  3 comment:4 by Simon Charette, 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 Ien Cheng, 6 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.)

Last edited 6 years ago by Ien Cheng (previous) (diff)

comment:6 by Jacob Walls, 4 years ago

Easy pickings: set
Has patch: set
Owner: changed from nobody to Jacob Walls
Status: newassigned

PR

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 Jacob Walls, 4 years ago

Needs documentation: unset

comment:8 by Jacob Walls, 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 Mariusz Felisiak, 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 Mariusz Felisiak, 4 years ago

Component: DocumentationDatabase layer (models, ORM)
Triage Stage: AcceptedReady for checkin

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In c7e7f17:

Fixed #26977 -- Made abstract models raise TypeError when instantiating.

Note: See TracTickets for help on using tickets.
Back to Top