fix(body-transformer): xml2lua: replace empty table with empty string #9669

kingluo · 2023-06-15T08:13:24Z

Description

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)

apisix/plugins/body-transformer.lua

monkeyDluffy6017 · 2023-06-19T09:24:54Z

t/plugin/body-transformer.t

+location /demo {
+    content_by_lua_block {
+        ngx.print([[
+<SOAP-ENV:Envelope xmlns:SOAP-ENV="http://schemas.xmlsoap.org/soap/envelope/" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xrd="http://x-road.eu/xsd/xroad.xsd" xmlns:prod="http://rr.x-road.eu/producer" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:id="http://x-road.eu/xsd/identifiers" xmlns:repr="http://x-road.eu/xsd/representation.xsd" xmlns:SOAP-ENC="http://schemas.xmlsoap.org/soap/encoding/">
+  <SOAP-ENV:Body>
+    <prod:RR58isikEpiletResponse>
+      <request><Isikukood>33333333333</Isikukood></request>
+      <response>
+        <Isikukood>33333333333</Isikukood>
+        <KOVKood></KOVKood>
+      </response>
+    </prod:RR58isikEpiletResponse>
+  </SOAP-ENV:Body>
+</SOAP-ENV:Envelope>
+        ]])
+    }
+}
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin")
+
+            local rsp_template = ngx.encode_base64[[
+{ "KOVKood":"{{Envelope.Body.RR58isikEpiletResponse.response.KOVKood}}" }
+            ]]
+
+            local code, body = t.test('/apisix/admin/routes/1',
+                ngx.HTTP_PUT,
+                string.format([[{
+                    "uri": "/ws",
+                    "plugins": {
+                        "proxy-rewrite": {
+                            "uri": "/demo"
+                        },
+                        "body-transformer": {
+                            "response": {
+                                "input_format": "xml",
+                                "template": "%s"
+                            }
+                        }
+                    },
+                    "upstream": {
+                        "type": "roundrobin",
+                        "nodes": {
+                            "127.0.0.1:%d": 1
+                        }
+                    }
+                }]], rsp_template, ngx.var.server_port)
+            )
+
+            if code >= 300 then
+                ngx.status = code
+                return
+            end
+            ngx.sleep(0.5)
+
+            local core = require("apisix.core")
+            local http = require("resty.http")
+            local uri = "http://127.0.0.1:" .. ngx.var.server_port .. "/ws"
+            local opt = {method = "GET"}
+            local httpc = http.new()
+            local res = httpc:request_uri(uri, opt)
+            assert(res.status == 200)
+            local data1 = core.json.decode(res.body)
+            local data2 = core.json.decode[[{"KOVKood":""}]]
+            assert(core.json.stably_encode(data1) == core.json.stably_encode(data2))
+        }
+    }


Suggested change

location /demo {

content_by_lua_block {

ngx.print([[

<SOAP-ENV:Envelope xmlns:SOAP-ENV="http://schemas.xmlsoap.org/soap/envelope/" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xrd="http://x-road.eu/xsd/xroad.xsd" xmlns:prod="http://rr.x-road.eu/producer" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:id="http://x-road.eu/xsd/identifiers" xmlns:repr="http://x-road.eu/xsd/representation.xsd" xmlns:SOAP-ENC="http://schemas.xmlsoap.org/soap/encoding/">

<SOAP-ENV:Body>

<prod:RR58isikEpiletResponse>

<request><Isikukood>33333333333</Isikukood></request>

<response>

<Isikukood>33333333333</Isikukood>

<KOVKood></KOVKood>

</response>

</prod:RR58isikEpiletResponse>

</SOAP-ENV:Body>

</SOAP-ENV:Envelope>

]])

}

}

location /t {

content_by_lua_block {

local t = require("lib.test_admin")

local rsp_template = ngx.encode_base64[[

{ "KOVKood":"{{Envelope.Body.RR58isikEpiletResponse.response.KOVKood}}" }

]]

local code, body = t.test('/apisix/admin/routes/1',

ngx.HTTP_PUT,

string.format([[{

"uri": "/ws",

"plugins": {

"proxy-rewrite": {

"uri": "/demo"

},

"body-transformer": {

"response": {

"input_format": "xml",

"template": "%s"

}

}

},

"upstream": {

"type": "roundrobin",

"nodes": {

"127.0.0.1:%d": 1

}

}

}]], rsp_template, ngx.var.server_port)

)

if code >= 300 then

ngx.status = code

return

end

ngx.sleep(0.5)

local core = require("apisix.core")

local http = require("resty.http")

local uri = "http://127.0.0.1:" .. ngx.var.server_port .. "/ws"

local opt = {method = "GET"}

local httpc = http.new()

local res = httpc:request_uri(uri, opt)

assert(res.status == 200)

local data1 = core.json.decode(res.body)

local data2 = core.json.decode[[{"KOVKood":""}]]

assert(core.json.stably_encode(data1) == core.json.stably_encode(data2))

}

}

location /demo {

content_by_lua_block {

ngx.print([[

<SOAP-ENV:Envelope xmlns:SOAP-ENV="http://schemas.xmlsoap.org/soap/envelope/" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xrd="http://x-road.eu/xsd/xroad.xsd" xmlns:prod="http://rr.x-road.eu/producer" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:id="http://x-road.eu/xsd/identifiers" xmlns:repr="http://x-road.eu/xsd/representation.xsd" xmlns:SOAP-ENC="http://schemas.xmlsoap.org/soap/encoding/">

<SOAP-ENV:Body>

<prod:RR58isikEpiletResponse>

<request><Isikukood>33333333333</Isikukood></request>

<response>

<Isikukood>33333333333</Isikukood>

<KOVKood></KOVKood>

</response>

</prod:RR58isikEpiletResponse>

</SOAP-ENV:Body>

</SOAP-ENV:Envelope>

]])

}

}

location /t {

content_by_lua_block {

local t = require("lib.test_admin")

local rsp_template = ngx.encode_base64[[

{ "KOVKood":"{{Envelope.Body.RR58isikEpiletResponse.response.KOVKood}}" }

]]

local code, body = t.test('/apisix/admin/routes/1',

ngx.HTTP_PUT,

string.format([[{

"uri": "/ws",

"plugins": {

"proxy-rewrite": {

"uri": "/demo"

},

"body-transformer": {

"response": {

"template": "%s"

}

}

},

"upstream": {

"type": "roundrobin",

"nodes": {

"127.0.0.1:%d": 1

}

}

}]], rsp_template, ngx.var.server_port)

)

if code >= 300 then

ngx.status = code

return

end

ngx.sleep(0.5)

local core = require("apisix.core")

local http = require("resty.http")

local uri = "http://127.0.0.1:" .. ngx.var.server_port .. "/ws"

local opt = {method = "GET"}

local httpc = http.new()

local res = httpc:request_uri(uri, opt)

assert(res.status == 200)

local data1 = core.json.decode(res.body)

local data2 = core.json.decode[[{"KOVKood":""}]]

assert(core.json.stably_encode(data1) == core.json.stably_encode(data2))

}

}

What's your opinion? Why make this change? Please describe it. Don't leave a long code block without any description. I can not figure out what's the difference.

I format the code for you, just copy and paste it

ok. Please describe the code change next time so that anyone could get a brief.

monkeyDluffy6017 · 2023-06-19T09:25:55Z

apisix/plugins/body-transformer.lua

+        else
+            core.log.warn("no input format to parse ", typ, " body")


It should return 400 if the format is nil

The format is optional! Notice the document for the requirement before you make a suggestion.
Here it's just a warning log to remind the user.

It will return 500 if there are no content-type and input_format

Check this test case, where's 500?

apisix/t/plugin/body-transformer.t

Line 458 in e19a466

=== TEST 7: parse body in yaml format

The conf[typ].input_format is set by set_input_format, so if there is no content_type and input_format, the conf[typ].input_format will be nil, if the format is nil, the out will get wrong.
When i remove the parameter input_format of test 12 which you just added, i get this error:

I think it's better to handle this case, we need one of the content_type and input_format

The input_format nil case is for manual parsing by the user via template code block, which is the expected case in the proposal! For example, for the time being, we don't support yaml parsing, then what if the user needs to parse himself? We should enable him to do this via this plugin, but not 500! We cannot handle all content types in the world. And you should not assume the content type exists, especially for unknown and unmanaged upstream websites. Since we cannot support all types to parse the body, what's the meaning to ensure content_type or input_format exists? After all, for unknown types, the user is in charge of parsing via the template.

In TEST 12, the XML format is expected, so it's expected to accept input_format as XML. The user should ensure the template matches the actual input content. Think that what if the user composes an invalid template even for the correct type and content, it will still raise errors for non-exist fields! Then how to handle this case? That is, you cannot avoid such errors you show only via configuration sanity check!

And, the most important is, please do not argue the proposal and requirement in this PR, if you're in doubt, you can propose your opinion via the mailing list and describe your change request there!

This PR is for empty xml value bugfix only!

* upstream/master: (70 commits) fix(workflow): enhance schema check (apache#9782) docs: add chinese documentation for loki-logger (apache#9687) chore(update): stand-alone text (apache#9736) docs: add Secret chinese document to Admin API (apache#9522) fix(log-rotate): can not keep max files when using custom name (apache#9749) docs: fix typo and added useful information (apache#8900) docs: explain in more details for the batch-requests plugin (apache#9629) docs: update `apisix` section in `config-default.yaml` (apache#9611) chore: add missing `report_interval` option for `skywalking` plugin in `config-default.yaml` (apache#9662) refactor(jwt-auth): remove unused parameter (apache#9716) change(request-id): remove snowflake algorithm (apache#9715) fix test case (apache#9706) docs: add correct link for openresty arm64 repo (apache#9713) fix: get the correct revision (apache#9635) fix(body-transformer): xml2lua: replace empty table with empty string (apache#9669) feat(prometheus): allow user configure DEFAULT_BUCKETS (apache#9673) docs: add example for timeout (apache#9708) docs: replace some urls that point to github with relative paths (apache#9684) docs: update Debian Installation Guide (apache#9680) docs: update how to install apisix on debian (apache#9693) ...

fix(body-transformer): xml2lua: replace empty table with empty string

d164ea7

kingluo requested review from monkeyDluffy6017 and leslie-tsang June 15, 2023 08:13

kingluo added 3 commits June 15, 2023 16:14

fix PR

799aa45

fix PR

3d22efb

fix PR

93d58cb

AlinsRan previously approved these changes Jun 16, 2023

View reviewed changes

leslie-tsang requested changes Jun 19, 2023

View reviewed changes

apisix/plugins/body-transformer.lua Outdated Show resolved Hide resolved

fix PR

54900d3

kingluo dismissed AlinsRan’s stale review via 54900d3 June 19, 2023 02:31

monkeyDluffy6017 reviewed Jun 19, 2023

View reviewed changes

fix PR

90f4720

monkeyDluffy6017 approved these changes Jun 20, 2023

View reviewed changes

monkeyDluffy6017 added the approved label Jun 20, 2023

leslie-tsang approved these changes Jun 25, 2023

View reviewed changes

monkeyDluffy6017 merged commit 632c7c0 into apache:master Jun 25, 2023
37 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(body-transformer): xml2lua: replace empty table with empty string #9669

fix(body-transformer): xml2lua: replace empty table with empty string #9669

kingluo commented Jun 15, 2023

monkeyDluffy6017 Jun 19, 2023

kingluo Jun 19, 2023 •

edited

Loading

monkeyDluffy6017 Jun 19, 2023

kingluo Jun 19, 2023

monkeyDluffy6017 Jun 19, 2023

kingluo Jun 19, 2023

monkeyDluffy6017 Jun 19, 2023

kingluo Jun 19, 2023 •

edited

Loading

monkeyDluffy6017 Jun 19, 2023 •

edited

Loading

kingluo Jun 19, 2023

		else
		core.log.warn("no input format to parse ", typ, " body")

fix(body-transformer): xml2lua: replace empty table with empty string #9669

fix(body-transformer): xml2lua: replace empty table with empty string #9669

Conversation

kingluo commented Jun 15, 2023

Description

Checklist

monkeyDluffy6017 Jun 19, 2023

Choose a reason for hiding this comment

kingluo Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

monkeyDluffy6017 Jun 19, 2023

Choose a reason for hiding this comment

kingluo Jun 19, 2023

Choose a reason for hiding this comment

monkeyDluffy6017 Jun 19, 2023

Choose a reason for hiding this comment

kingluo Jun 19, 2023

Choose a reason for hiding this comment

monkeyDluffy6017 Jun 19, 2023

Choose a reason for hiding this comment

kingluo Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

monkeyDluffy6017 Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

kingluo Jun 19, 2023

Choose a reason for hiding this comment

kingluo Jun 19, 2023 •

edited

Loading

kingluo Jun 19, 2023 •

edited

Loading

monkeyDluffy6017 Jun 19, 2023 •

edited

Loading