rabbit_vhost:set_tags/2 avoids notifying if tags are unchanged

Additionally, tags are now always sorted when set

Author: HoloRin

deps/rabbit/BUILD.bazel

@@ -1043,6 +1043,9 @@ rabbitmq_integration_suite(
     name = "vhost_SUITE",
     size = "medium",
     flaky = True,
+    additional_srcs = [
+        "test/test_rabbit_event_handler.erl",
+    ],
 )
 
 rabbitmq_suite(

deps/rabbit/src/rabbit_db_vhost.erl

@@ -132,7 +132,7 @@ merge_metadata_in_mnesia_tx(VHostName, Metadata) ->
 
 set_tags(VHostName, Tags)
   when is_binary(VHostName) andalso is_list(Tags) ->
-    ConvertedTags = [rabbit_data_coercion:to_atom(Tag) || Tag <- Tags],
+    ConvertedTags = lists:usort([rabbit_data_coercion:to_atom(Tag) || Tag <- Tags]),
     rabbit_db:run(
       #{mnesia => fun() -> set_tags_in_mnesia(VHostName, ConvertedTags) end}).
 

deps/rabbit/src/rabbit_vhost.erl

@@ -518,15 +518,36 @@ assert(VHostName) ->
         false -> throw({error, {no_such_vhost, VHostName}})
     end.
 
+are_different0([], []) ->
+    false;
+are_different0([], [_ | _]) ->
+    true;
+are_different0([_ | _], []) ->
+    true;
+are_different0([E], [E]) ->
+    false;
+are_different0([E | R1], [E | R2]) ->
+    are_different0(R1, R2);
+are_different0(_, _) ->
+    true.
+
+are_different(L1, L2) ->
+    are_different0(lists:usort(L1), lists:usort(L2)).
+
 -spec update_tags(vhost:name(), [vhost_tag()], rabbit_types:username()) -> vhost:vhost().
 update_tags(VHostName, Tags, ActingUser) ->
     try
+        CurrentTags = case rabbit_db_vhost:get(VHostName) of
+                          undefined -> [];
+                          V -> vhost:get_tags(V)
+                      end,
         VHost = rabbit_db_vhost:set_tags(VHostName, Tags),
         ConvertedTags = vhost:get_tags(VHost),
         rabbit_log:info("Successfully set tags for virtual host '~ts' to ~tp", [VHostName, ConvertedTags]),
-        rabbit_event:notify(vhost_tags_set, [{name, VHostName},
-                                             {tags, ConvertedTags},
-                                             {user_who_performed_action, ActingUser}]),
+        rabbit_event:notify_if(are_different(CurrentTags, ConvertedTags),
+                               vhost_tags_set, [{name, VHostName},
+                                                {tags, ConvertedTags},
+                                                {user_who_performed_action, ActingUser}]),
         VHost
     catch
         throw:{error, {no_such_vhost, _}} = Error ->

deps/rabbit/test/test_rabbit_event_handler.erl

@@ -0,0 +1,33 @@
+-module(test_rabbit_event_handler).
+
+-behaviour(gen_event).
+
+-export([okay/0]).
+-export([init/1, handle_call/2, handle_event/2, handle_info/2,
+    terminate/2, code_change/3]).
+
+-include_lib("rabbit_common/include/rabbit.hrl").
+
+% an exported callable func, used to allow rabbit_ct_broker_helpers
+% to load this code when rpc'd
+okay() -> ok.
+
+init([]) ->
+    {ok, #{events => []}}.
+
+handle_event(#event{} = Event, #{events := Events} = State) ->
+    {ok, State#{events => [Event | Events]}};
+handle_event(_, State) ->
+    {ok, State}.
+
+handle_call(events,  #{events := Events} = State) ->
+    {ok, Events, State}.
+
+handle_info(_Info, State) ->
+    {ok, State}.
+
+terminate(_Arg, _State) ->
+    ok.
+
+code_change(_OldVsn, State, _Extra) ->
+    {ok, State}.

deps/rabbit/test/vhost_SUITE.erl

@@ -28,6 +28,7 @@ groups() ->
         single_node_vhost_deletion_forces_connection_closure,
         vhost_failure_forces_connection_closure,
         vhost_creation_idempotency,
+        vhost_update_idempotency,
         parse_tags
     ],
     ClusterSize2Tests = [
@@ -321,6 +322,42 @@ vhost_creation_idempotency(Config) ->
         rabbit_ct_broker_helpers:delete_vhost(Config, VHost)
     end.
 
+vhost_update_idempotency(Config) ->
+    VHost = <<"update-idempotency-test">>,
+    ActingUser = <<"acting-user">>,
+    try
+        % load the dummy event handler on the node
+        ok = rabbit_ct_broker_helpers:rpc(Config, 0, test_rabbit_event_handler, okay, []),
+
+        ok = rabbit_ct_broker_helpers:rpc(Config, 0, gen_event, add_handler,
+                                          [rabbit_event, test_rabbit_event_handler, []]),
+
+        ?assertEqual(ok, rabbit_ct_broker_helpers:add_vhost(Config, VHost)),
+
+        ?assertMatch({vhost,VHost, [], #{tags := [private,replicate]}},
+                     rabbit_ct_broker_helpers:rpc(Config, 0,
+                                                  rabbit_vhost, update_tags,
+                                                  [VHost, [private, replicate], ActingUser])),
+        ?assertMatch({vhost,VHost, [], #{tags := [private,replicate]}},
+                     rabbit_ct_broker_helpers:rpc(Config, 0,
+                                                  rabbit_vhost, update_tags,
+                                                  [VHost, [replicate, private], ActingUser])),
+
+        Events = rabbit_ct_broker_helpers:rpc(Config, 0,
+                                              gen_event, call,
+                                              [rabbit_event, test_rabbit_event_handler, events, 100]),
+        ct:pal(?LOW_IMPORTANCE, "Events: ~p", [lists:reverse(Events)]),
+        TagsSetEvents = lists:filter(fun
+                                         (#event{type = vhost_tags_set}) -> true;
+                                         (_) -> false
+                                     end, Events),
+        ?assertMatch([#event{}], TagsSetEvents)
+    after
+        rabbit_ct_broker_helpers:rpc(Config, 0,
+                                     gen_event, delete_handler, [rabbit_event, test_rabbit_event_handler, []]),
+        rabbit_ct_broker_helpers:delete_vhost(Config, VHost)
+    end.
+
 vhost_is_created_with_default_limits(Config) ->
     VHost = <<"vhost1">>,
     Limits = [{<<"max-connections">>, 10}, {<<"max-queues">>, 1}],