OneOf Enforcement?

[dan-fritchman]

Thanks for all of your work on betterproto, it's been immensely helpful.

What is betterproto's canonical way of enforcing Protobuf oneofs are only set to one variant?

From this demo-protobuf:

syntax = "proto3";
package demo;

message Thing1 {
    string name = 1;
}
message Thing2 {
    string name = 1;
}
message Thing3 {
    string name = 1;
}
message Thing4 {
    string name = 1;
}

message OneThing {
    oneof t {
        Thing1 t1 = 1;
        Thing2 t2 = 2;
        Thing3 t3 = 3;
        Thing4 t4 = 4;
    }
}

Creating a OneThing appears produce default values of non-used oneof variants.

In [2]: OneThing(t1=Thing1(name='cy'))
Out[2]: OneThing(t1=Thing1(name='cy'), t2=Thing2(name=''), t3=Thing3(name=''), t4=Thing4(name=''))

Moreover it'll initialize more than one of them:

In [5]: o = OneThing(t1=Thing1(name='cy'), t2=Thing2(name='huh'))
In [6]: o
Out[6]: OneThing(t1=Thing1(name='cy'), t2=Thing2(name='huh'), t3=Thing3(name=''), t4=Thing4(name=''))

And that combination passes a round-trip through serialization and de-serialization:

In [7]: OneThing.FromString(bytes(o))
Out[7]: OneThing(t1=Thing1(name=''), t2=Thing2(name='huh'), t3=Thing3(name=''), t4=Thing4(name=''))

What's the intent as to where this should be enforced? Outside of betterproto?

[upcFrost]

This actually seem to break the "Empty" value case (aka null vs nil) as well. So in the following case after sending the msg over the wire, you can't really tell which of the fields was set (before sending, which_one_of works fine)

In [2]: OneThing(t1=Thing1(name=''))

the official doc says "For message fields, the field is not set. Its exact value is language-dependent". So even if it is language-dependent, it should still appear as "not set" and not "set to all children empty"

[upcFrost]

What works for me is to set fields as "optional" in the generated code. I.e.

@dataclass(eq=False, repr=False)
class Parent(betterproto.Message):
    value: Optional["Child"] = betterproto.message_field(200, group="val")

Most likely here in the plugin code it should be Optional. But I'm not 100% sure what it might break https://github.com/danielgtaylor/python-betterproto/blob/master/src/betterproto/compile/importing.py#L96

[upcFrost]

Update: What broke in this case is to_dict. It does not check if the TYPE_MESSAGE value is None before running the condition on value._serialized_on_wire

The obvious patch adding the if value is None clause to the to_dict method. I think I'll try to prepare a patch for both of those problems, at least to run tests and to see what else might break

upd2: ok, it also breaks from_dict on value instantiation. Can be fixed using typing.get_type_hints(MyClass)['field_name'].__args__[0]() (arg number == the one which is not NoneType)