Opened 5 years ago

Closed 3 years ago

#31357 closed Bug (fixed)

Stale content types can cause exception in ContentType.get_for_models

Reported by: Andy Chosak Owned by: Biel Frontera
Component: contrib.contenttypes Version: 3.0
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

ContentType.get_for_models (source) retrieves content types from the database for a given list of models. Due to the way this method works, it's possible to trigger an unexpected AttributeError by passing in a list of models whose (app_label, model_name) pairs can be permuted to refer to a model with a stale content type in the database.

Consider this example:

  1. A project has an app named foo with models named Author and Book. Content types for these exist in the database.
  2. A new app named bar is created providing an alternate Book model. The content type for this also exists.
  3. The model foo.Book is deleted. The database is migrated, but the stale content type is left behind.

The following code will raise an exception:

>> from foo.models import Author
>>> from bar.models import Book
>>> from django.contrib.contenttypes.models import ContentType
>>> ContentType.objects.get_for_models(Author, Book)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/path/to/myproject/lib/python3.6/site-packages/django/contrib/contenttypes/models.py", line 89, in get_for_models
    opts_models = needed_opts.pop(ct.model_class()._meta, [])
AttributeError: 'NoneType' object has no attribute '_meta'

This occurs because the ct.model_class() call returns None for stale content types.

The problem is in this database lookup that tries to pull back content types for the models that were passed in. Instead of pulling back only the ContentType instances for those models, it filters by app_label__in=needed_app_labels, model__in=needed_models. For the above case, this pulls back not just (foo, Author) and (bar, Book) but also (foo, Book), which in this case is stale.

A fix here could either alter the query to be more specific or check the returned content types to make sure they correspond to one of the desired models.

Change History (7)

comment:1 by Simon Charette, 5 years ago

Triage Stage: UnreviewedAccepted

The proper way of performing the query is likely to group models by app_label and filter by unions of Q(app_label=app_label, model__in=app_models) for each of them.

That will likely allow the database to keep using the unique index on (app_label, model).

comment:2 by Amartya Gaur, 5 years ago

Owner: changed from nobody to Amartya Gaur
Status: newassigned

comment:3 by Biel Frontera, 3 years ago

Owner: changed from Amartya Gaur to Biel Frontera

comment:4 by Biel Frontera, 3 years ago

Has patch: set

PR sent to fix this issue: https://github.com/django/django/pull/15508

I think there is no need to change the query as long unwanted content types are discarded.

comment:5 by Mariusz Felisiak, 3 years ago

Patch needs improvement: set

comment:6 by Mariusz Felisiak, 3 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:7 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 859a87d8:

Fixed #31357 -- Fixed get_for_models() crash for stale content types when model with the same name exists in another app.

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