fix(builder): handle case better when message struct not matched

Author: tony612

lib/protobuf/builder.ex

@@ -39,36 +39,51 @@ defmodule Protobuf.Builder do
 
   defp new_maybe_strict(mod, attrs, strict?) do
     case attrs do
-      %{__struct__: _} ->
+      # If the attrs is the module, we just return it
+      %{__struct__: ^mod} ->
         attrs
 
+      # If the module in the attrs doesn't match with mod
+      # raise error in strict and try changing it to a map in non-strict
+      %{__struct__: _} ->
+        if strict? do
+          raise ArgumentError,
+            message: "The __struct__ in the struct doesn't with the message module"
+        else
+          do_new_maybe_strict(mod, Map.from_struct(attrs), strict?)
+        end
+
       _ ->
-        props = mod.__message_props__()
-        default_struct = mod.__default_struct__()
-        msg = if strict?, do: struct!(default_struct, attrs), else: struct(default_struct, attrs)
+        do_new_maybe_strict(mod, attrs, strict?)
+    end
+  end
 
-        Enum.reduce(props.embedded_fields, msg, fn k, acc ->
-          case msg do
-            %{^k => v} when not is_nil(v) ->
-              f_props = props.field_props[props.field_tags[k]]
+  defp do_new_maybe_strict(mod, attrs, strict?) do
+    props = mod.__message_props__()
+    default_struct = mod.__default_struct__()
+    msg = if strict?, do: struct!(default_struct, attrs), else: struct(default_struct, attrs)
 
-              v =
-                if f_props.embedded? do
-                  if f_props.repeated? do
-                    Enum.map(v, fn i -> f_props.type.new(i) end)
-                  else
-                    f_props.type.new(v)
-                  end
-                else
-                  v
-                end
+    Enum.reduce(props.embedded_fields, msg, fn k, acc ->
+      case msg do
+        %{^k => v} when not is_nil(v) ->
+          f_props = props.field_props[props.field_tags[k]]
 
-              Map.put(acc, k, v)
+          v =
+            if f_props.embedded? do
+              if f_props.repeated? do
+                Enum.map(v, fn i -> f_props.type.new(i) end)
+              else
+                f_props.type.new(v)
+              end
+            else
+              v
+            end
 
-            _ ->
-              acc
-          end
-        end)
-    end
+          Map.put(acc, k, v)
+
+        _ ->
+          acc
+      end
+    end)
   end
 end

mix.exs

@@ -33,7 +33,7 @@ defmodule Protobuf.Mixfile do
       {:dialyxir, "~> 0.5", only: [:dev, :test], runtime: false},
       {:credo, "~> 0.8", only: [:dev, :test], runtime: false},
       {:ex_doc, "~> 0.14", only: :dev, runtime: false},
-      {:eqc_ex, "~> 1.4", only: [:dev, :test]}
+      {:eqc_ex, github: "tony612/eqc_ex", branch: "fix-elixir-1-10", only: [:dev, :test]}
     ]
   end
 

mix.lock

@@ -1,12 +1,12 @@
 %{
-  "bunt": {:hex, :bunt, "0.2.0", "951c6e801e8b1d2cbe58ebbd3e616a869061ddadcc4863d0a2182541acae9a38", [:mix], [], "hexpm"},
-  "credo": {:hex, :credo, "0.9.3", "76fa3e9e497ab282e0cf64b98a624aa11da702854c52c82db1bf24e54ab7c97a", [:mix], [{:bunt, "~> 0.2.0", [hex: :bunt, repo: "hexpm", optional: false]}, {:poison, ">= 0.0.0", [hex: :poison, repo: "hexpm", optional: false]}], "hexpm"},
-  "dialyxir": {:hex, :dialyxir, "0.5.1", "b331b091720fd93e878137add264bac4f644e1ddae07a70bf7062c7862c4b952", [:mix], [], "hexpm"},
-  "earmark": {:hex, :earmark, "1.3.2", "b840562ea3d67795ffbb5bd88940b1bed0ed9fa32834915125ea7d02e35888a5", [:mix], [], "hexpm"},
-  "eqc_ex": {:hex, :eqc_ex, "1.4.2", "c89322cf8fbd4f9ddcb18141fb162a871afd357c55c8c0198441ce95ffe2e105", [:mix], [], "hexpm"},
-  "ex_doc": {:hex, :ex_doc, "0.19.3", "3c7b0f02851f5fc13b040e8e925051452e41248f685e40250d7e40b07b9f8c10", [:mix], [{:earmark, "~> 1.2", [hex: :earmark, repo: "hexpm", optional: false]}, {:makeup_elixir, "~> 0.10", [hex: :makeup_elixir, repo: "hexpm", optional: false]}], "hexpm"},
-  "makeup": {:hex, :makeup, "0.8.0", "9cf32aea71c7fe0a4b2e9246c2c4978f9070257e5c9ce6d4a28ec450a839b55f", [:mix], [{:nimble_parsec, "~> 0.5.0", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm"},
-  "makeup_elixir": {:hex, :makeup_elixir, "0.13.0", "be7a477997dcac2e48a9d695ec730b2d22418292675c75aa2d34ba0909dcdeda", [:mix], [{:makeup, "~> 0.8", [hex: :makeup, repo: "hexpm", optional: false]}], "hexpm"},
-  "nimble_parsec": {:hex, :nimble_parsec, "0.5.0", "90e2eca3d0266e5c53f8fbe0079694740b9c91b6747f2b7e3c5d21966bba8300", [:mix], [], "hexpm"},
-  "poison": {:hex, :poison, "3.1.0", "d9eb636610e096f86f25d9a46f35a9facac35609a7591b3be3326e99a0484665", [:mix], [], "hexpm"},
+  "bunt": {:hex, :bunt, "0.2.0", "951c6e801e8b1d2cbe58ebbd3e616a869061ddadcc4863d0a2182541acae9a38", [:mix], [], "hexpm", "7af5c7e09fe1d40f76c8e4f9dd2be7cebd83909f31fee7cd0e9eadc567da8353"},
+  "credo": {:hex, :credo, "0.9.3", "76fa3e9e497ab282e0cf64b98a624aa11da702854c52c82db1bf24e54ab7c97a", [:mix], [{:bunt, "~> 0.2.0", [hex: :bunt, repo: "hexpm", optional: false]}, {:poison, ">= 0.0.0", [hex: :poison, repo: "hexpm", optional: false]}], "hexpm", "dcd1d45626f6a02abeef3fc424eaf101b05a851d3cceb9535b8ea3e14c3c17e6"},
+  "dialyxir": {:hex, :dialyxir, "0.5.1", "b331b091720fd93e878137add264bac4f644e1ddae07a70bf7062c7862c4b952", [:mix], [], "hexpm", "6c32a70ed5d452c6650916555b1f96c79af5fc4bf286997f8b15f213de786f73"},
+  "earmark": {:hex, :earmark, "1.3.2", "b840562ea3d67795ffbb5bd88940b1bed0ed9fa32834915125ea7d02e35888a5", [:mix], [], "hexpm", "e3be2bc3ae67781db529b80aa7e7c49904a988596e2dbff897425b48b3581161"},
+  "eqc_ex": {:git, "https://github.com/tony612/eqc_ex.git", "43759ba1fa56dec40b0346f2e23268d18f3ccda1", [branch: "fix-elixir-1-10"]},
+  "ex_doc": {:hex, :ex_doc, "0.19.3", "3c7b0f02851f5fc13b040e8e925051452e41248f685e40250d7e40b07b9f8c10", [:mix], [{:earmark, "~> 1.2", [hex: :earmark, repo: "hexpm", optional: false]}, {:makeup_elixir, "~> 0.10", [hex: :makeup_elixir, repo: "hexpm", optional: false]}], "hexpm", "0e11d67e662142fc3945b0ee410c73c8c956717fbeae4ad954b418747c734973"},
+  "makeup": {:hex, :makeup, "0.8.0", "9cf32aea71c7fe0a4b2e9246c2c4978f9070257e5c9ce6d4a28ec450a839b55f", [:mix], [{:nimble_parsec, "~> 0.5.0", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm", "5fbc8e549aa9afeea2847c0769e3970537ed302f93a23ac612602e805d9d1e7f"},
+  "makeup_elixir": {:hex, :makeup_elixir, "0.13.0", "be7a477997dcac2e48a9d695ec730b2d22418292675c75aa2d34ba0909dcdeda", [:mix], [{:makeup, "~> 0.8", [hex: :makeup, repo: "hexpm", optional: false]}], "hexpm", "adf0218695e22caeda2820eaba703fa46c91820d53813a2223413da3ef4ba515"},
+  "nimble_parsec": {:hex, :nimble_parsec, "0.5.0", "90e2eca3d0266e5c53f8fbe0079694740b9c91b6747f2b7e3c5d21966bba8300", [:mix], [], "hexpm", "5c040b8469c1ff1b10093d3186e2e10dbe483cd73d79ec017993fb3985b8a9b3"},
+  "poison": {:hex, :poison, "3.1.0", "d9eb636610e096f86f25d9a46f35a9facac35609a7591b3be3326e99a0484665", [:mix], [], "hexpm", "fec8660eb7733ee4117b85f55799fd3833eb769a6df71ccf8903e8dc5447cfce"},
 }

test/protobuf/builder_test.exs

@@ -53,4 +53,18 @@ defmodule Protobuf.BuilderTest do
       Foo.new!(nonexisting_field: "foo")
     end
   end
+
+  test "new!/2 raises for non matched strct" do
+    assert_raise ArgumentError, fn ->
+      Foo.new!(Foo2.new())
+    end
+  end
+
+  test "new/2 build correct message for non matched strct" do
+    foo = Foo.new(Foo2.new(non_matched: 1))
+
+    assert_raise Protobuf.EncodeError, fn ->
+      Foo.encode(foo)
+    end
+  end
 end

test/protobuf/dsl_test.exs

@@ -21,7 +21,7 @@ defmodule Protobuf.DSLTest do
     msg_props = Foo.__message_props__()
 
     tags_map =
-      Enum.reduce([1, 2, 3] ++ Enum.to_list(5..17), %{}, fn i, acc -> Map.put(acc, i, i) end)
+      Enum.reduce([1, 2, 3] ++ Enum.to_list(5..17) ++ [101], %{}, fn i, acc -> Map.put(acc, i, i) end)
 
     assert tags_map == msg_props.tags_map
     field_props = msg_props.field_props
@@ -62,7 +62,7 @@ defmodule Protobuf.DSLTest do
 
   test "saves ordered tags" do
     msg_props = Foo.__message_props__()
-    assert [1, 2, 3] ++ Enum.to_list(5..17) == msg_props.ordered_tags
+    assert [1, 2, 3] ++ Enum.to_list(5..17) ++ [101] == msg_props.ordered_tags
   end
 
   test "supports embedded fields" do
@@ -176,7 +176,8 @@ defmodule Protobuf.DSLTest do
              m: :UNKNOWN,
              n: 0.0,
              o: [],
-             p: ""
+             p: "",
+             non_matched: ""
            } == Foo.__default_struct__()
   end
 

test/support/test_msg.ex

@@ -33,7 +33,7 @@ defmodule TestMsg do
   defmodule Foo do
     use Protobuf, syntax: :proto3
 
-    defstruct [:a, :b, :c, :d, :e, :f, :g, :h, :i, :j, :k, :l, :m, :n, :o, :p]
+    defstruct [:a, :b, :c, :d, :e, :f, :g, :h, :i, :j, :k, :l, :m, :n, :o, :p, :non_matched]
 
     field :a, 1, type: :int32
     field :b, 2, type: :fixed64
@@ -52,12 +52,14 @@ defmodule TestMsg do
     field :n, 15, type: :double
     field :o, 16, repeated: true, type: EnumFoo, enum: true
     field :p, 17, type: :string, deprecated: true
+    # Used for testing against same field name with different types(in Foo2)
+    field :non_matched, 101, type: :string
   end
 
   defmodule Foo2 do
     use Protobuf, syntax: :proto2
 
-    defstruct [:a, :b, :c, :d, :e, :f, :g, :h, :i, :j, :k, :l, :m]
+    defstruct [:a, :b, :c, :d, :e, :f, :g, :h, :i, :j, :k, :l, :m, :non_matched]
 
     field :a, 1, required: true, type: :int32
     field :b, 2, optional: true, type: :fixed64, default: 5
@@ -72,7 +74,7 @@ defmodule TestMsg do
     # field :j, 11, optional: true, type: EnumFoo, enum: true
     # field :k, 12, optioanl: true, type: :bool
     field :l, 13, repeated: true, type: MapFoo, map: true
-    # field :m, 14, optional: true, type: EnumFoo, default: :B, enum: true
+    field :non_matched, 101, type: :int32, optional: true
   end
 
   defmodule Oneof do