Opened 13 months ago

Closed 4 months ago

#29459 closed Cleanup/optimization (fixed)

Make Form data/files initialize with an empty MultiValueDict rather than dict

Reported by: Sven R. Kunze Owned by: Andra Denis Ionescu
Component: Forms Version: 2.0
Severity: Normal Keywords: Form QueryDict
Cc: Herbert Fortes Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: yes

Description

You might have a look here:

https://github.com/django/django/blob/362813d6287925b8f63f/django/forms/forms.py#L78

None is converted to a regular dict but not to a QueryDict.

Methods of the form might rely on the API of a QueryDict such as 'iterlists' or 'getlist' which a regular dict doesn't provide.

Change History (16)

comment:1 Changed 13 months ago by Herbert Fortes

Cc: Herbert Fortes added

Hi,

To be clear, QueryDict is a subclass of dict.

Is the idea to add a feature?

At a first look, the dictionary is there to avoid that the code breaks.
# edited to fix english

Regards,

Last edited 13 months ago by Herbert Fortes (previous) (diff)

comment:2 in reply to:  1 Changed 13 months ago by Sven R. Kunze

Replying to Herbert Fortes:

Is the idea to add a feature?

Sorry, if that wasn't clear from the issue itself. Instead of:

        self.data = {} if data is None else data

this would be 100% compatible with what views usually pass into forms:

        self.data = QueryDict() if data is None else data

At a first look, the dictionary is there to avoid that the code breaks.

I think so as well and it works in many places until people think they can rely on the QueryDict API such as 'iterlists' or 'getlist' which is not always true.

comment:3 Changed 13 months ago by Tim Graham

A similar issue was raised in #27989. The main concern I have is that this would couple django.forms to django.http.

comment:4 Changed 13 months ago by Sven R. Kunze

Yes, I thought that #27989 was the reason to change it from "data or {}" to ternary operator.

What do you think about:

1) either using MultiValueDict instead of QueryDict? That would couple django.forms to django.utils.datastructures

2) or moving QueryDict to django.utils.datastructures and then using it in the BaseForm constructor?

At least, MultiValueDict supports the getlist, setlist and lists APIs. So, those would behave the same because the dicts are empty anyway. Although, I would rather tend to option 2.

What do you think?

Last edited 13 months ago by Sven R. Kunze (previous) (diff)

comment:5 Changed 13 months ago by Simon Charette

I like the MultiValueDict approach, +1.

comment:6 Changed 13 months ago by Herbert Fortes

MultiValueDict +1

comment:7 Changed 13 months ago by Sven R. Kunze

Okay seems like we have winner. :D

Just exploring the other option a bit more: why is moving QueryDict to datastructures not a feasible option? It actually sounds like a more reasonable place for it.

comment:8 Changed 13 months ago by Tim Graham

Summary: data=None in Form results NOT in empty QueryDictMake Form data/files initialize with an empty MultiValueDict rather than dict
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

I agree that QueryDict should remain in django.http. I don't feel that it's something meant for use outside of the request/response cycle.

comment:9 Changed 13 months ago by Sven R. Kunze

I don't feel that it's something meant for use outside of the request/response cycle.

Sorry, for still nagging here, but I still don't understand why it cannot be moved to datastructures.

Its doc string: "A specialized MultiValueDict which represents a query string." <<< That sounds like a perfect valid datastructure handling encoding and query string syntax. Its source code has no reference to request/responses.


Also APIs MultiValueDict doesn't provide:

  • urlencode
  • encoding (getter and setter)
  • fromkeys
  • mutability
  • isinstance check


I just want to make sure we don't need to touch this part in 3 years AGAIN when somebody else needs the real thing.

Last edited 13 months ago by Sven R. Kunze (previous) (diff)

comment:10 Changed 12 months ago by Jeff

Do we want to move ahead with the MultiValueDict, or delay for more discussion concerning the QueryDict?

comment:11 in reply to:  4 Changed 12 months ago by Herbert Fortes

IMHO, an empty dict should not make too much noise on the code. For now at least.

comment:12 Changed 4 months ago by Andra Denis Ionescu

Owner: changed from nobody to Andra Denis Ionescu
Status: newassigned

comment:13 Changed 4 months ago by Tim Graham

Has patch: set
Needs tests: set

PR. Please uncheck "Needs tests" after updating.

comment:14 Changed 4 months ago by Andra Denis Ionescu

Needs tests: unset

comment:15 Changed 4 months ago by Simon Charette

Triage Stage: AcceptedReady for checkin

comment:16 Changed 4 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 4c086d7d:

Fixed #29459 -- Initialized form data/files with empty MultiValueDicts.

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