(#8561, #7290) Implement the option contract fully.

Rewrite the process of validating and updating the options to fully reflect
the contract - we fail if there are unknown options passed, report nicely
errors of duplicate names, pass only the canonical names to the action code,
and generally enforce nicely.

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

2 files changed, 55 insertions, 40 deletions

diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index 2a2f48e03..bd47a36ea 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -196,14 +196,12 @@ class Puppet::Interface::Action
     wrapper = <<WRAPPER
 def #{@name}(#{decl.join(", ")})
   #{optn}
-  args = #{args}
-  options = args.last
-
-  action = get_action(#{name.inspect})
-  args   = action.validate_and_clean(args)
-  __invoke_decorations(:before, action, args, options)
+  args    = #{args}
+  action  = get_action(#{name.inspect})
+  args   << action.validate_and_clean(args.pop)
+  __invoke_decorations(:before, action, args, args.last)
   rval = self.__send__(#{internal_name.inspect}, *args)
-  __invoke_decorations(:after, action, args, options)
+  __invoke_decorations(:after, action, args, args.last)
   return rval
 end
 WRAPPER
@@ -253,43 +251,59 @@ WRAPPER
     option
   end
 
-  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
-
-    # 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
-      # probably fail, but that wasn't our API, and I don't want to perturb
-      # behaviour this late in the RC cycle. --daniel 2011-04-29
+  def validate_and_clean(original)
+    # The final set of arguments; effectively a hand-rolled shallow copy of
+    # the original, which protects the caller from the surprises they might
+    # get if they passed us a hash and we mutated it...
+    result = {}
+
+    # Check for multiple aliases for the same option, and canonicalize the
+    # name of the argument while we are about it.
+    overlap = Hash.new do |h, k| h[k] = [] end
+    unknown = []
+    original.keys.each do |name|
       if option = get_option(name) then
-        overlap = (option.aliases & args.last.keys)
-        unless overlap.length == 1 then
-          raise ArgumentError, "Multiple aliases for the same option passed: #{overlap.join(', ')}"
+        canonical = option.name
+        if result.has_key? canonical
+          overlap[canonical] << name
+        else
+          result[canonical] = original[name]
         end
+      else
+        unknown << name
       end
     end
 
-    # Check for missing mandatory options.
-    required = options.map do |name|
-      get_option(name)
-    end.select(&:required?).collect(&:name) - args.last.keys
+    unless overlap.empty?
+      msg = overlap.map {|k, v| "(#{k}, #{v.sort.join(', ')})" }.join(", ")
+      raise ArgumentError, "Multiple aliases for the same option passed: #{msg}"
+    end
+
+    unless unknown.empty?
+      msg = unknown.sort.join(", ")
+      raise ArgumentError, "Unknown options passed: #{msg}"
+    end
+
+    # Inject default arguments and check for missing mandating options.
+    missing = []
+    options.map {|x| get_option(x) }.each do |option|
+      name = option.name
+      next if result.has_key? name
+
+      if option.has_default?
+        result[name] = option.default
+      elsif option.required?
+        missing << name
+      end
+    end
 
-    unless required.empty?
-      raise ArgumentError, "The following options are required: #{required.join(', ')}"
+    unless missing.empty?
+      msg = missing.sort.join(', ')
+      raise ArgumentError, "The following options are required: #{msg}"
     end
 
     # All done.
-    return args
+    return result
   end
 
   ########################################################################
diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb
index c1491488e..c3f08e817 100755
--- a/spec/unit/interface/action_spec.rb
+++ b/spec/unit/interface/action_spec.rb
@@ -121,6 +121,7 @@ describe Puppet::Interface::Action do
       let :face do
         Puppet::Interface.new(:ruby_api, '1.0.0') do
           action :bar do
+            option "--bar"
             when_invoked do |*args|
               args.last
             end
@@ -139,7 +140,7 @@ describe Puppet::Interface::Action do
       end
 
       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.get_action(:bar).expects(:validate_and_clean).with({}).returns({})
         face.bar 1, :two, 'three'
       end
     end
@@ -450,12 +451,12 @@ describe Puppet::Interface::Action do
         end
 
         it "should be invoked when calling a child action" do
-          subject.on_child(:foo => true, :bar => true).should == :on_child
+          subject.on_child(:foo => true).should == :on_child
           subject.reported.should == [ :child_before ]
         end
 
         it "should be invoked when calling a parent action" do
-          subject.on_parent(:foo => true, :bar => true).should == :on_parent
+          subject.on_parent(:foo => true).should == :on_parent
           subject.reported.should == [ :child_before ]
         end
       end
@@ -467,12 +468,12 @@ describe Puppet::Interface::Action do
         end
 
         it "should be invoked when calling a child action" do
-          subject.on_child(:foo => true, :bar => true).should == :on_child
+          subject.on_child(:foo => true).should == :on_child
           subject.reported.should == [ :parent_before ]
         end
 
         it "should be invoked when calling a parent action" do
-          subject.on_parent(:foo => true, :bar => true).should == :on_parent
+          subject.on_parent(:foo => true).should == :on_parent
           subject.reported.should == [ :parent_before ]
         end
       end