Inconsistent behaviour when encoding oneof

[nsweeting]

Hi there,

We're seeing (what feels like) inconsistent behaviour when encoding oneof values.

Given the following protobuf:

syntax = "proto3";

message Foo {
  message Bar {
    string value = 1;
  }

  message Baz {}

  oneof value {
    Bar bar = 1;
    Baz baz = 2;
  }
}

Which compiles to the following elixir code:

defmodule Foo.Bar do
  @moduledoc false

  use Protobuf, protoc_gen_elixir_version: "0.12.0", syntax: :proto3

  field(:value, 1, type: :string)
end

defmodule Foo.Baz do
  @moduledoc false

  use Protobuf, protoc_gen_elixir_version: "0.12.0", syntax: :proto3
end

defmodule Foo do
  @moduledoc false

  use Protobuf, protoc_gen_elixir_version: "0.12.0", syntax: :proto3

  oneof(:value, 0)

  field(:bar, 1, type: Foo.Bar, oneof: 0)
  field(:baz, 2, type: Foo.Baz, oneof: 0)
end

When encoding/decoding a message that contains a map for Foo.Bar (not the struct) and it contains a value - things work as expected:

foo = %Foo{value: {:bar, %{value: "bar"}}}
foo |> Foo.encode() |> Foo.decode()

%Foo{
  value: {:bar, %Foo.Bar{value: "bar", __unknown_fields__: []}},
  __unknown_fields__: []
}

But when encoding/decoding an empty map - it simply gets dropped entirely - even though we are passing the proper tuple for the oneof:

foo = %Foo{value: {:baz, %{}}}
foo |> Foo.encode() |> Foo.decode()

%Foo{value: nil, __unknown_fields__: []}

Since we've noted which oneof it is - it seems like it should be included.

Thanks

[ericmj]

What happens if you use a struct instead of an empty map?

%Foo{value: {:baz, %Foo.Baz{}}}

The library should probably not accept maps in place of structs but it is possible that it sees the empty map as a default value which are not encoded and since it's not encoded no oneof field was selected.

[whatyouhide]

There's two issues here:

1. This should raise, I think:

Foo.encode(%Foo{value: {:bar, %{value: "bar"}}})

because {:bar, _} should only work with a Foo.Bar struct.

2. The other issue is that %Foo{value: {:baz, %Foo.Baz{}}} should not empty the field because :baz is not the default value.

We're investigating this!

cc @savhappy

[whatyouhide]

Also, 1. above might be related to #365.

[v0idpwn]

The other issue is that %Foo{value: {:baz, %Foo.Baz{}}} should not empty the field because :baz is not the default value.

It seems to me that oneofs are always sent over the wire, even if the value is default, both for proto 2 and proto 3. I'll double check this, but if I'm right here, I think my PR at #379 implements the correct behaviour.

With regards to the 1st problem:

because {:bar, _} should only work with a Foo.Bar struct.

This may be a pretty big breaking change. We can afford to do it as we are pre-1.0, but it's probably worth it to at least add a deprecation warning first, etc. WDYT?

[whatyouhide]

@v0idpwn why a "big breaking change"? Isn't that the expected behavior rather?

[v0idpwn]

I may be overestimating it's prevalence, but in my experience, the following snippet is rather common:

Protobuf.encode!(%StructA{inner_struct: %{b: 1}})

That is, the caller explicitly says what's the most external struct, but relies in the library to deal with the rest. This works for maps and keywords, currently.

Making it an exception straight away might make it hard to update the library, hence I'd prefer the warning path (but honestly I wouldn't mind it staying this way 😄 )

[whatyouhide]

but honestly I wouldn't mind it staying this way

But this way is semantically just wrong, isn't it? When encoding, you own the data you're encoding so I find it surprising that you'd need the flexibility.

Do you have example use cases of this kind of patterns in real-world code that you could point me to to help me understand?

Btw, @ericmj any opinions here?

[v0idpwn]

Originally, you'd have something like:

StructA.new!(inner_struct: [b: 1])

Then, when new/1 and new!/1 were deprecated, it was suggested to migrate to %StructA{...}. If you (naively) converted the above code to %StructA{inner_struct: [b: 1]), it would work perfectly, and without any warning. I assume this is what originated this sort of code in our codebases.

[v0idpwn]

I've myself relied on this, on APIs that accepted arbitrary keyword lists and transformed into the protobuf to send a request. Manually converting is... verbose. The following code is not very assertive, but generally works:

def create_user(keyword) do
  request = struct!(CreateUserRequest, keyword)
  encoded_request = CreateUserRequest.encode!(request)
  call_external_api(encoded_request)
end

Before, you'd use new!