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

Fix invalid response for empty reply() (v8.x regression) #2277

Closed
wants to merge 1 commit into from

Conversation

@kanongil
Copy link
Contributor

@kanongil kanongil commented Dec 11, 2014

The content-type header for empty responses used to not be set when calling reply(null), reply(''), etc in the route handler.

In 8.0 the header is set as application/json. This is a problem, as an empty string is not valid JSON, causing eg. $.ajax() to cast a TypeError when in JSON mode.

This patch changes the content-type to plain text/html for empty responses, which is valid.

@kanongil kanongil added the bug label Dec 11, 2014
@gergoerdosi
Copy link
Contributor

@gergoerdosi gergoerdosi commented Dec 12, 2014

I'm not sure about this change. If client sets Accept: application/json then returning text/html may be unexpected.

I think this is more of a client error. An empty response should use the status code 204. And in that case client should not try to parse the response. This has been actually reported:

http://bugs.jquery.com/ticket/13292
jquery/jquery#1142

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Dec 12, 2014

I'm pretty sure it's a bad idea to change the response to a 204 code in a generic way, and anyway is not within the scope of a bug fix.

Regarding Accepts, this should already be handled at the application level by explicitly specifying the mime type, and if you return an empty JSON response without a 204 code, it is an application bug.

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Dec 12, 2014

If this is likely to cause issues, I'm open to changing it to only set text/html when you reply('') which is clearly intended as text. Another possible fix is to revert to the old behavior and not set the content-type header at all.

@gergoerdosi
Copy link
Contributor

@gergoerdosi gergoerdosi commented Dec 12, 2014

I'm pretty sure it's a bad idea to change the response to a 204 code in a generic way, and anyway is not within the scope of a bug fix.

I didn't suggest to change the status code in hapi. I meant developers should use reply().code(204) instead of reply(null, null) in the handler.

Actually, I'm not sure your proposed change can be considered a bug fix. It can be considered a breaking change too as it changes the response. If clients are configured to take action based on the type of the response, then this change will break their behavior. Application code needs to be changed too (reply().code(204) to reply().code(204).type('application/json')).

Regarding Accepts, this should already be handled at the application level by explicitly specifying the mime type, and if you return an empty JSON response without a 204 code, it is an application bug.

You are right. It is an application bug and your proposed change makes sense now.

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Dec 12, 2014

The bug fix can indeed be considered a breaking change depending on which angle you look at it from.

Since the changed behavior is not mentioned in #2186, and we are very early in the current release, I want to label it as a bug fix for an unannounced regression. In any case, the docs doesn't mention what content-type can be expected from an empty reply(). Also note my earlier suggestions towards a less invasive fix.

@gergoerdosi
Copy link
Contributor

@gergoerdosi gergoerdosi commented Dec 12, 2014

Your proposed change makes sense, an empty 200 response with the application/json header is just wrong.

Maybe the old behavior was correct in some way, because a reply().code(204) should not set the content-type header. In case of a 204 response text/html is just as good/wrong as application/json. In any case, now that hapi 8.0.0 is out, both behavior (old and your proposed) would be a breaking change. However I would like to get this fixed too.

@leftieFriele
Copy link

@leftieFriele leftieFriele commented Dec 12, 2014

We encountered the same issue. Issuing a request with Wreck and replying reply().code(204) and we get 'Unexpected end of line' when Wreck tries to parse the response.
The proposed change solves the problem and the solution makes sense.

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Dec 12, 2014

@leftieFriele That sounds like a bug in Wreck. It should not try to parse a response with code 204.

Edit: Related wreck pull request: hapijs/wreck#69.

@arb
Copy link
Contributor

@arb arb commented Dec 15, 2014

I think reply('') should set the header to "text/html". Any other falsy values (null or undefined) should leave the header unset IMO.

@hueniverse hueniverse closed this in 7e1722d Jan 5, 2015
@hueniverse hueniverse self-assigned this Jan 5, 2015
@hueniverse hueniverse added this to the 8.1.0 milestone Jan 5, 2015
@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants