The need for non-empty oneof came up in conversations with users. The other
evidence for this being a real user need is that PGV has a validator for
non-empty oneofs <https://github.com/envoyproxy/protoc-gen-validate#oneofs> in
their code gen, so it wouldn't be unreasonable if someone implemented this
via reflection rather than codegen.
Totally understand that this may be the safest way to roll this change out.
This is a pretty significant change to proto3, so it might be worth adding
to the changelog the motivation for this change and considerations that
were taken (i.e. possibly wrapper types are not good enough due to the
extra space)
On Tue, May 5, 2020 at 6:50 PM Josh Haberman <haber...@google.com> wrote:
> On Tue, May 5, 2020 at 6:24 PM Nadav Samet <thesa...@gmail.com> wrote:
>
>> What representations do you mean? In general it is not expected that
>>> oneof names are exposed in wire formats. Authors of .proto files are free
>>> to change them at will without breaking the wire.
>>>
>>
>> An example would be code that produces an alternative JSON representation
>> that groups all the fields of a oneof inside a nested object. For example
>> if a message like
>> message MyMessage {
>> oneof foo {
>> int32 v = 1
>> }
>> }
>>
>> gets encoded to JSON by user's code as:
>> "my_one_of": {"foo": 5}}
>>
>> Then such code would have to be modified to treat synthetic
>> oneofs differently.
>>
>
> Can you point to code that does this, or is it hypothetical?
>
> I agree that if such a format existed, serialization code would need to
> change to use the "real_" variants of the oneof accessors. But the parsers
> and serializers for all of the real formats we use with reflection
> (protobuf binary, TextFormat, JSON) did not need to change to support
> proto3 optional, which is a significant benefit. I suspect lots of other
> reflection-based algorithms will benefit from this too.
>
>
>> Can you give an example of some of the code that would be affected?
>>>
>>
>> Some users use oneofs to build ADTs, and for them oneofs should never be
>> empty <https://github.com/scalapb/ScalaPB/issues/633>. To enforce this
>> at runtime, they may have written a validator that takes an instance of a
>> message, and uses its descriptor to traverse all the oneofs and fail when
>> it finds a oneof where none of its fields is set. With the proposed change,
>> this validator would have to be changed to only care about non-synthetic
>> oneofs.
>>
>
> This sounds like it is also hypothetical? I think it's helpful to talk
> about real examples when possible.
>
> It's true that in some cases code will need to change to ignore synthetic
> oneofs. But in the cases where we've had to implement this, it is not a
> very intrusive change. It mainly consists of two simple transformations:
>
> 1. f->containing_oneof() -> f->real_containing_oneof()
> 2. f->oneof_decl_count() -> f->real_oneof_decl_count()
>
> I agree it would have been nicer not to have to use synthetic oneofs. But
> there is a *lot* of code out there that manipulates protos using
> reflection. Synthetic oneofs allow many/most of these algorithms to work
> with proto3 optional fields with no modifications. Without them it would be
> a riskier change to roll out, because we don't know how many algorithms
> would simply not be aware that proto3 optional fields exist and need to
> preserve presence.
>
--
-Nadav
--
You received this message because you are subscribed to the Google Groups
"Protocol Buffers" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to protobuf+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/protobuf/CANZcNEoewchNP9qLNAsVRmhQUbbMnzpC0APjjU9EDw80myB2RQ%40mail.gmail.com.
