[Django] #22838: ModelChoiceField.cache_choices
Django
---------------------------------------+------------------------
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
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 |
---------------------------------------+------------------------
`ModelChoiceField` (and `MultipleModelChoiceField`) take an option
`cache_choices`. This was introduced as a backwards compatibility effort
in February 2007 (ee96c7eb2dea86e5bdaf93f7afa91b6f0128dd72) when the
fields were made to not always cache their choices (#3534).
The option is undocumented, untested and unused within Django.
There are two possible courses of action here.
Firstly, we could simply remove the option. It should have a deprecation
cycle - in particular for people with custom subclasses of
`ModelChoiceField` who pass in the option to the super.
The alternative course of action which I prefer is to harden this into an
actual feature. It already works as follows:
- Create the `ModelChoiceField` with `cache_choices=True`.
- The `ModelChoiceIterator` will cache its results on the
`ModelChoiceField` and use that cache.
What would be needed to make this a fully fledged feature in my opinion:
- An API to clear the cache (`ModelChoiceField.clear_cache()`)
- [Very possibly] Plug it in to the cache framework properly
- Tests
- Documentation
I think this would be a very useful feature, and one which I have had use
cases for often. The most obvious use cases is when working with the same
form multiple times on a page (either as part of a formset or not) the
same query is run for every form. I can call
`MyForm.fields['modelchoicefield'].clear_cache()` after all the forms are
rendered.
The biggest possible issue here is that the iterator is not used until the
form is rendered. We may need to change when the iterator is evaluated for
this case.
I may be able to provide a patch if the feature is accepted.
--
Ticket URL: <https://code.djangoproject.com/ticket/22838>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
--------------------------------------+------------------------------------
Reporter: mjtamlyn | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | 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 mjtamlyn):
* type: New feature => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
On further investigation and discussion with @bmispelon we have discovered
a few things.
The current implementation of `cache_choices` is pretty useless as it does
not share between multiple forms, only between multiple renderings of the
same form object. This is because of how we copy fields when moving from
`form.base_fields` to `form.fields`. As such, `cache_choices` as is should
be removed. Searching on github it appears to be unused "in the wild" and
I'd be inclined to remove it without deprecation - the deprecation cycle
would be tricky.
I'm reclassifying this ticket to just concern the removal of the "dead"
feature `cache_choices`. I'll open a new one with a more detailed
explanation of my motivations for new APIs for `ModelChoiceField`.
--
Ticket URL: <https://code.djangoproject.com/ticket/22838#comment:1>
Django
--------------------------------------+------------------------------------
Reporter: mjtamlyn | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by mjtamlyn):
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/2811
--
Ticket URL: <https://code.djangoproject.com/ticket/22838#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: mjtamlyn | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Forms | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/22838#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: mjtamlyn | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Forms | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by russellm):
This was raised on IRC, so weighing in for posterity: The cache behaviour
implemented by this feature is naive (since it won't be shared between web
servers). Since this isn't documented, and isn't used anywhere, I'm OK
with an expedited deprecation cycle (i.e., 1 release with a visible
warning, then kill it).
There's a reasonable argument for providing caching for query-backed
ModelChoice fields, but it should be using an interface to the cache
backend, not implementing an in-memory cache. However, that's a separate
feature.
--
Ticket URL: <https://code.djangoproject.com/ticket/22838#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: mjtamlyn | Owner: nobody
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Forms | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* status: new => closed
* resolution: => fixed
Comment:
In 2764146586509c2c81ad14da4ac86efa3a949308:
Fixed #22838 -- Deprecated ModelChoiceField.cache_choices.
Undocumented, untested and probably not even useful feature.
--
Ticket URL: <https://code.djangoproject.com/ticket/22838#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: mjtamlyn | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Forms | Version: master
Severity: Normal | Resolution: fixed
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
Comment (by Tim Graham <timograham@…>):
In [changeset:"2788c46d46bc4d7afb906765ce1f484faef180c5"]:
{{{
#!CommitTicketReference repository=""
revision="2788c46d46bc4d7afb906765ce1f484faef180c5"
Removed Multiple/ModelChoiceField cache_choices option; refs #22838.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22838#comment:6>