Opened 7 years ago

Closed 7 years ago

#29082 closed Cleanup/optimization (fixed)

Make the test client automatically encode JSON data

Reported by: Nick Sarbicki Owned by: nobody
Component: Testing framework Version: 2.0
Severity: Normal Keywords:
Cc: Adam Johnson Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (23)

comment:1 by Tom Forbes, 7 years ago

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

comment:2 by Nick Sarbicki, 7 years ago

I can agree with moving it up a level. Although in that case maybe it is worth moving it even further upstream to post:

https://github.com/django/django/blob/86de930f413e0ad902e11d78ac988e6743202ea6/django/test/client.py#L334

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.

comment:3 by Tom Forbes, 7 years ago

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?

comment:4 by Tim Graham, 7 years ago

In the absence of a fix, 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.

comment:5 by Nick Sarbicki, 7 years ago

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.

comment:6 by Adam Johnson, 7 years ago

Cc: Adam Johnson added

comment:7 by Adam Johnson, 7 years ago

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.

comment:8 by Tim Graham, 7 years ago

Component: UtilitiesTesting framework
Summary: force_bytes from utils.encoding.py produces invalid JSON for RequestFactoryMake the test client automatically encode JSON data
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

comment:9 by Tom Forbes, 7 years ago

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:

https://github.com/django/django/blob/3e9aa298719f19d5f09dbe0df29b6bb8d2136229/django/core/serializers/json.py#L76

comment:10 by Nick Sarbicki, 7 years ago

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.

comment:11 by Jonas Haag, 7 years ago

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 :)

comment:12 by Nick Sarbicki, 7 years ago

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?

comment:14 by Nick Sarbicki, 7 years ago

PR updated for testing all routes.

comment:15 by Tim Graham, 7 years ago

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.

comment:16 by Carlton Gibson, 7 years ago

Has patch: set
Patch needs improvement: set

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.

comment:17 by Nick Sarbicki, 7 years ago

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', ...)

comment:18 by Nick Sarbicki, 7 years ago

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.

comment:19 by Nick Sarbicki, 7 years ago

PR updated to reflect the above.

comment:20 by Carlton Gibson, 7 years ago

Triage Stage: AcceptedReady for checkin

All good. I think add the json_encoder argument to __init__ works well.

I'm going to mark as RFC. Good one!

comment:21 by Carlton Gibson, 7 years ago

Patch needs improvement: unset

comment:22 by Nick Sarbicki, 7 years ago

Git went a bit crazy so moved to a new PR here: https://github.com/django/django/pull/9645

comment:23 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: newclosed

In 47268242:

Fixed #29082 -- Allowed the test client to encode JSON request data.

Note: See TracTickets for help on using tickets.
Back to Top