fix(responses) to not set content type header for 204 (no content) #3351
Conversation
kong/tools/responses.lua
Outdated
| @@ -123,7 +123,11 @@ local function send_response(status_code) | |||
| end | |||
|
|
|||
| ngx.status = status_code | |||
| ngx.header["Content-Type"] = "application/json; charset=utf-8" | |||
|
|
|||
| if status_code ~= 204 then | |||
There was a problem hiding this comment.
Should it be _M.status_codes.HTTP_NO_CONTENT instead of 204?
| @@ -123,7 +123,11 @@ local function send_response(status_code) | |||
| end | |||
|
|
|||
| ngx.status = status_code | |||
| ngx.header["Content-Type"] = "application/json; charset=utf-8" | |||
There was a problem hiding this comment.
Does it really fix #3342?
Here it is about application/json and in the issue it was text/plain.
There was a problem hiding this comment.
@aleksandr-vin good point, does this come out of Nginx custom error pages, then?
Edit: actually not, it comes from Nginx directive default_type being text/plain (updated PR to remove the default).
There was a problem hiding this comment.
@aleksandr-vin I made a new push, this one solves text/plain case as well.
There was a problem hiding this comment.
I don't think this should be removed? This is Kong-produced responses, not upstream responses.
There was a problem hiding this comment.
^- that was discussed below.
bungle
force-pushed
the
fix/remove-content-type-204
branch
from
685f80f to
f65b9fa
Compare
kong/templates/nginx_kong.lua
Outdated
| @@ -1,5 +1,6 @@ | |||
| return [[ | |||
| charset UTF-8; | |||
| default_type ''; | |||
There was a problem hiding this comment.
unfortunately any configuration template change means it is breaking (for custom nginx configs) and should be opened against next instead...
There was a problem hiding this comment.
Also, all changed applied to nginx_kong.lua must be applied to the test template as well. And also, it would be better to nest this setting under the proxy server {} block.
There was a problem hiding this comment.
Thanks for reminding me about the test ones.
There was a problem hiding this comment.
Test template is updated. This is now rebased on next
There was a problem hiding this comment.
I think we should still have it moved to the proxy server {} block?
There was a problem hiding this comment.
I moved it to proxy location / block. I think it is fine like that.
kong/tools/responses.lua
Outdated
| @@ -144,6 +143,7 @@ local function send_response(status_code) | |||
| ngx.log(ngx.ERR, "[admin] could not encode value: ", err) | |||
| end | |||
|
|
|||
| ngx.header["Content-Type"] = "application/json; charset=utf-8" | |||
There was a problem hiding this comment.
Oh, you moved it here... Better not moving it down here unnecessarily? This instruction is currently voluntarily placed before the response body is produced for cognitive reasons.
There was a problem hiding this comment.
There was a problem hiding this comment.
Yeah, maybe better to keep it in this branch then (but if we hit an error with the JSON-encoding, we might not want to set this header either).
There was a problem hiding this comment.
Yes, I made it only happen when we are actually outputting someting.
bungle
force-pushed
the
fix/remove-content-type-204
branch
from
f65b9fa to
8b2726b
Compare
bungle
force-pushed
the
fix/remove-content-type-204
branch
from
8b2726b to
9e98db8
Compare
bungle
changed the title
bungle
force-pushed
the
fix/remove-content-type-204
branch
from
9e98db8 to
7074537
Compare
| ngx.say(encoded) | ||
| else | ||
| ngx.header["Content-Type"] = "application/json; charset=utf-8" | ||
| ngx.say(encoded) |
There was a problem hiding this comment.
👍
We may need some unit tests for this (they exist for this module already)
There was a problem hiding this comment.
Ah, my bad, I see you already added it :)
|
Let's wait on CI and merge if it passes, thanks! |
Summary
When there is no content (204 status code), there is no need to set Content-Type header. It may even be misleading.
Issues resolved
Fix #3342