Support maps and lists in AMQP array elements

rabbitmq · May 1, 2024 · 805c23e · 805c23e
1 parent 7ed86f5
commit 805c23e
Show file tree

Hide file tree

Showing 3 changed files with 64 additions and 26 deletions.
diff --git a/deps/amqp10_common/src/amqp10_binary_generator.erl b/deps/amqp10_common/src/amqp10_binary_generator.erl
@@ -149,12 +149,12 @@ generate1({list, List}) ->
            [16#c0, S + 1, Count, Compound]
     end;
 
-generate1({map, ListOfPairs}) ->
-    Count = length(ListOfPairs) * 2,
+generate1({map, KvList}) ->
+    Count = length(KvList) * 2,
     Compound = lists:map(fun ({Key, Val}) ->
                                  [(generate1(Key)),
                                   (generate1(Val))]
-                         end, ListOfPairs),
+                         end, KvList),
     S = iolist_size(Compound),
     %% See generate1({list, ...}) for an explanation of this test.
     if Count >= (256 - 1) orelse (S + 1) >= 256 ->
@@ -172,16 +172,12 @@ generate1({array, Type, List}) ->
     if Count >= (256 - 1) orelse (S + 1) >= 256 ->
            [<<16#f0, (S + 4):32, Count:32>>, Array];
        true ->
-           [16#e0, S + 1,  Count, Array]
+           [16#e0, S + 1, Count, Array]
     end;
 
 generate1({as_is, TypeCode, Bin}) ->
     <<TypeCode, Bin>>.
 
-%% TODO again these are a stub to get SASL working. New codec? Will
-%% that ever happen? If not we really just need to split generate/1
-%% up into things like these...
-%% for these constructors map straight-forwardly
 constructor(symbol) -> 16#b3;
 constructor(ubyte) -> 16#50;
 constructor(ushort) -> 16#60;
@@ -198,13 +194,14 @@ constructor(timestamp) -> 16#83;
 constructor(uuid) -> 16#98;
 constructor(null) -> 16#40;
 constructor(boolean) -> 16#56;
-constructor(array) -> 16#f0; % use large array type for all nested arrays
 constructor(binary) -> 16#b0;
 constructor(utf8) -> 16#b1;
+constructor(list) -> 16#d0;  % use large list type for all array elements
+constructor(map) -> 16#d1;   % use large map type for all array elements
+constructor(array) -> 16#f0; % use large array type for all nested arrays
 constructor({described, Descriptor, Primitive}) ->
     [16#00, generate1(Descriptor), constructor(Primitive)].
 
-% returns io_list
 generate2(symbol, {symbol, V}) -> [<<(size(V)):32>>, V];
 generate2(utf8, {utf8, V}) -> [<<(size(V)):32>>, V];
 generate2(binary, {binary, V}) -> [<<(size(V)):32>>, V];
@@ -228,10 +225,22 @@ generate2(timestamp, {timestamp,V}) -> <<V:64/signed>>;
 generate2(uuid, {uuid, V}) -> <<V:16/binary>>;
 generate2({described, D, P}, {described, D, V}) ->
     generate2(P, V);
+generate2(list, {list, List}) ->
+    Count = length(List),
+    Compound = lists:map(fun generate1/1, List),
+    S = iolist_size(Compound),
+    [<<(S + 4):32, Count:32>>, Compound];
+generate2(map, {map, KvList}) ->
+    Count = length(KvList) * 2,
+    Compound = lists:map(fun ({Key, Val}) ->
+                                 [(generate1(Key)),
+                                  (generate1(Val))]
+                         end, KvList),
+    S = iolist_size(Compound),
+    [<<(S + 4):32, Count:32>>, Compound];
 generate2(array, {array, Type, List}) ->
     Count = length(List),
     Array = [constructor(Type),
              [generate2(Type, I) || I <- List]],
     S = iolist_size(Array),
-    %% See generate1({list, ...}) for an explanation of this test.
     [<<(S + 4):32, Count:32>>, Array].
diff --git a/deps/amqp10_common/src/amqp10_binary_parser.erl b/deps/amqp10_common/src/amqp10_binary_parser.erl
@@ -5,6 +5,12 @@
 %% Copyright (c) 2007-2024 Broadcom. All Rights Reserved. The term “Broadcom” refers to Broadcom Inc. and/or its subsidiaries. All rights reserved.
 %%
 
+%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
+%% NB: When compiling this file with "ERL_COMPILER_OPTIONS=bin_opt_info"
+%% make sure that all code outputs "OPTIMIZED: match context reused",
+%% i.e. neither "BINARY CREATED" nor "NOT OPTIMIZED" should be output.
+%% The only exception are arrays since arrays aren't used in the hot path.
+%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
 -module(amqp10_binary_parser).
 
 -include("amqp10_framing.hrl").
@@ -72,8 +78,8 @@ parse(<<16#98, Uuid:16/binary,_/binary>>, B) -> {{uuid, Uuid},    B+17};
 %% Variable-widths
 parse(<<16#a0, S:8, V:S/binary,_/binary>>, B)-> {{binary, V}, B+2+S};
 parse(<<16#a1, S:8, V:S/binary,_/binary>>, B)-> {{utf8, V},   B+2+S};
-parse(<<?CODE_SYM_8, S:8, V:S/binary,_/binary>>, B)-> {{symbol, V}, B+2+S};
-parse(<<?CODE_SYM_32, S:32,V:S/binary,_/binary>>, B)-> {{symbol, V}, B+5+S};
+parse(<<?CODE_SYM_8, S:8, V:S/binary,_/binary>>, B) -> {{symbol, V}, B+2+S};
+parse(<<?CODE_SYM_32, S:32,V:S/binary,_/binary>>, B) -> {{symbol, V}, B+5+S};
 parse(<<16#b0, S:32,V:S/binary,_/binary>>, B)-> {{binary, V}, B+5+S};
 parse(<<16#b1, S:32,V:S/binary,_/binary>>, B)-> {{utf8, V},   B+5+S};
 %% Compounds
@@ -157,10 +163,12 @@ parse_constructor(?CODE_ULONG) -> ulong;
 parse_constructor(16#81) -> long;
 parse_constructor(16#40) -> null;
 parse_constructor(16#56) -> boolean;
-parse_constructor(16#f0) -> array;
 parse_constructor(16#83) -> timestamp;
 parse_constructor(16#98) -> uuid;
-parse_constructor(0) -> described; %%TODO add test with descriptor in constructor
+parse_constructor(16#d0) -> list;
+parse_constructor(16#d1) -> map;
+parse_constructor(16#f0) -> array;
+parse_constructor(0) -> described;
 parse_constructor(X) ->
     exit({failed_to_parse_constructor, X}).
 
@@ -178,12 +186,6 @@ mapify([]) ->
 mapify([Key, Value | Rest]) ->
     [{Key, Value} | mapify(Rest)].
 
-%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
-%% NB: When compiling this file with "ERL_COMPILER_OPTIONS=bin_opt_info"
-%% make sure that all code below here outputs only "OPTIMIZED: match context reused"!
-%% Neither "BINARY CREATED" nor "NOT OPTIMIZED" must be output!
-%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
-
 %% Parses all AMQP types (or, in server_mode, stops when the body is reached).
 %% This is an optimisation over calling parse/1 repeatedly.
 %% We re-use the match context avoiding creation of sub binaries.

diff --git a/deps/amqp10_common/test/binary_generator_SUITE.erl b/deps/amqp10_common/test/binary_generator_SUITE.erl
@@ -109,7 +109,7 @@ numerals(_Config) ->
 
 utf8(_Config) ->
     roundtrip({utf8, <<"hi">>}),
-    roundtrip({utf8, binary:copy(<<"asdfghjk">>, 64)}),
+    roundtrip({utf8, binary:copy(<<"abcdefgh">>, 64)}),
     ok.
 
 char(_Config) ->
@@ -169,15 +169,42 @@ array(_Config) ->
     ?assertEqual({array, boolean, [true, false]},
                  roundtrip_return({array, boolean, [{boolean, true}, {boolean, false}]})),
 
-    % array of arrays
-    % TODO: does the inner type need to be consistent across the array?
+    %% array of arrays
     roundtrip({array, array, []}),
     roundtrip({array, array, [{array, symbol, [{symbol, <<"ANONYMOUS">>}]}]}),
+    roundtrip({array, array, [{array, symbol, [{symbol, <<"ANONYMOUS">>}]},
+                              {array, symbol, [{symbol, <<"sym1">>},
+                                               {symbol, <<"sym2">>}]}]}),
+
+    %% array of lists
+    roundtrip({array, list, []}),
+    roundtrip({array, list, [{list, [{symbol, <<"sym">>}]},
+                             {list, [null,
+                                     {described,
+                                      {utf8, <<"URL">>},
+                                      {utf8, <<"http://example.org/hello-world">>}}]},
+                             {list, []},
+                             {list, [true, false, {byte, -128}]}
+                            ]}),
+
+    %% array of maps
+    roundtrip({array, map, []}),
+    roundtrip({array, map, [{map, [{{symbol, <<"k1">>}, {utf8, <<"v1">>}}]},
+                            {map, []},
+                            {map, [{{described,
+                                     {utf8, <<"URL">>},
+                                     {utf8, <<"http://example.org/hello-world">>}},
+                                    {byte, -1}},
+                                   {{int, 0}, {ulong, 0}}
+                                  ]}
+                           ]}),
 
     Desc = {utf8, <<"URL">>},
-    roundtrip({array, {described, Desc, utf8},
-               [{described, Desc, {utf8, <<"http://example.org/hello">>}}]}),
     roundtrip({array, {described, Desc, utf8}, []}),
+    roundtrip({array, {described, Desc, utf8},
+               [{described, Desc, {utf8, <<"http://example.org/hello1">>}},
+                {described, Desc, {utf8, <<"http://example.org/hello2">>}}]}),
+
     %% array:array32
     roundtrip({array, boolean, [true || _ <- lists:seq(1, 256)]}),
     ok.