Skip to content

Conversation

@j0hnsmith
Copy link

When I run the tests the failure complains about unbalanced braces, however here's a demonstration of the problem (using 3.1.1 from pypi)

In [18]: class NestedSerializer(serializers.Serializer):
   ....:     one = serializers.IntegerField(max_value=10)
   ....:     two = serializers.IntegerField(max_value=10)
   ....:     

In [19]: class TestSerializer(serializers.Serializer):
   ....:     nested = NestedSerializer()
   ....:     

In [20]: input_data = {'nested': {'one': 1, 'two': 2}}

In [21]: input_data_q_dict = QueryDict('', mutable=True)

In [22]: input_data_q_dict.update(input_data)

In [23]: s = TestSerializer(data=input_data_q_dict)

In [24]: s.is_valid()
Out[24]: False

In [25]: s.errors
Out[25]: {'nested': {'two': [u'This field is required.'], 'one': [u'This field is required.']}}

The same test with a normal dict passes.

I'm pretty sure this is a stripped back demonstration of the problem discussed in #1936. As suggested by @jamescooke in (#1936 (comment)), it seems like the QueryDict should be handled here https://github.com/tomchristie/django-rest-framework/blob/35efbe41abcff15ea2f60096aaa95bd41f2532a3/rest_framework/serializers.py#L350-356

@jamescooke
Copy link
Contributor

@j0hnsmith Looks like a nice simple test to me. Thanks for linking me in.

@lovelydinosaur
Copy link
Contributor

Ok having a dig at this. Could either of you give some more context about how/when are you folks encountering this?
QueryDict is only used for multipart input which doesn't support nested objects so I'm unclear about what you're actually doing in order to see an issue here. Is this being raised in test code perhaps?

@j0hnsmith
Copy link
Author

Yes, I'm seeing this in tests however I've realised my test wasn't setting Content-Type correctly. With self.client.post(url, data=data) request.data is a QueryDict however my test should be self.client.post(url, data=json.dumps(data), content_type='application/json'), then request.data is an ordinary dict (and my test passes).

So this probably down to people accidentally sending json data as multipart/form-data (like I did), usually it wouldn't be a problem, it's just in this case the nesting is handled differently.

@xordoquy
Copy link
Contributor

xordoquy commented May 8, 2015

@j0hnsmith self.client.post(url, data=json.dumps(data), content_type='application/json') won't work as it'll double the JSon encode (provided you're using the DRF's client).

@j0hnsmith
Copy link
Author

@xordoquy thanks for the heads up. I get confused between the apis of requests, the django test client and drf test client. As per the drf testing docs I've modified my test to self.client.post(url, data=data, format='json') and it still passes.

@lovelydinosaur
Copy link
Contributor

Fails due to serializers using the "parent.child" style that we use to simulate nesting in HTML input that cannot do it properly.

We could change it so that the test doesn't fail, but actually it's not really a valid case. A better solution might be to ensure that multipart encoded test client requests raise an error, like "what the flip are you trying to do there, kiddo. Nope, you can't - HTML Form input doesn't support that"

@j0hnsmith
Copy link
Author

"actually it's not really a valid case", agreed. A warning would be useful.

@lovelydinosaur lovelydinosaur changed the title add failing test to demonstrate nested serializer QueryDict problem MultiPart encoding from test client should error on nested data. May 14, 2015
@lovelydinosaur
Copy link
Contributor

@j0hnsmith Sounds like a good plan - retitling this issue. We should raise an error when this is used in the test client, like "MultiPart form content cannot support nested dictionaries. Consider using format="json" instead."

Possibly related - we could consider raising a similar error if a file object is passed to JSON in the test client, like "JSON cannot support file uploads. Consider using format="multipart" instead."

@xordoquy
Copy link
Contributor

@j0hnsmith I just removed some suspicious messages coming from you.

@j0hnsmith
Copy link
Author

@xordoquy thanks, my email got hacked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants