You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/12/29 10:02:21 UTC
[GitHub] [apisix] MekelCon opened a new pull request, #8587: Fix issue 8511
MekelCon opened a new pull request, #8587:
URL: https://github.com/apache/apisix/pull/8587
### Description
In the response.lua, improve management of the content type of the response.
If the response is a json, and if the original request accept it, we set the content-type to "application/json".
In other case we keep the old response type (text/plain)
Fixes # (8511)
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [?] I have updated the documentation to reflect this change
- [?] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8587:
URL: https://github.com/apache/apisix/pull/8587#discussion_r1060291836
##########
apisix/core/response.lua:
##########
@@ -80,7 +115,20 @@ function resp_exit(code, ...)
end
if idx > 0 then
- ngx_print(concat_tab(t, "", 1, idx))
+ local response = concat_tab(t, "", 1, idx)
+ local _, err = decode_json(response)
+ if not err then
+ local content_type = ngx_header["Content-type"]
+ local accept_header = ngx.req.get_headers()["Accept"]
Review Comment:
We can use https://github.com/apache/apisix/blob/c0c906eb4d62886ffa4b0938b017dc4810cfe639/apisix/core/request.lua#L104 which gets the header from cache
##########
apisix/core/response.lua:
##########
@@ -80,7 +115,20 @@ function resp_exit(code, ...)
end
if idx > 0 then
- ngx_print(concat_tab(t, "", 1, idx))
+ local response = concat_tab(t, "", 1, idx)
+ local _, err = decode_json(response)
Review Comment:
It would be more effective to check the header first before decoding, so we can save lots of unnecessary decoding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] MekelCon commented on a diff in pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
Posted by GitBox <gi...@apache.org>.
MekelCon commented on code in PR #8587:
URL: https://github.com/apache/apisix/pull/8587#discussion_r1061184787
##########
t/core/response.t:
##########
@@ -199,3 +199,39 @@ aaa:
GET /t
--- response_body
hello world
+
+
+
+=== TEST 9: exit with string then content-type is text/plain
+--- config
+ location = /t {
+ access_by_lua_block {
+ local core = require("apisix.core")
+ core.response.exit(201, "done\n")
+ }
+ }
+--- request
+GET /t
+--- error_code: 201
+--- response_body
+done
+--- response_headers
+Content-Type: text/plain
+
+
+
+=== TEST 10: exit with table then content-type is application/json
Review Comment:
Fixed by [Add test : request not accepting application/json](https://github.com/apache/apisix/pull/8587/commits/e8d205a7b9d2467a72e38ee5b8e0b29b11bab23e)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] MekelCon commented on pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
Posted by GitBox <gi...@apache.org>.
MekelCon commented on PR #8587:
URL: https://github.com/apache/apisix/pull/8587#issuecomment-1370557320
@spacewander I saw the CI failed due to this line, 121 :
`local accept_header = core_request.header(ctx, "Accept")`
Because ctx is 'not defined'
Wich is true, but it works, i am a beginner in .lua so maybe there is something i am missing.
To have the correct syntax should we add the `ctx `parameter to the method resp_exit ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8587:
URL: https://github.com/apache/apisix/pull/8587#discussion_r1070880929
##########
apisix/init.lua:
##########
@@ -372,7 +372,7 @@ function _M.handle_upstream(api_ctx, route, enable_websocket)
local upstream = apisix_upstream.get_by_id(up_id)
if not upstream then
if is_http then
- return core.response.exit(502)
+ return core.response.exit(core.ctx, 502)
Review Comment:
Sorry, what does the core.ctx here mean? Maybe it should be `ctx` (or sometimes called `api_ctx`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8587:
URL: https://github.com/apache/apisix/pull/8587#discussion_r1070880929
##########
apisix/init.lua:
##########
@@ -372,7 +372,7 @@ function _M.handle_upstream(api_ctx, route, enable_websocket)
local upstream = apisix_upstream.get_by_id(up_id)
if not upstream then
if is_http then
- return core.response.exit(502)
+ return core.response.exit(core.ctx, 502)
Review Comment:
Sorry, what does the core.ctx here mean? It should be `ctx` (or sometimes called `api_ctx`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #8587:
URL: https://github.com/apache/apisix/pull/8587#issuecomment-1367740008
Please make the CI pass, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] github-actions[bot] commented on pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8587:
URL: https://github.com/apache/apisix/pull/8587#issuecomment-1595698484
This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] MekelCon commented on pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
Posted by GitBox <gi...@apache.org>.
MekelCon commented on PR #8587:
URL: https://github.com/apache/apisix/pull/8587#issuecomment-1367785438
> Please make the CI pass, thanks!
Should be done now !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #8587:
URL: https://github.com/apache/apisix/pull/8587#issuecomment-1371808519
> @spacewander I saw the CI failed due to this line, 121 : `local accept_header = core_request.header(ctx, "Accept")` Because `ctx` is 'not defined' Wich is true, but it works, i am a beginner in .lua so maybe there is something i am missing.
>
> To have the correct syntax should we add the `ctx `parameter to the method resp_exit ?
I think a bit it seems that there is no better way to do it. It's OK to add the parameter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] github-actions[bot] commented on pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8587:
URL: https://github.com/apache/apisix/pull/8587#issuecomment-1555878489
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] github-actions[bot] closed pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
URL: https://github.com/apache/apisix/pull/8587
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8587:
URL: https://github.com/apache/apisix/pull/8587#discussion_r1061151260
##########
t/core/response.t:
##########
@@ -199,3 +199,39 @@ aaa:
GET /t
--- response_body
hello world
+
+
+
+=== TEST 9: exit with string then content-type is text/plain
+--- config
+ location = /t {
+ access_by_lua_block {
+ local core = require("apisix.core")
+ core.response.exit(201, "done\n")
+ }
+ }
+--- request
+GET /t
+--- error_code: 201
+--- response_body
+done
+--- response_headers
+Content-Type: text/plain
+
+
+
+=== TEST 10: exit with table then content-type is application/json
Review Comment:
Let's add a test in that we don't set the content-type because the Accept header is not matched.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] MekelCon commented on a diff in pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
Posted by GitBox <gi...@apache.org>.
MekelCon commented on code in PR #8587:
URL: https://github.com/apache/apisix/pull/8587#discussion_r1060372343
##########
apisix/core/response.lua:
##########
@@ -80,7 +115,20 @@ function resp_exit(code, ...)
end
if idx > 0 then
- ngx_print(concat_tab(t, "", 1, idx))
+ local response = concat_tab(t, "", 1, idx)
+ local _, err = decode_json(response)
Review Comment:
Fiexed by @MekelCon
[perf: decode json later](https://github.com/apache/apisix/pull/8587/commits/b4b60d28eb1ec0b1c0078633219d846e7be839a6)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] MekelCon commented on pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
Posted by "MekelCon (via GitHub)" <gi...@apache.org>.
MekelCon commented on PR #8587:
URL: https://github.com/apache/apisix/pull/8587#issuecomment-1432582106
> Hi, @MekelCon Will you continue to complete this PR ?
Yes i will !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] MekelCon commented on pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
Posted by GitBox <gi...@apache.org>.
MekelCon commented on PR #8587:
URL: https://github.com/apache/apisix/pull/8587#issuecomment-1375273053
> > > > @spacewander I saw the CI failed due to this line, 121 : `local accept_header = core_request.header(ctx, "Accept")` Because `ctx` is 'not defined' Wich is true, but it works, i am a beginner in .lua so maybe there is something i am missing.
> > > > To have the correct syntax should we add the `ctx `parameter to the method resp_exit ?
> > >
> > >
> > > I think a bit it seems that there is no better way to do it. It's OK to add the parameter.
> >
> >
> > @spacewander , so i checked quickly, i see at least 93 call to `response.exit(.......)` SO before doing the update i want to be sure of what need to be done.
> > As today we use this function with this definition : `function resp_exit(code, ...)`
> > i must update it to : `function resp_exit(ctx, code, ...)`
> > And then it means check all the usage to add the parameter ? For example `return core.response.exit(502)` will become `return core.response.exit(ctx, 502)`
>
> Yes, you are right
1st try done, i must say i am not very confident about what i did :/
At least i can confirm Apisix still run locally, and my test are passing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] MekelCon commented on pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
Posted by GitBox <gi...@apache.org>.
MekelCon commented on PR #8587:
URL: https://github.com/apache/apisix/pull/8587#issuecomment-1372033540
> > @spacewander I saw the CI failed due to this line, 121 : `local accept_header = core_request.header(ctx, "Accept")` Because `ctx` is 'not defined' Wich is true, but it works, i am a beginner in .lua so maybe there is something i am missing.
> > To have the correct syntax should we add the `ctx `parameter to the method resp_exit ?
>
> I think a bit it seems that there is no better way to do it. It's OK to add the parameter.
@spacewander ,
so i checked quickly,
i see at least 93 call to `response.exit(.......)`
SO before doing the update i want to be sure of what need to done.
As today we use this function with this definition :
`function resp_exit(code, ...)`
i must update it to :
`function resp_exit(ctx, code, ...)`
And then it means check all the usage to add the parameter ? For example
`return core.response.exit(502)`
will become
`return core.response.exit(ctx, 502)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] MekelCon commented on a diff in pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
Posted by GitBox <gi...@apache.org>.
MekelCon commented on code in PR #8587:
URL: https://github.com/apache/apisix/pull/8587#discussion_r1060372658
##########
apisix/core/response.lua:
##########
@@ -80,7 +115,20 @@ function resp_exit(code, ...)
end
if idx > 0 then
- ngx_print(concat_tab(t, "", 1, idx))
+ local response = concat_tab(t, "", 1, idx)
+ local _, err = decode_json(response)
+ if not err then
+ local content_type = ngx_header["Content-type"]
+ local accept_header = ngx.req.get_headers()["Accept"]
Review Comment:
fixed by [get Accept header from cache](https://github.com/apache/apisix/pull/8587/commits/5e439faa537a97d825699059cc71e3b17df78aa9)
##########
apisix/core/response.lua:
##########
@@ -80,7 +115,20 @@ function resp_exit(code, ...)
end
if idx > 0 then
- ngx_print(concat_tab(t, "", 1, idx))
+ local response = concat_tab(t, "", 1, idx)
+ local _, err = decode_json(response)
Review Comment:
Fiexed by
[perf: decode json later](https://github.com/apache/apisix/pull/8587/commits/b4b60d28eb1ec0b1c0078633219d846e7be839a6)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
Posted by "soulbird (via GitHub)" <gi...@apache.org>.
soulbird commented on PR #8587:
URL: https://github.com/apache/apisix/pull/8587#issuecomment-1430642850
Hi, @MekelCon Will you continue to complete this PR ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #8587: fix: issue 8511 The format of response body that returned by plugin should match Content-Type
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #8587:
URL: https://github.com/apache/apisix/pull/8587#issuecomment-1373219611
> > > @spacewander I saw the CI failed due to this line, 121 : `local accept_header = core_request.header(ctx, "Accept")` Because `ctx` is 'not defined' Wich is true, but it works, i am a beginner in .lua so maybe there is something i am missing.
> > > To have the correct syntax should we add the `ctx `parameter to the method resp_exit ?
> >
> >
> > I think a bit it seems that there is no better way to do it. It's OK to add the parameter.
>
> @spacewander , so i checked quickly, i see at least 93 call to `response.exit(.......)` SO before doing the update i want to be sure of what need to be done.
>
> As today we use this function with this definition : `function resp_exit(code, ...)`
>
> i must update it to : `function resp_exit(ctx, code, ...)`
>
> And then it means check all the usage to add the parameter ? For example `return core.response.exit(502)` will become `return core.response.exit(ctx, 502)`
Yes, you are right
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org