key not found when encoding optional fields

[sh41]

Describe the bug
When using optional fields there seems to be somekind of problem with the naming:

** (KeyError) key :_optional not found in: %Bar{__uf__: [], optional: "### OPTIONAL ###"}. Did you mean one of:

           * :optional

     code: Bar.json_encode!(msg)

To Reproduce
I wrote this test case to show the issue

defmodule OptionalTest do
  use ExUnit.Case

  use Protox,
    schema: """
      syntax = "proto3";

      message Foo {
        string required = 1;
      }

       message Bar {
        optional string optional = 1;
      }
    """

  test "not optional string" do
    msg = %Foo{required: "### NOT OPTIONAL ###"}

    assert ["{", ["\"required\"", ":", "\"### NOT OPTIONAL ###\""], "}"] ==
             Foo.json_encode!(msg)
  end

  test "optional string" do
    msg = %Bar{optional: "### OPTIONAL ###"}

    assert ["{", ["\"optional\"", ":", "\"### OPTIONAL ###\""], "}"] ==
             Bar.json_encode!(msg)
  end
end

output:

  1) test optional string (OptionalTest)
     test/timestamp_test.exs:24
     ** (KeyError) key :_optional not found in: %Bar{__uf__: [], optional: "### OPTIONAL ###"}. Did you mean one of:

           * :optional

     code: Bar.json_encode!(msg)
     stacktrace:
       :erlang.map_get(:_optional, %Bar{__uf__: [], optional: "### OPTIONAL ###"})
       (protox 1.7.0) lib/protox/json_encode.ex:76: Protox.JsonEncode.encode_msg_field/3
       (protox 1.7.0) lib/protox/json_encode.ex:19: anonymous fn/4 in Protox.JsonEncode.encode_message/2
       (elixir 1.11.4) lib/enum.ex:2193: Enum."-reduce/3-lists^foldl/2-0-"/3
       (protox 1.7.0) lib/protox/json_encode.ex:18: Protox.JsonEncode.encode_message/2
       test/timestamp_test.exs:28: (test)



Finished in 0.3 seconds
2 tests, 1 failure

Randomized with seed 860092

Expected behavior
The struct with the optional field is encoded.

Environment (please complete the following information):

• Elixir version: Elixir 1.11.4
• Erlang version: 23.3.4.17
• protoc version: libprotoc 3.21.7

Additional context
This is a minimal reproduction. I've observed in binary decode as well as json_decode. Linked to #49/#50.

[ahamez]

Thank you for the report! I can reproduce this problem. However, you wrote that you observed this problem also with binary encoding, which I could not reproduced. Do you have a minimal example for this?
In the meantime, I'll investigate the JSON encoding.

[sh41]

Thank you @ahamez 💚 💙 💜

I haven't managed to produce a test case for the binary case yet but I will keep trying.

I see the error when I generate code and then convert a binary stream coming from a c# implementation using the same proto file. I can't share either the proto or the binary due to it being a work thing.

[ahamez]

I think I might have fixed this. Could you try the branch fix_optional to check if it resolves your issue? Thank you :-)

[sh41]

Thank you @ahamez that has fixed the JSON problem. It hasn't helped the binary decode though. I'll try and produce a concrete test case for binary as soon as I can.

[sh41]

Hi @ahamez, I have got a test case for the binary decode error.

defmodule OptionalTest do
  use ExUnit.Case

  use Protox,
    schema: """
        syntax = "proto3";

        message Msg1 {
          Msg2 foo = 1;
          optional Msg2 bar = 2;
        }

        message Msg2 {
          int32 buz = 1;
        }
    """

  test "non-optional = nil, optional = nil" do
    msg1 = %Msg1{foo: nil, bar: nil}

    assert msg1 |> Msg1.encode!() |> :binary.list_to_bin() |> Msg1.decode!() == msg1
  end

  test "non-optional = Msg2, optional = nil" do
    msg1 = %Msg1{foo: %Msg2{}, bar: nil}

    assert msg1 |> Msg1.encode!() |> :binary.list_to_bin() |> Msg1.decode!() == msg1
  end

  test "non-optional = nil, optional = Msg2" do
    msg1 = %Msg1{foo: nil, bar: %Msg2{}}

    assert msg1 |> Msg1.encode!() |> :binary.list_to_bin() |> Msg1.decode!() == msg1
  end

  test "non-optional = Msg2, optional = Msg2" do
    msg1 = %Msg1{foo: %Msg2{}, bar: %Msg2{}}

    assert msg1 |> Msg1.encode!() |> :binary.list_to_bin() |> Msg1.decode!() == msg1
  end
end

$ mix test test/optional_test.exs --trace --seed=0
Excluding tags: [conformance: true]


OptionalTest [test/optional_test.exs]
  * test non-optional = nil, optional = nil (0.00ms) [L#18]
  * test non-optional = Msg2, optional = nil (0.6ms) [L#24]
  * test non-optional = nil, optional = Msg2 (2.4ms) [L#30]

  1) test non-optional = nil, optional = Msg2 (OptionalTest)
     test/optional_test.exs:30
     ** (KeyError) key :_bar not found in: %Msg1{__uf__: [], bar: nil, foo: nil}. Did you mean one of:

           * :bar

     code: assert msg1 |> Msg1.encode!() |> :binary.list_to_bin() |> Msg1.decode!() == msg1
     stacktrace:
       test/optional_test.exs:4: Msg1.parse_key_value/2
       test/optional_test.exs:33: (test)

  * test non-optional = Msg2, optional = Msg2 (0.01ms) [L#36]

  2) test non-optional = Msg2, optional = Msg2 (OptionalTest)
     test/optional_test.exs:36
     ** (KeyError) key :_bar not found in: %Msg1{__uf__: [], bar: nil, foo: nil}. Did you mean one of:

           * :bar

     code: assert msg1 |> Msg1.encode!() |> :binary.list_to_bin() |> Msg1.decode!() == msg1
     stacktrace:
       test/optional_test.exs:4: Msg1.parse_key_value/2
       test/optional_test.exs:39: (test)



Finished in 0.3 seconds
4 tests, 2 failures

Randomized with seed 0

It appears to be linked to an optional field being a message. We first noticed it with optional google.protobuf.Timestamp but it appears to happen with any embedded message.

Thanks so much for looking at this.

[ahamez]

Thank your for this test case! You're most probably right, it must be linked to optional nested messages.
I'll dig into it as soon as possible !

[ahamez]

I was easier than I thought 😁. Could you try with the latest commit of the branch fix_optional?

[sh41]

Amazing, thank you again @ahamez. That has solved all our decoding issues.

There is one last thing that might be linked to this. When I compile generated code for a proto that has one of these optional messages there is a compiler warning.

Another test case for you:

In the fix_optional branch create this file in optional_test/optional.proto

syntax = "proto3";

message Msg1 {
  Msg2 foo = 1;
  optional Msg2 bar = 2;
}

message Msg2 {
  int32 buz = 1;
}

and run these commands

mkdir -p lib/optional
mix protox.generate --output-path=./lib/optional --multiple-files ./optional_test/optional.proto
mix compile

and then you should see:

Compiling 2 files (.ex)
warning: function encode__bar/2 is unused
  lib/optional/elixir_msg1.ex:25

Generated protox app

I think this is linked to the same thing. I think there is an extra underscore in the function name encode__bar/2?

[ahamez]

Thanks for the test case! Just to give you an idea of why there is this extra _: optional fields in proto3 are juste syntactic sugar for oneof. For instance, the following message:

message Msg1 {
  optional int32 foo = 1;
}

is exactly the same as:

message Msg2 {
  oneof _foo {
    int32 foo = 1;
  }
}

I'll try to suppress this warning this week-end 🙂.

[ahamez]

It's now fixed in 1.7.1. Thank you for your report and all your test cases, they were very useful!