[Django] #34148: Removing a field from form.fields previously added to _bound_fields_cache has no effect
Django
has no effect
--------------------------------------------------+------------------------
Reporter: Jan Pieter Waagmeester | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 4.0
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 |
--------------------------------------------------+------------------------
The fix for issue "`BaseForm.__getitem__()` does unneeded work in the
happy path" (#32901) added to main in
https://github.com/django/django/commit/edde2a069929c93e37835dc3f7c9a229040058e2,
A shortcut was added to retrieve bound fields from
`form._bound_fields_cache`.
If a form removes a field after the cache entry for that field was
created, the field can be still retrieved from the cache, even though the
field is not in `form.fields` anymore:
{{{
import django
from django import forms
from django.test import SimpleTestCase
class Form(forms.Form):
action = forms.CharField()
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
# The form was iterated (in my case, separating basic and advanced
fields)
[field for field in self if getattr(field, 'advanced', False)]
# After that, we remove a field (i.e. because it is not relevant
for admins)
del self.fields["action"]
class FormTestCase(SimpleTestCase):
def test_form(self):
"""
The field should be removed from the instance,
but it can be still retrieved from self._bound_fields_cache.
"""
print("django version", django.VERSION)
form = Form()
with self.assertRaises(KeyError):
form["action"]
}}}
This succeeds with `django==3.2`:
{{{
pip install django==3.2.16
...
./manage.py test tests
System check identified no issues (0 silenced).
django version (3, 2, 16, 'final', 0)
.
----------------------------------------------------------------------
Ran 1 test in 0.000s
OK
}}}
But fails with `django==4.0.8` (and `django==4.1.3`):
{{{
pip install django==4.0.8
...
./manage test tests
Found 1 test(s).
System check identified no issues (0 silenced).
django version (4, 0, 8, 'final', 0)
F
======================================================================
FAIL: test_form (test_forms.FormTestCase)
The field should be removed from the instance,
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/jieter/workspace/django-test/tests/test_forms.py", line 27,
in test_form
form["action"]
AssertionError: KeyError not raised
----------------------------------------------------------------------
Ran 1 test in 0.000s
FAILED (failures=1)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34148>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
has no effect
Reporter: Jan Pieter | Owner: nobody
Waagmeester |
Type: Bug | Status: new
Component: Forms | Version: 4.0
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 Carlton Gibson):
* cc: Chris Jerdonek (added)
* stage: Unreviewed => Accepted
Comment:
Thanks for the report.
Looks like a bug yes. I think likely we just need to revert
edde2a069929c93e37835dc3f7c9a229040058e2. 🤔
--
Ticket URL: <https://code.djangoproject.com/ticket/34148#comment:1>
Django
has no effect
-------------------------------------+-------------------------------------
Waagmeester | Panico
Type: Bug | Status: assigned
Component: Forms | Version: 4.0
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
-------------------------------------+-------------------------------------
* owner: nobody => Francesco Panico
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34148#comment:2>
Django
has no effect
-------------------------------------+-------------------------------------
Reporter: Jan Pieter | Owner: Francesco
Waagmeester | Panico
Type: Bug | Status: assigned
Component: Forms | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Francesco Panico):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/16306 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/34148#comment:3>
Django
has no effect
-------------------------------------+-------------------------------------
Reporter: Jan Pieter | Owner: Francesco
Waagmeester | Panico
Type: Bug | Status: assigned
Component: Forms | Version: 4.0
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/34148#comment:4>
Django
has no effect
-------------------------------------+-------------------------------------
Reporter: Jan Pieter | Owner: Francesco
Waagmeester | Panico
Component: Forms | Version: 4.0
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
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"51faf4bd172cd4cb219a9793facbfa00246c9f3c" 51faf4bd]:
{{{
#!CommitTicketReference repository=""
revision="51faf4bd172cd4cb219a9793facbfa00246c9f3c"
Fixed #34148 -- Reverted "Fixed #32901 -- Optimized
BaseForm.__getitem__()."
This reverts commit edde2a069929c93e37835dc3f7c9a229040058e2.
Thanks Jan Pieter Waagmeester for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34148#comment:5>