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

Dont try to parse JSON if the statusCode is 204 (No Content) #69

Merged
merged 1 commit into from Dec 15, 2014

Conversation

@Torsph
Copy link
Contributor

Torsph commented Dec 12, 2014

I dont think that wreck should try to parse json when the status code is 204 no content.
We keep getting '''Unexpected end of line''' and other errors.

I have created a test and updated the code not to parse JSON if the statusCode is 204.

If there is any problem with it, just let me know :)

@kanongil

This comment has been minimized.

Copy link
Member

kanongil commented Dec 12, 2014

This looks useful, though 204 responses are not allowed to contain any content. If there is any content, the response is malformed and I guess some kind of error should be returned?

@Torsph

This comment has been minimized.

Copy link
Contributor Author

Torsph commented Dec 13, 2014

An error will be ok if you own the api your self. However, it will be a problem if you dont.
One could maybe just log a warning?

@kanongil

This comment has been minimized.

Copy link
Member

kanongil commented Dec 13, 2014

Considering http://support.microsoft.com/KB/246921, which returns content in a 204 response, disregarding the spec, it would seem that you need to expect this non-spec case. Ie. your patch must be the right way to handle this.

The only sticking issue I see, is what payload data is returned. Eg. should it return the actual (non-spec) response data, return a zero length buffer, or null? And should it be different for JSON vs. non-JSON responses? Your implementation always returns the buffer, which I'm not sure is the best solution.

@leftieFriele

This comment has been minimized.

Copy link

leftieFriele commented Dec 15, 2014

I think it makes sense to return a buffer / zero length buffer, which leaves Wreck with no opinion on the matter. 204 responses are just returned with a buffer with a zero or larger length. If you own client and API you are still free to implement some semantic like in the sample by reading the buffer and creating JSON.

@kanongil

This comment has been minimized.

Copy link
Member

kanongil commented Dec 15, 2014

@leftieFriele That seems to align with the bare-bones nature of this module. My only concern is the JSON response, where consumers might be surprised by receiving a Buffer. Note, if you really need access to the raw response buffer, you can always disable JSON response parsing.

@Torsph

This comment has been minimized.

Copy link
Contributor Author

Torsph commented Dec 15, 2014

We can make it so that it will parse the response, but it should not return an error if it cannot parse an response on 204.

@kanongil

This comment has been minimized.

Copy link
Member

kanongil commented Dec 15, 2014

Personally I'm inclined to say that whenever the json option is true, 204 responses should always return null (even for other content-types).

  • null because there is no other sensible way to represent an empty JSON object.
  • For all content-types, to accommodate servers that forget to set application/json for 204 responses.

This could be implemented through an emptyObject option, where the consumer can configure the reply. Ie. depending on the application, you can configure it to return null, {}, [], or whatever. This also allows the functionality to be implemented without breaking the existing behavior, which is to return an error.

@gergoerdosi

This comment has been minimized.

Copy link

gergoerdosi commented Dec 15, 2014

I'm starting to feel this is actually not an issue in Wreck. A 204 response must not return a message body.

The 204 response MUST NOT include a message-body, and thus is always terminated by the first empty line after the header fields.

I see two options:

  1. Support the HTTP specification. If the message body contains something, then ignore it or return a bad request error.
  2. Support non-standard HTTP responses (like IIS). In this case something is returned in the message body. We should use the content-type header to determine how to deal with the message body. If content-type is application/json, then parse it.

The question is what to do when there is no message body, but content-type is set to application/json. I think it's a bad request too. The description of the content-type header:

The Content-Type entity-header field indicates the media type of the entity-body sent to the recipient or, in the case of the HEAD method, the media type that would have been sent had the request been a GET.

So it indicates the media type of the message body. And an empty message body is not a valid JSON. So in this case the error is in the client, it should not set the content-type header.

@leftieFriele

This comment has been minimized.

Copy link

leftieFriele commented Dec 15, 2014

@gergoerdosi I agree that complying with the standards is a good thing. however wouldn't implementing it break a lot of existing applications?

@kanongil 👍 on your suggested solution

@geek

This comment has been minimized.

Copy link
Member

geek commented Dec 15, 2014

Instead, what if we don't try to parse when the buffer is empty?

@Torsph

This comment has been minimized.

Copy link
Contributor Author

Torsph commented Dec 15, 2014

That is also a solution I can live with.

@geek

This comment has been minimized.

Copy link
Member

geek commented Dec 15, 2014

@Torsph mind updating the PR to not try to parse an empty buffer? It should likely return null in this case.

@Torsph

This comment has been minimized.

Copy link
Contributor Author

Torsph commented Dec 15, 2014

Will do.

@Torsph Torsph force-pushed the Torsph:http_204_dont_parse_json branch from 3367fd0 to 564a105 Dec 15, 2014
@Torsph

This comment has been minimized.

Copy link
Contributor Author

Torsph commented Dec 15, 2014

@geek Updated with a simple if check on buffer length. And just return null on the result.

@@ -231,6 +231,10 @@ exports.read = function (res, options, callback) {
return callback(null, buffer);
}

if(buffer.length === 0) {

This comment has been minimized.

Copy link
@geek

geek Dec 15, 2014

Member

if (

Missing a space after the if.

@@ -231,6 +231,10 @@ exports.read = function (res, options, callback) {
return callback(null, buffer);
}

if(buffer.length === 0) {
return callback(null, null);

This comment has been minimized.

Copy link
@geek

geek Dec 15, 2014

Member

Might be an issue with the github source viewer, but looks like this needs 4 spaces

This comment has been minimized.

Copy link
@Torsph

Torsph Dec 15, 2014

Author Contributor

Arg, just my home computer format that is wrong.

@Torsph Torsph force-pushed the Torsph:http_204_dont_parse_json branch 2 times, most recently from abec9d1 to 4ab026d Dec 15, 2014

var server = Http.createServer(function (req, res) {

res.writeHead(200, { 'Content-Type': 'application/json' });

This comment has been minimized.

Copy link
@geek

geek Dec 15, 2014

Member

It doesn't matter too much, but might want to change to return 204, covers the original issue.

This comment has been minimized.

Copy link
@Torsph

Torsph Dec 15, 2014

Author Contributor

done

@Torsph Torsph force-pushed the Torsph:http_204_dont_parse_json branch from 4ab026d to 3e04c72 Dec 15, 2014
@geek geek added the bug label Dec 15, 2014
@geek geek added this to the 5.1.0 milestone Dec 15, 2014
@geek geek self-assigned this Dec 15, 2014
geek added a commit that referenced this pull request Dec 15, 2014
Dont try to parse JSON if the statusCode is 204 (No Content)
@geek geek merged commit 207ebaf into hapijs:master Dec 15, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.