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/08/17 23:58:49 UTC
[GitHub] [apisix] Hazel6869 opened a new pull request, #7727: feat: upgrade response rewrite policy and add test cases
Hazel6869 opened a new pull request, #7727:
URL: https://github.com/apache/apisix/pull/7727
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
https://github.com/apache/apisix/issues/7722
<!-- Please also include relevant motivation and context. -->
Fixes # (issue)
### Checklist
- [ ] I have explained the need for this PR and the problem it solves
- [ ] I have explained the changes or the new features added to this PR
- [ ] 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)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
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] Hazel6869 commented on pull request #7727: fix: response-rewrite plugin might cause Apache AIPSIX hanging
Posted by GitBox <gi...@apache.org>.
Hazel6869 commented on PR #7727:
URL: https://github.com/apache/apisix/pull/7727#issuecomment-1231456740
> hi @Hazel6869 I think you merged the branches by mistake.
i will fix it, 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] tokers commented on a diff in pull request #7727: fix: response-rewrite plugin might cause Apache AIPSIX hanging
Posted by GitBox <gi...@apache.org>.
tokers commented on code in PR #7727:
URL: https://github.com/apache/apisix/pull/7727#discussion_r957116000
##########
t/plugin/response-rewrite.t:
##########
@@ -702,24 +702,43 @@ X-B: from 127.0.0.1 to 127.0.0.1:1980
-=== TEST 25: set an nil body with setting body_base64 to false
+=== TEST 25: set header(rewrite body with "")
Review Comment:
```suggestion
=== TEST 25: set header and rewrite body to empty
```
--
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] tokers commented on a diff in pull request #7727: bug: response-rewrite plugin might cause Apache AIPSIX hanging
Posted by GitBox <gi...@apache.org>.
tokers commented on code in PR #7727:
URL: https://github.com/apache/apisix/pull/7727#discussion_r954521647
##########
t/plugin/response-rewrite.t:
##########
@@ -699,3 +699,26 @@ X-A: 127.0.0.1
X-B: from 127.0.0.1 to 127.0.0.1:1980
--- no_error_log
[error]
+
+
+
+=== TEST 25: set an nil body with setting body_base64 to false
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.response-rewrite")
+ local ok, err = plugin.check_schema({
Review Comment:
@Hazel6869 You should write another case which really uses the configuration.
--
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] Hazel6869 closed pull request #7727: fix: response-rewrite plugin might cause Apache AIPSIX hanging
Posted by GitBox <gi...@apache.org>.
Hazel6869 closed pull request #7727: fix: response-rewrite plugin might cause Apache AIPSIX hanging
URL: https://github.com/apache/apisix/pull/7727
--
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 #7727: feat: upgrade response rewrite plugin and add test cases
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7727:
URL: https://github.com/apache/apisix/pull/7727#discussion_r949988703
##########
t/plugin/response-rewrite.t:
##########
@@ -699,3 +699,26 @@ X-A: 127.0.0.1
X-B: from 127.0.0.1 to 127.0.0.1:1980
--- no_error_log
[error]
+
+
+
+=== TEST 25: set an nil body with setting body_base64 to false
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.response-rewrite")
+ local ok, err = plugin.check_schema({
Review Comment:
The newly added test doesn't cover the changed part?
--
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] tzssangglass commented on pull request #7727: fix: response-rewrite plugin might cause Apache AIPSIX hanging
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #7727:
URL: https://github.com/apache/apisix/pull/7727#issuecomment-1231072728
hi @Hazel6869 I think you merged the branches by mistake.
--
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 #7727: feat: upgrade response rewrite plugin and add test cases
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #7727:
URL: https://github.com/apache/apisix/pull/7727#issuecomment-1220446575
Please put the issue id after `Fixes #` so it can be linked.
--
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] tzssangglass commented on a diff in pull request #7727: feat: upgrade response rewrite plugin and add test cases
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7727:
URL: https://github.com/apache/apisix/pull/7727#discussion_r949992831
##########
t/plugin/response-rewrite.t:
##########
@@ -699,3 +699,26 @@ X-A: 127.0.0.1
X-B: from 127.0.0.1 to 127.0.0.1:1980
--- no_error_log
[error]
+
+
+
+=== TEST 25: set an nil body with setting body_base64 to false
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.response-rewrite")
+ local ok, err = plugin.check_schema({
+ body_base64 = false
+ })
+ if not ok then
+ ngx.say(err)
+ return
+ end
+ }
+ }
+--- request
+GET /t
+--- response_body
+
+--- no_error_log
+[error]
Review Comment:
This test case would have passed even without the code changes.
--
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