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/04/01 03:45:29 UTC
[GitHub] [apisix] spacewander commented on a change in pull request #6750: feat(response-rewrite): support filters
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org