[Django] #29082: force_bytes from utils.encoding.py produces invalid JSON for RequestFactory
Django
--------------------------------------+------------------------
Reporter: Nick | Owner: nobody
Type: Bug | Status: new
Component: Utilities | Version: 2.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
--------------------------------------+------------------------
I stumbled on this while testing some code which expects a JSON body.
When creating a RequestFactory post request with a dict for the data and
content_type='application/json' the resultant body is invalid JSON.
This is because force_bytes simply detects it is not a string and forces
it to a string before encoding.
Code to reproduce is here:
{{{
In [1]: from django import test
In [2]: factory = test.RequestFactory()
In [3]: req = factory.post('/', content_type='application/json',
data={"test": "data"})
In [4]: req.body
Out[4]: b"{'test': 'data'}"
}}}
And the offending code is here:
https://github.com/django/django/blob/master/django/utils/encoding.py#L103
Assuming this is not an intended "feature" I will happily submit a patch
adding _another_ isinstance line checking for dicts and encoding with
json.dumps.
--
Ticket URL: <https://code.djangoproject.com/ticket/29082>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Nick | Owner: nobody
Type: Bug | Status: new
Component: Utilities | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tom Forbes):
`force_bytes` is working as intended in this case, and we should
definitely not coerce objects to json there. If this is accepted you
should add any changes to the `_encode_data()` method in RequestFactory:
https://github.com/django/django/blob/86de930f413e0ad902e11d78ac988e6743202ea6/django/test/client.py#L303
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:1>
Django
Reporter: Nick | Owner: nobody
Type: Bug | Status: new
Component: Utilities | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nick):
I can agree with moving it up a level. Although in that case maybe it is
worth moving it even further upstream to post:
We already instantiate a dict here if there is no data - even if it is
empty - so we are effectively expecting a json body by default.
encode_data is not used anywhere else at this point and as it is mostly
concerned with encoding the data from a detected charset instead of
mutating it's type you could argue that it is also working as expected.
However I would argue that .post is not working as expected.
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:2>
Django
Reporter: Nick | Owner: nobody
Type: Bug | Status: new
Component: Utilities | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tom Forbes):
Hmm good point. `put`, `patch` and `delete` would also need this
functionality, not just post, so there might be a bit more to this. For
example what encoder class should it use? Should we use the
DjangoJSONEncoder from the serializations framework? How would (or
should?) we let people customize this?
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:3>
Django
Reporter: Nick | Owner: nobody
Type: Bug | Status: new
Component: Utilities | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
In the absence of a fix, [https://stackoverflow.com/questions/8583290
/sending-json-using-the-django-test-client Stack Overflow] (and other
things I found on the web) suggest using `data=json.dumps(python_dict)`
It may make sense to that conversion automatically as long as there's
minimal chance of breaking existing code.
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:4>
Django
Reporter: Nick | Owner: nobody
Type: Bug | Status: new
Component: Utilities | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nick):
That is the current solution I am using.
Which leads me to say that for me I'd be happy using the default python
`json` encoder - which is already imported.
Maybe we could add an `_encode_json` method akin to the `_parse_json`
method in `Client` which takes kwargs from the calling method although I'm
loathe to add a `**kwargs` argument just to pass it into a json encoder.
Maybe a `data_kwargs` argument which takes a dict?
I'm of the opinion that not much customisation is required except maybe a
custom serializer for some fields which you can do via `json.dumps`.
Assuming we are only converting dictionaries code breakage should be null
unless people are intentionally ingesting broken JSON only.
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:5>
Django
Reporter: Nick | Owner: nobody
Type: Bug | Status: new
Component: Utilities | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Adam (Chainz) Johnson):
* cc: Adam (Chainz) Johnson (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:6>
Django
Reporter: Nick | Owner: nobody
Type: Bug | Status: new
Component: Utilities | Version: 2.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam (Chainz) Johnson):
> Maybe we could add an `_encode_json` method akin to the `_parse_json`
method in `Client` which takes kwargs from the calling method although I'm
loathe to add a `**kwargs` argument just to pass it into a json encoder.
Maybe a `data_kwargs` argument which takes a dict?
I think it's fine not worrying about the customization; `_encode_json`
should be simple enough that users can subclass and replace if it doesn't
work the way they want.
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:7>
Django
--------------------------------------+------------------------------------
Reporter: Nick | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* component: Utilities => Testing framework
* stage: Unreviewed => Accepted
* type: Bug => Cleanup/optimization
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:8>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tom Forbes):
It would be nice if we could use the Django encoder by default, it does
some pretty useful things around encoding datetimes, decimals and uuid's
that I find pretty useful:
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:9>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nick):
Fine by me, I have no particular attachment to the encoders and you are
right, it is nice to have those extras.
With this accepted I'll try write up a patch tonight/tomorrow.
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:10>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jonas Haag):
How about adding a new `json` parameter as in python-requests? From their
docs:
{{{
>>> url = 'https://api.github.com/some/endpoint'
>>> payload = {'some': 'data'}
>>> r = requests.post(url, json=payload)
}}}
Dealing with requests regularly (and I'm sure I'm not the only one in the
Python web world who does that) this has become the natural way to specify
a JSON request body for me. On multiple occasions I just assumed there was
a `json` parameter on the test client just to be disappointed :)
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:11>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nick Sarbicki):
I like the idea but they're two different systems with different APIs, I
don't want to add more to the interface just to make it feel like
requests.
Especially as it would duplicate the keywords functionality with json and
dicts.
Maybe it should be a separate ticket?
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:12>
Django
--------------------------------------+------------------------------------
Reporter: Nick Sarbicki | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nick Sarbicki):
PR for the ticket https://github.com/django/django/pull/9636
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:13>
Django
--------------------------------------+------------------------------------
Reporter: Nick Sarbicki | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nick Sarbicki):
PR updated for testing all routes.
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:14>
Django
--------------------------------------+------------------------------------
Reporter: Nick Sarbicki | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
At first glance, I think adding a `json_encoder` argument to every HTTP
method in the test client looks like more clutter than its worth. After
all, a user can use `data=json.dumps({...}, encoder=CustomClass)` for that
case, correct? That could be documented.
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:15>
Django
--------------------------------------+------------------------------------
Reporter: Nick Sarbicki | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
Even if using a custom encoder, it's not something I want to pass every
time, either calling the request method, or encoding the data explicitly.
Can we make it an attribute on `RequestFactory` (and so `Client`)?
`_encode_json` will use this directly, rather than take a parameter: `...
json.dumps(data, cls=self.json_encoder_class)`
When testing I set my custom encoder at the point of use (or subclass if I
prefer):
{{{
factory = test.RequestFactory()
factory.json_encoder_class = MyCustomEncoder
req = factory.post('/', content_type='application/json', ...)
}}}
If we do this then we just need a test that the user **can** set a custom
encoder.
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:16>
Django
--------------------------------------+------------------------------------
Reporter: Nick Sarbicki | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Nick Sarbicki):
I agree, and I like the property idea a lot more - good idea.
However, I'm not a fan of changing properties after instantiation.
So I'm going to put it as an `__init__` kwarg. Still have the option of
changing it post instantiation but you can do it in one line if you want.
{{{
factory = test.RequestFactory(json_encoder=MyCustomEncoder)
req = factory.post('/', content_type='application/json', ...)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:17>
Django
--------------------------------------+------------------------------------
Reporter: Nick Sarbicki | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Nick Sarbicki):
I was also pondering putting the `if content_type == JSON_CONTENT`
statement within the `_encode_json` method.
But I don't think it would be clear enough that we won't actually encode
JSON if the content_type is something else.
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:18>
Django
--------------------------------------+------------------------------------
Reporter: Nick Sarbicki | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Nick Sarbicki):
PR updated to reflect the above.
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:19>
Django
Reporter: Nick Sarbicki | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Accepted => Ready for checkin
Comment:
All good. I think add the `json_encoder` argument to `__init__` works
well.
I'm going to mark as RFC. Good one!
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Nick Sarbicki | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: Nick Sarbicki | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nick Sarbicki):
Git went a bit crazy so moved to a new PR here:
https://github.com/django/django/pull/9645
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: Nick Sarbicki | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: 2.0
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"47268242b071895dd269d97540e45dce646f675c" 47268242]:
{{{
#!CommitTicketReference repository=""
revision="47268242b071895dd269d97540e45dce646f675c"
Fixed #29082 -- Allowed the test client to encode JSON request data.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29082#comment:23>