From b721b1a01b1593d7d7581e0696ae02f5663cb104 Mon Sep 17 00:00:00 2001 From: David Ansari Date: Wed, 1 May 2024 11:58:09 +0200 Subject: [PATCH] Support maps and lists in AMQP array elements --- .../src/amqp10_binary_generator.erl | 31 ++++++++++------ .../src/amqp10_binary_parser.erl | 22 ++++++----- .../test/binary_generator_SUITE.erl | 37 ++++++++++++++++--- 3 files changed, 64 insertions(+), 26 deletions(-) diff --git a/deps/amqp10_common/src/amqp10_binary_generator.erl b/deps/amqp10_common/src/amqp10_binary_generator.erl index 0f3789c6eb..b829d797f2 100644 --- 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}) -> <>. -%% 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}) -> <>; generate2(uuid, {uuid, V}) -> <>; 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 index 7b4ad00781..f42a01180a 100644 --- 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(<>, B)-> {{symbol, V}, B+2+S}; -parse(<>, B)-> {{symbol, V}, B+5+S}; +parse(<>, B) -> {{symbol, V}, B+2+S}; +parse(<>, 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 index 877c4a553d..ac63d1b7a6 100644 --- 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.