(#8561) Unify validation and modification of action arguments.

Rather than having multiple, separate operations that modify and validate the
arguments to an action, a single pass makes sense.  This also means less walks
across the set of data, and a few less expensive method calls in Ruby.

Additionally, we work on a duplicate of the arguments hash rather than
directly modifying the original.  Because everything we do is at the top level
key/value mapping, this is sufficient to isolate the original.

While mostly theoretical, we now don't mutilate the hash passed in, so the
user won't get nastily surprised by the fact that we could have done so.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>

2 files changed, 17 insertions, 10 deletions

diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index ce9c60b49..2a2f48e03 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -200,8 +200,7 @@ def #{@name}(#{decl.join(", ")})
   options = args.last
 
   action = get_action(#{name.inspect})
-  action.add_default_args(args)
-  action.validate_args(args)
+  args   = action.validate_and_clean(args)
   __invoke_decorations(:before, action, args, options)
   rval = self.__send__(#{internal_name.inspect}, *args)
   __invoke_decorations(:after, action, args, options)
@@ -254,15 +253,19 @@ WRAPPER
     option
   end
 
-  def add_default_args(args)
+  def validate_and_clean(original_args)
+    # Make a shallow copy, so as not to surprise callers.  We only modify the
+    # top level keys and all, so this should be sufficient without being too
+    # costly overall.
+    args = original_args.dup
+
+    # Inject default arguments.
     options.map {|x| get_option(x) }.each do |option|
       if option.has_default? and not option.aliases.any? {|x| args.last.has_key? x}
         args.last[option.name] = option.default
       end
     end
-  end
 
-  def validate_args(args)
     # Check for multiple aliases for the same option...
     args.last.keys.each do |name|
       # #7290: If this isn't actually an option, ignore it for now.  We should
@@ -281,8 +284,12 @@ WRAPPER
       get_option(name)
     end.select(&:required?).collect(&:name) - args.last.keys
 
-    return if required.empty?
-    raise ArgumentError, "The following options are required: #{required.join(', ')}"
+    unless required.empty?
+      raise ArgumentError, "The following options are required: #{required.join(', ')}"
+    end
+
+    # All done.
+    return args
   end
 
   ########################################################################
diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb
index 66fff2571..c1491488e 100755
--- a/spec/unit/interface/action_spec.rb
+++ b/spec/unit/interface/action_spec.rb
@@ -138,8 +138,8 @@ describe Puppet::Interface::Action do
         options.should == { :bar => "beer" }
       end
 
-      it "should call #validate_args on the action when invoked" do
-        face.get_action(:bar).expects(:validate_args).with([1, :two, 'three', {}])
+      it "should call #validate_and_clean on the action when invoked" do
+        face.get_action(:bar).expects(:validate_and_clean).with([1, :two, 'three', {}])
         face.bar 1, :two, 'three'
       end
     end
@@ -548,7 +548,7 @@ describe Puppet::Interface::Action do
     it "should return the block if asked"
   end
 
-  context "#validate_args" do
+  context "#validate_and_clean" do
     subject do
       Puppet::Interface.new(:validate_args, '1.0.0') do
         script :test do |options| true end