Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MultipleChoiceField empties incorrectly on a partial update using multipart/form-data #2894

Closed
carljm opened this issue Apr 30, 2015 · 13 comments
Labels
Milestone

Comments

@carljm
Copy link
Contributor

carljm commented Apr 30, 2015

The root of this issue is that using multipart/form-data with a PATCH (partial=True on the serializer) there's no way to distinguish between "I am not including data for this MCF" vs "please empty this MCF", because they both look the same (zero occurrences of that key in the data).

The current implementation of MultipleChoiceField in DRF always assumes the latter, which is the more dangerous choice, since it leads to data loss. The cause of this is that MultipleChoiceField.get_value() calls QueryDict.getlist if the data is a QueryDict, and QueryDict.getlist() returns [] if the key does not occur in the QueryDict at all.

I think that for safety it would be better if MultipleChoiceField returned empty in case of no key found, rather than returning []. It could do this only if the root serializer has partial=True, so that in case of PUT emptying the field still works correctly.

The implication would be that there would be no way to empty a MultipleChoiceField via PATCH when using multipart/form-data, but I think that's better than always emptying a MultipleChoiceField when the intention was to not include it in the PATCH.

@carljm
Copy link
Contributor Author

carljm commented Apr 30, 2015

The fix looks like this code in MultipleChoiceField.get_value():

if html.is_html_input(data):
    ret = data.getlist(self.field_name)
    if getattr(self.root, 'partial', False) and not ret:
        ret = serializers.empty
    return ret

instead of just blindly returning data.getlist(self.field_name)

I will submit this as a PR (with test) if the issue is accepted.

@tomchristie tomchristie added the Bug label May 1, 2015
@tomchristie tomchristie added this to the 2.4.6 Release milestone May 1, 2015
@tomchristie
Copy link
Member

tomchristie commented May 1, 2015

The implication would be that there would be no way to empty a MultipleChoiceField via PATCH when using multipart/form-data, but I think that's better than always emptying a MultipleChoiceField when the intention was to not include it in the PATCH.

Yup that sounds like the right trade-off to me too. We should note this in the documentation for MultipleChoiceField. It may also be worth double checking if there are any other tickets currently referencing this issue.

@tomchristie
Copy link
Member

tomchristie commented May 1, 2015

Alternative - don't display the PATCH button with the HTML form, and instead only display it for raw JSON input.

See also #2883

@carljm
Copy link
Contributor Author

carljm commented May 1, 2015

Not displaying the PATCH button on the HTML form is fine (if you feel that the safer behavior is still perhaps too confusing), but it's not an alternative to actually fixing MultipleChoiceField.

I didn't run into this via the HTML form UI at all; on our project we have a PATCH in our real production JS that has to submit via multipart/form-data because it includes a file upload, and that's where I hit this issue - it was emptying out an unrelated ArrayField on the same model which was intended to be excluded from the PATCH altogether.

@carljm
Copy link
Contributor Author

carljm commented May 1, 2015

I noticed this bug has "Milestone 2.4.6 Release" on it. I don't think this particular bug applies to the 2.4 series, since (if I'm not mistaken) MultipleChoiceField was only added in 3.x.

@jpadilla jpadilla modified the milestones: 3.1.2 Release, 2.4.6 Release May 1, 2015
@jpadilla
Copy link
Member

jpadilla commented May 1, 2015

...but it's not an alternative to actually fixing MultipleChoiceField.

I agree with @carljm. After also seeing #2883 I think this should go into our next release, if possible.

@xordoquy xordoquy modified the milestones: 3.1.2 Release, 3.1.3 Release May 13, 2015
@xordoquy
Copy link
Collaborator

xordoquy commented Jun 1, 2015

@carljm any objection I'll work on the PR today to get this fixed before the next release which I'm planning any time soon ?

xordoquy added a commit to linovia/django-rest-framework that referenced this issue Jun 1, 2015
xordoquy added a commit to linovia/django-rest-framework that referenced this issue Jun 1, 2015
@carljm
Copy link
Contributor Author

carljm commented Jun 1, 2015

@xordoquy Of course! Thanks for creating the PR.

xordoquy added a commit that referenced this issue Jun 1, 2015
MultipleChoiceField empties incorrectly on a partial update using multipart/form-data (#2894)
@xordoquy
Copy link
Collaborator

xordoquy commented Jun 1, 2015

Closed as #2894 has been merged.

@xordoquy xordoquy closed this as completed Jun 1, 2015
@Alexerson
Copy link

Alexerson commented Mar 24, 2016

I noticed the same behavior with ListSerializer. Looking at the code the fix you introduced for ListField is not present in the ListSerializer implementation. Is there a good reason for this that I don't see ? Should I open a new bug ?

@xordoquy
Copy link
Collaborator

xordoquy commented Mar 24, 2016

Are ListSerializer supposed to work with forms ?

@Alexerson
Copy link

Alexerson commented Mar 24, 2016

:\ Probably not.
But I'm using multipart encoding to PATCH a model picture field that contains as well ListSerializers.
Sending a new picture erase the ListSerializers... Is there a reason not to use the same kind of fix for ListSerializer then ?

@xordoquy
Copy link
Collaborator

xordoquy commented Mar 24, 2016

My take on this, ListSerializer are a list of instances which could be rendererd as Formsets are with forms in Django. This is very different from MultipleChoiceField which is rendered as html select.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants