Code

Opened 3 years ago

Closed 19 months ago

Last modified 12 months ago

#14567 closed Bug (fixed)

ModelMultipleChoiceField inconsistently returns a list if empty.

Reported by: melinath Owned by: akaariai
Component: Forms Version: master
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 melinath 3 years ago.
empty_queryset_test.diff (2.1 KB) - added by melinath 3 years ago.
empty_queryset_and_tests.diff (4.8 KB) - added by melinath 3 years ago.
empty_queryset_and_tests_r14864.diff (5.8 KB) - added by melinath 3 years ago.
empty_queryset_and_tests_r16322.diff (5.9 KB) - added by melinath 3 years ago.

Download all attachments as: .zip

Change History (20)

Changed 3 years ago by melinath

comment:1 Changed 3 years ago by melinath

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Version changed from 1.2 to SVN

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

Changed 3 years ago by melinath

comment:2 Changed 3 years ago by dmoisset

  • Triage Stage changed from Unreviewed to Accepted

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

comment:3 Changed 3 years ago by dmoisset

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

Changed 3 years ago by melinath

comment:4 Changed 3 years ago by melinath

  • Owner changed from nobody to melinath
  • Patch needs improvement unset
  • Status changed from new to 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 Changed 3 years ago by melinath

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

Changed 3 years ago by melinath

comment:6 Changed 3 years ago by melinath

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 Changed 3 years ago by melinath

  • 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 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Bug

Changed 3 years ago by melinath

comment:9 Changed 3 years ago by melinath

  • Easy pickings unset

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

comment:10 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:11 Changed 19 months ago by melinath

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 Changed 19 months ago by akaariai

  • Owner changed from melinath to akaariai
  • Status changed from assigned to 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 Changed 19 months ago by Anssi Kääriäinen <akaariai@…>

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

In 218abcc9e550d266a9979e10f562fc21b8f34c6a:

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

comment:14 Changed 19 months ago by akaariai

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

comment:15 Changed 12 months ago by Carl Meyer <carl@…>

In a98465c040319b74b697bace11da5b13cc42427f:

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.