Opened 2 years ago

Closed 2 years ago

#27709 closed Bug (fixed)

ContentTypes.objects.get_for_models returns inconsistent results for proxy models

Reported by: Peter Inglesby Owned by: nobody
Component: contrib.contenttypes Version: master
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

If UserProxy is a proxy model, then the result of calling ContentTypes.objects.get_for_models(UserProxy) depends on whether the content types cache is empty:

>>> ContentType.objects.clear_cache()
>>> ContentType.objects.get_for_models(UserProxy)
{<class 'django.contrib.auth.models.User'>: <ContentType: user>}
>>> ContentType.objects.get_for_models(UserProxy)
{<class 'gallery.models.UserProxy'>: <ContentType: user>}

In particular, on the first call to get_for_models, the key in the results dictionary is the concrete model, while on subsequent calls it is the proxy model.

I believe the correct behaviour is to return the proxy model as the key.

I have two proposed fixes, each with pros and cons. Should I open two separate pull requests?

Change History (6)

comment:1 Changed 2 years ago by Simon Charette

Triage Stage: UnreviewedAccepted
Type: UncategorizedBug
Version: 1.10master

I believe the correct behaviour is to return the proxy model as the key.

The proxy model should only be returned as a key if for_concrete_models=False is specified.

I have two proposed fixes, each with pros and cons. Should I open two separate pull requests?

A single one changing this line to results[opts.concrete_model if for_concrete_models else model] = ct and adding tests to this module should do.

comment:2 Changed 2 years ago by Peter Inglesby

That fix doesn't work, and breaks an existing test.

I've submitted a pull request that changes get_for_models() to use get_for_model(). This simplifies the code a lot, but means there will be one database query for each model that's not in the cache, rather than at most one database query per call to get_for_models(). Given that calls to get_for_model() are cached, I believe this is acceptable.

I have an alternative fix that preserves the existing behaviour with respect to the number of database queries, but makes the code slightly harder to follow.

comment:3 Changed 2 years ago by Peter Inglesby

Has patch: set

comment:4 Changed 2 years ago by Simon Charette

comment:5 Changed 2 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:6 Changed 2 years ago by Simon Charette <charette.s@…>

Resolution: fixed
Status: newclosed

In 4e48cfc:

Fixed #27709 -- Fixed get_for_models() for proxies with an empty cache.

Thanks Peter Inglesby for the report and tests.

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