Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#30735 closed New feature (wontfix)

Testing client encode_multipart may also support dict format.

Reported by: Yannick Chabbert Owned by: Yannick Chabbert
Component: Testing framework Version: dev
Severity: Normal Keywords: test
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

When using implicitly multipart-form data with the testing client (post / put methods), we can't unfortunetly provide data like dict, whereas we could provide file, list and even list of files...
Of course, the actual workaround is to do it manually (cast dict to json) but it should be really great if it could be done automatically!

Furthermore, some third party libraries like Django Rest Framework are limited by this, see https://github.com/encode/django-rest-framework/blob/3.10.2/rest_framework/renderers.py#L907

Concretly with DRF, we would be able to test file upload with nested data on an endpoint at the same time. I highlight test because otherwise in real life, it works if the HTTP client send the dict data as a json string.

Here is just a basic example, with the native Django test client where we can't do this:

        with open(filepath, 'rb') as fp:
            response = self.client.post('/post/', data={
                'nested_data': {
                    'firstname': 'foo',
                    'lastname': 'foo',
                },
                'testfile': fp,
            })

Indeed instead, we have to cast it manually:

        import json
        with open(filepath, 'rb') as fp:
            response = self.client.post('/post/', data={
                'nested_data': json.dumps({
                    'firstname': 'foo',
                    'lastname': 'foo',
                }),
                'testfile': fp,
            })

In that case, it would be ok and nested data would be properly parsed.

Finally, the feature request should not be so hard and I have already a working patch. However, I need first to know if it is an acceptable feature request? Am I missing something else?

Thanks for the review

Change History (8)

comment:1 by Yannick Chabbert, 5 years ago

Owner: changed from nobody to Yannick Chabbert
Status: newassigned

comment:2 by Yannick Chabbert, 5 years ago

Needs tests: set

comment:3 by Yannick Chabbert, 5 years ago

Here is the merge request, awaiting tests I can done if everything is ok: https://github.com/django/django/pull/11720/files

Thanks

comment:4 by Mariusz Felisiak, 5 years ago

Resolution: wontfix
Status: assignedclosed
Summary: Testing client encode_multipart may also support dict formatTesting client encode_multipart may also support dict format.
Version: 2.2master

Thanks for this report, however you cannot provide nested dicts because as far as I'm concerned it is not a RFC compliant. I don't think that Django should assume that users want to test JSON data. Moreover I don't see how DRF is limited by Django in this area, assertion that you pointed is rather a conscious choice.

comment:5 by Carlton Gibson, 5 years ago

Aside: if you were to propose the adjustment here to DRF, we'd say something like, "subclass MultiPartRenderer to automatically convert dicts to JSON, if that's what you know you want for your project". I'd suggest that as a way forward, although, I'd probably advise just keeping the json.dumps() inline, since it's nicely visible there, and not much of a hardship... (HTH).

in reply to:  4 comment:6 by Yannick Chabbert, 5 years ago

Thanks for the quick answer. I agree with you and it clearly needs more search before suggesting an helper like this.

What I'm still not sure and I still don't understand yet is why it works in real life (sending data dict as json string along with file)!!?

  • It is the DRF parser which magically detect that the string is a json format and thus, deserialize it?
  • Or it happen elsewhere, at a lower level in Django itself?
  • This parsing behaviour is wanted or just a side effect?
  • Afterall, even if it work, is it RFC compliant to send json data as string with multipart content type?

The big deal to me is not really to have this helper just like for list or file (because finally, we could do it manually) but to understand why this exception is raised and if there is a good reason to do this? If true, it means that we should't do this and parser should be fixed to failed instead. Otherwise, exception shouldn't be raised and give an helper to dumps data as json at a lowest level in Django (that's why I open this issue in the Django project and not in DRF)...

Indeed at the begining, I was thinking: "ok, test failed because an exception is raised to tell me it is not possible". Sad story but ok, this is it. But after discovering that it works in real life, I'm saying: "Oooh good catch! In fact, it works!! So this exception is maybe wrong??"

I will try to investigate more deeply on it and give some feedbacks.

in reply to:  5 comment:7 by Yannick Chabbert, 5 years ago

Replying to Carlton Gibson:

Aside: if you were to propose the adjustment here to DRF, we'd say something like, "subclass MultiPartRenderer to automatically convert dicts to JSON, if that's what you know you want for your project". I'd suggest that as a way forward, although, I'd probably advise just keeping the json.dumps() inline, since it's nicely visible there, and not much of a hardship... (HTH).

Didn't see your comment, good suggestion thanks !

comment:8 by Yannick Chabbert, 5 years ago

Ok, I can confirm that it work because this is an expected behavior in DRF.

Request is first parsed by the Django MutiPartParser which just extract the raw json string: https://github.com/django/django/blob/2.2.4/django/http/multipartparser.py#L192

It is then handled by the corresponding serializer field (DRF) by implementing the to_internal_value method.

RFC compliant or not (I still didn't know but finally, a string is a string, no matter what it contains!), this is not an issue with Django and sorry for the bad report!

Last edited 5 years ago by Yannick Chabbert (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top