#14567 closed Bug (fixed)
ModelMultipleChoiceField inconsistently returns a list if empty.
| Reported by: | Stephen Burrows | Owned by: | Anssi Kääriäinen |
|---|---|---|---|
| Component: | Forms | Version: | dev |
| Severity: | Normal | Keywords: | |
| Cc: | stephen.r.burrows@… | Triage Stage: | Accepted |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
The ModelMultipleChoiceField is described as "A MultipleChoiceField whose choices are a model QuerySet." If some models have been selected, then the cleaned value of this field is a queryset. However, if the field is not required and the value is empty, the cleaned value returned is a list.
This is inconsistent and causes problems if, say, you want to check that the value passed to a method is a queryset and, if so, access queryset.model. The attached patch replaces [] with self.queryset.none(). I'll work on tests tomorrow if nobody beats me to it.
Attachments (5)
Change History (20)
by , 15 years ago
| Attachment: | queryset_none.patch added |
|---|
comment:1 by , 15 years ago
| Version: | 1.2 → SVN |
|---|
by , 15 years ago
| Attachment: | empty_queryset_test.diff added |
|---|
comment:2 by , 15 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
Looks legitimate... taking a look at the patch to see if it's ready for checkin
comment:3 by , 15 years ago
| Patch needs improvement: | set |
|---|
I have two tests failures after applying the patch (which weren't there before):
====================================================================== FAIL: test_callable_initial_value (regressiontests.forms.models.ModelFormCallableModelDefault) The initial value for a callable default returning a queryset is the pk (refs #13769) ---------------------------------------------------------------------- ====================================================================== FAIL: test_initial_instance_value (regressiontests.forms.models.ModelFormCallableModelDefault) Initial instances for model fields may also be instances (refs #7287) ----------------------------------------------------------------------
by , 15 years ago
| Attachment: | empty_queryset_and_tests.diff added |
|---|
comment:4 by , 15 years ago
| Owner: | changed from to |
|---|---|
| Patch needs improvement: | unset |
| Status: | new → assigned |
empty_queryset_and_tests.diff corrects the failures by accounting for the changes to the forms rendered in test_callable_initial_value and test_initial_instance_value caused by the additional field on ChoiceFieldModel which was introduced in my previous patch. I've run the regression test suite this time and have no unexpected failures. I can continue working on this if there's anything else to do for it.
comment:5 by , 15 years ago
Updated the patch to r14864 to account for changes in file structure. The full test suite passes.
by , 15 years ago
| Attachment: | empty_queryset_and_tests_r14864.diff added |
|---|
comment:6 by , 15 years ago
Updated empty_queryset_and_tests_r14864.diff to include necessary documentation changes. Note that as things stand, the documentation is already incorrect: it claims that ModelMultipleChoiceField normalizes to a list, when in fact it normalizes to a QuerySet.
comment:7 by , 15 years ago
| Cc: | added |
|---|
Added git branch for this ticket @ http://github.com/melinath/django/tree/ticket14567. The patch is still current as of r15353.
comment:8 by , 14 years ago
| Severity: | → Normal |
|---|---|
| Type: | → Bug |
by , 14 years ago
| Attachment: | empty_queryset_and_tests_r16322.diff added |
|---|
comment:9 by , 14 years ago
| Easy pickings: | unset |
|---|
Incidentally, I don't get any extra test failures with the patch.
comment:11 by , 13 years ago
I've updated the patch to apply to the latest develop and opened a pull request: https://github.com/django/django/pull/398.
comment:12 by , 13 years ago
| Owner: | changed from to |
|---|---|
| Status: | assigned → new |
To me it seems this needs a minor note as a backwards incompatible change. In any case, this is a change we should do. The return value should always be of consistent type (that is, a queryset). The docs aren't exactly accurate currently anyways as of the return value.
I will take care of committing this.
comment:13 by , 13 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
comment:14 by , 13 years ago
Committed with some docs additions and some changes to the tests to avoid even larger HTML output comparisons.
The test patch is off SVN r14358, so switched the version.