(#7122) Enforce call arity on actions in the CLI wrapper.

We had a problem, previously, in the generic translation of command line
arguments to Ruby method calls: we could mistake the options, added by the CLI
wrapper, for a positional argument to the action method.

This was caused by a combination of factors, but primarily that the wrapper
methods for actions are designed to present a friendly, helpful Ruby API for
internal use.  Consequently, they have a default value if you don't wish to
pass options.

Unfortunately, this meant that the options that the CLI *always* passed could
be treated as a positional argument instead, and the default set of options
added to the back of the call.

To resolve this we now check the number of positional arguments in the CLI
wrapper, and raise an exception if they are mismatched.  This makes the
generic CLI handling do the right thing in adapting the command line
arguments to the Ruby API.

(As an aside, we would have had a similar-but-different failure mode if we
 type-checked positional arguments: these calls would have failed with an
 invalid argument validation error.)

Reviewed-By: Max Martin <max@puppetlabs.com>

7 files changed, 68 insertions, 19 deletions

diff --git a/lib/puppet/application/face_base.rb b/lib/puppet/application/face_base.rb
index d02769412..69c3ad5ad 100644
--- a/lib/puppet/application/face_base.rb
+++ b/lib/puppet/application/face_base.rb
@@ -214,12 +214,55 @@ class Puppet::Application::FaceBase < Puppet::Application
     # Call the method associated with the provided action (e.g., 'find').
     if @action
       begin
+        # We need to do arity checking here because this is generic code
+        # calling generic methods – that have argument defaulting.  We need to
+        # make sure we don't accidentally pass the options as the first
+        # argument to a method that takes one argument.  eg:
+        #
+        #   puppet facts find
+        #   => options => {}
+        #      @arguments => [{}]
+        #   => @face.send :bar, {}
+        #
+        #   def face.bar(argument, options = {})
+        #   => bar({}, {})  # oops!  we thought the options were the
+        #                   # positional argument!!
+        #
+        # We could also fix this by making it mandatory to pass the options on
+        # every call, but that would make the Ruby API much more annoying to
+        # work with; having the defaulting is a much nicer convention to have.
+        #
+        # We could also pass the arguments implicitly, by having a magic
+        # 'options' method that was visible in the scope of the action, which
+        # returned the right stuff.
+        #
+        # That sounds attractive, but adds complications to all sorts of
+        # things, especially when you think about how to pass options when you
+        # are writing Ruby code that calls multiple faces.  Especially if
+        # faces are involved in that. ;)
+        #
+        # --daniel 2011-04-27
+        if (arity = @action.positional_arg_count) > 0
+          unless (count = arguments.length) == arity then
+            raise ArgumentError, "wrong number of arguments (#{count} for #{arity})"
+          end
+        end
+
         result = @face.send(@action.name, *arguments)
         puts render(result) unless result.nil?
         status = true
       rescue Exception => detail
         puts detail.backtrace if Puppet[:trace]
-        Puppet.err detail.to_s
+
+        case detail
+        when ArgumentError then
+          got, want = /\((\d+) for (\d+)\)/.match(detail.to_s).to_a.map {|x| x.to_i }
+          Puppet.err "puppet #{@face.name} #{@action.name}: #{want} argument expected but #{got} given"
+          Puppet.err "Try 'puppet help #{@face.name} #{@action.name}' for usage"
+
+        else # generic exception handling, alas.
+          Puppet.err detail.to_s
+        end
       end
     else
       puts "#{face} does not respond to action #{arguments.first}"
diff --git a/lib/puppet/face/indirector.rb b/lib/puppet/face/indirector.rb
index a7ff7e1f0..16ffcd311 100644
--- a/lib/puppet/face/indirector.rb
+++ b/lib/puppet/face/indirector.rb
@@ -25,11 +25,9 @@ that we should describe in this file somehow.
     Puppet::Indirector::Terminus.terminus_classes(indirection.to_sym).collect { |t| t.to_s }.sort
   end
 
-  def call_indirection_method(method, *args)
-    options = args.last
-
+  def call_indirection_method(method, key, options)
     begin
-      result = indirection.__send__(method, *args)
+      result = indirection.__send__(method, key, options)
     rescue => detail
       puts detail.backtrace if Puppet[:trace]
       raise "Could not call '#{method}' on '#{indirection_name}': #{detail}"
@@ -39,19 +37,19 @@ that we should describe in this file somehow.
   end
 
   action :destroy do
-    when_invoked { |*args| call_indirection_method(:destroy, *args) }
+    when_invoked { |key, options| call_indirection_method(:destroy, key, options) }
   end
 
   action :find do
-    when_invoked { |*args| call_indirection_method(:find, *args) }
+    when_invoked { |key, options| call_indirection_method(:find, key, options) }
   end
 
   action :save do
-    when_invoked { |*args| call_indirection_method(:save, *args) }
+    when_invoked { |key, options| call_indirection_method(:save, key, options) }
   end
 
   action :search do
-    when_invoked { |*args| call_indirection_method(:search, *args) }
+    when_invoked { |key, options| call_indirection_method(:search, key, options) }
   end
 
   # Print the configuration for the current terminus class
diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index ac66d2946..9c9741b52 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -168,11 +168,12 @@ class Puppet::Interface::Action
   # this stuff work, because it would have been cleaner.  Which gives you an
   # idea how motivated we were to make this cleaner.  Sorry.
   # --daniel 2011-03-31
+  attr_reader :positional_arg_count
   def when_invoked=(block)
 
     internal_name = "#{@name} implementation, required on Ruby 1.8".to_sym
 
-    arity = block.arity
+    arity = @positional_arg_count = block.arity
     if arity == 0 then
       # This will never fire on 1.8.7, which treats no arguments as "*args",
       # but will on 1.9.2, which treats it as "no arguments".  Which bites,
diff --git a/spec/unit/application/face_base_spec.rb b/spec/unit/application/face_base_spec.rb
index d6ca28d64..d8c970157 100755
--- a/spec/unit/application/face_base_spec.rb
+++ b/spec/unit/application/face_base_spec.rb
@@ -206,11 +206,11 @@ describe Puppet::Application::FaceBase do
 
       app.render_as = :json
       app.face      = Puppet::Face[:basetest, '0.0.1']
-      app.arguments = []
+      app.arguments = [{}]      # we always have options in there...
     end
 
     it "should exit 0 when the action returns true" do
-      app.action = app.face.get_action :return_true
+      app.action    = app.face.get_action :return_true
       expect { app.main }.to exit_with 0
     end
 
diff --git a/spec/unit/application/indirection_base_spec.rb b/spec/unit/application/indirection_base_spec.rb
index 833fb424e..e0a9bebb4 100755
--- a/spec/unit/application/indirection_base_spec.rb
+++ b/spec/unit/application/indirection_base_spec.rb
@@ -28,10 +28,11 @@ describe Puppet::Application::IndirectionBase do
     Puppet::Indirector::Indirection.expects(:instance).
       with(:testindirection).returns(terminus)
 
-    subject.command_line.instance_variable_set('@args', %w{--terminus foo save})
+    subject.command_line.instance_variable_set('@args', %w{--terminus foo save bar})
 
     # Not a very nice thing. :(
     $stderr.stubs(:puts)
+    Puppet.stubs(:err)
 
     expect { subject.run }.to exit_with 0
   end
diff --git a/spec/unit/face/facts_spec.rb b/spec/unit/face/facts_spec.rb
index 06b229aa1..27b5b9e3d 100755
--- a/spec/unit/face/facts_spec.rb
+++ b/spec/unit/face/facts_spec.rb
@@ -9,9 +9,15 @@ describe Puppet::Face[:facts, '0.0.1'] do
 
   describe "when uploading" do
     it "should set the terminus_class to :facter"
+    it "should set the cache_class to :rest"
+    it "should find the current certname"
+  end
 
-    it "should set the cach_eclass to :rest"
+  describe "#find" do
+    it { should be_action :find }
 
-    it "should find the current certname"
+    it "should fail without a key" do
+      expect { subject.find }.to raise_error ArgumentError, /wrong number of arguments/
+    end
   end
 end
diff --git a/spec/unit/face/indirector_spec.rb b/spec/unit/face/indirector_spec.rb
index bb06fcfe2..e7dd44f3d 100755
--- a/spec/unit/face/indirector_spec.rb
+++ b/spec/unit/face/indirector_spec.rb
@@ -34,12 +34,12 @@ describe Puppet::Face::Indirector do
     end
 
     it "should call the indirection method with options when the '#{method}' action is invoked" do
-      subject.indirection.expects(method).with(:test, "myargs", {})
-      subject.send(method, :test, "myargs")
+      subject.indirection.expects(method).with(:test, {})
+      subject.send(method, :test)
     end
     it "should forward passed options" do
-      subject.indirection.expects(method).with(:test, "action", {'one'=>'1'})
-      subject.send(method, :test, 'action', {'one'=>'1'})
+      subject.indirection.expects(method).with(:test, {'one'=>'1'})
+      subject.send(method, :test, {'one'=>'1'})
     end
   end