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.

Author: ansd

deps/amqp10_common/src/amqp10_binary_parser.erl

@@ -52,8 +52,8 @@ parse_primitive(16#52, <<V:8/unsigned,  R/binary>>) -> {{uint, V},       R};
 parse_primitive(16#53, <<V:8/unsigned,  R/binary>>) -> {{ulong, V},      R};
 parse_primitive(16#54, <<V:8/signed,    R/binary>>) -> {{int, V},        R};
 parse_primitive(16#55, <<V:8/signed,    R/binary>>) -> {{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, <<V:16/unsigned, R/binary>>) -> {{ushort, V},     R};
 parse_primitive(16#61, <<V:16/signed,   R/binary>>) -> {{short, V},      R};
 parse_primitive(16#70, <<V:32/unsigned, R/binary>>) -> {{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)];

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,13 +178,22 @@ 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
 
 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.

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">>},

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).

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)),

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.

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),
 

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;

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

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)