spacewander commented on a change in pull request #6750:
URL: https://github.com/apache/apisix/pull/6750#discussion_r840194071
##########
File path: apisix/plugins/response-rewrite.lua
##########
@@ -115,6 +152,23 @@ function _M.check_schema(conf)
end
end
+ if conf.filters then
+ for _, filter in ipairs(conf.filters) do
+ for field, value in pairs(filter) do
Review comment:
We can use `require` in the schema to do the check.
##########
File path: docs/en/latest/plugins/response-rewrite.md
##########
@@ -32,13 +32,25 @@ response rewrite plugin, rewrite the content returned by
the upstream as well as
## Attributes
-| Name | Type | Requirement | Default | Valid | Description
|
-| ----------- | ------- | ----------- | ------- | ---------- |
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
|
-| status_code | integer | optional | | [200, 598] | New `status
code` to client, keep the original response code by default.
|
-| body | string | optional | | | New `body` to
client, and the content-length will be reset too.
|
-| body_base64 | boolean | optional | false | | Identify if
`body` in configuration need base64 decoded before rewrite to client.
|
-| headers | object | optional | | | Set the new
`headers` for client, can set up multiple. If it exists already from upstream,
will rewrite the header, otherwise will add the header. You can set the
corresponding value to an empty string to remove a header. The value can
contain Nginx variables in `$var` format, like `$remote_addr $balancer_ip` |
-| vars | array[] | optional | | | A DSL to evaluate
with the given ngx.var. See `vars`
[lua-resty-expr](https://github.com/api7/lua-resty-expr#operator-list). if the
`vars` is empty, then all rewrite operations will be executed unconditionally |
+| Name | Type | Requirement | Default | Valid | Description
|
+|-------------|---------|-------------|---------|------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| status_code | integer | optional | | [200, 598] | New `status
code` to client, keep the original response code by default.
|
+| body | string | optional | | | New `body` to
client, and the content-length will be reset too.
|
+| body_base64 | boolean | optional | false | | Identify if
`body` in configuration need base64 decoded before rewrite to client.
|
+| headers | object | optional | | | Set the new
`headers` for client, can set up multiple. If it exists already from upstream,
will rewrite the header, otherwise will add the header. You can set the
corresponding value to an empty string to remove a header. The value can
contain Nginx variables in `$var` format, like `$remote_addr $balancer_ip`. |
+| vars | array[] | optional | | | A DSL to
evaluate with the given ngx.var. See `vars`
[lua-resty-expr](https://github.com/api7/lua-resty-expr#operator-list). if the
`vars` is empty, then all rewrite operations will be executed unconditionally.
|
+| filters | array[] | optional | | | A group of
filters that modify response body by replacing one specified string by another.
|
+
+Only one of `body`, `filters` can be specified.
+
+### Attributes of filters
+
+| Name | Type | Requirement | Default | Valid | Description
|
+|---------|--------|-------------|---------|----------------|------------------------------------------------------------------------------------------------|
+| regex | string | required | | | match pattern on
response body. |
+| scope | string | required | "one" | "one","global" | substitution
range.
|
Review comment:
We should provide the doc of the nested field like `tls.client_cert` in
https://github.com/apache/apisix/blob/master/docs/en/latest/admin-api.md.
And it is optional as people don't need to configure it.
##########
File path: apisix/plugins/response-rewrite.lua
##########
@@ -126,7 +180,24 @@ function _M.body_filter(conf, ctx)
return
end
- if conf.body then
+ if conf.filters then
+
+ local body = core.response.hold_body_chunk(ctx)
+ if not body then
+ return
+ end
+
+ for _, filter in ipairs(conf.filters) do
+ if filter.scope == "once" then
+ body = re_sub(body, filter.regex, filter.replace,
filter.options)
+ else
+ body = re_gsub(body, filter.regex, filter.replace,
filter.options)
+ end
+ end
+
+ ngx.arg[1] = body
+
Review comment:
Would be better to `return` from here
##########
File path: apisix/plugins/response-rewrite.lua
##########
@@ -148,7 +219,8 @@ function _M.header_filter(conf, ctx)
ngx.status = conf.status_code
end
- if conf.body then
+ -- fixme: if filters have no any match, response body won't be modified.
Review comment:
```suggestion
-- if filters have no any match, response body won't be modified.
```
##########
File path: t/plugin/response-rewrite.t
##########
@@ -699,3 +699,512 @@ 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: add plugin with valid filters
Review comment:
Let's move the new test to t/plugin/response-rewrite2.t. Otherwise this
test file is too long.
##########
File path: apisix/plugins/response-rewrite.lua
##########
@@ -115,6 +152,23 @@ function _M.check_schema(conf)
end
end
+ if conf.filters then
+ for _, filter in ipairs(conf.filters) do
+ for field, value in pairs(filter) do
+ if type(field) ~= 'string' then
Review comment:
We don't need to check the type of field. It doesn't do any harm as we
only consume known fields.
##########
File path: apisix/plugins/response-rewrite.lua
##########
@@ -115,6 +152,23 @@ function _M.check_schema(conf)
end
end
+ if conf.filters then
+ for _, filter in ipairs(conf.filters) do
+ for field, value in pairs(filter) do
+ if type(field) ~= 'string' then
+ return false, 'invalid type as filter field'
+ end
+ if field ~= "replace" and value == "" then
+ return false, 'invalid value as filter field ' .. field
+ end
+ end
+ local ok, err = re_compile(filter.regex, filter.options)
+ if not ok then
+ return false, err
Review comment:
Better to add a prefix to the err
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
