[Django] #27161: TypedChoiceField validation bug
Django
----------------------------+------------------------------
Reporter: th13f | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Keywords: TypedChoiceField
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
----------------------------+------------------------------
Example model:
{{{
from django.contrib.postgres.fields import ArrayField
class ArrayFieldTestModel(models.Model):
CHOICES = map(lambda x: (x, str(x)), range(10))
test_field = ArrayField(models.IntegerField(choices=CHOICES),
blank=True, null=True)
}}}
On form saving we'll got validation error for valid choices:
[[Image(https://snag.gy/DGNVtR.jpg)]]
This is caused by validation ordering:
1. TypedChoiceField clean method called
2. Field clean method called (super)
3. to_python method called, which should transform value to correct, but
it doesn't.
3. TypedChoiceField validate called, which raise ValidationError: no such
choice. He is trying to find string value in integers array and can't do
this.
4. after super clean method, TypedChoiceField is trying to coerce value
(it's too late, error was raised before this).
this can be easily fixed by moving _coerce call into `to_python`.
--
Ticket URL: <https://code.djangoproject.com/ticket/27161>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
------------------------------+----------------------------
Reporter: th13f | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: TypedChoiceField | Triage Stage: Unreviewed
Has patch: 1 | Easy pickings: 0
UI/UX: 0 |
Changes (by th13f):
* Attachment "Running_coerce_in_TypedChoiceField_to_python_method.patch"
added.
patch for ticket
Django
----------------------------------+--------------------------------------
Reporter: th13f | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: TypedChoiceField | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
I've already struggled with this in the past. Did you check if tests are
passing with your proposal?
--
Ticket URL: <https://code.djangoproject.com/ticket/27161#comment:1>
Django
----------------------------------+------------------------------------
Reporter: th13f | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: TypedChoiceField | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* stage: Unreviewed => Accepted
Comment:
I observed one test failure with the patch:
`test_typedchoicefield_special_coerce`.
If it's possible to add a test that's not dependent on `contrib.postgres`,
that would be ideal. Of course, we could add that test too.
--
Ticket URL: <https://code.djangoproject.com/ticket/27161#comment:2>
Django
----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: TypedChoiceField | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
It may be that the proper resolution is #27704. Roman, you might be
interested to test the patch there.
--
Ticket URL: <https://code.djangoproject.com/ticket/27161#comment:3>
Django
Reporter: Roman Karpovich | Owner: Rômulo
| Rosa Furtado
Type: Bug | Status: assigned
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: TypedChoiceField | Triage Stage:
| Unreviewed
Easy pickings: 0 | UI/UX: 0
Changes (by Rômulo Rosa Furtado):
* owner: nobody => Rômulo Rosa Furtado
* status: new => assigned
* stage: Accepted => Unreviewed
--
Ticket URL: <https://code.djangoproject.com/ticket/27161#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Roman Karpovich | Owner: Rômulo
| Rosa Furtado
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: TypedChoiceField | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Rômulo Rosa Furtado):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/27161#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Roman Karpovich | Owner: Rômulo
| Rosa Furtado
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: TypedChoiceField | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by Jeroen Dekkers):
* cc: jeroen@… (added)
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
Comment:
The problem with moving `_coerce` into `to_python` is that you can coerce
to a value not present in `choices` and `clean` calls `to_python` before
checking whether the value is in `choices`. I think the best way to fix
this is having SimpleArrayField's `to_python` call `clean` on each array
item to get the coerced value instead of calling `to_python`. Pull request
with that fix and test case: https://github.com/django/django/pull/8358
--
Ticket URL: <https://code.djangoproject.com/ticket/27161#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Roman Karpovich | Owner: Rômulo
| Rosa Furtado
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: TypedChoiceField | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27161#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Roman Karpovich | Owner: Rômulo
| Rosa Furtado
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: TypedChoiceField | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 1 => 0
Comment:
An alternate [https://github.com/django/django/pull/8311 PR] adds
`ArrayField.clean()` that calls `base_field.clean()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/27161#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Roman Karpovich | Owner: Rômulo
| Rosa Furtado
Component: Forms | Version: master
Severity: Normal | Resolution: fixed
Keywords: TypedChoiceField | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"9dd244394236388c3479ab202a0ec31055f7ec09" 9dd2443]:
{{{
#!CommitTicketReference repository=""
revision="9dd244394236388c3479ab202a0ec31055f7ec09"
Fixed #27161 -- Fixed form validation when an ArrayField's base_field has
choices.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27161#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Roman Karpovich | Owner: Rômulo
| Rosa Furtado
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: TypedChoiceField | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by José Jorge Lorenzo Vila):
Any plan to port this to django 1.X? Thanks
--
Ticket URL: <https://code.djangoproject.com/ticket/27161#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Roman Karpovich | Owner: Rômulo
| Rosa Furtado
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: TypedChoiceField | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Tim Graham):
No. Per our [https://docs.djangoproject.com/en/dev/internals/release-
process/#supported-versions supported versions policy], 1.11 is only
receiving data loss and security fixes.
--
Ticket URL: <https://code.djangoproject.com/ticket/27161#comment:11>