Parameter API renaming and policy tweaks

7 files changed, 143 insertions, 98 deletions

diff --git a/src/rabbit_control_main.erl b/src/rabbit_control_main.erl
index 2d99030b..d85418c5 100644
--- a/src/rabbit_control_main.erl
+++ b/src/rabbit_control_main.erl
@@ -445,26 +445,31 @@ action(set_parameter, Node, [Component, Key, Value], Opts, Inform) ->
     VHostArg = list_to_binary(proplists:get_value(?VHOST_OPT, Opts)),
     Inform("Setting runtime parameter ~p for component ~p to ~p",
            [Key, Component, Value]),
-    rpc_call(Node, rabbit_runtime_parameters, parse_set,
+    rpc_call(Node, rabbit_runtime_parameters, parse_set_param,
              [VHostArg, list_to_binary(Component), list_to_binary(Key), Value]);
 
 action(clear_parameter, Node, [Component, Key], Opts, Inform) ->
     VHostArg = list_to_binary(proplists:get_value(?VHOST_OPT, Opts)),
     Inform("Clearing runtime parameter ~p for component ~p", [Key, Component]),
-    rpc_call(Node, rabbit_runtime_parameters, clear, [VHostArg,
-                                                      list_to_binary(Component),
-                                                      list_to_binary(Key)]);
+    rpc_call(Node, rabbit_runtime_parameters, clear_param,
+             [VHostArg, list_to_binary(Component), list_to_binary(Key)]);
 
 action(list_parameters, Node, [], Opts, Inform) ->
     VHostArg = list_to_binary(proplists:get_value(?VHOST_OPT, Opts)),
     Inform("Listing runtime parameters", []),
     display_info_list(
-      rpc_call(Node, rabbit_runtime_parameters, list_formatted, [VHostArg]),
+      rpc_call(Node, rabbit_runtime_parameters, list_formatted_param,
+               [VHostArg]),
       rabbit_runtime_parameters:info_keys());
 
-action(set_policy, Node, [Key, Pattern, Defn | Priority], Opts, Inform) ->
+action(set_policy, Node, [Key, Pattern, Defn | Priority], Opts, Inform)
+  when Priority == [] orelse length(Priority) == 1 ->
+    Msg = "Setting policy ~p for pattern ~p to ~p",
+    InformMsg = case Priority of []  -> Msg;
+                                 [_] -> Msg ++ " with priority ~p"
+                end,
     VHostArg = list_to_binary(proplists:get_value(?VHOST_OPT, Opts)),
-    Inform("Setting policy ~p for pattern ~p to ~p", [Key, Pattern, Defn]),
+    Inform(InformMsg, [Key, Pattern, Defn] ++ Priority),
     rpc_call(Node, rabbit_runtime_parameters, parse_set_policy,
              [VHostArg, list_to_binary(Key), Pattern, Defn] ++ Priority);
 
@@ -478,8 +483,9 @@ action(list_policies, Node, [], Opts, Inform) ->
     VHostArg = list_to_binary(proplists:get_value(?VHOST_OPT, Opts)),
     Inform("Listing policies", []),
     display_info_list(
-      rpc_call(Node, rabbit_runtime_parameters, list_formatted_policies, [VHostArg]),
-      rabbit_runtime_parameters:info_keys_policies());
+      rpc_call(Node, rabbit_runtime_parameters, list_formatted_policies,
+               [VHostArg]),
+      rabbit_runtime_parameters:info_keys_policy());
 
 action(report, Node, _Args, _Opts, Inform) ->
     Inform("Reporting server status on ~p~n~n", [erlang:universaltime()]),
diff --git a/src/rabbit_mirror_queue_misc.erl b/src/rabbit_mirror_queue_misc.erl
index 201e1690..8da0e209 100644
--- a/src/rabbit_mirror_queue_misc.erl
+++ b/src/rabbit_mirror_queue_misc.erl
@@ -27,7 +27,7 @@
 -include("rabbit.hrl").
 
 -rabbit_boot_step({?MODULE,
-                   [{description, "HA policy validator hook"},
+                   [{description, "HA policy validation capability"},
                     {mfa, {rabbit_registry, register,
                            [policy_validator, <<"ha-mode">>, ?MODULE]}},
                     {mfa, {rabbit_registry, register,
diff --git a/src/rabbit_policy.erl b/src/rabbit_policy.erl
index e0bed10c..266c05a6 100644
--- a/src/rabbit_policy.erl
+++ b/src/rabbit_policy.erl
@@ -170,7 +170,7 @@ validation0(Validators, Terms) ->
          {ok, []} ->
              ok;
          {ok, Unvalidated} ->
-             {error, "~p are not recognised policy settings", Unvalidated};
+             {error, "~p are not recognised policy settings", [Unvalidated]};
          {Error, _} ->
              Error
     end.
diff --git a/src/rabbit_runtime_parameters.erl b/src/rabbit_runtime_parameters.erl
index ab3ab303..c2031759 100644
--- a/src/rabbit_runtime_parameters.erl
+++ b/src/rabbit_runtime_parameters.erl
@@ -18,13 +18,16 @@
 
 -include("rabbit.hrl").
 
--export([parse_set/4, parse_set_policy/4, parse_set_policy/5, set_policy/4,
-         set_policy/5,
-         clear/3, clear_policy/2,
-         list/0, list/1, list_strict/1, list/2, list_strict/2, list_policies/0,
-         list_policies/1, list_formatted/1, list_formatted_policies/1,
-         list_policies_raw/1,
-         lookup/3, value/3, value/4, info_keys/0, info_keys_policies/0]).
+
+-export([parse_set_param/4, set_param/4,
+         parse_set_policy/4, parse_set_policy/5, set_policy/4, set_policy/5,
+         clear_param/3, clear_policy/2,
+         list_param/0, list_param/1, list_param/2,
+         list_param_strict/1, list_param_strict/2,
+         list_policies/0, list_policies/1,
+         list_formatted_param/1, list_formatted_policies/1, list_policies_raw/1,
+         lookup_param/3, lookup_policy/2,
+         value/3, value/4, info_keys/0, info_keys_policy/0]).
 
 %%----------------------------------------------------------------------------
 
@@ -32,34 +35,42 @@
 
 -type(ok_or_error_string() :: 'ok' | {'error_string', string()}).
 
--spec(parse_set/4 :: (rabbit_types:vhost(), binary(), binary(), string())
-                     -> ok_or_error_string()).
+-spec(parse_set_param/4 :: (rabbit_types:vhost(), binary(), binary(), string())
+                            -> ok_or_error_string()).
+-spec(set_param/4 :: (rabbit_types:vhost(), binary(), binary(), term())
+                      -> ok_or_error_string()).
 -spec(parse_set_policy/4 :: (rabbit_types:vhost(), binary(), string(),
-                            string()) -> ok_or_error_string()).
+                             string()) -> ok_or_error_string()).
 -spec(parse_set_policy/5 :: (rabbit_types:vhost(), binary(), string(), string(),
-                            string()) -> ok_or_error_string()).
+                             string()) -> ok_or_error_string()).
 -spec(set_policy/4 :: (rabbit_types:vhost(), binary(), term(), term())
-                     -> ok_or_error_string()).
+                       -> ok_or_error_string()).
 -spec(set_policy/5 :: (rabbit_types:vhost(), binary(), term(), term(), term())
-                     -> ok_or_error_string()).
--spec(clear/3 :: (rabbit_types:vhost(), binary(), binary())
-                 -> ok_or_error_string()).
--spec(clear_policy/2 :: (rabbit_types:vhost(), binary())
+                       -> ok_or_error_string()).
+-spec(clear_param/3 :: (rabbit_types:vhost(), binary(), binary())
                         -> ok_or_error_string()).
--spec(list/0 :: () -> [rabbit_types:infos()]).
--spec(list/1 :: (rabbit_types:vhost()) -> [rabbit_types:infos()]).
--spec(list_strict/1 :: (binary()) -> [rabbit_types:infos()] | 'not_found').
--spec(list/2 :: (rabbit_types:vhost(), binary()) -> [rabbit_types:infos()]).
--spec(list_strict/2 :: (rabbit_types:vhost(), binary())
+-spec(clear_policy/2 :: (rabbit_types:vhost(), binary())
+                         -> ok_or_error_string()).
+-spec(list_param/0 :: () -> [rabbit_types:infos()]).
+-spec(list_param/1 :: (rabbit_types:vhost()) -> [rabbit_types:infos()]).
+-spec(list_param_strict/1 :: (binary())
+                              -> [rabbit_types:infos()] | 'not_found').
+-spec(list_param/2 :: (rabbit_types:vhost(), binary())
+                       -> [rabbit_types:infos()]).
+-spec(list_param_strict/2 :: (rabbit_types:vhost(), binary())
                        -> [rabbit_types:infos()] | 'not_found').
--spec(list_formatted/1 :: (rabbit_types:vhost()) -> [rabbit_types:infos()]).
+-spec(list_formatted_param/1 :: (rabbit_types:vhost())
+                                 -> [rabbit_types:infos()]).
 -spec(list_formatted_policies/1 :: (rabbit_types:vhost()) ->
                                     [rabbit_types:infos()]).
--spec(lookup/3 :: (rabbit_types:vhost(), binary(), binary())
-                  -> rabbit_types:infos()).
+-spec(lookup_param/3 :: (rabbit_types:vhost(), binary(), binary())
+                         -> rabbit_types:infos()).
+-spec(lookup_policy/2 :: (rabbit_types:vhost(), binary())
+                         -> rabbit_types:infos()).
 -spec(value/3 :: (rabbit_types:vhost(), binary(), binary()) -> term()).
 -spec(value/4 :: (rabbit_types:vhost(), binary(), binary(), term()) -> term()).
 -spec(info_keys/0 :: () -> rabbit_types:info_keys()).
+-spec(info_keys_policy/0 :: () -> rabbit_types:info_keys()).
 
 -endif.
 
@@ -73,19 +84,28 @@
 
 % used by rabbit_control_main
 
-parse_set(_, <<"policy">>, _, _) ->
+parse_set_param(_, <<"policy">>, _, _) ->
     {error_string, "policies may not be set using this method"};
-parse_set(VHost, Component, Key, String) ->
+parse_set_param(VHost, Component, Key, String) ->
     case rabbit_misc:json_decode(String) of
-        {ok, JSON} -> set(VHost, Component, Key, rabbit_misc:json_to_term(JSON));
+        {ok, JSON} -> set_param(VHost, Component, Key,
+                                rabbit_misc:json_to_term(JSON));
         error      -> {error_string, "JSON decoding error"}
     end.
-set(VHost, Component, Key, Term) when Component /= <<"policy">> ->
+
+% used by management plugin tests and rabbit_mgmt_wm_parameter
+
+set_param(_, <<"policy">>, _, _) ->
+    {error_string, "policies may not be set using this method"};
+set_param(VHost, Component, Key, Term) ->
     case set0(VHost, Component, Key, Term) of
         ok          -> ok;
         {errors, L} -> format_error(L)
     end.
 
+
+% used by rabbit_control_main
+
 parse_set_policy(VHost, Key, Pat, Defn) ->
     parse_set_policy0(VHost, Key, Pat, Defn, []).
 parse_set_policy(VHost, Key, Pat, Defn, Priority) ->
@@ -108,15 +128,16 @@ parse_set_policy0(VHost, Key, Pattern, Defn, Priority) ->
 
 % used by management plugin
 
-set_policy(VHost, Key, Pattern, Defn, Priority) ->
-    set_policy0(VHost, Key, pset(<<"pattern">>, Pattern,
-                                 pset(<<"policy">>, Defn,
-                                      [{<<"priority">>, Priority}]))).
 set_policy(VHost, Key, Pattern, Defn) ->
-    set_policy0(VHost, Key, pset(<<"pattern">>, Pattern,
-                                 [{<<"policy">>, Defn}])).
+    set_policy0(VHost, Key, policy_values(Pattern, Defn)).
+set_policy(VHost, Key, Pattern, Defn, Priority) ->
+    set_policy0(VHost, Key, [{<<"priority">>, Priority} |
+                             policy_values(Pattern, Defn)]).
 
-% common interface used by both parameters and policies
+policy_values(Pattern, Defn) ->
+    [{<<"pattern">>, Pattern}, {<<"policy">>, Defn}].
+
+% common, used by both parameters and policies
 
 set_policy0(VHost, Key, Term) ->
     case set0(VHost, <<"policy">>, Key, Term) of
@@ -154,9 +175,9 @@ mnesia_update(VHost, Component, Key, Term) ->
 
 %%---------------------------------------------------------------------------
 
-clear(_, <<"policy">> , _) ->
+clear_param(_, <<"policy">> , _) ->
     {error_string, "policies may not be cleared using this method"};
-clear(VHost, Component, Key) ->
+clear_param(VHost, Component, Key) ->
     case clear0(VHost, Component, Key) of
         ok          -> ok;
         {errors, L} -> format_error(L)
@@ -188,18 +209,20 @@ mnesia_clear(VHost, Component, Key) ->
 
 %%---------------------------------------------------------------------------
 
-list() ->
+% used by broker internally (rabbit_vhost)
+
+list_param() ->
     [p(P) || #runtime_parameters{ key = {_VHost, Comp, _Key}} = P <-
              rabbit_misc:dirty_read_all(?TABLE), Comp /= <<"policy">>].
 
-list(VHost)                   -> list(VHost, '_', []).
-list_strict(Component)        -> list('_',   Component, not_found).
-list(VHost, Component)        -> list(VHost, Component, []).
-list_strict(VHost, Component) -> list(VHost, Component, not_found).
+list_param(VHost)                   -> list_param(VHost, '_', []).
+list_param_strict(Component)        -> list_param('_',   Component, not_found).
+list_param(VHost, Component)        -> list_param(VHost, Component, []).
+list_param_strict(VHost, Component) -> list_param(VHost, Component, not_found).
 
-list(_VHost, <<"policy">>, _Default) ->
+list_param(_VHost, <<"policy">>, _Default) ->
     {error, "policies may not be listed using this method"};
-list(VHost, Component, Default) ->
+list_param(VHost, Component, Default) ->
     case component_good(Component) of
         true -> Match = #runtime_parameters{key = {VHost, Component, '_'},
                                             _ = '_'},
@@ -207,38 +230,29 @@ list(VHost, Component, Default) ->
         _    -> Default
     end.
 
+% used by management plugin REST interface
+
 list_policies() ->
     list_policies('_').
 
 list_policies(VHost) ->
-     list_as_proplist(list_policies0(VHost)).
-
-list_policies0(VHost) ->
-    Match = #runtime_parameters{key = {VHost, <<"policy">>, '_'}, _ = '_'},
-    [p(P) || P <- mnesia:dirty_match_object(?TABLE, Match)].
+    list_policies0(VHost, fun ident/1).
 
 % used by rabbit_control_main
 
-list_formatted(VHost) ->
-    [pset(value, format(pget(value, P)), P) || P <- list(VHost)].
+list_formatted_param(VHost) ->
+    [pset(value, format(pget(value, P)), P) || P <- list_param(VHost)].
 
 list_formatted_policies(VHost) ->
-    [pset(definition, format(pget(definition, Props)), Props) ||
-     Props <- list_as_proplist(list_policies0(VHost))].
-
-list_as_proplist(Source) ->
-    [begin
-       Key = pget(key,   P),
-       Val = pget(value, P),
-       [{vhost,      pget(vhost, P)},
-        {key,        Key},
-        {pattern,    pget(<<"pattern">>, Val)},
-        {definition, pget(<<"policy">>,   Val)}] ++
-        case pget(<<"priority">>, Val) of
-            undefined -> [];
-            Priority  -> [{priority, Priority}]
-        end
-     end || P <- Source].
+     order_policies(list_policies0(VHost, fun format/1)).
+
+list_policies0(VHost, DefnFun) ->
+    Match = #runtime_parameters{key = {VHost, <<"policy">>, '_'}, _ = '_'},
+    [pol(P, DefnFun) || P <- mnesia:dirty_match_object(?TABLE, Match)].
+
+order_policies(PropList) ->
+    lists:sort(fun (A, B) -> pget(priority, A, 0) < pget(priority, B, 0) end,
+               PropList).
 
 % used by rabbit_policy
 
@@ -248,12 +262,20 @@ list_policies_raw(VHost) ->
 
 %%---------------------------------------------------------------------------
 
-lookup(VHost, Component, Key) ->
+% used by management plugin (rabbit_mgmt_wm_policy and _parameter)
+
+lookup_param(VHost, Component, Key) ->
     case lookup0(VHost, Component, Key, rabbit_misc:const(not_found)) of
         not_found -> not_found;
         Params    -> p(Params)
     end.
 
+lookup_policy(VHost, Key) ->
+    case lookup0(VHost, <<"policy">>, Key, rabbit_misc:const(not_found)) of
+        not_found -> not_found;
+        Policy    -> pol(Policy, fun ident/1)
+    end.
+
 value(VHost, Component, Key) ->
     case lookup0(VHost, Component, Key, rabbit_misc:const(not_found)) of
         not_found -> not_found;
@@ -294,8 +316,19 @@ p(#runtime_parameters{key = {VHost, Component, Key}, value = Value}) ->
      {key,       Key},
      {value,     Value}].
 
-info_keys()          -> [component, key, value].
-info_keys_policies() -> [vhost, key, pattern, definition, priority].
+pol(#runtime_parameters{key = {VHost, <<"policy">>, Key}, value = Value},
+    DefnFun) ->
+    [{vhost,      VHost},
+     {key,        Key},
+     {pattern,    pget(<<"pattern">>, Value)},
+     {definition, DefnFun(pget(<<"policy">>,  Value))}] ++
+     case pget(<<"priority">>, Value) of
+         undefined -> [];
+         Priority  -> [{priority, Priority}]
+     end.
+
+info_keys()        -> [component, key, value].
+info_keys_policy() -> [vhost, key, pattern, definition, priority].
 
 %%---------------------------------------------------------------------------
 
@@ -320,8 +353,11 @@ format(Term) ->
     {ok, JSON} = rabbit_misc:json_encode(rabbit_misc:term_to_json(Term)),
     list_to_binary(JSON).
 
+ident(X) -> X.
+
 flatten_errors(L) ->
     case [{F, A} || I <- lists:flatten([L]), {error, F, A} <- [I]] of
         [] -> ok;
         E  -> {errors, E}
     end.
+
diff --git a/src/rabbit_runtime_parameters_test.erl b/src/rabbit_runtime_parameters_test.erl
index 44ae4d0f..807655a3 100644
--- a/src/rabbit_runtime_parameters_test.erl
+++ b/src/rabbit_runtime_parameters_test.erl
@@ -53,13 +53,13 @@ unregister_policy_validator() ->
     rabbit_registry:unregister(policy_validator, <<"testpos">>).
 
 validate_policy([{<<"testeven">>, Terms}]) when is_list(Terms) ->
-    case length(Terms) rem 2 =:= 0 of
+    case check_even(Terms) of
         true  -> ok;
         false -> {error, "meh", []}
     end;
 
 validate_policy([{<<"testpos">>, Terms}]) when is_list(Terms) ->
-    case lists:all(fun (N) -> is_integer(N) andalso N > 0 end, Terms) of
+    case check_pos(Terms) of
         true  -> ok;
         false -> {error, "meh", []}
     end;
@@ -67,15 +67,17 @@ validate_policy([{<<"testpos">>, Terms}]) when is_list(Terms) ->
 validate_policy([{Tag1, Arg1}, {Tag2, Arg2}])
   when is_list(Arg1), is_list(Arg2) ->
     case [Tag1, Tag2] -- [<<"testpos">>, <<"testeven">>] of
-        [] ->
-            case {lists:all(fun (N) -> is_integer(N) andalso N > 0 end,
-                            Arg1 ++ Arg2),
-                  length(Arg1) rem 2, length(Arg2) rem 2} of
-                {true, 0, 0} -> ok;
-                _            -> {error, "meh", []}
-            end;
-        _ -> {error, "meh", []}
+        [] -> case check_pos (Arg1) andalso check_pos (Arg2) andalso
+                   check_even(Arg1) andalso check_even(Arg2) of
+                  true -> ok;
+                  _    -> {error, "meh", []}
+              end;
+        _  -> {error, "meh", []}
    end;
 
 validate_policy(_) ->
     {error, "meh", []}.
+
+check_even(Terms) -> length(Terms) rem 2 =:= 0.
+check_pos(Terms)  -> lists:all(fun (N) -> is_integer(N) andalso
+                                          N > 0 end, Terms).
diff --git a/src/rabbit_tests.erl b/src/rabbit_tests.erl
index 9dca1a5a..a4cc89a0 100644
--- a/src/rabbit_tests.erl
+++ b/src/rabbit_tests.erl
@@ -1040,8 +1040,7 @@ test_policy_validation() ->
                          end, "", TagValList),
                 control_action(
                   set_policy,
-                  ["name", rabbit_misc:format("{\"pattern\":\".*\", \"policy\":"
-                                            "{~s}}", [string:join(Frag, ",")])])
+                  ["name", ".*", "{" ++ string:join(Frag, ",") ++ "}"])
         end,
 
     ok                 = SetPol([{"testeven", []}]),
@@ -1056,7 +1055,8 @@ test_policy_validation() ->
     {error_string, _}  = SetPol([{"testpos",  [2, 16, 32]}, {"testeven", [12, 24]}]),
     {error_string, _}  = SetPol([{"testpos",  [2, 16]},     {"testeven", [12, -2]}]),
     {error_string, _}  = SetPol([{"not_registered", []}]),
-    rabbit_runtime_parameters_test:unregister_policy_validator().
+    rabbit_runtime_parameters_test:unregister_policy_validator(),
+    passed.
 
 test_server_status() ->
     %% create a few things so there is some useful information to list
diff --git a/src/rabbit_vhost.erl b/src/rabbit_vhost.erl
index 03dfbe24..f29f0104 100644
--- a/src/rabbit_vhost.erl
+++ b/src/rabbit_vhost.erl
@@ -93,10 +93,11 @@ internal_delete(VHostPath) ->
     [ok = rabbit_auth_backend_internal:clear_permissions(
             proplists:get_value(user, Info), VHostPath)
      || Info <- rabbit_auth_backend_internal:list_vhost_permissions(VHostPath)],
-    [ok = rabbit_runtime_parameters:clear(VHostPath,
-                                          proplists:get_value(component, Info),
-                                          proplists:get_value(key, Info))
-     || Info <- rabbit_runtime_parameters:list(VHostPath)],
+    [ok = rabbit_runtime_parameters:clear_param(
+            VHostPath,
+            proplists:get_value(component, Info),
+            proplists:get_value(key, Info))
+     || Info <- rabbit_runtime_parameters:list_param(VHostPath)],
     ok = mnesia:delete({rabbit_vhost, VHostPath}),
     ok.