Repository: thrift
Updated Branches:
  refs/heads/master 0eda90957 -> e544a8992


THRIFT-4266 Erlang library throws during skipping fields of composite type 
(maps, lists, structs, sets)
Client: Erlang
Patch: David Hull <david.h...@openx.com>

This closes #1316


Project: http://git-wip-us.apache.org/repos/asf/thrift/repo
Commit: http://git-wip-us.apache.org/repos/asf/thrift/commit/e544a899
Tree: http://git-wip-us.apache.org/repos/asf/thrift/tree/e544a899
Diff: http://git-wip-us.apache.org/repos/asf/thrift/diff/e544a899

Branch: refs/heads/master
Commit: e544a89924114ef11ba9af28ca7ad36583e54297
Parents: 0eda909
Author: David Hull <david.h...@openx.com>
Authored: Thu Jul 27 02:15:00 2017 +0200
Committer: Jens Geyer <je...@apache.org>
Committed: Sat Aug 5 14:04:21 2017 +0200

----------------------------------------------------------------------
 .gitignore                           |  8 ++--
 lib/erl/Makefile.am                  |  7 +++
 lib/erl/include/thrift_protocol.hrl  | 12 ++---
 lib/erl/src/thrift_protocol.erl      | 15 +++---
 lib/erl/test/Thrift_omit_with.thrift | 22 +++++++++
 lib/erl/test/test_omit.erl           | 79 +++++++++++++++++++++++++++++++
 6 files changed, 126 insertions(+), 17 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/thrift/blob/e544a899/.gitignore
----------------------------------------------------------------------
diff --git a/.gitignore b/.gitignore
index 00cf8bb..bdb84ea 100644
--- a/.gitignore
+++ b/.gitignore
@@ -207,13 +207,15 @@ project.lock.json
 /lib/delphi/test/typeregistry/*.identcache
 /lib/delphi/test/typeregistry/*.local
 /lib/delphi/test/typeregistry/*.dcu
-/lib/erl/.generated
 /lib/erl/.eunit
-/lib/erl/ebin
+/lib/erl/.generated
+/lib/erl/.rebar/
 /lib/erl/deps/
+/lib/erl/ebin
 /lib/erl/src/thrift.app.src
-/lib/erl/test/*.hrl
 /lib/erl/test/*.beam
+/lib/erl/test/*.hrl
+/lib/erl/test/Thrift_omit_without.thrift
 /lib/haxe/test/bin
 /lib/hs/dist
 /lib/java/build

http://git-wip-us.apache.org/repos/asf/thrift/blob/e544a899/lib/erl/Makefile.am
----------------------------------------------------------------------
diff --git a/lib/erl/Makefile.am b/lib/erl/Makefile.am
index 92e2204..20aadeb 100644
--- a/lib/erl/Makefile.am
+++ b/lib/erl/Makefile.am
@@ -18,7 +18,9 @@
 #
 
 THRIFT = ../../compiler/cpp/thrift
+THRIFT_OMIT_FILE = test/Thrift_omit_without.thrift
 THRIFT_FILES = $(wildcard test/*.thrift) \
+                 $(THRIFT_OMIT_FILE) \
                  ../../test/ConstantsDemo.thrift \
                  ../../test/NameConflictTest.thrift \
                  ../../test/ThriftTest.thrift
@@ -33,6 +35,10 @@ ERL_FLAG = erl
 ERL_FLAG_LEGACY = erl:legacynames
 ERL_FLAG_MAPS = erl:maps
 endif
+
+$(THRIFT_OMIT_FILE): test/Thrift_omit_with.thrift
+       grep -v omit $< >$@
+
 .generated: $(THRIFT) $(THRIFT_FILES)
        for f in $(THRIFT_FILES) ; do \
                $(THRIFT) --gen $(ERL_FLAG) -o test $$f ; \
@@ -65,6 +71,7 @@ uninstall:
 clean:
        rm -f .generated
        rm -rf test/gen-erl/
+       rm -f $(THRIFT_OMIT_FILE)
        $(REBAR) clean
 
 maintainer-clean-local:

http://git-wip-us.apache.org/repos/asf/thrift/blob/e544a899/lib/erl/include/thrift_protocol.hrl
----------------------------------------------------------------------
diff --git a/lib/erl/include/thrift_protocol.hrl 
b/lib/erl/include/thrift_protocol.hrl
index f85f455..bc0acc8 100644
--- a/lib/erl/include/thrift_protocol.hrl
+++ b/lib/erl/include/thrift_protocol.hrl
@@ -20,12 +20,12 @@
 -ifndef(THRIFT_PROTOCOL_INCLUDED).
 -define(THRIFT_PROTOCOL_INCLUDED, true).
 
--record(protocol_message_begin, {name, type, seqid}).
--record(protocol_struct_begin, {name}).
--record(protocol_field_begin, {name, type, id}).
--record(protocol_map_begin, {ktype, vtype, size}).
--record(protocol_list_begin, {etype, size}).
--record(protocol_set_begin, {etype, size}).
+-record(protocol_message_begin, {name :: string(), type :: integer(), seqid :: 
integer()}).
+-record(protocol_struct_begin, {name :: string()}).
+-record(protocol_field_begin, {name :: string(), type :: integer(), id :: 
integer()}).
+-record(protocol_map_begin, {ktype :: integer(), vtype :: integer(), size :: 
integer()}).
+-record(protocol_list_begin, {etype :: integer(), size :: integer()}).
+-record(protocol_set_begin, {etype :: integer(), size :: integer()}).
 
 -type tprot_header_val() :: #protocol_message_begin{}
                           | #protocol_struct_begin{}

http://git-wip-us.apache.org/repos/asf/thrift/blob/e544a899/lib/erl/src/thrift_protocol.erl
----------------------------------------------------------------------
diff --git a/lib/erl/src/thrift_protocol.erl b/lib/erl/src/thrift_protocol.erl
index dc3bfef..2fe10d6 100644
--- a/lib/erl/src/thrift_protocol.erl
+++ b/lib/erl/src/thrift_protocol.erl
@@ -219,12 +219,11 @@ read_struct_loop(IProto0, SDict, RTuple) ->
     end.
 
 skip_field(FType, IProto0, SDict, RTuple) ->
-    FTypeAtom = thrift_protocol:typeid_to_atom(FType),
-    {IProto1, ok} = thrift_protocol:skip(IProto0, FTypeAtom),
+    {IProto1, ok} = skip(IProto0, typeid_to_atom(FType)),
     {IProto2, ok} = read(IProto1, field_end),
     read_struct_loop(IProto2, SDict, RTuple).
 
--spec skip(#protocol{}, any()) -> {#protocol{}, ok}.
+-spec skip(#protocol{}, atom()) -> {#protocol{}, ok}.
 
 skip(Proto0, struct) ->
     {Proto1, ok} = read(Proto0, struct_begin),
@@ -261,7 +260,7 @@ skip_struct_loop(Proto0) ->
         ?tType_STOP ->
             {Proto1, ok};
         _Else ->
-            {Proto2, ok} = skip(Proto1, Type),
+            {Proto2, ok} = skip(Proto1, typeid_to_atom(Type)),
             {Proto3, ok} = read(Proto2, field_end),
             skip_struct_loop(Proto3)
     end.
@@ -271,8 +270,8 @@ skip_map_loop(Proto0, Map = #protocol_map_begin{ktype = 
Ktype,
                                                 size = Size}) ->
     case Size of
         N when N > 0 ->
-            {Proto1, ok} = skip(Proto0, Ktype),
-            {Proto2, ok} = skip(Proto1, Vtype),
+            {Proto1, ok} = skip(Proto0, typeid_to_atom(Ktype)),
+            {Proto2, ok} = skip(Proto1, typeid_to_atom(Vtype)),
             skip_map_loop(Proto2,
                           Map#protocol_map_begin{size = Size - 1});
         0 -> {Proto0, ok}
@@ -282,7 +281,7 @@ skip_set_loop(Proto0, Map = #protocol_set_begin{etype = 
Etype,
                                                 size = Size}) ->
     case Size of
         N when N > 0 ->
-            {Proto1, ok} = skip(Proto0, Etype),
+            {Proto1, ok} = skip(Proto0, typeid_to_atom(Etype)),
             skip_set_loop(Proto1,
                           Map#protocol_set_begin{size = Size - 1});
         0 -> {Proto0, ok}
@@ -292,7 +291,7 @@ skip_list_loop(Proto0, Map = #protocol_list_begin{etype = 
Etype,
                                                   size = Size}) ->
     case Size of
         N when N > 0 ->
-            {Proto1, ok} = skip(Proto0, Etype),
+            {Proto1, ok} = skip(Proto0, typeid_to_atom(Etype)),
             skip_list_loop(Proto1,
                            Map#protocol_list_begin{size = Size - 1});
         0 -> {Proto0, ok}

http://git-wip-us.apache.org/repos/asf/thrift/blob/e544a899/lib/erl/test/Thrift_omit_with.thrift
----------------------------------------------------------------------
diff --git a/lib/erl/test/Thrift_omit_with.thrift 
b/lib/erl/test/Thrift_omit_with.thrift
new file mode 100644
index 0000000..8bffc7c
--- /dev/null
+++ b/lib/erl/test/Thrift_omit_with.thrift
@@ -0,0 +1,22 @@
+struct test1 {
+  1: i32 one
+  2: i32 two                    // omit
+  3: i32 three
+}
+
+struct test2 {
+  1: i32 one
+  2: test2 two                  // omit
+  3: i32 three
+}
+
+struct test3 {
+  1: i32 one
+  2: list<test1> two            // omit
+}
+
+struct test4 {
+  1: i32 one
+  2: map<i32,test1> two         // omit
+}
+

http://git-wip-us.apache.org/repos/asf/thrift/blob/e544a899/lib/erl/test/test_omit.erl
----------------------------------------------------------------------
diff --git a/lib/erl/test/test_omit.erl b/lib/erl/test/test_omit.erl
new file mode 100644
index 0000000..80841e2
--- /dev/null
+++ b/lib/erl/test/test_omit.erl
@@ -0,0 +1,79 @@
+-module(test_omit).
+
+-include("gen-erl/thrift_omit_with_types.hrl").
+
+-ifdef(TEST).
+-include_lib("eunit/include/eunit.hrl").
+
+omit_struct1_test() ->
+  %% In this test, the field that is deleted is a basic type (an i32).
+  A = #test1{one = 1, three = 3},
+  B = #test1{one = 1, two = 2, three = 3},
+  {ok, Transport} = thrift_membuffer_transport:new(),
+  {ok, P0} = thrift_binary_protocol:new(Transport),
+
+  {P1, ok} = thrift_protocol:write(P0, {{struct, {thrift_omit_with_types, 
element(1, A)}}, A}),
+  {P2, {ok, O0}} = thrift_protocol:read(P1, {struct, 
{thrift_omit_without_types, element(1, A)}}),
+  ?assertEqual(element(1, A), element(1, O0)),
+  ?assertEqual(element(2, A), element(2, O0)),
+  ?assertEqual(element(4, A), element(3, O0)),
+
+  {P3, ok} = thrift_protocol:write(P2, {{struct, {thrift_omit_with_types, 
element(1, B)}}, B}),
+  {_P4, {ok, O1}} = thrift_protocol:read(P3, {struct, 
{thrift_omit_without_types, element(1, A)}}),
+  ?assertEqual(element(1, A), element(1, O1)),
+  ?assertEqual(element(2, A), element(2, O1)),
+  ?assertEqual(element(4, A), element(3, O1)),
+
+  ok.
+
+omit_struct2_test() ->
+  %% In this test, the field that is deleted is a struct.
+  A = #test2{one = 1, two = #test2{one = 10, three = 30}, three = 3},
+  B = #test2{one = 1, two = #test2{one = 10, two = #test2{one = 100}, three = 
30}, three = 3},
+
+  {ok, Transport} = thrift_membuffer_transport:new(),
+  {ok, P0} = thrift_binary_protocol:new(Transport),
+
+  {P1, ok} = thrift_protocol:write(P0, {{struct, {thrift_omit_with_types, 
element(1, A)}}, A}),
+  {P2, {ok, O0}} = thrift_protocol:read(P1, {struct, 
{thrift_omit_without_types, element(1, A)}}),
+  ?assertEqual(element(1, A), element(1, O0)),
+  ?assertEqual(element(2, A), element(2, O0)),
+  ?assertEqual(element(4, A), element(3, O0)),
+
+  {P3, ok} = thrift_protocol:write(P2, {{struct, {thrift_omit_with_types, 
element(1, B)}}, B}),
+  {_P4, {ok, O1}} = thrift_protocol:read(P3, {struct, 
{thrift_omit_without_types, element(1, A)}}),
+  ?assertEqual(element(1, A), element(1, O1)),
+  ?assertEqual(element(2, A), element(2, O1)),
+  ?assertEqual(element(4, A), element(3, O1)),
+
+  ok.
+
+omit_list_test() ->
+  %% In this test, the field that is deleted is a list.
+  A = #test1{one = 1, two = 2, three = 3},
+  B = #test3{one = 1, two = [ A ]},
+
+  {ok, Transport} = thrift_membuffer_transport:new(),
+  {ok, P0} = thrift_binary_protocol:new(Transport),
+
+  {P1, ok} = thrift_protocol:write(P0, {{struct, {thrift_omit_with_types, 
element(1, B)}}, B}),
+  {_P2, {ok, O0}} = thrift_protocol:read(P1, {struct, 
{thrift_omit_without_types, element(1, B)}}),
+  ?assertEqual(element(2, B), element(2, O0)),
+
+  ok.
+
+omit_map_test() ->
+  %% In this test, the field that is deleted is a map.
+  A = #test1{one = 1, two = 2, three = 3},
+  B = #test4{one = 1, two = dict:from_list([ {2, A} ])},
+
+  {ok, Transport} = thrift_membuffer_transport:new(),
+  {ok, P0} = thrift_binary_protocol:new(Transport),
+
+  {P1, ok} = thrift_protocol:write(P0, {{struct, {thrift_omit_with_types, 
element(1, B)}}, B}),
+  {_P2, {ok, O0}} = thrift_protocol:read(P1, {struct, 
{thrift_omit_without_types, element(1, B)}}),
+  ?assertEqual(element(2, B), element(2, O0)),
+
+  ok.
+
+-endif. %% TEST

Reply via email to