Skip to content

Commit b4149d9

Browse files
committed
Fix mangled Protobuf.EncodeError for nested structs
Because of the recursive implementation of `encode_fields`, unrelated fields would get mixed up in encode errors from nested structs. 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) ```
1 parent 8cf602d commit b4149d9

File tree

2 files changed

+22
-7
lines changed

2 files changed

+22
-7
lines changed

lib/protobuf/encoder.ex

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -60,17 +60,21 @@ defmodule Protobuf.Encoder do
6060
end
6161

6262
acc =
63-
case encode_field(class_field(prop), val, syntax, prop) do
63+
case encode_field(prop, struct.__struct__, val, syntax) do
6464
{:ok, iodata} -> [acc | iodata]
6565
:skip -> acc
6666
end
6767

6868
encode_fields(rest, syntax, struct, oneofs, acc)
69+
end
70+
71+
defp encode_field(prop, struct_mod, val, syntax) do
72+
do_encode_field(class_field(prop), val, syntax, prop)
6973
rescue
7074
error ->
7175
raise Protobuf.EncodeError,
7276
message:
73-
"Got error when encoding #{inspect(struct.__struct__)}##{prop.name_atom}: #{Exception.format(:error, error)}"
77+
"Got error when encoding #{inspect(struct_mod)}##{prop.name_atom}: #{Exception.format(:error, error)}"
7478
end
7579

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

91-
defp encode_field(
95+
defp do_encode_field(
9296
:normal,
9397
val,
9498
syntax,
@@ -102,11 +106,11 @@ defmodule Protobuf.Encoder do
102106
end
103107
end
104108

105-
defp encode_field(:embedded, _val = nil, _syntax, _prop) do
109+
defp do_encode_field(:embedded, _val = nil, _syntax, _prop) do
106110
:skip
107111
end
108112

109-
defp encode_field(
113+
defp do_encode_field(
110114
:embedded,
111115
val,
112116
syntax,
@@ -131,7 +135,7 @@ defmodule Protobuf.Encoder do
131135
{:ok, result}
132136
end
133137

134-
defp encode_field(:packed, val, syntax, %FieldProps{type: type, encoded_fnum: fnum} = prop) do
138+
defp do_encode_field(:packed, val, syntax, %FieldProps{type: type, encoded_fnum: fnum} = prop) do
135139
if skip_field?(syntax, val, prop) or skip_enum?(syntax, val, prop) do
136140
:skip
137141
else
@@ -251,7 +255,7 @@ defmodule Protobuf.Encoder do
251255
Enum.reduce(pb_exts, [], fn {{ext_mod, key}, val}, acc ->
252256
case Protobuf.Extension.get_extension_props(mod, ext_mod, key) do
253257
%{field_props: prop} ->
254-
case encode_field(class_field(prop), val, :proto2, prop) do
258+
case do_encode_field(class_field(prop), val, :proto2, prop) do
255259
{:ok, iodata} -> [acc | iodata]
256260
:skip -> acc
257261
end

test/protobuf/encoder_test.exs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,17 @@ defmodule Protobuf.EncoderTest do
296296
end
297297
end
298298

299+
test "raises correct error for nested protobufs" do
300+
message = """
301+
Got error when encoding TestMsg.Foo#e: ** (Protobuf.EncodeError) Got error when \
302+
encoding TestMsg.Foo.Bar#a: ** (Protobuf.EncodeError) "not_a_number" is invalid for type :int32\
303+
"""
304+
305+
assert_raise Protobuf.EncodeError, message, fn ->
306+
Encoder.encode(%TestMsg.Foo{a: 90, e: %TestMsg.Foo.Bar{a: "not_a_number"}})
307+
end
308+
end
309+
299310
test "encodes with transformer module" do
300311
msg = %TestMsg.ContainsTransformModule{field: 0}
301312
assert Encoder.encode(msg) == <<10, 0>>

0 commit comments

Comments
 (0)