Adding better error-handling to format rendering

We now always get the format name and the method that
failed.

Signed-off-by: Luke Kanies <luke@madstop.com>

2 files changed, 84 insertions, 4 deletions

diff --git a/lib/puppet/network/format_handler.rb b/lib/puppet/network/format_handler.rb
index f3c3380e1..efeea79e3 100644
--- a/lib/puppet/network/format_handler.rb
+++ b/lib/puppet/network/format_handler.rb
@@ -3,6 +3,33 @@ require 'puppet/network'
 require 'puppet/network/format'
 
 module Puppet::Network::FormatHandler
+    class FormatError < Puppet::Error; end
+
+    class FormatProtector
+        attr_reader :format
+
+        def protect(method, args)
+            begin
+                Puppet::Network::FormatHandler.format(format).send(method, *args)
+            rescue => details
+                direction = method.to_s.include?("intern") ? "from" : "to"
+                error = FormatError.new("Could not %s %s %s: %s" % [method, direction, format, details])
+                error.set_backtrace(details.backtrace)
+                raise error
+            end
+        end
+
+        def initialize(format)
+            @format = format
+        end
+
+        [:intern, :intern_multiple, :render, :render_multiple].each do |method|
+            define_method(method) do |*args|
+                protect(method, args)
+            end
+        end
+    end
+
     @formats = {}
     def self.create(*args, &block)
         instance = Puppet::Network::Format.new(*args)
@@ -35,21 +62,29 @@ module Puppet::Network::FormatHandler
         @formats.values.find { |format| format.mime == mimetype }
     end
 
+    # Use a delegator to make sure any exceptions generated by our formats are
+    # handled intelligently.
+    def self.protected_format(name)
+        @format_protectors ||= {}
+        @format_protectors[name] ||= FormatProtector.new(name)
+        @format_protectors[name]
+    end
+
     module ClassMethods
         def format_handler
             Puppet::Network::FormatHandler
         end
 
         def convert_from(format, data)
-            format_handler.format(format).intern(self, data)
+            format_handler.protected_format(format).intern(self, data)
         end
 
         def convert_from_multiple(format, data)
-            format_handler.format(format).intern_multiple(self, data)
+            format_handler.protected_format(format).intern_multiple(self, data)
         end
 
         def render_multiple(format, instances)
-            format_handler.format(format).render_multiple(instances)
+            format_handler.protected_format(format).render_multiple(instances)
         end
 
         def default_format
@@ -72,7 +107,7 @@ module Puppet::Network::FormatHandler
         def render(format = nil)
             format ||= self.class.default_format
 
-            Puppet::Network::FormatHandler.format(format).render(self)
+            Puppet::Network::FormatHandler.protected_format(format).render(self)
         end
 
         def support_format?(name)
diff --git a/spec/unit/network/format_handler.rb b/spec/unit/network/format_handler.rb
index 3dcff6199..d4fb7da64 100755
--- a/spec/unit/network/format_handler.rb
+++ b/spec/unit/network/format_handler.rb
@@ -51,6 +51,26 @@ describe Puppet::Network::FormatHandler do
         FormatTester.default_format.should == "one"
     end
 
+    it "should be able to use a protected format for better logging on errors" do
+        Puppet::Network::FormatHandler.should respond_to(:protected_format)
+    end
+
+    it "should delegate all methods from the informative format to the specified format" do
+        format = mock 'format'
+        Puppet::Network::FormatHandler.expects(:format).with(:myformat).returns format
+
+        format.expects(:render).with("foo").returns "yay"
+        Puppet::Network::FormatHandler.protected_format(:myformat).render("foo").should == "yay"
+    end
+
+    it "should provide better logging if a failure is encountered when delegating from the informative format to the real format" do
+        format = mock 'format'
+        Puppet::Network::FormatHandler.expects(:format).with(:myformat).returns format
+
+        format.expects(:render).with("foo").raises "foo"
+        lambda { Puppet::Network::FormatHandler.protected_format(:myformat).render("foo") }.should raise_error(Puppet::Network::FormatHandler::FormatError)
+    end
+
     describe "when using formats" do
         before do
             @format = mock 'format'
@@ -76,17 +96,32 @@ describe Puppet::Network::FormatHandler do
             FormatTester.convert_from(:my_format, "mydata")
         end
 
+        it "should raise a FormatError when an exception is encountered when converting from a format" do
+            @format.expects(:intern).with(FormatTester, "mydata").raises "foo"
+            lambda { FormatTester.convert_from(:my_format, "mydata") }.should raise_error(Puppet::Network::FormatHandler::FormatError)
+        end
+
         it "should be able to use a specific hook for converting into multiple instances" do
             @format.expects(:intern_multiple).with(FormatTester, "mydata")
 
             FormatTester.convert_from_multiple(:my_format, "mydata")
         end
 
+        it "should raise a FormatError when an exception is encountered when converting multiple items from a format" do
+            @format.expects(:intern_multiple).with(FormatTester, "mydata").raises "foo"
+            lambda { FormatTester.convert_from_multiple(:my_format, "mydata") }.should raise_error(Puppet::Network::FormatHandler::FormatError)
+        end
+
         it "should be able to use a specific hook for rendering multiple instances" do
             @format.expects(:render_multiple).with("mydata")
 
             FormatTester.render_multiple(:my_format, "mydata")
         end
+
+        it "should raise a FormatError when an exception is encountered when rendering multiple items into a format" do
+            @format.expects(:render_multiple).with("mydata").raises "foo"
+            lambda { FormatTester.render_multiple(:my_format, "mydata") }.should raise_error(Puppet::Network::FormatHandler::FormatError)
+        end
     end
 
     describe "when managing formats" do
@@ -153,6 +188,16 @@ describe Puppet::Network::FormatHandler do
             FormatTester.new.should respond_to(:render)
         end
 
+        it "should raise a FormatError when a rendering error is encountered" do
+            format = stub 'rendering format', :supported? => true
+            Puppet::Network::FormatHandler.stubs(:format).with(:foo).returns format
+
+            tester = FormatTester.new
+            format.expects(:render).with(tester).raises "eh"
+
+            lambda { tester.render(:foo) }.should raise_error(Puppet::Network::FormatHandler::FormatError)
+        end
+
         it "should call the format-specific converter when asked to convert to a given format" do
             format = stub 'rendering format', :supported? => true