Skip to content

Commit

Permalink
Fix mangled Protobuf.EncodeError (#386)
Browse files Browse the repository at this point in the history
Because of the recursive implementation of `encode_fields`, unrelated
fields would get mixed up in encode errors.

For example, our struct `%TestMsg.Foo` has fields `:a`, `:b`, `:c`,
`:d`, `e` (and more...). If we had an error at `:e`, we would get the
error popping up for all fields coming before it:

```elixir
iex(1)>  %TestMsg.Foo{a: 90, e: %TestMsg.Foo.Bar{a: "not_a_number"}} |> Protobuf.encode()
** (Protobuf.EncodeError) Got error when encoding TestMsg.Foo#a: ** (Protobuf.EncodeError) Got error when encoding TestMsg.Foo#b: ** (Protobuf.EncodeError) Got error when encoding TestMsg.Foo#c: ** (Protobuf.EncodeError) Got error when encoding TestMsg.Foo#d: ** (Protobuf.EncodeError) Got error when encoding TestMsg.Foo#e: ** (Protobuf.EncodeError) Got error when encoding TestMsg.Foo.Bar#a: ** (Protobuf.EncodeError) "not_a_number" is invalid for type :int32
    (protobuf 0.13.0) lib/protobuf/encoder.ex:71: Protobuf.Encoder.encode_fields/5
    (protobuf 0.13.0) lib/protobuf/encoder.ex:33: Protobuf.Encoder.encode_with_message_props/2
    (protobuf 0.13.0) lib/protobuf/encoder.ex:18: Protobuf.Encoder.encode/1
    iex:1: (file)
```

After the patch, only the relevant fields are mentioned in the error:
```elixir
iex(3)> %TestMsg.Foo{a: 90, e: %TestMsg.Foo.Bar{a: "not_a_number"}} |> Protobuf.encode()
** (Protobuf.EncodeError) Got error when encoding TestMsg.Foo#e: ** (Protobuf.EncodeError) Got error when encoding TestMsg.Foo.Bar#a: ** (Protobuf.EncodeError) "not_a_number" is invalid for type :int32
    (protobuf 0.13.0) lib/protobuf/encoder.ex:75: Protobuf.Encoder.encode_field/4
    (protobuf 0.13.0) lib/protobuf/encoder.ex:63: Protobuf.Encoder.encode_fields/5
    (protobuf 0.13.0) lib/protobuf/encoder.ex:33: Protobuf.Encoder.encode_with_message_props/2
    (protobuf 0.13.0) lib/protobuf/encoder.ex:18: Protobuf.Encoder.encode/1
    iex:3: (file)
```
  • Loading branch information
v0idpwn authored Jan 2, 2025
1 parent db54215 commit 65faf65
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 7 deletions.
18 changes: 11 additions & 7 deletions lib/protobuf/encoder.ex
Original file line number Diff line number Diff line change
Expand Up @@ -60,17 +60,21 @@ defmodule Protobuf.Encoder do
end

acc =
case encode_field(class_field(prop), val, syntax, prop) do
case encode_field(prop, struct.__struct__, val, syntax) do
{:ok, iodata} -> [acc | iodata]
:skip -> acc
end

encode_fields(rest, syntax, struct, oneofs, acc)
end

defp encode_field(prop, struct_mod, val, syntax) do
do_encode_field(class_field(prop), val, syntax, prop)
rescue
error ->
raise Protobuf.EncodeError,
message:
"Got error when encoding #{inspect(struct.__struct__)}##{prop.name_atom}: #{Exception.format(:error, error)}"
"Got error when encoding #{inspect(struct_mod)}##{prop.name_atom}: #{Exception.format(:error, error)}"
end

defp skip_field?(_syntax, [], _prop), do: true
Expand All @@ -88,7 +92,7 @@ defmodule Protobuf.Encoder do
defp skip_field?(:proto3, false, %FieldProps{oneof: nil}), do: true
defp skip_field?(_syntax, _val, _prop), do: false

defp encode_field(
defp do_encode_field(
:normal,
val,
syntax,
Expand All @@ -102,11 +106,11 @@ defmodule Protobuf.Encoder do
end
end

defp encode_field(:embedded, _val = nil, _syntax, _prop) do
defp do_encode_field(:embedded, _val = nil, _syntax, _prop) do
:skip
end

defp encode_field(
defp do_encode_field(
:embedded,
val,
syntax,
Expand All @@ -131,7 +135,7 @@ defmodule Protobuf.Encoder do
{:ok, result}
end

defp encode_field(:packed, val, syntax, %FieldProps{type: type, encoded_fnum: fnum} = prop) do
defp do_encode_field(:packed, val, syntax, %FieldProps{type: type, encoded_fnum: fnum} = prop) do
if skip_field?(syntax, val, prop) or skip_enum?(syntax, val, prop) do
:skip
else
Expand Down Expand Up @@ -251,7 +255,7 @@ defmodule Protobuf.Encoder do
Enum.reduce(pb_exts, [], fn {{ext_mod, key}, val}, acc ->
case Protobuf.Extension.get_extension_props(mod, ext_mod, key) do
%{field_props: prop} ->
case encode_field(class_field(prop), val, :proto2, prop) do
case do_encode_field(class_field(prop), val, :proto2, prop) do
{:ok, iodata} -> [acc | iodata]
:skip -> acc
end
Expand Down
11 changes: 11 additions & 0 deletions test/protobuf/encoder_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,17 @@ defmodule Protobuf.EncoderTest do
end
end

test "raises correct error for nested protobufs" do
message = """
Got error when encoding TestMsg.Foo#e: ** (Protobuf.EncodeError) Got error when \
encoding TestMsg.Foo.Bar#a: ** (Protobuf.EncodeError) "not_a_number" is invalid for type :int32\
"""

assert_raise Protobuf.EncodeError, message, fn ->
Encoder.encode(%TestMsg.Foo{a: 90, e: %TestMsg.Foo.Bar{a: "not_a_number"}})
end
end

test "encodes with transformer module" do
msg = %TestMsg.ContainsTransformModule{field: 0}
assert Encoder.encode(msg) == <<10, 0>>
Expand Down

0 comments on commit 65faf65

Please sign in to comment.