Remove unnecessary call to xmerl_ucs:from_utf8

This commit removes a call to xmerl_ucs:from_utf8 which was used
for two purposes: converting the binary payload into an Erlang
Unicode string (a list of integer code points), and validating
that the payload is indeed utf8.

The call was necessary when it was introduced because mochijson
only supported Unicode strings. Nowadays JSON libraries also
support utf8 binaries so converting is not necessary.

The function also starts by doing a binary_to_list/1 call which
creates a lot of garbage and can lead to OOM situations when the
queue is large. We ended up with the payload binary, the temporary
list and the final Unicode string in memory. With this patch we
only ever have the binary in memory and the memory consumption
is divided by more or less 3.

To validate that the payload is utf8 and keep the functionality
intact a small function was added that makes use of the /utf8
binary matching specifier.

Since this was the only xmerl function used in the management
plugin I have also removed xmerl from the LOCAL_DEPS.

I have added a test for the different encodings that can be
requested to make sure that nothing was broken when doing
the change.

Author: essen

Makefile

@@ -21,7 +21,7 @@ endef
 
 DEPS = rabbit_common rabbit amqp_client cowboy cowlib rabbitmq_web_dispatch rabbitmq_management_agent
 TEST_DEPS = rabbitmq_ct_helpers rabbitmq_ct_client_helpers proper
-LOCAL_DEPS += xmerl mnesia ranch ssl crypto public_key
+LOCAL_DEPS += mnesia ranch ssl crypto public_key
 
 # FIXME: Add Ranch as a BUILD_DEPS to be sure the correct version is picked.
 # See rabbitmq-components.mk.

src/rabbit_mgmt_wm_queue_get.erl

@@ -162,13 +162,14 @@ maybe_truncate(Payload, Len) ->
 
 payload_part(Payload, Enc) ->
     {PL, E} = case Enc of
-                  auto -> try
-                              %% TODO mochijson does this but is it safe?
-                              _ = xmerl_ucs:from_utf8(Payload),
-                              {Payload, string}
-                          catch exit:{ucs, _} ->
-                                  {base64:encode(Payload), base64}
+                  auto -> case is_utf8(Payload) of
+                              true -> {Payload, string};
+                              false -> {base64:encode(Payload), base64}
                           end;
                   _    -> {base64:encode(Payload), base64}
               end,
     [{payload, PL}, {payload_encoding, E}].
+
+is_utf8(<<>>) -> true;
+is_utf8(<<_/utf8, Rest/bits>>) -> is_utf8(Rest);
+is_utf8(_) -> false.

test/rabbit_mgmt_http_SUITE.erl

@@ -116,6 +116,7 @@ all_tests() -> [
     format_output_test,
     columns_test,
     get_test,
+    get_encoding_test,
     get_fail_test,
     publish_test,
     publish_accept_json_test,
@@ -2245,6 +2246,41 @@ get_test(Config) ->
 
     passed.
 
+get_encoding_test(Config) ->
+    Utf8Text = <<"Loïc was here!"/utf8>>,
+    Utf8Payload = base64:encode(Utf8Text),
+    BinPayload = base64:encode(<<0:64, 16#ff, 16#fd, 0:64>>),
+    Utf8Msg = msg(<<"get_encoding_test">>, #{}, Utf8Payload, <<"base64">>),
+    BinMsg  = msg(<<"get_encoding_test">>, #{}, BinPayload, <<"base64">>),
+    http_put(Config, "/queues/%2f/get_encoding_test", #{}, {group, '2xx'}),
+    http_post(Config, "/exchanges/%2f/amq.default/publish", Utf8Msg, ?OK),
+    http_post(Config, "/exchanges/%2f/amq.default/publish", BinMsg,  ?OK),
+    timer:sleep(250),
+    [RecvUtf8Msg1, RecvBinMsg1] = http_post(Config, "/queues/%2f/get_encoding_test/get",
+                                                          [{ackmode, ack_requeue_false},
+                                                           {count,    2},
+                                                           {encoding, auto}], ?OK),
+    %% Utf-8 payload must be returned as a utf-8 string when auto encoding is used.
+    ?assertEqual(<<"string">>, maps:get(payload_encoding, RecvUtf8Msg1)),
+    ?assertEqual(Utf8Text, maps:get(payload, RecvUtf8Msg1)),
+    %% Binary payload must be base64-encoded when auto is used.
+    ?assertEqual(<<"base64">>, maps:get(payload_encoding, RecvBinMsg1)),
+    ?assertEqual(BinPayload, maps:get(payload, RecvBinMsg1)),
+    %% Good. Now try forcing the base64 encoding.
+    http_post(Config, "/exchanges/%2f/amq.default/publish", Utf8Msg, ?OK),
+    http_post(Config, "/exchanges/%2f/amq.default/publish", BinMsg,  ?OK),
+    [RecvUtf8Msg2, RecvBinMsg2] = http_post(Config, "/queues/%2f/get_encoding_test/get",
+                                                          [{ackmode, ack_requeue_false},
+                                                           {count,    2},
+                                                           {encoding, base64}], ?OK),
+    %% All payloads must be base64-encoded when base64 encoding is used.
+    ?assertEqual(<<"base64">>, maps:get(payload_encoding, RecvUtf8Msg2)),
+    ?assertEqual(Utf8Payload, maps:get(payload, RecvUtf8Msg2)),
+    ?assertEqual(<<"base64">>, maps:get(payload_encoding, RecvBinMsg2)),
+    ?assertEqual(BinPayload, maps:get(payload, RecvBinMsg2)),
+    http_delete(Config, "/queues/%2f/get_encoding_test", {group, '2xx'}),
+    passed.
+
 get_fail_test(Config) ->
     http_put(Config, "/users/myuser", [{password, <<"password">>},
                                        {tags, <<"management">>}], {group, '2xx'}),