Merge branch 'domcleal-tickets/master/19558-validation-errors'

* domcleal-tickets/master/19558-validation-errors:
  (#19558) Only catch Puppet::Error and ArgumentError when validating parameters
  (#19558) Add manifest file/line to parameter validation failures
  (#19558) Add resource and manifest context to type validation failures
  (maint) Add YARD to Puppet::Util::Errors

lib/puppet/parameter.rb

@@ -243,6 +243,9 @@ def required?
     # @overload validate {|| ... }
     # Defines an optional method that is used to validate the parameter's value.
     # Validation should raise appropriate exceptions, the return value of the given block is ignored.
+    # The easiest way to raise an appropriate exception is to call the method {Puppet::Util::Errors.fail} with
+    # the message as an argument.
+    #
     # @return [void]
     # @dsl type
     # @api public

lib/puppet/type.rb

@@ -636,6 +636,8 @@ def [](name)
   # Sets the 'should' (wanted state) value of a property, or the value of a parameter.
   # @return 
   # @raise [Puppet::Error] if the setting of the value fails, or if the given name is nil.
+  # @raise [Puppet::ResourceError] when the parameter validation raises Puppet::Error or
+  #   ArgumentError
   def []=(name,value)
     name = name.intern
 
@@ -652,9 +654,9 @@ def []=(name,value)
       begin
         # make sure the parameter doesn't have any errors
         property.value = value
-      rescue => detail
-        error = Puppet::Error.new("Parameter #{name} failed on #{ref}: #{detail}")
-        error.set_backtrace(detail.backtrace)
+      rescue Puppet::Error, ArgumentError => detail
+        error = Puppet::ResourceError.new("Parameter #{name} failed on #{ref}: #{detail}")
+        adderrorcontext(error, detail)
         raise error
       end
     end
@@ -2161,8 +2163,12 @@ def log(msg)
   #
   # @overaload initialize(hsh)
   #   @param hsh [Hash] 
+  #   @raise [Puppet::ResourceError] when the type validation raises
+  #     Puppet::Error or ArgumentError
   # @overload initialize(resource)
   #   @param resource [Puppet:Resource]
+  #   @raise [Puppet::ResourceError] when the type validation raises
+  #     Puppet::Error or ArgumentError
   #
   def initialize(resource)
     resource = self.class.hash2resource(resource) unless resource.is_a?(Puppet::Resource)
@@ -2195,7 +2201,13 @@ def initialize(resource)
 
     set_parameters(@original_parameters)
 
-    self.validate if self.respond_to?(:validate)
+    begin
+      self.validate if self.respond_to?(:validate)
+    rescue Puppet::Error, ArgumentError => detail
+      error = Puppet::ResourceError.new("Validation of #{ref} failed: #{detail}")
+      adderrorcontext(error, detail)
+      raise error
+    end
   end
 
   private

lib/puppet/util/errors.rb

@@ -1,11 +1,24 @@
-# Some helper methods for throwing errors.
+# Some helper methods for throwing and populating errors.
+#
+# @api public
 module Puppet::Util::Errors
-  # Throw a dev error.
+  # Throw a Puppet::DevError with the specified message.  Used for unknown or
+  # internal application failures.
+  #
+  # @param msg [String] message used in raised error
+  # @raise [Puppet::DevError] always raised with the supplied message
   def devfail(msg)
     self.fail(Puppet::DevError, msg)
   end
 
-  # Add line and file info if available and appropriate.
+  # Add line and file info to the supplied exception if info is available from
+  # this object, is appropriately populated and the supplied exception supports
+  # it.  When other is supplied, the backtrace will be copied to the error
+  # object.
+  #
+  # @param error [Exception] exception that is populated with info
+  # @param other [Exception] original exception, source of backtrace info
+  # @return [Exception] error parameter
   def adderrorcontext(error, other = nil)
     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
@@ -15,6 +28,10 @@ def adderrorcontext(error, other = nil)
     error
   end
 
+  # Return a human-readable string of this object's file and line attributes,
+  # if set.
+  #
+  # @return [String] description of file and line
   def error_context
     if file and line
       " at #{file}:#{line}"
@@ -29,6 +46,15 @@ def error_context
 
   # Wrap a call in such a way that we always throw the right exception and keep
   # as much context as possible.
+  #
+  # @param options [Hash<Symbol,Object>] options used to create error
+  # @option options [Class] :type error type to raise, defaults to
+  #   Puppet::DevError
+  # @option options [String] :message message to use in error, default mentions
+  #   the name of this class
+  # @raise [Puppet::Error] re-raised with extra context if the block raises it
+  # @raise [Error] of type options[:type], when the block raises other
+  #   exceptions
   def exceptwrap(options = {})
     options[:type] ||= Puppet::DevError
     begin
@@ -48,6 +74,16 @@ def exceptwrap(options = {})
   end
 
   # Throw an error, defaulting to a Puppet::Error.
+  #
+  # @overload fail(message, ..)
+  #   Throw a Puppet::Error with a message concatenated from the given
+  #   arguments.
+  #   @param [String] message error message(s)
+  # @overload fail(error_klass, message, ..)
+  #   Throw an exception of type error_klass with a message concatenated from
+  #   the given arguments.
+  #   @param [Class] type of error
+  #   @param [String] message error message(s)
   def fail(*args)
     if args[0].is_a?(Class)
       type = args.shift

spec/unit/type_spec.rb

@@ -356,7 +356,23 @@
     end
 
     it "should fail if any invalid attributes have been provided" do
-      expect { Puppet::Type.type(:mount).new(:title => "/foo", :nosuchattr => "whatever") }.to raise_error(Puppet::Error)
+      expect { Puppet::Type.type(:mount).new(:title => "/foo", :nosuchattr => "whatever") }.to raise_error(Puppet::Error, /Invalid parameter/)
+    end
+
+    context "when an attribute fails validation" do
+      it "should fail with Puppet::ResourceError when PuppetError raised" do
+        expect { Puppet::Type.type(:file).new(:title => "/foo", :source => "unknown:///") }.to raise_error(Puppet::ResourceError, /Parameter source failed on File\[.*foo\]/)
+      end
+
+      it "should fail with Puppet::ResourceError when ArgumentError raised" do
+        expect { Puppet::Type.type(:file).new(:title => "/foo", :mode => "abcdef") }.to raise_error(Puppet::ResourceError, /Parameter mode failed on File\[.*foo\]/)
+      end
+
+      it "should include the file/line in the error" do
+        Puppet::Type.type(:file).any_instance.stubs(:file).returns("example.pp")
+        Puppet::Type.type(:file).any_instance.stubs(:line).returns(42)
+        expect { Puppet::Type.type(:file).new(:title => "/foo", :source => "unknown:///") }.to raise_error(Puppet::ResourceError, /example.pp:42/)
+      end
     end
 
     it "should set its name to the resource's title if the resource does not have a :name or namevar parameter set" do
@@ -419,6 +435,35 @@
     it "should delete the name via the namevar from the originally provided parameters" do
       Puppet::Type.type(:file).new(:name => make_absolute('/foo')).original_parameters[:path].should be_nil
     end
+
+    context "when validating the resource" do
+      it "should call the type's validate method if present" do
+        Puppet::Type.type(:file).any_instance.expects(:validate)
+        Puppet::Type.type(:file).new(:name => make_absolute('/foo'))
+      end
+
+      it "should raise Puppet::ResourceError with resource name when Puppet::Error raised" do
+        expect do
+          Puppet::Type.type(:file).new(
+            :name => make_absolute('/foo'),
+            :source => "puppet:///",
+            :content => "foo"
+          )
+        end.to raise_error(Puppet::ResourceError, /Validation of File\[.*foo.*\]/)
+      end
+
+      it "should raise Puppet::ResourceError with manifest file and line on failure" do
+        Puppet::Type.type(:file).any_instance.stubs(:file).returns("example.pp")
+        Puppet::Type.type(:file).any_instance.stubs(:line).returns(42)
+        expect do
+          Puppet::Type.type(:file).new(
+            :name => make_absolute('/foo'),
+            :source => "puppet:///",
+            :content => "foo"
+          )
+        end.to raise_error(Puppet::ResourceError, /Validation.*example.pp:42/)
+      end
+    end
   end
 
   it "should have a class method for converting a hash into a Puppet::Resource instance" do