Fix support for ListField default values in HTML mode #5812

awbacker · 2018-02-07T04:53:52Z

The ListField handles html forms data differently than json data. This section
would always return a value (an empty list) for a list field, even when that
field name was not posted at all. This change checks for the field being
present and returns empty if it is not; this lets the standard default-value and
required field handling take over.

Both tests cases will fail if run against master (key is present, value is [])
Suggestions for other test cases would be appreciated
All other tests pass (django 1.11.5 installed)

Change was done where it is to make sure everything was clear. There may be a better, more general, location to put it. I wasn't comfortable combining the handling with the first check, because I'm sure there is a reason to call out the 'partial' condition (for clarity?). One could certainly delete that line, and just use:

def get_value(self, dictionary):
    if self.field_name not in dictionary:
        return empty
    # no other changes

Suggestions welcome

refs #5807

The ListField handles html forms data differently than json data. This section would always return a value (an empty list) for a list field, even when that field name was not posted at all. This change checks for the field being present and returns `empty` if it is not; this lets the standard default-value and required field handling take over.

rpkilby · 2018-02-07T19:10:33Z

I need to put more thought into this, but I don't think this is correct. HTML forms always submit their inputs (no way to not submit an input), so form processing doesn't usually differentiate between an empty value and the lack of that input's presence. e.g., ?param= and ? are usually treated as semantically equivalent.

django-rest-framework/rest_framework/fields.py

Lines 1612 to 1616 in c456b3c

    
           val = dictionary.getlist(self.field_name, []) 
        
           if len(val) > 0: 
        
               # Support QueryDict lists in HTML input. 
        
               return val 
        
           return html.parse_html_list(dictionary, prefix=self.field_name)

I'm thinking it might be more appropriate to return an empty if the val is just an empty list. But the question is then if the correct behavior is to return an empty list, or an empty.

awbacker · 2018-02-08T09:21:41Z

Currently ?scores= will give you {score=[""]} as the validated_data, rather than an empty list or default as expected. I removed my test for that, since the behavior already existed, but I've added it back. I would expect that input to give [], or default, but a blanks string is technically valid, I suppose.

Also, its not just browsers that submit forms as multipart/form-data. In my case, we are submitting image data plus many other fields & arrays in one post from different apps and between services. Our requests come from:

Angular + manual generation of XHR requests
Python requests library for calls between backend services
iOS client networking library (okhttp?)
Android networking libaries (retrofit, i think)
Scripts (though these generally use httpie or requests)

Admittedly this is a bit of a sticky issue, and I can't see all the angles.

awbacker · 2018-02-13T07:41:07Z

Updated slightly to allow for posting fields in the form scores[0]=100&scores[1]=88. The html forms parser allows either.

The previous change ignored this, causing indexed fields to be ignored since the exact field name wasn't in the dictionary (scores). Checks for scores[ now as well.

awbacker · 2018-02-27T08:32:45Z

rest_framework/fields.py

@@ -1609,6 +1609,9 @@ def get_value(self, dictionary):
        # We override the default field access in order to support
        # lists in HTML forms.
        if html.is_html_input(dictionary):
+            if self.field_name not in dictionary:
+                if not any(k.startswith("%s[" % self.field_name) for k in dictionary):


Should this support alternate field separators, such as field.0?

awbacker · 2018-03-27T10:10:37Z

@rpkilby Any further thoughts? I know we are one of the few people to post fields and images, though.

lsjurczak · 2018-04-07T07:50:48Z

I think you're trying to do the same as in #4763

anx-ckreuzberger · 2018-04-09T06:28:55Z

Same as with @awbacker I am updating file fields via a PATCH request with a multi-part form.

What work needs to be done to get this merged? I'm willing to help.

anx-ckreuzberger · 2018-04-09T09:37:53Z

I've created PR #5927 which tackles the problem in another way. I believe my PR fixes your problem (ListField) aswell as my problem (ListSerializer).

Your PR does unfortunately not fix the issue for ListSerializer. In addition, your open question about supporting field.0 does concern me a bit, because you do introduce a check here, which should be done in utils.html.parse_html_list rather than the serializer.

To verify that my PR also solves your problem, I've tried running the tests you provided, which seems to work fine. I have however not included your tests in my PR (though if you want, we can include them).

awbacker · 2018-04-09T10:13:41Z

@anx-ckreuzberger If this is a better (more elegant) way to solve that, then wonderful.

Please just copy/paste the tests over if you feel they have some value; I do, but them I'm partial. I think that list handling was a bit under-tested before.

carltongibson · 2018-04-13T07:37:46Z

I'm going to close this in favour of #5927. Thanks for the effort @awbacker!

Allow for indexed keys in list field default value check (?field[0]=)

d3430ce

awbacker commented Feb 27, 2018

View reviewed changes

anx-ckreuzberger mentioned this pull request Apr 9, 2018

html.parse_html_list returns an empty list if a value is not set #5926

Closed

6 tasks

anx-ckreuzberger mentioned this pull request Apr 10, 2018

Fixed issue #5926: Ensure that html forms (multipart form data) respe… #5927

Merged

carltongibson closed this Apr 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix support for ListField default values in HTML mode #5812

Fix support for ListField default values in HTML mode #5812

Uh oh!

awbacker commented Feb 7, 2018 •

edited

Loading

Uh oh!

rpkilby commented Feb 7, 2018

Uh oh!

awbacker commented Feb 8, 2018 •

edited

Loading

Uh oh!

awbacker commented Feb 13, 2018

Uh oh!

awbacker Feb 27, 2018 •

edited

Loading

Uh oh!

awbacker commented Mar 27, 2018

Uh oh!

lsjurczak commented Apr 7, 2018

Uh oh!

anx-ckreuzberger commented Apr 9, 2018

Uh oh!

anx-ckreuzberger commented Apr 9, 2018

Uh oh!

awbacker commented Apr 9, 2018

Uh oh!

carltongibson commented Apr 13, 2018

Uh oh!

Uh oh!

Uh oh!

Fix support for ListField default values in HTML mode #5812

Fix support for ListField default values in HTML mode #5812

Uh oh!

Conversation

awbacker commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rpkilby commented Feb 7, 2018

Uh oh!

awbacker commented Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awbacker commented Feb 13, 2018

Uh oh!

awbacker Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awbacker commented Mar 27, 2018

Uh oh!

lsjurczak commented Apr 7, 2018

Uh oh!

anx-ckreuzberger commented Apr 9, 2018

Uh oh!

anx-ckreuzberger commented Apr 9, 2018

Uh oh!

awbacker commented Apr 9, 2018

Uh oh!

carltongibson commented Apr 13, 2018

Uh oh!

Uh oh!

awbacker commented Feb 7, 2018 •

edited

Loading

awbacker commented Feb 8, 2018 •

edited

Loading

awbacker Feb 27, 2018 •

edited

Loading