[Django] #27709: ContentTypes.objects.get_for_models returns inconsistent results for proxy models
Django
models
------------------------------------------------+------------------------
Reporter: Peter Inglesby | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.contenttypes | Version: 1.10
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 |
------------------------------------------------+------------------------
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/27709>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
models
Reporter: Peter Inglesby | Owner: nobody
Component: contrib.contenttypes | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* version: 1.10 => master
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
Comment:
> 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
[https://docs.djangoproject.com/en/1.10/ref/contrib/contenttypes/#django.contrib.contenttypes.models.ContentTypeManager.get_for_models
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
[https://github.com/django/django/blob/ed8c0c941df0f321fb5751db572a9ec773d5118a/django/contrib/contenttypes/models.py#L83
this line] to `results[opts.concrete_model if for_concrete_models else
model] = ct` and adding tests to
[https://github.com/django/django/blob/ed8c0c941df0f321fb5751db572a9ec773d5118a/tests/contenttypes_tests/test_models.py
this module should do].
--
Ticket URL: <https://code.djangoproject.com/ticket/27709#comment:1>
Django
models
Reporter: Peter Inglesby | Owner: nobody
Component: contrib.contenttypes | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27709#comment:2>
Django
models
Reporter: Peter Inglesby | Owner: nobody
Component: contrib.contenttypes | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Peter Inglesby):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27709#comment:3>
Django
models
Reporter: Peter Inglesby | Owner: nobody
Component: contrib.contenttypes | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
[https://github.com/django/django/pull/7842 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/27709#comment:4>
Django
models
Reporter: Peter Inglesby | Owner: nobody
Component: | Version: master
contrib.contenttypes |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/27709#comment:5>
Django
models
Reporter: Peter Inglesby | Owner: nobody
Component: | Version: master
contrib.contenttypes |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette <charette.s@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"4e48cfc108c6cf69052cc728e72dc2cb25a880a5" 4e48cfc]:
{{{
#!CommitTicketReference repository=""
revision="4e48cfc108c6cf69052cc728e72dc2cb25a880a5"
Fixed #27709 -- Fixed get_for_models() for proxies with an empty cache.
Thanks Peter Inglesby for the report and tests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27709#comment:6>