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

Handle empty body with json content-type as json #3649

Closed
wants to merge 2 commits into from

Conversation

sorenisanerd
Copy link

An empty request body should not imply HTML form semantics.

An attribute in a JSON object can either be:

  • not set at all,
  • null,
  • an empty list.

An HTML form isn't as flexible, so invariably turning an empty body into
a QueryDict makes it impossible to determine if a given value was passed
in as an empty list or if it was not set at all.

Fixes #3647

An empty request body should not imply HTML form semantics.

An attribute in a JSON object can either be:
 - not set at all,
 - null,
 - an empty list.

An HTML form isn't as flexible, so invariably turning an empty body into
a QueryDict makes it impossible to determine if a given value was passed
in as an empty list or if it was not set at all.
@tomchristie
Copy link
Member

When no content is passed we should ignore the media type - there's no media actually present.
Not sure if there's a better behavior here, but from my POV this is a bit of an unavoidable case.

@sorenisanerd
Copy link
Author

I sort of see where you're coming from, but "" interpreted as HTML form post data has quite different semantics from an empty JSON object, for instance, so the content-type matters, even though the body is empty.

However, come to think of it, "" is not valid JSON, so when httpie happily sends a 0-length body claiming it's JSON, it's doing something wrong.

Now I don't know what to do anymore. It seems like The Right Thing[tm] is to accept the content-type, parse the empty string as JSON and return a validation error saying it's invalid JSON, but I'm not sure that really helps anyone.

@tomchristie
Copy link
Member

I think the right thing to do might be to look at the cases where this empty object is causing behavioral problems. For instance, if we did return a MultiValueDict, but made sure that we don't do odd things with how that ends up being treated in serializers. (Eg. one way or another have html.is_html_dictionary() return False in that case)

It'd be interesting to get a better idea of what kind of view/serializer best exposes this case as being different from {}

@sorenisanerd
Copy link
Author

I presume you've read through #3647 ?

@tomchristie
Copy link
Member

Yup. Tho eg. Break "During validation, many Field implementations call" down into a specific example case for me - demonstrating exactly how the behavior differs. Can we come up with a simple concrete scenario case that demonstrates the issue succinctly?

@sorenisanerd
Copy link
Author

This very closely resembles what happens in my application.

sorenisanerd@cabe21a

@sorenisanerd
Copy link
Author

...and the point is that I feel the operations in each of those tests should yield the same result.

@johnraz
Copy link
Contributor

johnraz commented Jan 12, 2016

👍 for this change, I faced a similar issue explained here:
https://groups.google.com/forum/#!topic/django-rest-framework/wtLCkLiL4Y0

@tomchristie, @sorenh maybe my use case described there would help define a concrete scenario?

@xordoquy
Copy link
Collaborator

When no content is passed we should ignore the media type - there's no media actually present.
Not sure if there's a better behavior here, but from my POV this is a bit of an unavoidable case.

It seems that according the the media type, the parser will make a different analysis of the empty content.
Therefore I don't think the media type should be ignored as it's part of the context.

@tomchristie
Copy link
Member

Empty content is empty content, and I don't believe we should be using the parsers at all in the "no content" case. No doubt there's another tack we can take here by ensuring that we don't simply end up treating no content as equivalent to form encoded either (eg improve the is_html switches in the serializer to not trigger as True for the empty case)

@craigds
Copy link
Contributor

craigds commented Jul 27, 2016

As I just noted on #3647 (before seeing this ticket) I consider this a bug - empty body is invalid JSON and should return a parse error.

@tomchristie
Copy link
Member

Still open to consideration, but this isn't a priority just yet.

@tomchristie
Copy link
Member

Closing this. Empty content is empty content. It should not have a content-type set, and if it does we should ignore it.

Very happy to look into any other behavioral aspects of how we should handle requests with empty content.

@tomchristie
Copy link
Member

Okay, having spent some time with #3891 and #3647 it's clear that we don't have a choice - we do have to differentiate between empty form input and empty non-form input, and furthermore that it can't simply be "If the input is empty, then don't treat it as HTML".

An HTML form with an empty checkbox is a valid input, and we should treat it as a False value.

Looking into chrome's behavior on a form with a single empty checkbox, the Content-Type is set.

Ideally I'd probably want us to use .None if no media type is passed, and use parsing if it is (yes, this is a retraction from my previous position) However we've been using the empty-dict-like-object sentinal value for a long time now, and it ties in well with Django, so let's just address the narrower case, and use {} for empty non-form data, and QueryDict() for empty form data.

@tomchristie
Copy link
Member

Also for consideration: sucky clients that might always include the Content-Type: application/json header, even when no content is included.

That sounds like quite a feasible case, and I don't think that raising exceptions for those clients (empty string isn't valid JSON) is something we want to be doing.

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

Successfully merging this pull request may close these issues.

5 participants