starwing / lua-protobuf

A Lua module to work with Google protobuf
MIT License
1.71k stars 388 forks source link

add metatable to decoded array and message to tell array from map #240

Closed StarlightIbuki closed 1 year ago

StarlightIbuki commented 1 year ago

We cannot tell empty lua tables from arrays. This PR introduce metatable for them to solve the issue.

starwing commented 1 year ago

Maybe it’s better to get metatables with ‘pb.defaults’?

StarlightIbuki commented 1 year ago

Maybe it’s better to get metatables with pb.defaults?

Is it possible to set the default metastable for all arrays? Sometimes we do not know the field names when coding.

starwing commented 1 year ago

I think this solution is better than #241 , but maybe you should add meta tables to pb.defaults() table, like this:

if getmetatable(a.b) == pb.defaults "*array" then -- is array
 -- ...
end

the routine pb.defaults is just used to put metatables for types. The name defaults is not goods, but that's because the original version of this module only has one way to set default values -- the metatables.

StarlightIbuki commented 1 year ago

@starwing I think it's a good idea to use pb.defaults here. But if we do not use feature detection to coop with lua-cjson, users can neither set the default table to let it work. Should we support like pb.set_default? This way it can even work with more libraries.

StarlightIbuki commented 1 year ago

@starwing And how about we add an alias for pb.defaults, for example, pb.metatables?

starwing commented 1 year ago

@starwing And how about we add an alias for pb.defaults, for example, pb.metatables?

Yes, it's a good idea.

@starwing I think it's a good idea to use pb.defaults here. But if we do not use feature detection to coop with lua-cjson, users can neither set the default table to let it work. Should we support like pb.set_default? This way it can even work with more libraries.

pb.defaults() routine could just used to set default, like this:

pb.defaults("*array", {})
starwing commented 1 year ago

@StarlightIbuki I have pushed a new commit to implement using pb.defaults to set metatable for array/map. You could try it out to see whether it fit your needs.

StarlightIbuki commented 1 year ago

Thanks. However, there's an issue that prevents us from utilizing this feature: a repeated field is always treated as an optional field. In case of an empty array it will be decoded as "nil".

local cjson = require "cjson"
local cjson_array_mt = cjson.empty_array_mt

local inspect = require "inspect"
local protoc = require "protoc"
local pb = require "pb"

pb.option("use_default_values", true)

pb.defaults("*array", cjson_array_mt)

assert(protoc:load [[
    message Person {
        repeated int32 test = 1;
        optional int32 agg = 2;
    }
]])

local data = {
    test = {},
    agg = 11,
}

local bytes = assert(pb.encode("Person", data))
print(pb.tohex(bytes))
local data2 = assert(pb.decode("Person", bytes))
print("test meta ", inspect(getmetatable(data2.test)))
print("array meta ", inspect(pb.defaults "*array"))
print(cjson.encode(data2))
10 0B
test meta nil
array meta {}
{"agg":11}
starwing commented 1 year ago

you can add this option:

pb.option "decode_default_array"
StarlightIbuki commented 1 year ago

@starwing Thanks! That works as expected.

StarlightIbuki commented 1 year ago

By the way when will the next release be?

starwing commented 1 year ago

A new version has released.

StarlightIbuki commented 1 year ago

Closing as the issue is solved.

vm-001 commented 1 year ago

@starwing Really appreciate for making effort during the public holiday. 👍