Normalize policy definitions before merging them

Closes rabbitmq/rabbitmq-management#751.

(cherry picked from commit 751ece5)

Conflicts:
	src/rabbit_policy.erl

Author: michaelklishin

src/rabbit_policy.erl

@@ -42,7 +42,7 @@
 
 -export([register/0]).
 -export([invalidate/0, recover/0]).
--export([name/1, name_op/1, effective_definition/1, get/2, get_arg/3, set/1]).
+-export([name/1, name_op/1, effective_definition/1, merge_operator_definitions/2, get/2, get_arg/3, set/1]).
 -export([validate/5, notify/5, notify_clear/4]).
 -export([parse_set/7, set/7, delete/3, lookup/2, list/0, list/1,
          list_formatted/1, list_formatted/3, info_keys/0]).
@@ -69,23 +69,23 @@ name0(undefined) -> none;
 name0(Policy)    -> pget(name, Policy).
 
 effective_definition(#amqqueue{policy = Policy, operator_policy = OpPolicy}) ->
-    effective_definition0(Policy, OpPolicy);
+    merge_operator_definitions(Policy, OpPolicy);
 effective_definition(#exchange{policy = Policy, operator_policy = OpPolicy}) ->
-    effective_definition0(Policy, OpPolicy).
-
-effective_definition0(undefined, undefined) -> undefined;
-effective_definition0(Policy, undefined)    -> pget(definition, Policy);
-effective_definition0(undefined, OpPolicy)  -> pget(definition, OpPolicy);
-effective_definition0(Policy, OpPolicy) ->
-    OpDefinition = pget(definition, OpPolicy, []),
-    Definition = pget(definition, Policy, []),
-    {Keys, _} = lists:unzip(Definition),
-    {OpKeys, _} = lists:unzip(OpDefinition),
+    merge_operator_definitions(Policy, OpPolicy).
+
+merge_operator_definitions(undefined, undefined) -> undefined;
+merge_operator_definitions(Policy, undefined)    -> pget(definition, Policy);
+merge_operator_definitions(undefined, OpPolicy)  -> pget(definition, OpPolicy);
+merge_operator_definitions(Policy, OpPolicy) ->
+    OpDefinition = rabbit_data_coercion:to_map(pget(definition, OpPolicy, [])),
+    Definition   = rabbit_data_coercion:to_map(pget(definition, Policy, [])),
+    Keys = maps:keys(Definition),
+    OpKeys = maps:keys(OpDefinition),
     lists:map(fun(Key) ->
-        case {pget(Key, Definition), pget(Key, OpDefinition)} of
-            {Val, undefined}       -> {Key, Val};
-            {undefined, Val}       -> {Key, Val};
-            {Val, OpVal}           -> {Key, merge_policy_value(Key, Val, OpVal)}
+        case {maps:get(Key, Definition, undefined), maps:get(Key, OpDefinition, undefined)} of
+            {Val, undefined}   -> {Key, Val};
+            {undefined, OpVal} -> {Key, OpVal};
+            {Val, OpVal}       -> {Key, merge_policy_value(Key, Val, OpVal)}
         end
     end,
     lists:umerge(Keys, OpKeys)).
@@ -128,7 +128,7 @@ get0(Name, Policy, OpPolicy) ->
 merge_policy_value(Name, PolicyVal, OpVal) ->
     case policy_merge_strategy(Name) of
         {ok, Module}       -> Module:merge_policy_value(Name, PolicyVal, OpVal);
-        {error, not_found} -> PolicyVal
+        {error, not_found} -> rabbit_policies:merge_policy_value(Name, PolicyVal, OpVal)
     end.
 
 policy_merge_strategy(Name) ->

test/unit_SUITE.erl

@@ -47,6 +47,7 @@ groups() ->
           rabbitmqctl_encode,
           pmerge,
           plmerge,
+          merge_operator_policy_definitions,
           priority_queue,
           rabbit_direct_extract_extra_auth_props,
           {resource_monitor, [parallel], [
@@ -807,6 +808,76 @@ plmerge(_Config) ->
     [{a, 1}, {b, 2}, {c, 3}, {d, 4}] = rabbit_misc:plmerge(P1, P2),
     passed.
 
+merge_operator_policy_definitions(_Config) ->
+    P1 = undefined,
+    P2 = [{definition, [{<<"message-ttl">>, 3000}]}],
+    ?assertEqual([{<<"message-ttl">>, 3000}], rabbit_policy:merge_operator_definitions(P1, P2)),
+    ?assertEqual([{<<"message-ttl">>, 3000}], rabbit_policy:merge_operator_definitions(P2, P1)),
+
+    ?assertEqual([{<<"message-ttl">>, 3000}], rabbit_policy:merge_operator_definitions(P1, rabbit_data_coercion:to_map(P2))),
+    ?assertEqual([{<<"message-ttl">>, 3000}], rabbit_policy:merge_operator_definitions(rabbit_data_coercion:to_map(P2), P1)),
+
+    ?assertEqual(undefined, rabbit_policy:merge_operator_definitions(undefined, undefined)),
+
+    ?assertEqual([], rabbit_policy:merge_operator_definitions([],  [])),
+    ?assertEqual([], rabbit_policy:merge_operator_definitions(#{}, [])),
+    ?assertEqual([], rabbit_policy:merge_operator_definitions(#{}, #{})),
+    ?assertEqual([], rabbit_policy:merge_operator_definitions([],  #{})),
+
+    %% operator policy takes precedence
+    ?assertEqual([{<<"message-ttl">>, 3000}], rabbit_policy:merge_operator_definitions(
+      [{definition, [
+        {<<"message-ttl">>, 5000}
+      ]}],
+      [{definition, [
+        {<<"message-ttl">>, 3000}
+      ]}]
+    )),
+
+    ?assertEqual([{<<"delivery-limit">>, 20},
+                  {<<"message-ttl">>, 3000}],
+                  rabbit_policy:merge_operator_definitions(
+                    [{definition, [
+                      {<<"message-ttl">>, 5000},
+                      {<<"delivery-limit">>, 20}
+                    ]}],
+                    [{definition, [
+                      {<<"message-ttl">>, 3000}
+                    ]}])
+    ),
+
+    ?assertEqual(
+                 [{<<"delivery-limit">>, 20},
+                  {<<"message-ttl">>,    3000},
+                  {<<"unknown">>,        <<"value">>}],
+
+                  rabbit_policy:merge_operator_definitions(
+                    #{definition => #{
+                      <<"message-ttl">> => 5000,
+                      <<"delivery-limit">> => 20
+                    }},
+                    #{definition => #{
+                      <<"message-ttl">> => 3000,
+                      <<"unknown">> => <<"value">>
+                    }})
+    ),
+
+    ?assertEqual(
+                 [{<<"delivery-limit">>, 20},
+                  {<<"message-ttl">>, 3000}],
+
+                  rabbit_policy:merge_operator_definitions(
+                    #{definition => #{
+                      <<"message-ttl">> => 5000,
+                      <<"delivery-limit">> => 20
+                    }},
+                    [{definition, [
+                      {<<"message-ttl">>, 3000}
+                    ]}])
+    ),
+
+    passed.
+
 table_codec(_Config) ->
     %% FIXME this does not test inexact numbers (double and float) yet,
     %% because they won't pass the equality assertions