Opened 5 years ago

Last modified 5 years ago

#25980 new Bug

Disabled ModelMultipleChoiceField can't handle querysets as an initial value

Reported by: karyon Owned by: nobody
Component: Forms Version: 1.9
Severity: Normal Keywords:
Cc: johannes.linke@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by karyon)

So i provided a queryset as initial value for a non-required and disabled ModelMultipleChoiceField. it seemed to work as i saw the correct values in the rendered output.

but, when saving the form i got an error Enter a list of values.

  1. this happens because disabled fields send their initial value to cleaning, see [here. in case of model fields, it looks like this must be a list of PKs, which is quite unhandy to build (something like list(queryset.values_list('pk', flat=True)))
  2. the error did not show up when the queryset i provided yielded no results, which is inconsistent. the reason for that is that this boolean expression evaluates to true as soon as there are results in the queryset.
  1. something else appeared fishy to me: in one line the clean method returns a queryset (self.queryset.none()), but in the next line it raises an error if the value is neither a list nor a tuple.

if you folks tell me what the intended behaviour is / which types should be used, i can try to create a fix if you want.

  1. and the last (slightly unrelated) issue: since the field was disabled, i was wondering why cleaning it doesn't take a shortcut and does nothing at all... bumped into this a few times.

Change History (10)

comment:1 Changed 5 years ago by karyon

Cc: johannes.linke@… added

comment:2 Changed 5 years ago by karyon

Description: modified (diff)

comment:3 Changed 5 years ago by karyon

Description: modified (diff)
Summary: Inconsistent error behaviour when providing a queryset as initial value on ModelMultipleChoiceFieldWrong handling of initial value on disabled ModelMultipleChoiceField

comment:4 Changed 5 years ago by Tim Graham

Could you please provide a sample project or a test for Django's test suite that reproduces the issue? Thanks!

comment:5 Changed 5 years ago by karyon

i can try creating a test if you tell me what the expected behaviour is. so, 1. are querysets allowed as initial value or just lists and tuples? 2. should there be a difference between empty and non-empty querysets/tuples? and 3. should the clean method return a queryset or a list/tuple?

comment:6 Changed 5 years ago by Tim Graham

I can't give answers yet as I don't have a good understanding of the issue just from reading the ticket description. That's why I requested some code I can play with.

comment:7 Changed 5 years ago by karyon

Here is a PR that adds a failing test that verifies that the ModelMultipleChoiceField can handle querysets when cleaning, because this is what happens when the field is disabled and has a queryset as initial value, see code. I still don't know whether this properly reflects the intended behaviour.

The test might be "too small" to properly explain my problem, as it tests only the field, not the form... let me know if i should add another test.

Last edited 5 years ago by karyon (previous) (diff)

comment:8 Changed 5 years ago by Tim Graham

  1. As I noted on the pull request the documentation says, "Validates that every id in the given list of values exists in the queryset." so my understanding is that you should provide a list of ids for the initial values instead of a queryset.
  1. I don't think it's important that providing an incorrect initial type results in inconsistent behavior.
  1. ModelMultipleChoiceField.clean() returns model instances. Quoting the docs again, "Normalizes to: A QuerySet of model instances."

Maybe some documentation clarifications would be enough to resolve this ticket. What do you think?

comment:9 Changed 5 years ago by karyon

if inconsistent behaviour on incorrect input is acceptable, i agree that django is doing nothing wrong.

however, i think that the current situation isn't very user friendly. basically, i can provide a queryset as initial value and that is fine as long as i don't disable the field. if i want to disable the field, i necessarily need to apply a values_list('pk', flat=True) to the initial value. which seems unnecessary, especially since the field outputs a queryset, so why shouldn't the input be a queryset as well? afaics most other fields can deal with their output data format as input.

on a related note, says "initial values are not used as “fallback” data in validation. initial values are only intended for initial form display". why is the initial value validated/cleaned at all? skipping cleaning on disabled fields would solve this as well. is that possible or is the result of cleaning still needed on disabled fields?

so i'd vote for

  1. skipping validation on disabled fields if possible
  2. make ModelMultipleChoiceFields deal with querysets as initial values
  3. or clarify in the docs that initial values must be pks and one has to use values_list('pk', flat=True).

comment:10 Changed 5 years ago by Tim Graham

Component: UncategorizedForms
Summary: Wrong handling of initial value on disabled ModelMultipleChoiceFieldDisabled ModelMultipleChoiceField can't handle querysets as an initial value
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

On further inspection, it looks like ModelMultipleChoicField is supposed to accept querysets as initial values (according to 3318595c0bfeda9f6bae8aa11dda68647ae55fde).

I'm not sure about the answer to your second question. You could find the ticket that introduced the ability to disable fields and see if there is any discussion there. If not, try to implement what you described and see if any tests break.

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