Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSONField(binary=True) represents using binary strings, which JSONRenderer does not support. #4185

Closed
5 of 6 tasks
nikmolnar opened this issue Jun 10, 2016 · 6 comments
Closed
5 of 6 tasks
Labels
Milestone

Comments

@nikmolnar
Copy link

nikmolnar commented Jun 10, 2016

Checklist

  • I have verified that that issue exists against the master branch of Django REST framework.
  • I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
  • This is not a usage question. (Those should be directed to the discussion group instead.)
  • This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.)
  • I have reduced the issue to the simplest possible case.
  • I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

Steps to reproduce

from rest_framework import serializers

class TestSerializer(serializers.Serializer):
    json = serializers.JSONField()

s = TestSerializer(data={'json': "{broken JSON"})
assert s.is_valid() is False

Expected behavior

Invalid JSON should result in is_valid() returning False.

Actual behavior

is_valid() returns True

Specifying binary=True for the field definition results in the desired behavior for serialization, but causes deserialization to fail on Python 3.

@xordoquy xordoquy added the Bug label Jun 10, 2016
@tomchristie
Copy link
Member

tomchristie commented Jun 10, 2016

Unless you set binary=True the field will check for valid primitives, not a valid binary encoded bytestring.
As currently described, you're passing a valid primitive - a string literal.

What you actually want, as you've noted, is this...

class TestSerializer(serializers.Serializer):
    json = serializers.JSONField(binary=True)

Which then works as expected - is_valid() returns False.
I've tested this in python 3 and I can't see what you mean wrt "causes deserialization to fail" - can reconsider if that aspect gets more details.

@xordoquy
Copy link
Collaborator

xordoquy commented Jun 10, 2016

The tricky point with binary=True is that it isn't a json renderable data because it's a binary string and the json renderer expect unicodes.

I'd point out that the documentation for that field would benefit some clarification:

A field class that validates that the incoming data structure consists of valid JSON primitives. In its alternate binary mode, it will represent and validate JSON-encoded binary strings.

As it stands, I understand that it expects a valid JSON input for non binary mode.

@tomchristie
Copy link
Member

tomchristie commented Jun 10, 2016

It isn't a json renderable data because it's a binary string and the json renderer expect unicodes.

Right - then that's the core issue that we should resolve here.

@tomchristie
Copy link
Member

tomchristie commented Jun 10, 2016

Either that or we should output to unicode by default. (Not strictly correct, but it'd do the trick)

@nikmolnar
Copy link
Author

nikmolnar commented Jun 10, 2016

Sorry, I misspoke. It doesn't break the deserialization, it breaks the JSONRenderer, which I see is a separate issue.

So until #4187 is resolved, what is the best way to enforce valid JSON inputs for an API?

@tomchristie
Copy link
Member

tomchristie commented Jun 10, 2016

Well, using a binary json blob as a field isn't a particularly common use case, so poss you should just reconsider if that's what you actually want (ie a nested document of json primitives may be better) but if it really is what you need I'd suggest subclassing JSONField and forcing to_representation to do .decode('utf8') on the result so that you have a custom JSONField that returns Unicode, not bytestrings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants