lua-protobuf icon indicating copy to clipboard operation
lua-protobuf copied to clipboard

repeated wrappers are deserialized incorrectly

Open git-torrent opened this issue 3 years ago • 9 comments

Let's have message:

message Test {
  uint32 int = 1
  repeated uint32 ints = 2;
  repeated google.protobuf.UInt32Value wrapped_int = 3;
  repeated google.protobuf.UInt32Value wrapped_ints = 4;
}

filled with values

test = {
  int = 0,
  ints = { 0, 0, 0 },
  wrapped_int = { value = 0 },
  wrapped_ints = { {value = 0}, {value = 0}, {value = 0}}
}

test = pb.decode( pb.encode(test) ) would return:

test = {
  ints = { 0, 0, 0 },
  wrapped_ints = { {}, {}, {} }
}

native and wrapped types are not encoded in the same way. On the other side, google implementation (checked Go, python, C#) decodes this message to:

test = {
  ints = { 0, 0, 0 },
  wrapped_ints = { {value = 0}, {value = 0}, {value = 0} }
}

The same applies for other repeated wrappers. All of them should be intialized to default values.

Thanks you.

git-torrent avatar Apr 21 '22 23:04 git-torrent

This is intentional for now. Because to create tables for empty sub message may decrease the performance. If this is really needed, I can add a option to enable this behavior. Any pull request are welcome.

starwing avatar Apr 22 '22 00:04 starwing

That reminds me of old joke about Pentium FDIV bug - incorrect but fast :)

It is needed if the goal is to comply with google implementation. I would be glad to prepare PR, but I am not that skilled. I can fix the lua side by adding if wrapper then return value or default.

git-torrent avatar Apr 22 '22 11:04 git-torrent

This is a history issue. The module is written in the time of protobuf 2.0, at that time the sub message has not default empty value. But the semantic is changed when the protobuf 3 is out. and protobuf3 is designed by C++ semantic. That means to simulate that behaviour from Lua will largely decrease the performance: a message of C++ is just a struct that can be easily cleaned to match the protobuf 3 semantic, but this is not true for Lua.

starwing avatar Apr 23 '22 07:04 starwing

I just add a new option "decode_default_message" to support decode the empty but with default values sub messages. You could try the master HEAD for see whether it fit your need.

starwing avatar Apr 23 '22 10:04 starwing

Thank you for quick help and explanation.

I looked up the affected code but unfortunately not able to comprehend that. I will wait for release. Thank you again

git-torrent avatar Apr 25 '22 07:04 git-torrent

You say this?

    if (!e->LS->encode_default_values && ignoredlen != 0 && ignorezero)
        e->b->size -= (unsigned)(ignoredlen + hlen);

The protobuf3 do not encode the zero-value (and has not default value, that is a protobuf2 thing). the second line used to cancel the encoded zero-value, if these conditions are meet:

  • the user do not set the option "encode_default_values"
  • It really have a field need to encode. (sometimes lpbE_field does not encode anything)
  • and we should do cancel here. (sometimes we must not cancel the encoding, e.g. to encode a oneof field)

starwing avatar Apr 25 '22 08:04 starwing

Thanks again for explanation. Do I understand it right that it enables encoding of all the default values?

(this is useful option in case of proto->json conversion which I must do manually).

git-torrent avatar Apr 25 '22 15:04 git-torrent

Yes, it will. be.

starwing avatar Apr 27 '22 03:04 starwing

It might help in some scenarios. On the other side, turning it on for not loosing array members might bloat the protobufs, what goes against their efficacy.

We can leave this issue as a request for enhancement.

git-torrent avatar Apr 27 '22 06:04 git-torrent