Do not overwrite default_queue_type with undefined

deps/rabbit/src/rabbit_vhost.erl

@@ -21,8 +21,7 @@
 -export([dir/1, msg_store_dir_path/1, msg_store_dir_wildcard/0, msg_store_dir_base/0, config_file_path/1, ensure_config_file/1]).
 -export([delete_storage/1]).
 -export([vhost_down/1]).
--export([put_vhost/5,
-         put_vhost/6]).
+-export([put_vhost/6]).
 
 %%
 %% API
@@ -251,11 +250,10 @@ update_metadata(Name, Metadata0, ActingUser) ->
 
 -spec update(vhost:name(), binary(), [atom()], rabbit_queue_type:queue_type() | 'undefined', rabbit_types:username()) -> rabbit_types:ok_or_error(any()).
 update(Name, Description, Tags, DefaultQueueType, ActingUser) ->
-    Metadata = #{description => Description, tags => Tags, default_queue_type => DefaultQueueType},
+    Metadata = vhost:new_metadata(Description, Tags, DefaultQueueType),
     update_metadata(Name, Metadata, ActingUser).
 
 -spec delete(vhost:name(), rabbit_types:username()) -> rabbit_types:ok_or_error(any()).
-
 delete(VHost, ActingUser) ->
     %% FIXME: We are forced to delete the queues and exchanges outside
     %% the TX below. Queue deletion involves sending messages to the queue
@@ -296,19 +294,10 @@ delete(VHost, ActingUser) ->
     rabbit_vhost_sup_sup:delete_on_all_nodes(VHost),
     Ret.
 
--spec put_vhost(vhost:name(),
-    binary(),
-    vhost:tags(),
-    boolean(),
-    rabbit_types:username()) ->
-    'ok' | {'error', any()} | {'EXIT', any()}.
-put_vhost(Name, Description, Tags0, Trace, Username) ->
-    put_vhost(Name, Description, Tags0, undefined, Trace, Username).
-
 -spec put_vhost(vhost:name(),
     binary(),
     vhost:unparsed_tags() | vhost:tags(),
-    rabbit_queue_type:queue_type() | 'undefined',
+    rabbit_queue_type:queue_type() | 'undefined' | binary(),
     boolean(),
     rabbit_types:username()) ->
     'ok' | {'error', any()} | {'EXIT', any()}.
@@ -320,25 +309,14 @@ put_vhost(Name, Description, Tags0, DefaultQueueType0, Trace, Username) ->
       "null"      -> <<"">>;
       Other       -> Other
     end,
-    DefaultQueueType = case DefaultQueueType0 of
-        <<"undefined">> -> undefined;
-        _ -> DefaultQueueType0
-    end,
+    DefaultQueueType = rabbit_data_coercion:to_atom(DefaultQueueType0),
     ParsedTags = parse_tags(Tags),
     rabbit_log:debug("Parsed tags ~tp to ~tp", [Tags, ParsedTags]),
     Result = case exists(Name) of
                  true  ->
                      update(Name, Description, ParsedTags, DefaultQueueType, Username);
                  false ->
-                     Metadata0 = #{description => Description,
-                                   tags => ParsedTags},
-                     Metadata = case DefaultQueueType of
-                                    undefined ->
-                                        Metadata0;
-                                    _ ->
-                                        Metadata0#{default_queue_type =>
-                                                       DefaultQueueType}
-                                end,
+                     Metadata = vhost:new_metadata(Description, ParsedTags, DefaultQueueType),
                      case catch do_add(Name, Metadata, Username) of
                          ok ->
                              %% wait for up to 45 seconds for the vhost to initialise

deps/rabbit/src/vhost.erl

@@ -29,6 +29,7 @@
   set_limits/2,
   set_metadata/2,
   merge_metadata/2,
+  new_metadata/3,
   is_tagged_with/2
 ]).
 
@@ -163,10 +164,34 @@ set_metadata(VHost, Value) ->
     VHost#vhost{metadata = Value}.
 
 -spec merge_metadata(vhost(), metadata()) -> vhost().
-merge_metadata(VHost, Value) ->
-    Meta0 = get_metadata(VHost),
-    NewMeta = maps:merge(Meta0, Value),
-    VHost#vhost{metadata = NewMeta}.
+merge_metadata(VHost, NewVHostMeta) ->
+    CurrentVHostMeta = get_metadata(VHost),
+    FinalMeta =  maps:merge_with(
+                   fun metadata_merger/3, CurrentVHostMeta, NewVHostMeta),
+    VHost#vhost{metadata = FinalMeta}.
+
+%% This is the case where the existing VHost metadata has a default queue type
+%% value and the proposed value is `undefined`. We do not want the proposed
+%% value to overwrite the current value
+metadata_merger(default_queue_type, CurrentDefaultQueueType, undefined) ->
+    CurrentDefaultQueueType;
+%% This is the case where the existing VHost metadata has any default queue
+%% type value, and the proposed value is NOT `undefined`. It is OK for any
+%% proposed value to be used.
+metadata_merger(default_queue_type, _, NewVHostDefaultQueueType) ->
+    NewVHostDefaultQueueType;
+%% This is the case for all other VHost metadata keys.
+metadata_merger(_, _, NewMetadataValue) ->
+    NewMetadataValue.
+
+-spec new_metadata(binary(), [atom()], rabbit_queue_type:queue_type() | 'undefined') -> metadata().
+new_metadata(Description, Tags, undefined) ->
+    #{description => Description,
+      tags => Tags};
+new_metadata(Description, Tags, DefaultQueueType) ->
+    #{description => Description,
+      tags => Tags,
+      default_queue_type => DefaultQueueType}.
 
 -spec is_tagged_with(vhost(), tag()) -> boolean().
 is_tagged_with(VHost, Tag) ->

deps/rabbit/test/vhost_SUITE.erl

@@ -33,6 +33,7 @@ groups() ->
         vhost_failure_forces_connection_closure,
         vhost_creation_idempotency,
         vhost_update_idempotency,
+        vhost_update_default_queue_type_undefined,
         vhost_deletion,
         parse_tags
     ],
@@ -42,6 +43,8 @@ groups() ->
         vhost_failure_forces_connection_closure_on_failure_node,
         node_starts_with_dead_vhosts,
         vhost_creation_idempotency,
+        vhost_update_idempotency,
+        vhost_update_default_queue_type_undefined,
         vhost_deletion
     ],
     [
@@ -300,6 +303,30 @@ vhost_update_idempotency(Config) ->
         rabbit_ct_broker_helpers:delete_vhost(Config, VHost)
     end.
 
+vhost_update_default_queue_type_undefined(Config) ->
+    VHost = <<"update-default_queue_type-with-undefined-test">>,
+    Description = <<"rmqfpas-105 test vhost">>,
+    Tags = [replicate, private],
+    DefaultQueueType = quorum,
+    Trace = false,
+    ActingUser = <<"acting-user">>,
+    try
+        ?assertMatch(ok, rabbit_ct_broker_helpers:add_vhost(Config, VHost)),
+
+        PutVhostArgs0 = [VHost, Description, Tags, DefaultQueueType, Trace, ActingUser],
+        ?assertMatch(ok,
+                     rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_vhost, put_vhost, PutVhostArgs0)),
+
+        PutVhostArgs1 = [VHost, Description, Tags, undefined, Trace, ActingUser],
+        ?assertMatch(ok,
+                     rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_vhost, put_vhost, PutVhostArgs1)),
+
+        V = rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_vhost, lookup, [VHost]),
+        ?assertMatch(#{default_queue_type := DefaultQueueType}, vhost:get_metadata(V))
+    after
+        rabbit_ct_broker_helpers:delete_vhost(Config, VHost)
+    end.
+
 vhost_deletion(Config) ->
     VHost = <<"deletion-vhost">>,
     ActingUser = <<"acting-user">>,