protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard

`Protobuf.encode/1` does not actually verify struct in some cases

Open edisonywh opened this issue 5 months ago • 2 comments

Hey team! I am seeing a weird behaviour when we pass in a wrong struct to a protobuf - Protobuf.encode encodes it just fine, but I would expect that to raise an error instead?

Here's a reproduction test file.

defmodule Message do
  @moduledoc false
  use Protobuf, syntax: :proto3

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

defmodule NestedMessage do
  @moduledoc false
  use Protobuf, syntax: :proto3

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

defmodule RandomStruct do
  defstruct [:val]
end

defmodule ProtobufEncodeBugTest do
  use ExUnit.Case
  doctest ProtobufEncodeBug

  describe "bug" do
    test "correct struct as value should work (control)" do
      message = %Message{value: %NestedMessage{value: "a"}}

      result =
        message
        |> Protobuf.encode()
        |> Protobuf.decode(Message)


      assert result == %Message{value: %NestedMessage{value: "a"}}
    end

    test "incorrect struct as value should not work (bug)" do
      message = %Message{value: %RandomStruct{val: "a"}}

      assert_raise Protobuf.EncodeError, fn ->
        result =
          message
          |> Protobuf.encode() # I expect this to raise an encode error because I passed in the wrong struct, but it encodes fine
          |> Protobuf.decode(Message)

          assert result == %Message{value: %NestedMessage{value: ""}} # it just returns an empty string. Works if RandomStruct has the same key though.
      end
    end

    test "type error - encoding :int into :string should fail" do
      message = %Message{string: 123}
      # passing in `123` for a `:string` protobuf type raises error as expected
      assert_raise Protobuf.EncodeError, fn ->
        message
        |> Protobuf.encode()
      end
    end
  end
end

I would expect the second test case to fail, because I am passing in %RandomStruct{} whereas Message.value, according to the protobuf, should be of type NestedMessage.

Being able to encode map into a protobuf message is a good feature, but I feel like it should accept raw map | exact struct (so %{} | %{__struct__: NestedMessage} in this case), and not any map.

Doc says validation is done during encoding: https://github.com/elixir-protobuf/protobuf/blob/main/README.md?plain=1#L99-L100, and I would categorise this as a "mistyped value"

Is this expected behaviour?

edisonywh avatar Jan 05 '24 16:01 edisonywh

If this is indeed a bug I am happy to attempt to send in a PR (if someone can point me to the right place in the codebase for it that'll also help tremendously!

edisonywh avatar Jan 12 '24 09:01 edisonywh