feat: Body transformer plugin by kingluo · Pull Request #8766 · apache/apisix

kingluo · 2023-02-01T06:17:43Z

Description

This plugin is used to transform the request or response body from one
format to another format, e.g. JSON to XML.

It depends on xml2lua and lua-resty-template.

Use cases:

simple SOAP proxy
generic template-based transform

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 first)

spacewander · 2023-02-07T02:44:31Z

+
+
+-- luacheck: ignore
+function string.escape_xml(s)


Could we register these methods before we use them and deregister once the job is done? So we can avoid affecting other modules.

luacheck has no inline option to depress "setting undefine field of global" warning. But depress the whole lua file is not proper. And escape_* has no conflict so far, so I think it's ok to use these names.

The solution is not about making luacheck happy, but limiting the scope of the injection. If we don't deregister the methods, it will affect other modules including those not written by us.
Anyway, it is not a good idea to allow the monkey patch of the std lib outside our scope.

Yes, I know. But setting methods on demand on the global object will make luacheck warns: setting undefined field escape_xml of global string, which could not be depressed via inline comment, so ignore the whole file in .luacheckrc instead?

spacewander · 2023-02-07T02:46:31Z

+function _M.body_filter(conf, ctx)
+    if conf.response then
+        local body = core.response.hold_body_chunk(ctx)
+        if ngx.arg[2] == false and not body then


Suggested change

if ngx.arg[2] == false and not body then

if not body then

hold_body_chunk returns a body only when ngx.arg[2] is true

No, this plugiin needs to handle body=nil case.

spacewander · 2023-02-07T02:48:12Z

+        if type(k) == "string" then
+            k = k:match(".*:(.*)")
+            if k then
+                tbl[k] = v


It seems this code just inserts a new k

spacewander · 2023-02-07T02:49:40Z

+end
+
+
+function _M.rewrite(conf, ctx)


rewrite is only for plugins that require running before other plugins. For example, plugins to select consumers, plugins for tracing, plugins to set real IP, and so on. Most of the other plugin use access phase.

This plugin needs to use original content-type header before proxy-rewrite plugin overrrdes it.

And this plugin is just doing rewrite job, just like proxy-rewrite plugin.

We used to have a discussion to unify the "rewrite/access" methods in 2020.
Since this is not the only plugin that has inconsistent "rewrite/access" methods, and you insist on it, I am OK to accept it.

spacewander · 2023-02-07T03:20:20Z

+
+
+local function set_input_format(conf, typ, ct)
+    if conf[typ].input_format == nil and ct then


The conf is shared between reqs. Is there a race if the previous request set the input format different from the current one?

spacewander · 2023-02-07T03:36:53Z

+            local httpc = http.new()
+            local res, err = httpc:request_uri(uri, opt)
+            if not res then
+                ngx.say(err)


We should check --- response_body to ensure no err happens. Please update other similar places.

spacewander · 2023-02-07T03:37:38Z

+        }
+    }
+--- error_log
+attempt to call global 'name' (a string value)


Please add log prefix to ensure it comes from an expected log

spacewander · 2023-02-07T03:38:32Z

+            local uri = "http://127.0.0.1:" .. ngx.var.server_port .. "/foobar?name=hello"
+            local opt = {method = "GET"}
+            local httpc = http.new()
+            httpc:request_uri(uri, opt)


We need to check the res here?

spacewander · 2023-02-07T03:39:08Z

+| ----------- | ----------- | ----------- | ----------- |
+| `request`      | object       | False      | request body transformation configuration      |
+| `request.input_format`      | string       | False      | request body original format, if not specified, it would be determined from `Content-Type` header.      |
+| `request.template`      | string       | False      | request body transformation template       |


The template is required

spacewander · 2023-02-07T06:17:59Z

+| `request.template`      | string       | False      | request body transformation template       |
+| `response`      | object       | False      | response body transformation configuration      |
+| `response.input_format`      | string       | False      | response body original format       |
+| `response.template`      | string       | False      | response body transformation template, if not specified, it would be determined from `Content-Type` header.       |


Do you mean determining the input_format from Content-Type header?

spacewander · 2023-02-09T05:36:00Z

+
+function _M.body_filter(_, ctx)
+    local conf = ctx.body_transformer_conf
+    if conf.response then


It seems if neither conf.request nor conf.response are given, the conf will be nil here.
We should check if one of them is configured at least in check_schema.

* upstream/master: docs: change the file name to 'create-ssl.py'.If 'ssl.py' is used as … (apache#8623) feat: Body transformer plugin (apache#8766) fix: mocking plugin panic when response_example contain $ (apache#8810) (apache#8816) feat: file logger plugin support response body in variable (apache#8711) docs(getting-started): rewrite the install section (apache#8807) feat: allow each logger to define custom log format in its conf (apache#8806) fix(etcd): reloaded data may be in res.body.node (apache#8736) fix: fix fetch all service info from consul (apache#8651) docs: fix global-rule.md wrong curl address (apache#8793) feat: stream subsystem support consul service discovery (apache#8696) chore(kafka-logger): support configuration `meta_refresh_interval` parameter (apache#8762) feat: ready to release 2.15.2 (apache#8783)

kingluo added 15 commits January 17, 2023 10:47

feat: body-transformer plugin

ef79880

add code

be8059e

fix PR

b126acc

fix PR

bc5016c

Create body-transformer.md

7c5d0a8

Update config.json

f477ecc

fix t/admin/plugins.t

0e72138

fix conf/config-default.yaml

bd8b78c

add test

238c859

fix PR

e72486c

fix code

1ced007

add more test cases

739b9d9

fix doc

4df346c

fix lint

b4f13c3

Update body-transformer.md

7e0c480

kingluo marked this pull request as ready for review February 6, 2023 09:27

spacewander reviewed Feb 7, 2023

View reviewed changes

kingluo added 3 commits February 7, 2023 16:33

fix PR

9513b30

revert escape_xml and escape_json

a9d77cc

fix PR

ae46b46

spacewander reviewed Feb 9, 2023

View reviewed changes

fix schema

7442d51

spacewander approved these changes Feb 9, 2023

View reviewed changes

soulbird approved these changes Feb 9, 2023

View reviewed changes

spacewander merged commit cfeadfb into apache:master Feb 10, 2023



		local function set_input_format(conf, typ, ct)
		if conf[typ].input_format == nil and ct then

Uh oh!

Conversation

kingluo commented Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kingluo commented Feb 1, 2023 •

edited

Loading