Opened 13 years ago

Closed 11 years ago

Last modified 11 years ago

#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)

queryset_none.patch (566 bytes ) - added by Stephen Burrows 13 years ago.
empty_queryset_test.diff (2.1 KB ) - added by Stephen Burrows 13 years ago.
empty_queryset_and_tests.diff (4.8 KB ) - added by Stephen Burrows 13 years ago.
empty_queryset_and_tests_r14864.diff (5.8 KB ) - added by Stephen Burrows 13 years ago.
empty_queryset_and_tests_r16322.diff (5.9 KB ) - added by Stephen Burrows 13 years ago.

Download all attachments as: .zip

Change History (20)

by Stephen Burrows, 13 years ago

Attachment: queryset_none.patch added

comment:1 by Stephen Burrows, 13 years ago

Version: 1.2SVN

The test patch is off SVN r14358, so switched the version.

by Stephen Burrows, 13 years ago

Attachment: empty_queryset_test.diff added

comment:2 by Daniel F Moisset, 13 years ago

Triage Stage: UnreviewedAccepted

Looks legitimate... taking a look at the patch to see if it's ready for checkin

comment:3 by Daniel F Moisset, 13 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 Stephen Burrows, 13 years ago

comment:4 by Stephen Burrows, 13 years ago

Owner: changed from nobody to Stephen Burrows
Patch needs improvement: unset
Status: newassigned

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 Stephen Burrows, 13 years ago

Updated the patch to r14864 to account for changes in file structure. The full test suite passes.

by Stephen Burrows, 13 years ago

comment:6 by Stephen Burrows, 13 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 Stephen Burrows, 13 years ago

Cc: stephen.r.burrows@… 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 Julien Phalip, 13 years ago

Severity: Normal
Type: Bug

by Stephen Burrows, 13 years ago

comment:9 by Stephen Burrows, 13 years ago

Easy pickings: unset

Incidentally, I don't get any extra test failures with the patch.

comment:10 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:11 by Stephen Burrows, 11 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 Anssi Kääriäinen, 11 years ago

Owner: changed from Stephen Burrows to Anssi Kääriäinen
Status: assignednew

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 Anssi Kääriäinen <akaariai@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 218abcc9e550d266a9979e10f562fc21b8f34c6a:

Fixed #14567 -- Made ModelMultipleChoiceField return EmptyQuerySet as empty value

comment:14 by Anssi Kääriäinen, 11 years ago

Committed with some docs additions and some changes to the tests to avoid even larger HTML output comparisons.

comment:15 by Carl Meyer <carl@…>, 11 years ago

In a98465c040319b74b697bace11da5b13cc42427f:

Refs #14567 -- Fixed failing test that wasn't being run.

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