oniksan / godobuf

A Google Protobuf implementation for Godot / GDScript
BSD 3-Clause "New" or "Revised" License
260 stars 36 forks source link

oneof fields with default values get ignored when packed #14

Closed wooky closed 3 years ago

wooky commented 3 years ago

Consider a message like this:

message Test {
  oneof foo {
    uint32 field1 = 1;
    uint32 field2 = 2;
  }
}

If I set field1 to 0, which is a default value in proto3, and pack the message, the result is empty. However, I should expect the packed data to include that field1 is set, that it's set to 0, and that field2 is unset.

oniksan commented 3 years ago

Everything is done in accordance with the specification.

https://developers.google.com/protocol-buffers/docs/proto3#default

Note that for scalar message fields, once a message is parsed there's no way of telling whether a field was explicitly set to the default value (for example whether a boolean was set to false) or just not set at all: you should bear this in mind when defining your message types.

Oneof is one of the simple field.

In general, while writing the protobuf implementation, I realized that the protocol has many inconvenient nuances for users. As a result, the user himself has to solve these problems.

Please close issue, if you are satisfied with the answer or write here.

wooky commented 3 years ago

I decided to investigate this myself. The specifications only talk about parsing files, but not about writing them. I've written a small C++ program to see how the official library deals with it, using the protobuf file I posted above:

#include <iomanip>
#include "footest.pb.h"

int main()
{
    FooTest fooTest;
    fooTest.set_field1(0);

    std::string output = fooTest.SerializeAsString();
    for (char c : output) {
        std::cout << std::hex << std::setfill('0') << std::setw(2) << (int)c << " ";
    }
    std::cout << '\n';
}

The output is "08 00", and changing to set_field2 gives "10 00". So the official way does set a value for the oneof, even if it's the default.

If you'd like, I can submit a PR that fixes this issue.

oniksan commented 3 years ago

Please tell me which version of protobuf you are using 2 or 3?

wooky commented 3 years ago

Version 3 only. Did not test with 2.

oniksan commented 3 years ago

In reference realisation on C++ if you just use simple int32 (not oneof) and set it to 0, which data are generated?

oniksan commented 3 years ago

Sorry. I was wrong, I'll fix it. For oneof fields there is an exception to the serialization rules for default values:

If you set a oneof field to the default value (such as setting an int32 oneof field to 0), the "case" of that oneof field will be set, and the value will be serialized on the wire.