Please review pull request #741: (#13341) nested exceptions opened by (pcarlisle)
Description:
This refactors the base Puppet::Error exception to allow nested exceptions, enabling full context of an original error to be retained. It modifies our log_exception method to print these correctly.
- Opened: Fri May 04 19:03:10 UTC 2012
- Based on: puppetlabs:master (c9386b541e9ecf6c0daffde2825bad385ea4da2b)
- Requested merge: pcarlisle:ticket/master/13341-nested-exceptions (698f5f8497d5b1beccb2b86bce69a3b2b96b0380)
Diff follows:
diff --git a/lib/puppet/error.rb b/lib/puppet/error.rb
index d0a0c9c..43a8df0 100644
--- a/lib/puppet/error.rb
+++ b/lib/puppet/error.rb
@@ -1,41 +1,47 @@
module Puppet # :nodoc:
- # The base class for all Puppet errors. We want to make it easy to add
- # line and file information. This probably isn't necessary for all
- # errors, but...
+ # The base class for all Puppet errors. It can wrap another exception
class Error < RuntimeError
- attr_accessor :line, :file
-
- def backtrace
- if defined?(@backtrace)
- return @backtrace
- else
- return super
- end
+ attr_reader :original
+ def initialize(message, original=nil)
+ super(message)
+ @original = original
end
+ end
- def initialize(message, line = nil, file = nil)
- @message = message
+ module ExternalFileError
+ # This module implements logging with a filename and line number. Use this
+ # for errors that need to report a location in a non-ruby file that we
+ # parse.
+ attr_accessor :line, :file
- @line = line if line
- @file = file if file
+ def initialize(message, file=nil, line=nil, original=nil)
+ super(message, original)
+ @file = file
+ @line = line
end
def to_s
- str = nil
- if self.file and self.line
- str = "#{@message} at #{@file}:#{@line}"
- elsif self.line
- str = "#{@message} at line #{@line}"
- elsif self.file
- str = "#{@message} in #{self.file}"
+ msg = super
+ if @file and @line
+ "#{msg} at #{@file}:#{@line}"
+ elsif @line
+ "#{msg} at line #{@line}"
+ elsif @file
+ "#{msg} in #{@file}"
else
- str = @message.to_s
+ msg
end
-
- str
end
end
+ class ParseError < Puppet::Error
+ include ExternalFileError
+ end
+
+ class ResourceError < Puppet::Error
+ include ExternalFileError
+ end
+
# An error class for when I don't know what happened. Automatically
# prints a stack trace when in debug mode.
class DevError < Puppet::Error
diff --git a/lib/puppet/network/rights.rb b/lib/puppet/network/rights.rb
index 086ac74..dda2c39 100755
--- a/lib/puppet/network/rights.rb
+++ b/lib/puppet/network/rights.rb
@@ -74,12 +74,11 @@ def is_forbidden_and_why?(name, args = {})
msg = "#{(args[:node].nil? ? args[:ip] : "#{args[:node]}(#{args[:ip]})")} access to #{name} [#{args[:method]}]"
msg += " authenticated " if args[:authenticated]
-
- error = AuthorizationError.new("Forbidden request: #{msg}")
if right
- error.file = right.file
- error.line = right.line
+ msg += " at #{right.file}:#{right.line}"
end
+
+ error = AuthorizationError.new("Forbidden request: #{msg}")
else
# there were no rights allowing/denying name
# if name is not a path, let's throw
diff --git a/lib/puppet/parser/ast.rb b/lib/puppet/parser/ast.rb
index 8918d22..b24ee41 100644
--- a/lib/puppet/parser/ast.rb
+++ b/lib/puppet/parser/ast.rb
@@ -77,7 +77,7 @@ def safeevaluate(*options)
rescue Puppet::Error => detail
raise adderrorcontext(detail)
rescue => detail
- error = Puppet::Error.new(detail.to_s)
+ error = Puppet::ParseError.new(detail.to_s, nil, nil, detail)
# We can't use self.fail here because it always expects strings,
# not exceptions.
raise adderrorcontext(error, detail)
diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/compiler.rb
index 172ffe6..8568755 100644
--- a/lib/puppet/parser/compiler.rb
+++ b/lib/puppet/parser/compiler.rb
@@ -25,7 +25,7 @@ def self.compile(node)
rescue => detail
message = "#{detail} on node #{node.name}"
Puppet.log_exception(detail, message)
- raise Puppet::Error, message
+ raise Puppet::Error, message, detail.backtrace
end
attr_reader :node, :facts, :collections, :catalog, :resources, :relationships, :topscope
diff --git a/lib/puppet/parser/parser.rb b/lib/puppet/parser/parser.rb
index 828a9dd..b431b41 100644
--- a/lib/puppet/parser/parser.rb
+++ b/lib/puppet/parser/parser.rb
@@ -12,7 +12,6 @@
require 'puppet/parser/ast'
module Puppet
- class ParseError < Puppet::Error; end
class ImportError < Racc::ParseError; end
class AlreadyImportedError < ImportError; end
end
diff --git a/lib/puppet/parser/parser_support.rb b/lib/puppet/parser/parser_support.rb
index 7888fe1..86d7072 100644
--- a/lib/puppet/parser/parser_support.rb
+++ b/lib/puppet/parser/parser_support.rb
@@ -151,31 +151,12 @@ def parse(string = nil)
begin
@yydebug = false
main = yyparse(@lexer,:scan)
- rescue Racc::ParseError => except
- error = Puppet::ParseError.new(except)
- error.line = @lexer.line
- error.file = @lexer.file
- error.set_backtrace except.backtrace
- raise error
rescue Puppet::ParseError => except
except.line ||= @lexer.line
except.file ||= @lexer.file
raise except
- rescue Puppet::Error => except
- # and this is a framework error
- except.line ||= @lexer.line
- except.file ||= @lexer.file
- raise except
- rescue Puppet::DevError => except
- except.line ||= @lexer.line
- except.file ||= @lexer.file
- raise except
rescue => except
- error = Puppet::DevError.new(except.message)
- error.line = @lexer.line
- error.file = @lexer.file
- error.set_backtrace except.backtrace
- raise error
+ raise Puppet::ParseError.new(except.message, @lexer.file, @lexer.line, except)
end
end
# Store the results as the top-level class.
diff --git a/lib/puppet/property.rb b/lib/puppet/property.rb
index f0d62f0..af47444 100644
--- a/lib/puppet/property.rb
+++ b/lib/puppet/property.rb
@@ -82,7 +82,7 @@ def call_valuemethod(name, value)
rescue Puppet::Error
raise
rescue => detail
- error = Puppet::Error.new("Could not set '#{value}' on #{self.class.name}: #{detail}", @resource.line, @resource.file)
+ error = Puppet::ResourceError.new("Could not set '#{value}' on #{self.class.name}: #{detail}", @resource.line, @resource.file, detail)
error.set_backtrace detail.backtrace
Puppet.log_exception(detail, error.message)
raise error
diff --git a/lib/puppet/util/errors.rb b/lib/puppet/util/errors.rb
index 5a7a763..fd9b988 100644
--- a/lib/puppet/util/errors.rb
+++ b/lib/puppet/util/errors.rb
@@ -7,10 +7,10 @@ def devfail(msg)
# Add line and file info if available and appropriate.
def adderrorcontext(error, other = nil)
- error.line ||= self.line if self.respond_to?(:line) and self.line
- error.file ||= self.file if self.respond_to?(:file) and self.file
+ error.line ||= self.line if error.respond_to?(:line=) and self.respond_to?(:line) and self.line
+ error.file ||= self.file if error.respond_to?(:file=) and self.respond_to?(:file) and self.file
- error.set_backtrace other.backtrace if other and other.respond_to?(:backtrace)
+ error.set_backtrace(other.backtrace) if other and other.respond_to?(:backtrace)
error
end
@@ -60,4 +60,3 @@ def fail(*args)
raise error
end
end
-
diff --git a/lib/puppet/util/logging.rb b/lib/puppet/util/logging.rb
index 803b402..8990e24 100644
--- a/lib/puppet/util/logging.rb
+++ b/lib/puppet/util/logging.rb
@@ -23,26 +23,34 @@ def send_log(level, message)
# If you pass a String here, your string will be logged instead. You may also pass nil if you don't
# wish to log a message at all; in this case it is likely that you are only calling this method in order
# to take advantage of the backtrace logging.
- # [options] supported options:
- # :force_console => if true, will ensure that the error is written to the console, even if the console is not
- # on the configured list of logging destinations
def log_exception(exception, message = :default, options = {})
+ err(format_exception(exception, message, Puppet[:trace] || options[:trace]))
+ end
+
+ def format_exception(exception, message = :default, trace = true)
+ arr = []
case message
- when :default
- err(exception.message)
- when nil
- # don't log anything if they passed a nil; they are just calling for the optional backtrace logging
- else
- err(message)
+ when :default
+ arr << exception.message
+ when nil
+ # don't log anything if they passed a nil; they are just calling for the optional backtrace logging
+ else
+ arr << message
end
- err(Puppet::Util.pretty_backtrace(exception.backtrace)) if Puppet[:trace] && exception.backtrace
+ if trace and exception.backtrace
+ arr << Puppet::Util.pretty_backtrace(exception.backtrace)
+ end
+ if exception.respond_to?(:original) and exception.original
+ arr << "Wrapped exception:"
+ arr << format_exception(exception.original, :default, trace)
+ end
+ arr.flatten.join("\n")
end
-
def log_and_raise(exception, message)
log_exception(exception, message)
- raise Puppet::Error.new(message + "\n" + exception)
+ raise exception, message + "\n" + exception, exception.backtrace
end
class DeprecationWarning < Exception; end
diff --git a/lib/puppet/util/settings.rb b/lib/puppet/util/settings.rb
index 6deab0c..fc558f7 100644
--- a/lib/puppet/util/settings.rb
+++ b/lib/puppet/util/settings.rb
@@ -1207,7 +1207,7 @@ def parse_file(file)
section = $1.intern # Section names
#disallow application_defaults in config file
if section == :application_defaults
- raise Puppet::Error.new("Illegal section 'application_defaults' in config file", file, line)
+ raise Puppet::Error, "Illegal section 'application_defaults' in config file #{file} at line #{line}"
end
# Add a meta section
result[section][:_meta] ||= {}
diff --git a/spec/unit/util/logging_spec.rb b/spec/unit/util/logging_spec.rb
index 4eebad8..70e9d27 100755
--- a/spec/unit/util/logging_spec.rb
+++ b/spec/unit/util/logging_spec.rb
@@ -122,4 +122,30 @@ class LoggingTester
@logger.deprecation_warning 101
end
end
+
+ describe "when formatting exceptions" do
+ it "should be able to format a chain of exceptions" do
+ exc3 = Puppet::Error.new("original")
+ exc3.set_backtrace(["1.rb:4:in `a'","2.rb:2:in `b'","3.rb:1"])
+ exc2 = Puppet::Error.new("second", exc3)
+ exc2.set_backtrace(["4.rb:8:in `c'","5.rb:1:in `d'","6.rb:3"])
+ exc1 = Puppet::Error.new("third", exc2)
+ exc1.set_backtrace(["7.rb:31:in `e'","8.rb:22:in `f'","9.rb:9"])
+ # whoa ugly
+ @logger.format_exception(exc1).should =~ /third
+.*7\.rb:31
+.*8\.rb:22
+.*9\.rb:9
+Wrapped exception:
+second
+.*4\.rb:8
+.*5\.rb:1
+.*6\.rb:3
+Wrapped exception:
+original
+.*1\.rb:4
+.*2\.rb:2
+.*3\.rb:1/
+ end
+ end
end
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to [email protected].
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
