django-rest-framework icon indicating copy to clipboard operation
django-rest-framework copied to clipboard

ListField does not respect ordered sequence in form data with partial updates.

Open proofit404 opened this issue 7 years ago • 5 comments

Checklist

  • [X] I have verified that that issue exists against the master branch of Django REST framework.
  • [X] I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
  • [X] This is not a usage question. (Those should be directed to the discussion group instead.)
  • [X] This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third-party libraries where possible.)
  • [X] I have reduced the issue to the simplest possible case.
  • [ ] I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

Steps to reproduce

  1. For example, we have serializer like this:
class CommunitySerializer(Serializer):

    colors = ListField(allow_null=True, child=CharField(label='Colors', max_length=7), required=False)
  1. We submit partial update encoded as form data:
test_client.patch('/communities/1/', {'colors[0]': '#ffffff', 'colors[1]': '#000000'}, 'multipart')
  1. This will encode request body like this:
--BoUnDaRyStRiNg
Content-Disposition: form-data; name="colors[0]"

#000000
--BoUnDaRyStRiNg
Content-Disposition: form-data; name="colors[1]"

#ffffff
--BoUnDaRyStRiNg--

Expected behavior

validated_data should contain colors value parsed into a list with the respected order.

Actual behavior

validated_data is an empty dictionary.

Explanation

  1. ListField.get_value checks that field is present with in statement:

https://github.com/encode/django-rest-framework/blob/ed6340ee7619d7e8c3419ffacb973f7ca0c329d3/rest_framework/fields.py#L1639

  1. Since field_name does not match submitted keys, method checks in this is a partial update

https://github.com/encode/django-rest-framework/blob/ed6340ee7619d7e8c3419ffacb973f7ca0c329d3/rest_framework/fields.py#L1640

  1. Since this is also true, the method returns an empty marker.

https://github.com/encode/django-rest-framework/blob/ed6340ee7619d7e8c3419ffacb973f7ca0c329d3/rest_framework/fields.py#L1641

  1. Field name with ordered marker does not check to present.

Alternative

We can submit the partial update with list fields unordered:

test_client.patch('/communities/1/', {'colors': ['#ffffff', '#000000']}, 'multipart')

Form data will look like this

--BoUnDaRyStRiNg
Content-Disposition: form-data; name="colors"

#000000
--BoUnDaRyStRiNg
Content-Disposition: form-data; name="colors"

#ffffff
--BoUnDaRyStRiNg--

So in statement will evaluate to true, so dictionary will be processed by HTML parser.

https://github.com/encode/django-rest-framework/blob/ed6340ee7619d7e8c3419ffacb973f7ca0c329d3/rest_framework/fields.py#L1649

But this format does not guarantee items order in the list, so we can not use it for things like gradients.

Full update and create methods do not have this problem because partial check evaluates to false.

We can't use full updates because we have two file fields near. We don't want to force the user to upload two files just to change string values in the same request.

P.S.

If you think we should solve this, I'll be able to make a PR for it.

proofit404 avatar Sep 22 '18 06:09 proofit404

I don't know if you still having an issue with this but try changing

class CommunitySerializer(Serializer):
    colors = ListField(allow_null=True, child=CharField(label='Colors', max_length=7), required=False)

to

class CommunitySerializer(Serializer):
    colors = ListField(allow_empty=True, child=CharField(label='Colors', required=False), max_length=7)

rawteech avatar Apr 13 '19 14:04 rawteech

version:3.11.0 I have the same problem. When I use http post , the data {'colors[0]': '#ffffff', 'colors[1]': '#000000'} is OK, but use http patch get problem, there is no data, only use {'colors': '#ffffff'} get colors=["'#ffffff'"]

jttttttttttttt avatar Aug 03 '20 08:08 jttttttttttttt

Have same problem. Checked for version djangorestframework-3.10.3 and djangorestframework-3.12.2 Still working incorrect. Solved with nested class from ListField

class ListFieldExt(ListField):
    # this class fixes problem when PATCH with indexed params {'a[0]': 1, 'a[1]': 2}
    def get_value(self, dictionary):
        if self.field_name not in dictionary:
            if getattr(self.root, 'partial', False):
                # fix PATCH method problem
                parsed_list = html.parse_html_list(dictionary, prefix=self.field_name, default=empty)
                if parsed_list != empty:
                    return parsed_list

                return empty
        # We override the default field access in order to support
        # lists in HTML forms.
        if html.is_html_input(dictionary):
            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, default=empty)

        return dictionary.get(self.field_name, empty)

Megajoe17 avatar Jan 06 '21 13:01 Megajoe17

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Apr 30 '22 20:04 stale[bot]

I don't know if you still having an issue with this but try changing

class CommunitySerializer(Serializer):
    colors = ListField(allow_null=True, child=CharField(label='Colors', max_length=7), required=False)

to

class CommunitySerializer(Serializer):
    colors = ListField(allow_empty=True, child=CharField(label='Colors', required=False), max_length=7)

can we close this based on what you suggested?

auvipy avatar Jun 18 '23 07:06 auvipy