Opened 12 months ago

Closed 11 months ago

Last modified 4 months ago

#22838 closed Cleanup/optimization (fixed)

Deprecate ModelChoiceField.cache_choices

Reported by: mjtamlyn Owned by: nobody
Component: Forms 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

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.

Change History (6)

comment:1 Changed 12 months ago by mjtamlyn

  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from New feature to Cleanup/optimization

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.

comment:3 Changed 12 months ago by timo

  • Summary changed from ModelChoiceField.cache_choices to Deprecate ModelChoiceField.cache_choices
  • Triage Stage changed from Accepted to Ready for checkin

comment:4 Changed 12 months ago 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.

comment:5 Changed 11 months ago by timo

  • Resolution set to fixed
  • Status changed from new to closed

In 2764146586509c2c81ad14da4ac86efa3a949308:

Fixed #22838 -- Deprecated ModelChoiceField.cache_choices.

Undocumented, untested and probably not even useful feature.

comment:6 Changed 4 months ago by Tim Graham <timograham@…>

In 2788c46d46bc4d7afb906765ce1f484faef180c5:

Removed Multiple/ModelChoiceField cache_choices option; refs #22838.

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