From 8b151f43f7c38f34717baaae312dae0865f7e416 Mon Sep 17 00:00:00 2001 From: David Ansari Date: Fri, 16 Feb 2024 13:04:53 +0100 Subject: [PATCH] Parse 2 bytes encoded AMQP boolean to Erlang boolean An AMQP boolean can by encoded using 1 byte or 2 bytes: https://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-types-v1.0-os.html#type-boolean Prior to this commit, our Erlang parser returned: * Erlang terms `true` or `false` for the 1 byte AMQP encoding * Erlang terms `{boolean, true}` or `{boolean, false}` for the 2 byte AMQP enconding Having a serializer and parser that perform the opposite actions such that ``` Term = parse(serialize(Term)) ``` is desirable as it provides a symmetric property useful not only for property based testing, but also for avoiding altering message hashes when serializing and parsing the same term. However, dealing wth `{boolean, boolean()}` tuples instead of `boolean()` is very unhandy since all Erlang code must take care of both forms leading to subtle bugs as occurred in: * https://github.com/rabbitmq/rabbitmq-server/blob/4cbeab897471fb55f1f0da02dbd178553370abe0/deps/rabbitmq_amqp1_0/src/rabbit_amqp1_0_message.erl#L155-L158 * https://github.com/rabbitmq/rabbitmq-server/blob/b8173c9d3b0e906d6d523c5eb6e4e75b4870b921/deps/rabbitmq_mqtt/src/mc_mqtt.erl#L83-L88 * https://github.com/rabbitmq/rabbitmq-server/blob/b8173c9d3b0e906d6d523c5eb6e4e75b4870b921/deps/rabbit/src/mc_amqpl.erl#L123-L127 Therefore, this commits decides to take the safe approach and always parse to an Erlang `boolean()` independent of whether the AMQP boolean was encoded with 1 or 2 bytes. --- .../src/amqp10_binary_parser.erl | 8 +++--- .../test/binary_generator_SUITE.erl | 26 ++++++++++++++----- .../test/binary_parser_SUITE.erl | 2 +- deps/rabbit/src/mc_amqp.erl | 5 ++-- deps/rabbit/test/mc_unit_SUITE.erl | 2 +- .../src/rabbit_amqp1_0_message.erl | 11 ++++---- .../test/amqp10_client_SUITE.erl | 7 +++-- deps/rabbitmq_mqtt/src/mc_mqtt.erl | 8 ++---- deps/rabbitmq_mqtt/test/mc_mqtt_SUITE.erl | 6 ++--- .../test/protocol_interop_SUITE.erl | 4 ++- 10 files changed, 47 insertions(+), 32 deletions(-) diff --git a/deps/amqp10_common/src/amqp10_binary_parser.erl b/deps/amqp10_common/src/amqp10_binary_parser.erl index 6a57d322944f..b4c4188b2862 100644 --- a/deps/amqp10_common/src/amqp10_binary_parser.erl +++ b/deps/amqp10_common/src/amqp10_binary_parser.erl @@ -52,8 +52,8 @@ parse_primitive(16#52, <>) -> {{uint, V}, R}; parse_primitive(16#53, <>) -> {{ulong, V}, R}; parse_primitive(16#54, <>) -> {{int, V}, R}; parse_primitive(16#55, <>) -> {{long, V}, R}; -parse_primitive(16#56, <<0:8/unsigned, R/binary>>) -> {{boolean, false},R}; -parse_primitive(16#56, <<1:8/unsigned, R/binary>>) -> {{boolean, true}, R}; +parse_primitive(16#56, <<0:8/unsigned, R/binary>>) -> {false, R}; +parse_primitive(16#56, <<1:8/unsigned, R/binary>>) -> {true, R}; parse_primitive(16#60, <>) -> {{ushort, V}, R}; parse_primitive(16#61, <>) -> {{short, V}, R}; parse_primitive(16#70, <>) -> {{uint, V}, R}; @@ -224,8 +224,8 @@ parse_all(<<16#52, V:8/unsigned, R/binary>>) -> [{uint, V} | parse_all(R)]; parse_all(<<16#53, V:8/unsigned, R/binary>>) -> [{ulong, V} | parse_all(R)]; parse_all(<<16#54, V:8/signed, R/binary>>) -> [{int, V} | parse_all(R)]; parse_all(<<16#55, V:8/signed, R/binary>>) -> [{long, V} | parse_all(R)]; -parse_all(<<16#56, 0:8/unsigned, R/binary>>) -> [{boolean, false} | parse_all(R)]; -parse_all(<<16#56, 1:8/unsigned, R/binary>>) -> [{boolean, true} | parse_all(R)]; +parse_all(<<16#56, 0:8/unsigned, R/binary>>) -> [false | parse_all(R)]; +parse_all(<<16#56, 1:8/unsigned, R/binary>>) -> [true | parse_all(R)]; parse_all(<<16#60, V:16/unsigned, R/binary>>) -> [{ushort, V} | parse_all(R)]; parse_all(<<16#61, V:16/signed, R/binary>>) -> [{short, V} | parse_all(R)]; parse_all(<<16#70, V:32/unsigned, R/binary>>) -> [{uint, V} | parse_all(R)]; diff --git a/deps/amqp10_common/test/binary_generator_SUITE.erl b/deps/amqp10_common/test/binary_generator_SUITE.erl index 6fdc64a6299d..0f6ffe678e43 100644 --- a/deps/amqp10_common/test/binary_generator_SUITE.erl +++ b/deps/amqp10_common/test/binary_generator_SUITE.erl @@ -67,9 +67,8 @@ null(_Config) -> booleans(_Config) -> roundtrip(true), roundtrip(false), - roundtrip({boolean, false}), - roundtrip({boolean, true}), - ok. + ?assertEqual(true, roundtrip_return({boolean, true})), + ?assertEqual(false, roundtrip_return({boolean, false})). symbol(_Config) -> roundtrip({symbol, <<"SYMB">>}), @@ -164,7 +163,11 @@ array(_Config) -> roundtrip({array, ulong, [{ulong, 0}, {ulong, 16#FFFFFFFFFFFFFFFF}]}), roundtrip({array, long, [{long, 0}, {long, -16#8000000000000}, {long, 16#7FFFFFFFFFFFFF}]}), - roundtrip({array, boolean, [{boolean, true}, {boolean, false}]}), + roundtrip({array, boolean, [true, false]}), + + ?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? roundtrip({array, array, []}), @@ -175,7 +178,7 @@ array(_Config) -> [{described, Desc, {utf8, <<"http://example.org/hello">>}}]}), roundtrip({array, {described, Desc, utf8}, []}), %% array:array32 - roundtrip({array, boolean, [{boolean, true} || _ <- lists:seq(1, 256)]}), + roundtrip({array, boolean, [true || _ <- lists:seq(1, 256)]}), ok. %% Utility @@ -183,5 +186,14 @@ array(_Config) -> roundtrip(Term) -> Bin = iolist_to_binary(amqp10_binary_generator:generate(Term)), % generate returns an iolist but parse expects a binary - ?assertMatch({Term, _}, amqp10_binary_parser:parse(Bin)), - ?assertMatch([Term | _], amqp10_binary_parser:parse_all(Bin)). + ?assertEqual({Term, <<>>}, amqp10_binary_parser:parse(Bin)), + ?assertEqual([Term], amqp10_binary_parser:parse_all(Bin)). + +%% Return the roundtripped term. +roundtrip_return(Term) -> + Bin = iolist_to_binary(amqp10_binary_generator:generate(Term)), + %% We assert only that amqp10_binary_parser:parse/1 and + %% amqp10_binary_parser:parse_all/1 return the same term. + {RoundTripTerm, <<>>} = amqp10_binary_parser:parse(Bin), + ?assertEqual([RoundTripTerm], amqp10_binary_parser:parse_all(Bin)), + RoundTripTerm. diff --git a/deps/amqp10_common/test/binary_parser_SUITE.erl b/deps/amqp10_common/test/binary_parser_SUITE.erl index 4aec1f5dac28..76918e66c395 100644 --- a/deps/amqp10_common/test/binary_parser_SUITE.erl +++ b/deps/amqp10_common/test/binary_parser_SUITE.erl @@ -61,7 +61,7 @@ roundtrip(_Config) -> {symbol, <<"URL">>}, {binary, <<"https://rabbitmq.com">>}}, {array, ubyte, [{ubyte, 1}, {ubyte, 255}]}, - {boolean, false}, + true, {list, [{utf8, <<"hi">>}, {described, {symbol, <<"URL">>}, diff --git a/deps/rabbit/src/mc_amqp.erl b/deps/rabbit/src/mc_amqp.erl index 70cbb4b1aeae..16e25f14d135 100644 --- a/deps/rabbit/src/mc_amqp.erl +++ b/deps/rabbit/src/mc_amqp.erl @@ -313,8 +313,9 @@ application_properties_as_simple_map(#msg{application_properties = Content}, ({{utf8, K}, {_T, V}}, Acc) when ?SIMPLE_VALUE(V) -> Acc#{K => V}; - ({{utf8, K}, undefined}, Acc) -> - Acc#{K => undefined}; + ({{utf8, K}, V}, Acc) + when V =:= undefined orelse is_boolean(V) -> + Acc#{K => V}; (_, Acc)-> Acc end, M, Content). diff --git a/deps/rabbit/test/mc_unit_SUITE.erl b/deps/rabbit/test/mc_unit_SUITE.erl index 17182b05cd5b..1655394deb0c 100644 --- a/deps/rabbit/test/mc_unit_SUITE.erl +++ b/deps/rabbit/test/mc_unit_SUITE.erl @@ -333,7 +333,7 @@ amqpl_amqp_bin_amqpl(_Config) -> ?assertEqual({long, 99}, Get(<<"a-stream-offset">>, AP10)), ?assertEqual({utf8, <<"a string">>}, Get(<<"a-string">>, AP10)), - ?assertEqual({boolean, false}, Get(<<"a-bool">>, AP10)), + ?assertEqual(false, Get(<<"a-bool">>, AP10)), ?assertEqual({ubyte, 1}, Get(<<"a-unsignedbyte">>, AP10)), ?assertEqual({ushort, 1}, Get(<<"a-unsignedshort">>, AP10)), ?assertEqual({uint, 1}, Get(<<"a-unsignedint">>, AP10)), diff --git a/deps/rabbitmq_amqp1_0/src/rabbit_amqp1_0_message.erl b/deps/rabbitmq_amqp1_0/src/rabbit_amqp1_0_message.erl index 7d39ddc15140..2f1d30c03f30 100644 --- a/deps/rabbitmq_amqp1_0/src/rabbit_amqp1_0_message.erl +++ b/deps/rabbitmq_amqp1_0/src/rabbit_amqp1_0_message.erl @@ -322,19 +322,20 @@ amqp10_app_props_to_amqp091_headers(CurrentHeaders, AppPropsBin) -> end, lists:foldl(fun(Prop, Acc) -> case Prop of - {{utf8, Key}, {ValueType, Value}} -> - case type10_to_type091(Key, ValueType, Value) of + {{utf8, Key}, TypeVal} -> + case type10_to_type091(Key, TypeVal) of undefined -> Acc; Typed -> [Typed |Acc] end; - _ -> Acc + _ -> + Acc end end, Hs, AppProps); _ -> CurrentHeaders end. -type10_to_type091(Key, Type, Value) -> +type10_to_type091(Key, TypeVal) -> try - rabbit_msg_record:to_091(Key, {Type, Value}) + rabbit_msg_record:to_091(Key, TypeVal) catch _:function_clause -> undefined end. diff --git a/deps/rabbitmq_amqp1_0/test/amqp10_client_SUITE.erl b/deps/rabbitmq_amqp1_0/test/amqp10_client_SUITE.erl index ba7b5a2a8349..beeabe548e5b 100644 --- a/deps/rabbitmq_amqp1_0/test/amqp10_client_SUITE.erl +++ b/deps/rabbitmq_amqp1_0/test/amqp10_client_SUITE.erl @@ -128,7 +128,10 @@ reliable_send_receive(Config, Outcome) -> %% create an unsettled message, %% link will be in "mixed" mode by default Msg1 = amqp10_msg:new(DTag1, <<"body-1">>, false), - ok = amqp10_client:send_msg(Sender, Msg1), + %% Use the 2 byte AMQP boolean encoding, see AMQP §1.6.2 + True = {boolean, true}, + Msg2 = amqp10_msg:set_headers(#{durable => True}, Msg1), + ok = amqp10_client:send_msg(Sender, Msg2), ok = wait_for_settlement(DTag1), ok = amqp10_client:detach_link(Sender), @@ -143,8 +146,8 @@ reliable_send_receive(Config, Outcome) -> Address, unsettled), {ok, Msg} = amqp10_client:get_msg(Receiver), - ct:pal("got ~p", [amqp10_msg:body(Msg)]), + ?assertEqual(true, amqp10_msg:header(durable, Msg)), ok = amqp10_client:settle_msg(Receiver, Msg, Outcome), diff --git a/deps/rabbitmq_mqtt/src/mc_mqtt.erl b/deps/rabbitmq_mqtt/src/mc_mqtt.erl index 87f5583bdc10..a5663f64bccf 100644 --- a/deps/rabbitmq_mqtt/src/mc_mqtt.erl +++ b/deps/rabbitmq_mqtt/src/mc_mqtt.erl @@ -542,13 +542,9 @@ amqp_to_utf8_string({T, Val}) when T =:= double; T =:= float -> float_to_binary(Val); -amqp_to_utf8_string(Val) - when Val =:= true; - Val =:= {boolean, true} -> +amqp_to_utf8_string(true) -> <<"true">>; -amqp_to_utf8_string(Val) - when Val =:= false; - Val =:= {boolean, false} -> +amqp_to_utf8_string(false) -> <<"false">>; amqp_to_utf8_string({T, _Val}) when T =:= map; diff --git a/deps/rabbitmq_mqtt/test/mc_mqtt_SUITE.erl b/deps/rabbitmq_mqtt/test/mc_mqtt_SUITE.erl index 5b93eb819a21..c57e3e632497 100644 --- a/deps/rabbitmq_mqtt/test/mc_mqtt_SUITE.erl +++ b/deps/rabbitmq_mqtt/test/mc_mqtt_SUITE.erl @@ -219,7 +219,7 @@ amqp_to_mqtt_amqp_value_section_boolean(_Config) -> ?assertEqual(#{'Payload-Format-Indicator' => 1}, Props1), ?assertEqual(<<"true">>, iolist_to_binary(Payload1)), - Val2 = amqp_value({boolean, false}), + Val2 = amqp_value(false), #mqtt_msg{props = Props2, payload = Payload2} = amqp_to_mqtt([Val2]), ?assertEqual(#{'Payload-Format-Indicator' => 1}, Props2), @@ -460,7 +460,7 @@ amqp_mqtt(_Config) -> thead(float, 6.0), thead(timestamp, 7000), thead(byte, 128), - thead(boolean, true), + {{utf8, <<"boolean1">>}, true}, {{utf8, <<"boolean2">>}, false}, {utf8(<<"null">>), null} ], @@ -493,7 +493,7 @@ amqp_mqtt(_Config) -> <<"6.00000000000000000000e+00">>}, {<<"timestamp">>,<<"7">>}, {<<"byte">>,<<"128">>}, - {<<"boolean">>,<<"true">>}, + {<<"boolean1">>,<<"true">>}, {<<"boolean2">>,<<"false">>}, {<<"null">>,<<>>}], 'Correlation-Data' := CorrIdOut diff --git a/deps/rabbitmq_mqtt/test/protocol_interop_SUITE.erl b/deps/rabbitmq_mqtt/test/protocol_interop_SUITE.erl index cfa7a79dc491..2a50408692b2 100644 --- a/deps/rabbitmq_mqtt/test/protocol_interop_SUITE.erl +++ b/deps/rabbitmq_mqtt/test/protocol_interop_SUITE.erl @@ -220,7 +220,9 @@ amqp(Config) -> #{correlation_id => Correlation, content_type => ContentType}, Msg2a), - Msg2 = amqp10_msg:set_headers(#{durable => true}, Msg2b), + %% Use the 2 byte AMQP boolean encoding, see AMQP §1.6.2 + True = {boolean, true}, + Msg2 = amqp10_msg:set_headers(#{durable => True}, Msg2b), ok = amqp10_client:send_msg(Sender, Msg2), receive {amqp10_disposition, {accepted, DTag}} -> ok after 1000 -> ct:fail(settled_timeout)