[Django] #30735: Testing client encode_multipart may also support dict format

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[Django] #30735: Testing client encode_multipart may also support dict format

Django
#30735: Testing client encode_multipart may also support dict format
---------------------------------------------+------------------------
               Reporter:  ychab              |          Owner:  nobody
                   Type:  New feature        |         Status:  new
              Component:  Testing framework  |        Version:  2.2
               Severity:  Normal             |       Keywords:  test
           Triage Stage:  Unreviewed         |      Has patch:  1
    Needs documentation:  0                  |    Needs tests:  0
Patch needs improvement:  0                  |  Easy pickings:  1
                  UI/UX:  0                  |
---------------------------------------------+------------------------
 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

--
Ticket URL: <https://code.djangoproject.com/ticket/30735>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/048.60c017f0a590354853270aaa1fd2be09%40djangoproject.com.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30735: Testing client encode_multipart may also support dict format

Django
#30735: Testing client encode_multipart may also support dict format
-----------------------------------+--------------------------------------
     Reporter:  ychab              |                    Owner:  ychab
         Type:  New feature        |                   Status:  assigned
    Component:  Testing framework  |                  Version:  2.2
     Severity:  Normal             |               Resolution:
     Keywords:  test               |             Triage Stage:  Unreviewed
    Has patch:  1                  |      Needs documentation:  0
  Needs tests:  0                  |  Patch needs improvement:  0
Easy pickings:  1                  |                    UI/UX:  0
-----------------------------------+--------------------------------------
Changes (by ychab):

 * status:  new => assigned
 * owner:  nobody => ychab


--
Ticket URL: <https://code.djangoproject.com/ticket/30735#comment:1>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.4b782900f998f47c5d32e3dd40cda95a%40djangoproject.com.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30735: Testing client encode_multipart may also support dict format

Django
In reply to this post by Django
#30735: Testing client encode_multipart may also support dict format
-----------------------------------+--------------------------------------
     Reporter:  ychab              |                    Owner:  ychab
         Type:  New feature        |                   Status:  assigned
    Component:  Testing framework  |                  Version:  2.2
     Severity:  Normal             |               Resolution:
     Keywords:  test               |             Triage Stage:  Unreviewed
    Has patch:  1                  |      Needs documentation:  0
  Needs tests:  1                  |  Patch needs improvement:  0
Easy pickings:  1                  |                    UI/UX:  0
-----------------------------------+--------------------------------------
Changes (by ychab):

 * needs_tests:  0 => 1


--
Ticket URL: <https://code.djangoproject.com/ticket/30735#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.6027f4e61f5d5f218861aed6e2630d75%40djangoproject.com.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30735: Testing client encode_multipart may also support dict format

Django
In reply to this post by Django
#30735: Testing client encode_multipart may also support dict format
-----------------------------------+--------------------------------------
     Reporter:  ychab              |                    Owner:  ychab
         Type:  New feature        |                   Status:  assigned
    Component:  Testing framework  |                  Version:  2.2
     Severity:  Normal             |               Resolution:
     Keywords:  test               |             Triage Stage:  Unreviewed
    Has patch:  1                  |      Needs documentation:  0
  Needs tests:  1                  |  Patch needs improvement:  0
Easy pickings:  1                  |                    UI/UX:  0
-----------------------------------+--------------------------------------

Comment (by ychab):

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

 Thanks

--
Ticket URL: <https://code.djangoproject.com/ticket/30735#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.12a4ba818b673317340e6da579b8d443%40djangoproject.com.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30735: Testing client encode_multipart may also support dict format. (was: Testing client encode_multipart may also support dict format)

Django
In reply to this post by Django
#30735: Testing client encode_multipart may also support dict format.
-------------------------------------+-------------------------------------
     Reporter:  Yannick Chabbert     |                    Owner:  Yannick
                                     |  Chabbert
         Type:  New feature          |                   Status:  closed
    Component:  Testing framework    |                  Version:  master
     Severity:  Normal               |               Resolution:  wontfix
     Keywords:  test                 |             Triage Stage:
                                     |  Unreviewed
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  1                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by felixxm):

 * status:  assigned => closed
 * version:  2.2 => master
 * resolution:   => wontfix


Comment:

 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/30735#comment:4>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.366291630abc5eef01bc183b709f4c65%40djangoproject.com.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30735: Testing client encode_multipart may also support dict format.

Django
In reply to this post by Django
#30735: Testing client encode_multipart may also support dict format.
-------------------------------------+-------------------------------------
     Reporter:  Yannick Chabbert     |                    Owner:  Yannick
                                     |  Chabbert
         Type:  New feature          |                   Status:  closed
    Component:  Testing framework    |                  Version:  master
     Severity:  Normal               |               Resolution:  wontfix
     Keywords:  test                 |             Triage Stage:
                                     |  Unreviewed
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  1                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------

Comment (by 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).

--
Ticket URL: <https://code.djangoproject.com/ticket/30735#comment:5>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.cff522d27dd21136cb5161d6d838ddfc%40djangoproject.com.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30735: Testing client encode_multipart may also support dict format.

Django
In reply to this post by Django
#30735: Testing client encode_multipart may also support dict format.
-------------------------------------+-------------------------------------
     Reporter:  Yannick Chabbert     |                    Owner:  Yannick
                                     |  Chabbert
         Type:  New feature          |                   Status:  closed
    Component:  Testing framework    |                  Version:  master
     Severity:  Normal               |               Resolution:  wontfix
     Keywords:  test                 |             Triage Stage:
                                     |  Unreviewed
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  1                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------

Comment (by Yannick Chabbert):

 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/30735#comment:6>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.21a9e642a6221c76729772ddba5c3a7c%40djangoproject.com.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30735: Testing client encode_multipart may also support dict format.

Django
In reply to this post by Django
#30735: Testing client encode_multipart may also support dict format.
-------------------------------------+-------------------------------------
     Reporter:  Yannick Chabbert     |                    Owner:  Yannick
                                     |  Chabbert
         Type:  New feature          |                   Status:  closed
    Component:  Testing framework    |                  Version:  master
     Severity:  Normal               |               Resolution:  wontfix
     Keywords:  test                 |             Triage Stage:
                                     |  Unreviewed
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  1                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------

Comment (by Yannick Chabbert):

 Replying to [comment:5 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 !

--
Ticket URL: <https://code.djangoproject.com/ticket/30735#comment:7>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.54121c63c35524347f945c232f52de2f%40djangoproject.com.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30735: Testing client encode_multipart may also support dict format.

Django
In reply to this post by Django
#30735: Testing client encode_multipart may also support dict format.
-------------------------------------+-------------------------------------
     Reporter:  Yannick Chabbert     |                    Owner:  Yannick
                                     |  Chabbert
         Type:  New feature          |                   Status:  closed
    Component:  Testing framework    |                  Version:  master
     Severity:  Normal               |               Resolution:  wontfix
     Keywords:  test                 |             Triage Stage:
                                     |  Unreviewed
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  1                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------

Comment (by Yannick Chabbert):

 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/master/django/http/multipartparser.py#L196

 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!

--
Ticket URL: <https://code.djangoproject.com/ticket/30735#comment:8>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.d924da49c69a5dedc0c29558175773dd%40djangoproject.com.