fix(grpc-transcode): correctly serialize empty repeated fields to [] #12391

bytelazy · 2025-06-30T15:27:52Z

Description

In the grpc-transcode plugin, when a repeated field in the gRPC response is empty, it is incorrectly encoded as an empty JSON object ({}) instead of an empty JSON array ([]). This behavior does not comply with the gRPC-JSON transcoding specification and can cause type-related errors on the client-side, which expects an array.

Fixes #11440

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)

Changes or the new features added to this PR
The main changes are in the response.lua file to correctly handle the serialization of repeated fields.

Identify repeated fields: A new function fetch_proto_array_names has been added. It recursively traverses the loaded Protobuf schema definition to identify and collect the names of all fields marked as repeated.

Ensure correct JSON array serialization:

A new function set_default_array has been introduced. After the gRPC response is decoded into a Lua table, this function traverses the table.

For any field that was identified as repeated, it sets a specific metatable (core.json.array_mt).

This metatable forces the core.json.encode function to serialize the corresponding Lua table as a JSON array ([]), even when it is empty.

This logic is applied just before the decoded response is encoded into the final JSON output, ensuring the structure is correct.

…t did to {}

Baoyuantop

Thank you for your contribution!

Two things need to be done:

Need to add corresponding tests for this fix, please feel free to ask questions if you encounter any problems.
Need to pass all CI.

bytelazy · 2025-07-02T08:01:00Z

Thank you for your contribution!

Two things need to be done:

Need to add corresponding tests for this fix, please feel free to ask questions if you encounter any problems.

Need to pass all CI.

All CI have passed.
Under which directory should I add the corresponding tests for this fix ？

Baoyuantop · 2025-07-02T09:34:52Z

Thank you for your contribution!
Two things need to be done:

Need to add corresponding tests for this fix, please feel free to ask questions if you encounter any problems.

Need to pass all CI.

All CI have passed. Under which directory should I add the corresponding tests for this fix ？

Add test case for grpc-transcode plugin in t/plugin/grpc-transcode.t

bytelazy · 2025-07-02T14:04:35Z

Thank you for your contribution!
Two things need to be done:

Need to add corresponding tests for this fix, please feel free to ask questions if you encounter any problems.

Need to pass all CI.

All CI have passed. Under which directory should I add the corresponding tests for this fix ？

Add test case for grpc-transcode plugin in t/plugin/grpc-transcode.t

I don't know how to run the test file. Can you give me some reference documents?

Baoyuantop · 2025-07-03T01:46:10Z

Hi @3kis, you can refer to https://github.com/apache/apisix/blob/master/docs/en/latest/build-apisix-dev-environment-devcontainers.md and https://apisix.apache.org/docs/apisix/internal/testing-framework/

bytelazy · 2025-07-03T06:34:45Z

Thank you for your contribution!
Two things need to be done:

Need to add corresponding tests for this fix, please feel free to ask questions if you encounter any problems.

Need to pass all CI.

All CI have passed. Under which directory should I add the corresponding tests for this fix ？

Add test case for grpc-transcode plugin in t/plugin/grpc-transcode.t

Based on the documentation you provided, after running prove -I. -I../test-nginx/inc -I../test-nginx/lib -r t/plugin/grpc-transcode.t, I encountered connect() failed (111: Connection refused) while connecting to upstream, client: 127.0.0.1.

Upon observation, I noticed that the /t directory contains a /grpc_server_example directory. Does this mean that to test t/plugin/grpc-transcode.t, I need to download Golang in the dev container and run /grpc_server_example/main.go?

Baoyuantop · 2025-07-04T07:28:32Z

I need to download Golang in the dev container and run /grpc_server_example/main.go?

Yes

Baoyuantop · 2025-07-11T06:14:16Z

Hi @3kis, please fix lint error.

bytelazy · 2025-07-11T12:15:37Z

Hi @3kis, please fix lint error.

hi～ I have fixed lint error. What should I do next?

Baoyuantop · 2025-07-13T12:30:28Z

CI has all passed, the error has nothing to do with this PR, I will review this PR as soon as possible.

Baoyuantop

There are many spaces used incorrectly, please check carefully.

apisix/plugins/grpc-transcode/response.lua

bytelazy · 2025-07-15T14:06:20Z

There are many spaces used incorrectly, please check carefully.

Thanks your suggestions, I have reverted to additional modifications

adapt suggestions from copilt Co-authored-by: Copilot <[email protected]>

Copilot

Pull Request Overview

This PR fixes a serialization issue in the grpc-transcode plugin where empty repeated fields were incorrectly encoded as empty JSON objects ({}) instead of empty JSON arrays ([]) when transcoding gRPC responses to JSON.

Adds functions to identify repeated fields from protobuf schema definitions
Implements automatic array metatable assignment for repeated fields to ensure proper JSON serialization
Includes comprehensive test coverage for various scenarios with empty repeated fields

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
apisix/plugins/grpc-transcode/response.lua	Implements the core fix with functions to detect repeated fields and set appropriate metatables for JSON array serialization
t/plugin/grpc-transcode4.t	Adds new test file with comprehensive test cases covering various scenarios of empty repeated field serialization

Comments suppressed due to low confidence (2)

apisix/plugins/grpc-transcode/response.lua:33

[nitpick] The function name 'fetch_proto_array_names' could be more descriptive. Consider renaming to 'extract_repeated_field_names' or 'collect_repeated_field_names' to better reflect that it's collecting names of repeated fields, not array names.

local function fetch_proto_array_names(proto_obj)

apisix/plugins/grpc-transcode/response.lua:51

[nitpick] The function name 'set_default_array' is ambiguous. Consider renaming to 'apply_array_metatables' or 'set_array_metatables_for_repeated_fields' to clarify that it's setting metatables for repeated fields.

local function set_default_array(tab, array_names)

apisix/plugins/grpc-transcode/response.lua

Co-authored-by: Copilot <[email protected]>

apisix/plugins/grpc-transcode/response.lua

t/plugin/grpc-transcode4.t

apisix/plugins/grpc-transcode/response.lua

nic-6443 · 2025-07-31T02:58:50Z

t/plugin/grpc-transcode4.t

+                        "scheme": "grpc",
+                            "type": "roundrobin",
+                            "nodes": {
+                            "127.0.0.1:50051": 1


I can't found this test backend in APISIX e2e framework, I think this new added test cased can't be pass due to #12462 , try to merge master branch and re-run CI again.

When running tests, I used a custom grpc server instead of the original grpc server in apisix. Should I merge the master branch into my local deveop branch?

nic-6443 · 2025-07-31T03:22:16Z

apisix/plugins/grpc-transcode/response.lua

+local type          = type
+local pairs         = pairs
+local setmetatable  = setmetatable
+
+pb.option "decode_default_array"
+-- Protobuf repeated field label value
+local PROTOBUF_REPEATED_LABEL = 3
+local repeated_label = PROTOBUF_REPEATED_LABEL
+
+local function fetch_proto_array_names(proto_obj)
+    local names = {}
+    if type(proto_obj) == "table" then
+        for k,v in pairs(proto_obj) do
+            if type(v) == "table" then
+                local sub_names = fetch_proto_array_names(v)
+                for sub_name,_ in pairs(sub_names) do
+                    names[sub_name] = 1
+                end
+            end
+        end
+        if proto_obj["label"] == repeated_label then
+            if proto_obj["name"] then
+                names[proto_obj["name"]] = 1
+            end
+        end
+    end
+    return names
+end
+
+local function set_default_array(tab, array_names)
+    if type(tab) ~= "table" then
+        return
+    end
+    for k, v in pairs(tab) do
+        if type(v) == "table" then
+            if array_names[k] == 1 then
+                setmetatable(v, core.json.array_mt)
+            end
+            set_default_array(v, array_names)
+        end
+    end
+end


please check starwing/lua-protobuf#240, I think we don't need to deal with this issue manually.

In starwing/lua-protobuf#240 , the author said that this change will go online in the new version, but the latest version does not directly deal with the problem of empty arrays.

please check starwing/lua-protobuf#240, I think we don't need to deal with this issue manually.

I understand, you're saying that it's better to meet the requirements with simple options rather than making extensive code changes.

Baoyuantop · 2025-08-15T09:32:56Z

Hi @bytelazy, there are still some review comments that need to be addressed.

bytelazy · 2025-08-15T09:58:35Z

Hi @bytelazy, there are still some review comments that need to be addressed.

thanks for reminding me, I have checked the comment and reply

fix bug: grpc-transcode cant transcode empty array list to [] , but i…

2b91c99

…t did to {}

bytelazy marked this pull request as ready for review June 30, 2025 15:36

dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working plugin labels Jun 30, 2025

Baoyuantop assigned bytelazy Jul 1, 2025

Baoyuantop added this to ⚡️ Apache APISIX Roadmap Jul 1, 2025

Baoyuantop moved this to 👀 In review in ⚡️ Apache APISIX Roadmap Jul 1, 2025

Baoyuantop reviewed Jul 1, 2025

View reviewed changes

Update response.lua to pass code lint ci

512df1e

add test to grpc-transcode plugin

e00d8f0

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jul 9, 2025

delete spaces at the end of lines

ea930fc

bytelazy added 2 commits July 11, 2025 16:50

fix lint error

acd48b3

Update grpc-transcode4.t to fix lint error

20c05d7

Baoyuantop self-requested a review July 13, 2025 12:29

Baoyuantop reviewed Jul 15, 2025

View reviewed changes

revert addtional changes

4f078be

Baoyuantop requested a review from Copilot July 21, 2025 09:36

This comment was marked as outdated.

Sign in to view

Apply suggestions from code review

8893acd

adapt suggestions from copilt Co-authored-by: Copilot <[email protected]>

Baoyuantop requested a review from Copilot July 21, 2025 10:10

Copilot AI reviewed Jul 21, 2025

View reviewed changes

apisix/plugins/grpc-transcode/response.lua Outdated Show resolved Hide resolved

apisix/plugins/grpc-transcode/response.lua Outdated Show resolved Hide resolved

Apply suggestions from code review

6cced4f

Co-authored-by: Copilot <[email protected]>

Baoyuantop reviewed Jul 23, 2025

View reviewed changes

apisix/plugins/grpc-transcode/response.lua Outdated Show resolved Hide resolved

Baoyuantop requested review from nic-6443, Revolyssup and AlinsRan July 23, 2025 01:44

check if proto_obj.name exists before accessing it

09fe4af

Baoyuantop approved these changes Jul 24, 2025

View reviewed changes

Baoyuantop requested a review from membphis July 24, 2025 09:10

nic-6443 reviewed Jul 31, 2025

View reviewed changes

t/plugin/grpc-transcode4.t Show resolved Hide resolved

nic-6443 reviewed Jul 31, 2025

View reviewed changes

apisix/plugins/grpc-transcode/response.lua Show resolved Hide resolved

nic-6443 reviewed Jul 31, 2025

View reviewed changes

Baoyuantop added the wait for update wait for the author's response in this issue/PR label Aug 1, 2025

github-actions bot added the user responded label Aug 15, 2025

fix(grpc-transcode): correctly serialize empty repeated fields to [] #12391

Are you sure you want to change the base?

fix(grpc-transcode): correctly serialize empty repeated fields to [] #12391

Conversation

bytelazy commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Baoyuantop left a comment

Choose a reason for hiding this comment

Uh oh!

bytelazy commented Jul 2, 2025

Uh oh!

Baoyuantop commented Jul 2, 2025

Uh oh!

bytelazy commented Jul 2, 2025

Uh oh!

Baoyuantop commented Jul 3, 2025

Uh oh!

bytelazy commented Jul 3, 2025

Uh oh!

Baoyuantop commented Jul 4, 2025

Uh oh!

Baoyuantop commented Jul 11, 2025

Uh oh!

bytelazy commented Jul 11, 2025

Uh oh!

Baoyuantop commented Jul 13, 2025

Uh oh!

Baoyuantop left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bytelazy commented Jul 15, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nic-6443 Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

bytelazy Aug 16, 2025

Choose a reason for hiding this comment

Uh oh!

nic-6443 Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

bytelazy Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

bytelazy Aug 16, 2025

Choose a reason for hiding this comment

Uh oh!

Baoyuantop commented Aug 15, 2025

Uh oh!

bytelazy commented Aug 15, 2025

Uh oh!

Uh oh!

bytelazy commented Jun 30, 2025 •

edited

Loading