Fix merge_defaults implementations

...and don't default inside run methods.

Defaults must go on left-hand-side of `Map.merge`, or they'll clobber
user-defined settings.

Remove some unnecessary pattern matching.

Issue #92

[#127359637]

deps/rabbitmq_cli/lib/rabbitmq/cli/ctl/commands/clear_parameter_command.ex

@@ -21,7 +21,7 @@ defmodule RabbitMQ.CLI.Ctl.Commands.ClearParameterCommand do
   def switches(), do: []
   def aliases(), do: []
   def merge_defaults(args, opts) do
-    default_opts = Map.merge(opts, %{vhost: "/"})
+    default_opts = Map.merge(%{vhost: "/"}, opts)
     {args, default_opts}
   end
 

deps/rabbitmq_cli/lib/rabbitmq/cli/ctl/commands/clear_permissions_command.ex

@@ -16,10 +16,13 @@
 
 defmodule RabbitMQ.CLI.Ctl.Commands.ClearPermissionsCommand do
   @behaviour RabbitMQ.CLI.CommandBehaviour
-  @default_vhost "/"
   @flags [:vhost]
 
-  def merge_defaults(args, opts), do: {args, opts}
+  def merge_defaults(args, opts) do
+    default_opts = Map.merge(%{vhost: "/"}, opts)
+    {args, default_opts}
+  end
+
   def validate([], _) do
     {:validation_failure, :not_enough_args}
   end
@@ -36,10 +39,6 @@ defmodule RabbitMQ.CLI.Ctl.Commands.ClearPermissionsCommand do
       :rabbit_auth_backend_internal, :clear_permissions, [username, vhost])
   end
 
-  def run([username], %{node: _} = opts) do
-    run([username], Map.merge(opts, %{vhost: @default_vhost}))
-  end
-
   def usage, do: "clear_permissions [-p vhost] <username>"
 
   def banner([username], %{vhost: vhost}), do: "Clearing permissions for user \"#{username}\" in vhost \"#{vhost}\" ..."

deps/rabbitmq_cli/lib/rabbitmq/cli/ctl/commands/list_consumers_command.ex

@@ -31,7 +31,7 @@ defmodule RabbitMQ.CLI.Ctl.Commands.ListConsumersCommand do
       end
   end
   def merge_defaults([], opts) do
-    {Enum.map(@info_keys, &Atom.to_string/1), opts}
+    {Enum.map(@info_keys, &Atom.to_string/1), Map.merge(%{vhost: "/"}, opts)}
   end
   def merge_defaults(args, opts), do: {args, opts}
 
@@ -57,13 +57,6 @@ defmodule RabbitMQ.CLI.Ctl.Commands.ListConsumersCommand do
       RpcStream.receive_list_items(node_name, :rabbit_amqqueue, :emit_consumers_all,
         [nodes, vhost], timeout, info_keys)
   end
-  def run(args, %{node: _node_name, timeout: _timeout} = opts) do
-      run(args, Map.merge(default_opts, opts))
-  end
-
-  defp default_opts() do
-      %{vhost: "/"}
-  end
 
   def banner(_, %{vhost: vhost}), do: "Listing consumers on vhost #{vhost} ..."
 end

deps/rabbitmq_cli/lib/rabbitmq/cli/ctl/commands/list_exchanges_command.ex

@@ -28,10 +28,14 @@ defmodule RabbitMQ.CLI.Ctl.Commands.ListExchangesCommand do
         err -> err
       end
   end
+
   def merge_defaults([], opts) do
-    {~w(name type), Map.merge(opts, default_opts())}
+    merge_defaults(~w(name type), opts)
+  end
+
+  def merge_defaults(args, opts) do
+    {args, Map.merge(%{vhost: "/"}, opts)}
   end
-  def merge_defaults(args, opts), do: {args, opts}
 
   def switches(), do: []
   def aliases(), do: []
@@ -57,9 +61,5 @@ defmodule RabbitMQ.CLI.Ctl.Commands.ListExchangesCommand do
         info_keys)
   end
 
-  defp default_opts() do
-      %{vhost: "/"}
-  end
-
   def banner(_,_), do: "Listing exchanges ..."
 end

deps/rabbitmq_cli/lib/rabbitmq/cli/ctl/commands/list_parameters_command.ex

@@ -18,7 +18,7 @@ defmodule RabbitMQ.CLI.Ctl.Commands.ListParametersCommand do
   @behaviour RabbitMQ.CLI.CommandBehaviour
   @flags [:vhost]
   def merge_defaults([], opts) do
-    {[], Map.merge(opts, %{vhost: "/"})}
+    {[], Map.merge(%{vhost: "/"}, opts)}
   end
 
   def switches(), do: []

deps/rabbitmq_cli/lib/rabbitmq/cli/ctl/commands/list_permissions_command.ex

@@ -19,7 +19,7 @@ defmodule RabbitMQ.CLI.Ctl.Commands.ListPermissionsCommand do
   @flags [:vhost]
 
   def merge_defaults(args, opts) do
-    {args, Map.merge(opts, %{vhost: "/"})}
+    {args, Map.merge(%{vhost: "/"}, opts)}
   end
 
   def switches(), do: []

deps/rabbitmq_cli/lib/rabbitmq/cli/ctl/commands/purge_queue_command.ex

@@ -42,7 +42,7 @@ defmodule RabbitMQ.CLI.Ctl.Commands.PurgeQueueCommand do
   end
 
   def merge_defaults(args, opts) do
-    {args, Map.merge(opts, %{vhost: "/"})}
+    {args, Map.merge(%{vhost: "/"}, opts)}
   end
 
   def validate(args, _) when length(args) > 1 do

deps/rabbitmq_cli/lib/rabbitmq/cli/ctl/commands/set_permissions_command.ex

@@ -17,7 +17,10 @@
 defmodule RabbitMQ.CLI.Ctl.Commands.SetPermissionsCommand do
   @behaviour RabbitMQ.CLI.CommandBehaviour
   @flags [:vhost]
-  def merge_defaults(args, opts), do: {args, opts}
+
+  def merge_defaults(args, opts) do
+    {args, Map.merge(%{vhost: "/"}, opts)}
+  end
 
   def switches(), do: []
   def aliases(), do: []
@@ -42,11 +45,6 @@ defmodule RabbitMQ.CLI.Ctl.Commands.SetPermissionsCommand do
     )
   end
 
-  def run([_, _, _, _] = args, %{node: _} = opts) do
-    default_opts = Map.merge(opts, %{vhost: "/"})
-    run(args, default_opts)
-  end
-
   def usage, do: "set_permissions [-p <vhost>] <user> <conf> <write> <read>"
 
   def flags, do: @flags

deps/rabbitmq_cli/lib/rabbitmq/cli/ctl/commands/trace_off_command.ex

@@ -15,9 +15,7 @@
 
 
 defmodule RabbitMQ.CLI.Ctl.Commands.TraceOffCommand do
-
   @behaviour RabbitMQ.CLI.CommandBehaviour
-  @default_vhost "/"
   @flags [:vhost]
 
   def validate([_|_], _), do: {:validation_failure, :too_many_args}
@@ -25,7 +23,7 @@ defmodule RabbitMQ.CLI.Ctl.Commands.TraceOffCommand do
   def switches(), do: []
   def aliases(), do: []
   def merge_defaults(_, opts) do
-    {[], Map.merge(opts, %{vhost: @default_vhost})}
+    {[], Map.merge(%{vhost: "/"}, opts)}
   end
 
   def run([], %{node: node_name, vhost: vhost}) do

deps/rabbitmq_cli/lib/rabbitmq/cli/ctl/commands/trace_on_command.ex

@@ -16,15 +16,13 @@
 
 defmodule RabbitMQ.CLI.Ctl.Commands.TraceOnCommand do
   @behaviour RabbitMQ.CLI.CommandBehaviour
-  @default_vhost "/"
   @flags [:vhost]
 
   def validate([_|_], _), do: {:validation_failure, :too_many_args}
   def validate(_, _), do: :ok
-  def merge_defaults([], %{node: _} = opts) do
+  def merge_defaults(_, opts) do
-    {[], Map.merge(opts, %{vhost: @default_vhost})}
+    {[], Map.merge(%{vhost: "/"}, opts)}
   end
-  def merge_defaults(args, opts), do: {args, opts}
   def switches(), do: []
   def aliases(), do: []
 

deps/rabbitmq_cli/test/clear_parameter_command_test.exs

@@ -58,6 +58,11 @@ defmodule ClearParameterCommandTest do
     assert @command.merge_defaults([], %{}) == {[], %{vhost: "/"}}
   end
 
+  test "merge_defaults: defaults can be overridden" do
+    assert @command.merge_defaults([], %{}) == {[], %{vhost: "/"}}
+    assert @command.merge_defaults([], %{vhost: "non_default"}) == {[], %{vhost: "non_default"}}
+  end
+
   test "validate: argument validation" do
     assert @command.validate(["one", "two"], %{}) == :ok 
     assert @command.validate([], %{}) == {:validation_failure, :not_enough_args}

deps/rabbitmq_cli/test/clear_permissions_command_test.exs

@@ -43,11 +43,15 @@ defmodule ClearPermissionsTest do
 
     {
       :ok,
-      opts: %{node: get_rabbit_hostname},
+      opts: %{node: get_rabbit_hostname, vhost: context[:vhost]}
-      vhost_options: %{node: get_rabbit_hostname, vhost: context[:vhost]}
     }
   end
 
+  test "merge_defaults: defaults can be overridden" do
+    assert @command.merge_defaults([], %{}) == {[], %{vhost: "/"}}
+    assert @command.merge_defaults([], %{vhost: "non_default"}) == {[], %{vhost: "non_default"}}
+  end
+
   test "validate: argument count validates" do
     assert @command.validate(["one"], %{}) == :ok
     assert @command.validate([], %{}) == {:validation_failure, :not_enough_args}
@@ -59,7 +63,7 @@ defmodule ClearPermissionsTest do
     assert @command.run([context[:user]], context[:opts]) == {:error, {:no_such_user, context[:user]}}
   end
 
-  @tag user: @user
+  @tag user: @user, vhost: @default_vhost
   test "run: a valid username clears permissions", context do
     assert @command.run([context[:user]], context[:opts]) == :ok
 
@@ -70,14 +74,14 @@ defmodule ClearPermissionsTest do
   test "run: on an invalid node, return a badrpc message" do
     bad_node = :jake@thedog
     arg = ["some_name"]
-    opts = %{node: bad_node}
+    opts = %{node: bad_node, vhost: ""}
 
     assert @command.run(arg, opts) == {:badrpc, :nodedown}
   end
 
   @tag user: @user, vhost: @specific_vhost
   test "run: on a valid specified vhost, clear permissions", context do
-    assert @command.run([context[:user]], context[:vhost_options]) == :ok
+    assert @command.run([context[:user]], context[:opts]) == :ok
 
     assert list_permissions(context[:vhost])
     |> Enum.filter(fn(record) -> record[:user] == context[:user] end) == []
@@ -85,12 +89,12 @@ defmodule ClearPermissionsTest do
 
   @tag user: @user, vhost: "bad_vhost"
   test "run: on an invalid vhost, return no_such_vhost error", context do
-    assert @command.run([context[:user]], context[:vhost_options]) == {:error, {:no_such_vhost, context[:vhost]}}
+    assert @command.run([context[:user]], context[:opts]) == {:error, {:no_such_vhost, context[:vhost]}}
   end
 
   @tag user: @user, vhost: @specific_vhost
   test "banner", context do
-    s = @command.banner([context[:user]], context[:vhost_options])
+    s = @command.banner([context[:user]], context[:opts])
 
     assert s =~ ~r/Clearing permissions/
     assert s =~ ~r/\"#{context[:user]}\"/

deps/rabbitmq_cli/test/list_consumers_command_test.exs

@@ -7,6 +7,7 @@ defmodule ListConsumersCommandTest do
   @vhost "test1"
   @user "guest"
   @default_timeout :infinity
+  @info_keys ~w(queue_name channel_pid consumer_tag ack_required prefetch_count arguments)
 
   setup_all do
     RabbitMQ.CLI.Distribution.start()
@@ -36,9 +37,9 @@ defmodule ListConsumersCommandTest do
     }
   end
 
-  test "merge_defaults: all keys merged in if none specificed" do
+  test "merge_defaults: defaults can be overridden" do
-    assert @command.merge_defaults([], %{})
+    assert @command.merge_defaults([], %{}) == {@info_keys, %{vhost: "/"}}
-      == {~w(queue_name channel_pid consumer_tag ack_required prefetch_count arguments), %{}}
+    assert @command.merge_defaults([], %{vhost: "non_default"}) == {@info_keys, %{vhost: "non_default"}}
   end
 
   test "validate: returns bad_info_key on a single bad arg", context do

deps/rabbitmq_cli/test/list_exchanges_command_test.exs

@@ -55,6 +55,11 @@ defmodule ListExchangesCommandTest do
       == {["name", "type"], %{vhost: "/"}}
   end
 
+  test "merge_defaults: defaults can be overridden" do
+    assert @command.merge_defaults([], %{}) == {["name", "type"], %{vhost: "/"}}
+    assert @command.merge_defaults([], %{vhost: "non_default"}) == {["name", "type"], %{vhost: "non_default"}}
+  end
+
   test "validate: returns bad_info_key on a single bad arg", context do
     assert @command.validate(["quack"], context[:opts]) ==
       {:validation_failure, {:bad_info_key, [:quack]}}

deps/rabbitmq_cli/test/list_parameters_command_test.exs

@@ -76,6 +76,11 @@ defmodule ListParametersCommandTest do
     }
   end
 
+  test "merge_defaults: defaults can be overridden" do
+    assert @command.merge_defaults([], %{}) == {[], %{vhost: "/"}}
+    assert @command.merge_defaults([], %{vhost: "non_default"}) == {[], %{vhost: "non_default"}}
+  end
+
   test "validate: wrong number of arguments leads to an arg count error" do
     assert @command.validate(["this", "is", "too", "many"], %{}) == {:validation_failure, :too_many_args}
   end

deps/rabbitmq_cli/test/list_permissions_command_test.exs

@@ -56,6 +56,11 @@ defmodule ListPermissionsCommandTest do
     assert {[], %{vhost: "/"}} == @command.merge_defaults([], %{})
   end
 
+  test "merge_defaults: defaults can be overridden" do
+    assert @command.merge_defaults([], %{}) == {[], %{vhost: "/"}}
+    assert @command.merge_defaults([], %{vhost: "non_default"}) == {[], %{vhost: "non_default"}}
+  end
+
   test "validate: invalid parameters yield an arg count error" do
     assert @command.validate(["extra"], %{}) == {:validation_failure, :too_many_args}
   end

deps/rabbitmq_cli/test/purge_queue_command_test.exs

@@ -42,6 +42,11 @@ defmodule PurgeQueueCommandTest do
       }}
   end
 
+  test "merge_defaults: defaults can be overridden" do
+    assert @command.merge_defaults([], %{}) == {[], %{vhost: "/"}}
+    assert @command.merge_defaults([], %{vhost: "non_default"}) == {[], %{vhost: "non_default"}}
+  end
+
   @tag test_timeout: 15
   test "request to an existent queue on active node succeeds", context do
     add_vhost @vhost

deps/rabbitmq_cli/test/set_permissions_command_test.exs

@@ -48,11 +48,17 @@ defmodule SetPermissionsCommandTest do
     {
       :ok,
       opts: %{
-        node: get_rabbit_hostname
+        node: get_rabbit_hostname,
+        vhost: context[:vhost]
       }
     }
   end
 
+  test "merge_defaults: defaults can be overridden" do
+    assert @command.merge_defaults([], %{}) == {[], %{vhost: "/"}}
+    assert @command.merge_defaults([], %{vhost: "non_default"}) == {[], %{vhost: "non_default"}}
+  end
+
   test "validate: wrong number of arguments leads to an arg count error" do
     assert @command.validate([], %{}) == {:validation_failure, :not_enough_args}
     assert @command.validate(["insufficient"], %{}) == {:validation_failure, :not_enough_args}
@@ -76,21 +82,11 @@ defmodule SetPermissionsCommandTest do
   test "run: An invalid rabbitmq node throws a badrpc" do
     target = :jake@thedog
     :net_kernel.connect_node(target)
-    opts = %{node: target}
+    opts = %{node: target, vhost: @vhost}
 
     assert @command.run([@user, ".*", ".*", ".*"], opts) == {:badrpc, :nodedown}
   end
 
-  @tag user: @user, vhost: @root
-  test "run: a well-formed command with no vhost runs against the default", context do
-    assert @command.run(
-      [context[:user], "^#{context[:user]}-.*", ".*", ".*"],
-      context[:opts]
-    ) == :ok
-
-    assert List.first(list_permissions(context[:vhost]))[:configure] == "^#{context[:user]}-.*"
-  end
-
   @tag user: "interloper", vhost: @root
   test "run: an invalid user returns a no-such-user error", context do
     assert @command.run(

deps/rabbitmq_cli/test/trace_off_command_test.exs

@@ -43,6 +43,11 @@ defmodule TraceOffCommandTest do
     {:ok, opts: %{node: get_rabbit_hostname, vhost: context[:vhost]}}
   end
 
+  test "merge_defaults: defaults can be overridden" do
+    assert @command.merge_defaults([], %{}) == {[], %{vhost: "/"}}
+    assert @command.merge_defaults([], %{vhost: "non_default"}) == {[], %{vhost: "non_default"}}
+  end
+
   test "validate: wrong number of arguments triggers arg count error" do
     assert @command.validate(["extra"], %{}) == {:validation_failure, :too_many_args}
   end

deps/rabbitmq_cli/test/trace_on_command_test.exs

@@ -51,6 +51,11 @@ defmodule TraceOnCommandTest do
     trace_off(@default_vhost)
   end
 
+  test "merge_defaults: defaults can be overridden" do
+    assert @command.merge_defaults([], %{}) == {[], %{vhost: "/"}}
+    assert @command.merge_defaults([], %{vhost: "non_default"}) == {[], %{vhost: "non_default"}}
+  end
+
   test "validate: wrong number of arguments triggers arg count error" do
     assert @command.validate(["extra"], %{}) == {:validation_failure, :too_many_args}
   end