(#7269) Fix error reporting for bad render formats...

The previous change was incomplete, and could result in internal errors with
the wrong combination of inputs.  Now we have more testing, and a logically
consistent model, so all works.

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

2 files changed, 91 insertions, 81 deletions

diff --git a/lib/puppet/application/face_base.rb b/lib/puppet/application/face_base.rb
index d28a8af3b..d02769412 100644
--- a/lib/puppet/application/face_base.rb
+++ b/lib/puppet/application/face_base.rb
@@ -15,14 +15,8 @@ class Puppet::Application::FaceBase < Puppet::Application
     Puppet::Util::Log.level = :info
   end
 
-  option("--render-as FORMAT") do |_arg|
-    format = _arg.to_sym
-    unless @render_method = Puppet::Network::FormatHandler.format(format)
-      unless format == :for_humans or format == :json
-        raise ArgumentError, "I don't know how to render '#{format}'"
-      end
-    end
-    @render_as = format
+  option("--render-as FORMAT") do |format|
+    self.render_as = format.to_sym
   end
 
   option("--mode RUNMODE", "-r") do |arg|
@@ -34,20 +28,33 @@ class Puppet::Application::FaceBase < Puppet::Application
 
   attr_accessor :face, :action, :type, :arguments, :render_as
 
-  def render(result)
-    format = render_as || action.render_as || :for_humans
+  def render_as=(format)
+    if format == :for_humans or format == :json
+      @render_as = format
+    elsif network_format = Puppet::Network::FormatHandler.format(format)
+      method = network_format.render_method
+      if method == "to_pson" then
+        @render_as = :json
+      else
+        @render_as = method.to_sym
+      end
+    else
+      raise ArgumentError, "I don't know how to render '#{format}'"
+    end
+  end
 
+  def render(result)
     # Invoke the rendering hook supplied by the user, if appropriate.
-    if hook = action.when_rendering(format) then
+    if hook = action.when_rendering(render_as) then
       result = hook.call(result)
     end
 
-    if format == :for_humans then
+    if render_as == :for_humans then
       render_for_humans(result)
-    elsif format == :json or @render_method == "to_pson"
+    elsif render_as == :json
       PSON::pretty_generate(result, :allow_nan => true, :max_nesting => false)
     else
-      result.send(@render_method)
+      result.send(render_as)
     end
   end
 
@@ -129,8 +136,13 @@ class Puppet::Application::FaceBase < Puppet::Application
     end
 
     if @action.nil?
-      @action = @face.get_default_action()
-      @is_default_action = true
+      if @action = @face.get_default_action() then
+        @is_default_action = true
+      else
+        Puppet.err "#{face.name} does not have a default action, and no action was given"
+        Puppet.err Puppet::Face[:help, :current].help(@face.name)
+        exit false
+      end
     end
 
     # Now we can interact with the default option code to build behaviour
@@ -138,7 +150,7 @@ class Puppet::Application::FaceBase < Puppet::Application
     @action.options.each do |option|
       option = @action.get_option(option) # make it the object.
       self.class.option(*option.optparse) # ...and make the CLI parse it.
-    end if @action
+    end
 
     # ...and invoke our parent to parse all the command line options.
     super
@@ -190,6 +202,9 @@ class Puppet::Application::FaceBase < Puppet::Application
     # would invoke the action with options set as global state in the
     # interface object.  --daniel 2011-03-28
     @arguments << options
+
+    # If we don't have a rendering format, set one early.
+    self.render_as ||= (@action.render_as || :for_humans)
   end
 
 
@@ -207,13 +222,8 @@ class Puppet::Application::FaceBase < Puppet::Application
         Puppet.err detail.to_s
       end
     else
-      if arguments.first.is_a? Hash
-        puts "#{face} does not have a default action"
-      else
-        puts "#{face} does not respond to action #{arguments.first}"
-      end
-
-      puts Puppet::Face[:help, :current].help(@face.name, *arguments)
+      puts "#{face} does not respond to action #{arguments.first}"
+      puts Puppet::Face[:help, :current].help(@face.name)
     end
 
     exit status
diff --git a/spec/unit/application/face_base_spec.rb b/spec/unit/application/face_base_spec.rb
index 0c75236f8..275702085 100755
--- a/spec/unit/application/face_base_spec.rb
+++ b/spec/unit/application/face_base_spec.rb
@@ -54,7 +54,7 @@ describe Puppet::Application::FaceBase do
 
     it "should use the default action if not given any arguments" do
       app.command_line.stubs(:args).returns []
-      action = stub(:options => [])
+      action = stub(:options => [], :render_as => nil)
       Puppet::Face[:basetest, '0.0.1'].expects(:get_default_action).returns(action)
       app.stubs(:main)
       app.run
@@ -64,7 +64,7 @@ describe Puppet::Application::FaceBase do
 
     it "should use the default action if not given a valid one" do
       app.command_line.stubs(:args).returns %w{bar}
-      action = stub(:options => [])
+      action = stub(:options => [], :render_as => nil)
       Puppet::Face[:basetest, '0.0.1'].expects(:get_default_action).returns(action)
       app.stubs(:main)
       app.run
@@ -76,9 +76,8 @@ describe Puppet::Application::FaceBase do
       app.command_line.stubs(:args).returns %w{bar}
       Puppet::Face[:basetest, '0.0.1'].expects(:get_default_action).returns(nil)
       app.stubs(:main)
-      app.run
-      app.action.should be_nil
-      app.arguments.should == [ 'bar', { } ]
+      expect { app.run }.to exit_with 1
+      @logs.first.message.should =~ /does not have a default action/
     end
 
     it "should report a sensible error when options with = fail" do
@@ -150,7 +149,7 @@ describe Puppet::Application::FaceBase do
     end
 
     it "should handle application-level options" do
-      app.command_line.stubs(:args).returns %w{help --verbose help}
+      app.command_line.stubs(:args).returns %w{basetest --verbose return_true}
       app.preinit
       app.parse_options
       app.face.name.should == :basetest
@@ -191,7 +190,7 @@ describe Puppet::Application::FaceBase do
 
     it "should lookup help when it cannot do anything else" do
       app.action = nil
-      Puppet::Face[:help, :current].expects(:help).with(:basetest, *app.arguments)
+      Puppet::Face[:help, :current].expects(:help).with(:basetest)
       expect { app.main }.to exit_with 1
     end
 
@@ -205,6 +204,7 @@ describe Puppet::Application::FaceBase do
     before :each do
       app.stubs(:puts)          # don't dump text to screen.
 
+      app.render_as = :json
       app.face      = Puppet::Face[:basetest, '0.0.1']
       app.arguments = []
     end
@@ -228,45 +228,42 @@ describe Puppet::Application::FaceBase do
       app.action = app.face.get_action :return_raise
       expect { app.main }.not_to exit_with 0
     end
-
-    it "should exit non-0 when the action does not exist" do
-      app.action = nil
-      app.arguments = ["foo"]
-      expect { app.main }.to exit_with 1
-    end
   end
 
   describe "#render" do
     before :each do
-      app.face   = Puppet::Face[:basetest, '0.0.1']
-      app.action = app.face.get_action(:foo)
+      app.face      = Puppet::Face[:basetest, '0.0.1']
+      app.action    = app.face.get_action(:foo)
     end
 
-    ["hello", 1, 1.0].each do |input|
-      it "should just return a #{input.class.name}" do
-        app.render(input).should == input
+    context "default rendering" do
+      before :each do app.setup end
+
+      ["hello", 1, 1.0].each do |input|
+        it "should just return a #{input.class.name}" do
+          app.render(input).should == input
+        end
       end
-    end
 
-    [[1, 2], ["one"], [{ 1 => 1 }]].each do |input|
-      it "should render #{input.class} using the 'pp' library" do
-        app.render(input).should == input.pretty_inspect
+      [[1, 2], ["one"], [{ 1 => 1 }]].each do |input|
+        it "should render #{input.class} using the 'pp' library" do
+          app.render(input).should == input.pretty_inspect
+        end
       end
-    end
 
-    it "should render a non-trivially-keyed Hash with the 'pp' library" do
-      hash = { [1,2] => 3, [2,3] => 5, [3,4] => 7 }
-      app.render(hash).should == hash.pretty_inspect
-    end
+      it "should render a non-trivially-keyed Hash with the 'pp' library" do
+        hash = { [1,2] => 3, [2,3] => 5, [3,4] => 7 }
+        app.render(hash).should == hash.pretty_inspect
+      end
 
-    it "should render a {String,Numeric}-keyed Hash into a table" do
-      object = Object.new
-      hash = { "one" => 1, "two" => [], "three" => {}, "four" => object,
-        5 => 5, 6.0 => 6 }
+      it "should render a {String,Numeric}-keyed Hash into a table" do
+        object = Object.new
+        hash = { "one" => 1, "two" => [], "three" => {}, "four" => object,
+          5 => 5, 6.0 => 6 }
 
-      # Gotta love ASCII-betical sort order.  Hope your objects are better
-      # structured for display than my test one is. --daniel 2011-04-18
-      app.render(hash).should == <<EOT
+        # Gotta love ASCII-betical sort order.  Hope your objects are better
+        # structured for display than my test one is. --daniel 2011-04-18
+        app.render(hash).should == <<EOT
 5      5
 6.0    6
 four   #{object.pretty_inspect.chomp}
@@ -274,14 +271,14 @@ one    1
 three  {}
 two    []
 EOT
-    end
+      end
 
-    it "should render a hash nicely with a multi-line value" do
-      hash = {
-        "number" => { "1" => '1' * 40, "2" => '2' * 40, '3' => '3' * 40 },
-        "text"   => { "a" => 'a' * 40, 'b' => 'b' * 40, 'c' => 'c' * 40 }
-      }
-      app.render(hash).should == <<EOT
+      it "should render a hash nicely with a multi-line value" do
+        hash = {
+          "number" => { "1" => '1' * 40, "2" => '2' * 40, '3' => '3' * 40 },
+          "text"   => { "a" => 'a' * 40, 'b' => 'b' * 40, 'c' => 'c' * 40 }
+        }
+        app.render(hash).should == <<EOT
 number  {"1"=>"1111111111111111111111111111111111111111",
          "2"=>"2222222222222222222222222222222222222222",
          "3"=>"3333333333333333333333333333333333333333"}
@@ -289,20 +286,20 @@ text    {"a"=>"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
          "b"=>"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
          "c"=>"cccccccccccccccccccccccccccccccccccccccc"}
 EOT
-    end
+      end
 
-    it "should invoke the action rendering hook while rendering" do
-      app.action.set_rendering_method_for(:for_humans, proc { |value| "bi-winning!" })
-      app.action.render_as = :for_humans
-      app.render("bi-polar?").should == "bi-winning!"
-    end
+      it "should invoke the action rendering hook while rendering" do
+        app.action.set_rendering_method_for(:for_humans, proc { |value| "bi-winning!" })
+        app.render("bi-polar?").should == "bi-winning!"
+      end
 
-    it "should render JSON when asked for json" do
-      app.action.render_as = :json
-      json = app.render({ :one => 1, :two => 2 })
-      json.should =~ /"one":\s*1\b/
-      json.should =~ /"two":\s*2\b/
-      PSON.parse(json).should == { "one" => 1, "two" => 2 }
+      it "should render JSON when asked for json" do
+        app.render_as = :json
+        json = app.render({ :one => 1, :two => 2 })
+        json.should =~ /"one":\s*1\b/
+        json.should =~ /"two":\s*2\b/
+        PSON.parse(json).should == { "one" => 1, "two" => 2 }
+      end
     end
 
     it "should fail early if asked to render an invalid format" do
@@ -311,11 +308,14 @@ EOT
       # it, but this helps us fail if that slips up and all. --daniel 2011-04-27
       Puppet::Face[:help, :current].expects(:help).never
 
-      # ...and this is just annoying.  Thanks, puppet/application.rb.
-      $stderr.expects(:puts).
-        with "Could not parse options: I don't know how to render 'interpretive-dance'"
+      expect {
+        expect { app.setup; app.run }.to exit_with 1
+      }.to print(/I don't know how to render 'interpretive-dance'/)
+    end
 
-      expect { app.run }.to exit_with 1
+    it "should work if asked to render a NetworkHandler format" do
+      app.command_line.stubs(:args).returns %w{facts find dummy --render-as yaml}
+      expect { app.parse_options; app.setup; app.run }.to exit_with 0
     end
   end
 end