AlinsRan commented on PR #12649:
URL: https://github.com/apache/apisix/pull/12649#issuecomment-4932779468
Thanks for digging into this @bytelazy — the diagnosis in this PR is right,
and the repro cases were useful.
#11440 has been fixed by #13678, so I'm closing this one. Since we ended up
with a different implementation, here's the reasoning rather than just a
"superseded".
**The binary descriptor path isn't actually fixed here.**
`set_default_array()` keys off `field_info.label == 3` and `field.type == 11`.
Those numbers only hold for the text-proto path, where `protoc:parse()` builds
the descriptor directly. `compile_proto_bin()` instead goes through
`pb.decode("google.protobuf.FileDescriptorSet", content)`, and under the
default `enum_as_name` that yields strings:
```
TEXT path label=3 type=9
BIN path label=LABEL_REPEATED type=TYPE_STRING
```
So for any proto configured as a base64 `.pb` descriptor set, the
comparisons never match and empty repeated fields still encode as `{}` —
silently, with no error. That's the main reason for going another way, not a
style preference.
**Two smaller things.** Building a message index and walking the whole
decoded table per response costs something on large bodies, and it
re-implements bookkeeping that lua-protobuf already does at decode time. And
turning `response.lua` into `setmetatable(_M, {__call = ...})` just to expose
`_M._TEST.set_default_array` changes the module's calling convention to make an
internal helper testable.
**What #13678 does instead:** lua-protobuf 0.5.3 (the version pinned in the
rockspec) supports a `*array` default metatable, so `pb.decode` tags repeated
fields itself:
```lua
pb.defaults("*array", core.json.array_mt)
```
Set once per compiled proto state. Maps keep object semantics (lua-protobuf
distinguishes maps from arrays natively), nested/repeated messages and map
values are covered, both the text and binary paths work, and there's no
per-request traversal.
Your `t/plugin/grpc-transcode-arrays.t` cases don't overlap with the tests
in #13678. If any of them cover a scenario #13678 misses, a test-only PR would
be very welcome.
--
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]
