Shovel management: tests and simplification (backport #12047)

deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl

@@ -107,8 +107,7 @@ vhost_definitions(ReqData, VHost, Context) ->
                             export_binding(B, QNames)],
     {ok, Vsn} = application:get_key(rabbit, vsn),
     Parameters = [strip_vhost(
-                    rabbit_mgmt_format:parameter(
-                      rabbit_mgmt_wm_parameters:fix_shovel_publish_properties(P)))
+                    rabbit_mgmt_format:parameter(P))
                   || P <- rabbit_runtime_parameters:list(VHost)],
     rabbit_mgmt_util:reply(
       [{rabbit_version, rabbit_data_coercion:to_binary(Vsn)}] ++

deps/rabbitmq_management/src/rabbit_mgmt_wm_parameter.erl

@@ -40,8 +40,7 @@ resource_exists(ReqData, Context) ->
      end, ReqData, Context}.
 
 to_json(ReqData, Context) ->
-    rabbit_mgmt_util:reply(rabbit_mgmt_format:parameter(
-        rabbit_mgmt_wm_parameters:fix_shovel_publish_properties(parameter(ReqData))),
+    rabbit_mgmt_util:reply(rabbit_mgmt_format:parameter(parameter(ReqData)),
                            ReqData, Context).
 
 accept_content(ReqData0, Context = #context{user = User}) ->

deps/rabbitmq_management/src/rabbit_mgmt_wm_parameters.erl

@@ -9,7 +9,6 @@
 
 -export([init/2, to_json/2, content_types_provided/2, is_authorized/2,
          resource_exists/2, basic/1]).
--export([fix_shovel_publish_properties/1]).
 -export([variances/2]).
 
 -include_lib("rabbitmq_management_agent/include/rabbit_mgmt_records.hrl").
@@ -40,24 +39,6 @@ is_authorized(ReqData, Context) ->
 
 %%--------------------------------------------------------------------
 
-%% Hackish fix to make sure we return a JSON object instead of an empty list
-%% when the publish-properties value is empty.
-fix_shovel_publish_properties(P) ->
-    case lists:keyfind(component, 1, P) of
-        {_, <<"shovel">>} ->
-            case lists:keytake(value, 1, P) of
-                {value, {_, Values}, P2} ->
-                    case lists:keytake(<<"publish-properties">>, 1, Values) of
-                        {_, {_, []}, Values2} ->
-                            P2 ++ [{value, Values2 ++ [{<<"publish-properties">>, empty_struct}]}];
-                        _ ->
-                            P
-                    end;
-                _ -> P
-            end;
-        _ -> P
-    end.
-
 basic(ReqData) ->
     Raw = case rabbit_mgmt_util:id(component, ReqData) of
               none -> rabbit_runtime_parameters:list();
@@ -71,5 +52,5 @@ basic(ReqData) ->
           end,
     case Raw of
         not_found -> not_found;
-        _         -> [rabbit_mgmt_format:parameter(fix_shovel_publish_properties(P)) || P <- Raw]
+        _         -> [rabbit_mgmt_format:parameter(P) || P <- Raw]
     end.

deps/rabbitmq_shovel_management/src/rabbit_shovel_mgmt_shovel.erl

@@ -66,8 +66,7 @@ resource_exists(ReqData, Context) ->
 
 to_json(ReqData, Context) ->
     Shovel = parameter(ReqData),
-    rabbit_mgmt_util:reply(rabbit_mgmt_format:parameter(
-        rabbit_mgmt_wm_parameters:fix_shovel_publish_properties(Shovel)),
+    rabbit_mgmt_util:reply(rabbit_mgmt_format:parameter(Shovel),
         ReqData, Context).
 
 is_authorized(ReqData, Context) ->

deps/rabbitmq_shovel_management/test/http_SUITE.erl

@@ -29,6 +29,9 @@ groups() ->
      {dynamic_shovels, [], [
                   start_and_list_a_dynamic_amqp10_shovel,
                   start_and_get_a_dynamic_amqp10_shovel,
+                  start_and_get_a_dynamic_amqp091_shovel_with_publish_properties,
+                  start_and_get_a_dynamic_amqp091_shovel_with_missing_publish_properties,
+                  start_and_get_a_dynamic_amqp091_shovel_with_empty_publish_properties,
                   create_and_delete_a_dynamic_shovel_that_successfully_connects,
                   create_and_delete_a_dynamic_shovel_that_fails_to_connect
                  ]},
@@ -127,7 +130,7 @@ start_inets(Config) ->
 
 start_and_list_a_dynamic_amqp10_shovel(Config) ->
     remove_all_dynamic_shovels(Config, <<"/">>),
-    Name = <<"dynamic-amqp10-await-startup-1">>,
+    Name = rabbit_data_coercion:to_binary(?FUNCTION_NAME),
     ID = {<<"/">>, Name},
     await_shovel_removed(Config, ID),
 
@@ -144,15 +147,15 @@ start_and_list_a_dynamic_amqp10_shovel(Config) ->
 
 start_and_get_a_dynamic_amqp10_shovel(Config) ->
     remove_all_dynamic_shovels(Config, <<"/">>),
-    Name = <<"dynamic-amqp10-get-shovel-1">>,
+    Name = rabbit_data_coercion:to_binary(?FUNCTION_NAME),
     ID = {<<"/">>, Name},
     await_shovel_removed(Config, ID),
 
     declare_shovel(Config, Name),
     await_shovel_startup(Config, ID),
     Sh = get_shovel(Config, Name),
     ?assertEqual(Name, maps:get(name, Sh)),
-    delete_shovel(Config, <<"dynamic-amqp10-await-startup-1">>),
+    delete_shovel(Config, Name),
 
     ok.
 
@@ -167,6 +170,48 @@ start_and_get_a_dynamic_amqp10_shovel(Config) ->
                            vhost := <<"v">>,
                            type  := <<"dynamic">>}).
 
+start_and_get_a_dynamic_amqp091_shovel_with_publish_properties(Config) ->
+    remove_all_dynamic_shovels(Config, <<"/">>),
+    Name = rabbit_data_coercion:to_binary(?FUNCTION_NAME),
+    ID = {<<"/">>, Name},
+    await_shovel_removed(Config, ID),
+
+    declare_amqp091_shovel_with_publish_properties(Config, Name),
+    await_shovel_startup(Config, ID),
+    Sh = get_shovel(Config, Name),
+    ?assertEqual(Name, maps:get(name, Sh)),
+    delete_shovel(Config, Name),
+
+    ok.
+
+start_and_get_a_dynamic_amqp091_shovel_with_missing_publish_properties(Config) ->
+    remove_all_dynamic_shovels(Config, <<"/">>),
+    Name = rabbit_data_coercion:to_binary(?FUNCTION_NAME),
+    ID = {<<"/">>, Name},
+    await_shovel_removed(Config, ID),
+
+    declare_amqp091_shovel(Config, Name),
+    await_shovel_startup(Config, ID),
+    Sh = get_shovel(Config, Name),
+    ?assertEqual(Name, maps:get(name, Sh)),
+    delete_shovel(Config, Name),
+
+    ok.
+
+start_and_get_a_dynamic_amqp091_shovel_with_empty_publish_properties(Config) ->
+    remove_all_dynamic_shovels(Config, <<"/">>),
+    Name = rabbit_data_coercion:to_binary(?FUNCTION_NAME),
+    ID = {<<"/">>, Name},
+    await_shovel_removed(Config, ID),
+
+    declare_amqp091_shovel_with_publish_properties(Config, Name, #{}),
+    await_shovel_startup(Config, ID),
+    Sh = get_shovel(Config, Name),
+    ?assertEqual(Name, maps:get(name, Sh)),
+    delete_shovel(Config, Name),
+
+    ok.
+
 start_static_shovels(Config) ->
     http_put(Config, "/users/admin",
 	     #{password => <<"admin">>, tags => <<"administrator">>}, ?CREATED),
@@ -366,7 +411,48 @@ declare_shovel(Config, Name) ->
                 'dest-address' => <<"test2">>,
                 'dest-properties' => #{},
                 'dest-application-properties' => #{},
-                'dest-message-annotations' => #{}}
+                'dest-message-annotations' => #{}
+            }
+        }, ?CREATED).
+
+declare_amqp091_shovel(Config, Name) ->
+    Port = integer_to_binary(
+        rabbit_ct_broker_helpers:get_node_config(Config, 0, tcp_port_amqp)),
+    http_put(Config, io_lib:format("/parameters/shovel/%2f/~ts", [Name]),
+        #{
+            value => #{
+                <<"src-protocol">> => <<"amqp091">>,
+                <<"src-uri">> => <<"amqp://localhost:", Port/binary>>,
+                <<"src-queue">>  => <<"amqp091.src.test">>,
+                <<"src-delete-after">> => <<"never">>,
+                <<"dest-protocol">> => <<"amqp091">>,
+                <<"dest-uri">> => <<"amqp://localhost:", Port/binary>>,
+                <<"dest-queue">> => <<"amqp091.dest.test">>
+            }
+        }, ?CREATED).
+
+declare_amqp091_shovel_with_publish_properties(Config, Name) ->
+    Props = #{
+        <<"delivery_mode">> => 2,
+        <<"app_id">> => <<"shovel_management:http_SUITE">>
+    },
+    declare_amqp091_shovel_with_publish_properties(Config, Name, Props).
+
+declare_amqp091_shovel_with_publish_properties(Config, Name, Props) ->
+    Port = integer_to_binary(
+        rabbit_ct_broker_helpers:get_node_config(Config, 0, tcp_port_amqp)),
+    http_put(Config, io_lib:format("/parameters/shovel/%2f/~ts", [Name]),
+        #{
+            value => #{
+                <<"src-protocol">> => <<"amqp091">>,
+                <<"src-uri">> => <<"amqp://localhost:", Port/binary>>,
+                <<"src-queue">>  => <<"amqp091.src.test">>,
+                <<"src-delete-after">> => <<"never">>,
+                <<"dest-protocol">> => <<"amqp091">>,
+                <<"dest-uri">> => <<"amqp://localhost:", Port/binary>>,
+                <<"dest-queue">> => <<"amqp091.dest.test">>,
+                <<"dest-publish-properties">> => Props
+            }
         }, ?CREATED).
 
 await_shovel_startup(Config, Name) ->