Adding parameter validation to Puppet::Resource

This will allow us to remove all of the parameter
validation from the other Resource classes.

This is possible because resource types defined
in the language are visible outside of the parser,
via the environment.

This will enable lots of code removal and simplication.

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

14 files changed, 187 insertions, 38 deletions

diff --git a/lib/puppet/parser/resource.rb b/lib/puppet/parser/resource.rb
index 63d028c0c..428b9df50 100644
--- a/lib/puppet/parser/resource.rb
+++ b/lib/puppet/parser/resource.rb
@@ -396,7 +396,7 @@ class Puppet::Parser::Resource
         # Now make sure it's a valid argument to our class.  These checks
         # are organized in order of commonhood -- most types, it's a valid
         # argument and paramcheck is enabled.
-        if @ref.typeclass.validattr?(param)
+        if @ref.typeclass.valid_parameter?(param)
             true
         elsif %w{name title}.include?(param) # always allow these
             true
diff --git a/lib/puppet/provider/ldap.rb b/lib/puppet/provider/ldap.rb
index be6683891..38668e5e5 100644
--- a/lib/puppet/provider/ldap.rb
+++ b/lib/puppet/provider/ldap.rb
@@ -78,7 +78,7 @@ class Puppet::Provider::Ldap < Puppet::Provider
             param, values = ary
 
             # Skip any attributes we don't manage.
-            next result unless self.class.resource_type.validattr?(param)
+            next result unless self.class.resource_type.valid_parameter?(param)
 
             paramclass = self.class.resource_type.attrclass(param)
 
diff --git a/lib/puppet/provider/nameservice.rb b/lib/puppet/provider/nameservice.rb
index cc517ee5f..57441ddf6 100644
--- a/lib/puppet/provider/nameservice.rb
+++ b/lib/puppet/provider/nameservice.rb
@@ -44,7 +44,7 @@ class Puppet::Provider::NameService < Puppet::Provider
         end
 
         def options(name, hash)
-            unless resource_type.validattr?(name)
+            unless resource_type.valid_parameter?(name)
                 raise Puppet::DevError, "%s is not a valid attribute for %s" %
                     [name, resource_type.name]
             end
diff --git a/lib/puppet/resource.rb b/lib/puppet/resource.rb
index e47501791..010cd956e 100644
--- a/lib/puppet/resource.rb
+++ b/lib/puppet/resource.rb
@@ -1,16 +1,20 @@
 require 'puppet'
 require 'puppet/util/tagging'
-#require 'puppet/resource/reference'
 require 'puppet/util/pson'
 
 # The simplest resource class.  Eventually it will function as the
 # base class for all resource-like behaviour.
 class Puppet::Resource
+    require 'puppet/resource/reference'
     include Puppet::Util::Tagging
+
+    require 'puppet/resource/type_collection_helper'
+    include Puppet::Resource::TypeCollectionHelper
+
     extend Puppet::Util::Pson
     include Enumerable
-    attr_accessor :file, :line, :catalog, :exported, :virtual
-    attr_writer :type, :title
+    attr_accessor :file, :line, :catalog, :exported, :virtual, :namespace, :validate_parameters
+    attr_writer :type, :title, :environment
 
     require 'puppet/indirector'
     extend Puppet::Indirector
@@ -81,6 +85,7 @@ class Puppet::Resource
     # Set a given parameter.  Converts all passed names
     # to lower-case symbols.
     def []=(param, value)
+        validate_parameter(param) if validate_parameters
         @parameters[parameter_name(param)] = value
     end
 
@@ -117,7 +122,13 @@ class Puppet::Resource
 
     # Create our resource.
     def initialize(type, title, attributes = {})
+        # Doing this, instead of including it in the class,
+        # is the only way I could get the load order to work
+        # here.
+        extend Puppet::Node::Environment::Helper
+
         @parameters = {}
+        @namespace = ""
 
         (attributes[:parameters] || {}).each do |param, value|
             self[param] = value
@@ -139,6 +150,15 @@ class Puppet::Resource
         @reference.to_s
     end
 
+    def resource_type
+        case type.to_s.downcase
+        when "class"; find_hostclass
+        when "node"; find_node
+        else
+            find_builtin_resource_type || find_defined_resource_type
+        end
+    end
+
     # Get our title information from the reference, since it will canonize it for us.
     def title
         @reference.title
@@ -246,8 +266,33 @@ class Puppet::Resource
         self
     end
 
+    def valid_parameter?(name)
+        resource_type.valid_parameter?(name)
+    end
+
+    def validate_parameter(name)
+        raise ArgumentError, "Invalid parameter #{name}" unless valid_parameter?(name)
+    end
+
     private
 
+    def find_node
+        known_resource_types.node(title)
+    end
+
+    def find_hostclass
+        name = title == :main ? "" : title
+        known_resource_types.find_hostclass(namespace, name)
+    end
+
+    def find_builtin_resource_type
+        Puppet::Type.type(type.to_s.downcase.to_sym)
+    end
+
+    def find_defined_resource_type
+        known_resource_types.find_definition(namespace, type.to_s.downcase)
+    end
+
     # Produce a canonical method name.
     def parameter_name(param)
         param = param.to_s.downcase.to_sym
@@ -267,11 +312,6 @@ class Puppet::Resource
         end
     end
 
-    # Retrieve the resource type.
-    def resource_type
-        Puppet::Type.type(type)
-    end
-
     # Create an old-style TransBucket instance, for non-builtin resource types.
     def to_transbucket
         bucket = Puppet::TransBucket.new([])
diff --git a/lib/puppet/resource/type.rb b/lib/puppet/resource/type.rb
index 9baf1983e..d47658284 100644
--- a/lib/puppet/resource/type.rb
+++ b/lib/puppet/resource/type.rb
@@ -144,7 +144,7 @@ class Puppet::Resource::Type
         set = {}
         resource.to_hash.each do |param, value|
             param = param.to_sym
-            fail Puppet::ParseError, "#{resource.ref} does not accept attribute #{param}" unless validattr?(param)
+            fail Puppet::ParseError, "#{resource.ref} does not accept attribute #{param}" unless valid_parameter?(param)
 
             exceptwrap { scope.setvar(param.to_s, value) }
 
@@ -174,7 +174,7 @@ class Puppet::Resource::Type
     end
 
     # Check whether a given argument is valid.
-    def validattr?(param)
+    def valid_parameter?(param)
         param = param.to_s
 
         return true if param == "name"
diff --git a/lib/puppet/transportable.rb b/lib/puppet/transportable.rb
index 68977dca0..1970d9f1e 100644
--- a/lib/puppet/transportable.rb
+++ b/lib/puppet/transportable.rb
@@ -49,7 +49,7 @@ module Puppet
         def to_component
             trans = TransObject.new(ref, :component)
             @params.each { |param,value|
-                next unless Puppet::Type::Component.validattr?(param)
+                next unless Puppet::Type::Component.valid_parameter?(param)
                 Puppet.debug "Defining %s on %s" % [param, ref]
                 trans[param] = value
             }
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 2fb4abca8..31728c374 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -388,6 +388,11 @@ class Type
         end
     end
 
+    # This is a forward-compatibility method - it's the validity interface we'll use in Puppet::Resource.
+    def self.valid_parameter?(name)
+        validattr?(name)
+    end
+
     # Return either the attribute alias or the attribute.
     def attr_alias(name)
         name = symbolize(name)
diff --git a/lib/puppet/type/component.rb b/lib/puppet/type/component.rb
index 5fed1760e..bf9007ab4 100644
--- a/lib/puppet/type/component.rb
+++ b/lib/puppet/type/component.rb
@@ -14,14 +14,14 @@ Puppet::Type.newtype(:component) do
     # Override how parameters are handled so that we support the extra
     # parameters that are used with defined resource types.
     def [](param)
-        return super if self.class.validattr?(param)
+        return super if self.class.valid_parameter?(param)
         @extra_parameters[param.to_sym]
     end
 
     # Override how parameters are handled so that we support the extra
     # parameters that are used with defined resource types.
     def []=(param, value)
-        return super if self.class.validattr?(param)
+        return super if self.class.valid_parameter?(param)
         @extra_parameters[param.to_sym] = value
     end
 
diff --git a/spec/unit/provider/ldap.rb b/spec/unit/provider/ldap.rb
index fd5d1bdc3..6c5820883 100755
--- a/spec/unit/provider/ldap.rb
+++ b/spec/unit/provider/ldap.rb
@@ -131,7 +131,7 @@ describe Puppet::Provider::Ldap do
 
                 @property_class = stub 'property_class', :array_matching => :all, :superclass => Puppet::Property
                 @resource_class.stubs(:attrclass).with(:one).returns(@property_class)
-                @resource_class.stubs(:validattr?).returns true
+                @resource_class.stubs(:valid_parameter?).returns true
             end
 
             it "should store a copy of the hash as its ldap_properties" do
@@ -161,7 +161,7 @@ describe Puppet::Provider::Ldap do
             end
 
             it "should discard any properties not valid in the resource class" do
-                @resource_class.expects(:validattr?).with(:a).returns false
+                @resource_class.expects(:valid_parameter?).with(:a).returns false
                 @property_class.stubs(:array_matching).returns :all
 
                 instance = @class.new(:one => %w{two three}, :a => %w{b})
@@ -177,7 +177,7 @@ describe Puppet::Provider::Ldap do
             @instance = @class.new
 
             @property_class = stub 'property_class', :array_matching => :all, :superclass => Puppet::Property
-            @resource_class = stub 'resource_class', :attrclass => @property_class, :validattr? => true, :validproperties => [:one, :two]
+            @resource_class = stub 'resource_class', :attrclass => @property_class, :valid_parameter? => true, :validproperties => [:one, :two]
             @class.stubs(:resource_type).returns @resource_class
         end
 
diff --git a/spec/unit/resource.rb b/spec/unit/resource.rb
index bc47fa7ed..73bbfd865 100755
--- a/spec/unit/resource.rb
+++ b/spec/unit/resource.rb
@@ -90,20 +90,111 @@ describe Puppet::Resource do
         resource.should be_exported
     end
 
-    it "should support an environment attribute"
+    it "should support an environment attribute" do
+        Puppet::Resource.new("file", "/my/file", :environment => :foo).environment.name.should == :foo
+    end
+
+    it "should support a namespace attribute" do
+        Puppet::Resource.new("file", "/my/file", :namespace => :foo).namespace.should == :foo
+    end
+
+    it "should default to a namespace of an empty string" do
+        Puppet::Resource.new("file", "/my/file").namespace.should == ""
+    end
+
+    it "should be able to look up its resource type when the type is a builtin resource" do
+        Puppet::Resource.new("file", "/my/file").resource_type.should equal(Puppet::Type.type(:file))
+    end
+
+    it "should be able to look up its resource type via its environment when the type is a defined resource type" do
+        resource = Puppet::Resource.new("foobar", "/my/file")
+        type = Puppet::Resource::Type.new(:definition, "foobar")
+        resource.environment.known_resource_types.add type
+
+        resource.resource_type.should equal(type)
+    end
+
+    it "should be able to look up its resource type via its environment when the type is a node" do
+        resource = Puppet::Resource.new("node", "foobar")
+        node = Puppet::Resource::Type.new(:node, "foobar")
+        resource.environment.known_resource_types.add node
+
+        resource.resource_type.should equal(node)
+    end
+
+    it "should be able to look up its resource type via its environment when the type is a class" do
+        resource = Puppet::Resource.new("class", "foobar")
+        klass = Puppet::Resource::Type.new(:hostclass, "foobar")
+        resource.environment.known_resource_types.add klass
 
-    it "should convert its environment into an environment instance if one is provided"
+        resource.resource_type.should equal(klass)
+    end
+
+    it "should use its namespace when looking up defined resource types" do
+        resource = Puppet::Resource.new("bar", "/my/file", :namespace => "foo")
+        type = Puppet::Resource::Type.new(:definition, "foo::bar")
+        resource.environment.known_resource_types.add type
+
+        resource.resource_type.should equal(type)
+    end
 
-    it "should support a namespace attribute"
+    it "should use its namespace when looking up host classes" do
+        resource = Puppet::Resource.new("class", "bar", :namespace => "foo")
+        type = Puppet::Resource::Type.new(:hostclass, "foo::bar")
+        resource.environment.known_resource_types.add type
+
+        resource.resource_type.should equal(type)
+    end
+
+    it "should return nil when looking up resource types that don't exist" do
+        Puppet::Resource.new("foobar", "bar").resource_type.should be_nil
+    end
+
+    it "should fail when an invalid parameter is used and parameter validation is enabled" do
+        type = Puppet::Resource::Type.new(:definition, "foobar")
+        Puppet::Node::Environment.new.known_resource_types.add type
+        resource = Puppet::Resource.new("foobar", "/my/file", :validate_parameters => true)
+        lambda { resource[:yay] = true }.should raise_error(ArgumentError)
+    end
+
+    it "should not fail when an invalid parameter is used and parameter validation is disabled" do
+        type = Puppet::Resource::Type.new(:definition, "foobar")
+        Puppet::Node::Environment.new.known_resource_types.add type
+        resource = Puppet::Resource.new("foobar", "/my/file")
+        resource[:yay] = true
+    end
+
+    it "should not fail when a valid parameter is used and parameter validation is enabled" do
+        type = Puppet::Resource::Type.new(:definition, "foobar", :arguments => {"yay" => nil})
+        Puppet::Node::Environment.new.known_resource_types.add type
+        resource = Puppet::Resource.new("foobar", "/my/file", :validate_parameters => true)
+        resource[:yay] = true
+    end
 
     describe "when managing parameters" do
         before do
             @resource = Puppet::Resource.new("file", "/my/file")
         end
 
-        it "should be able to check whether parameters are valid when the resource models builtin resources"
+        it "should correctly detect when provided parameters are not valid for builtin types" do
+            Puppet::Resource.new("file", "/my/file").should_not be_valid_parameter("foobar")
+        end
+
+        it "should correctly detect when provided parameters are valid for builtin types" do
+            Puppet::Resource.new("file", "/my/file").should be_valid_parameter("mode")
+        end
+
+        it "should correctly detect when provided parameters are not valid for defined resource types" do
+            type = Puppet::Resource::Type.new(:definition, "foobar")
+            Puppet::Node::Environment.new.known_resource_types.add type
+            Puppet::Resource.new("foobar", "/my/file").should_not be_valid_parameter("myparam")
+        end
 
-        it "should be able to check whether parameters are valid when the resource models defined resources"
+        it "should correctly detect when provided parameters are valid for defined resource types" do
+            type = Puppet::Resource::Type.new(:definition, "foobar", :arguments => {"myparam" => nil})
+            Puppet::Node::Environment.new.known_resource_types.add type
+            Puppet::Resource.new("foobar", "/my/file").should be_valid_parameter("myparam")
+        end
 
         it "should allow setting and retrieving of parameters" do
             @resource[:foo] = "bar"
@@ -138,6 +229,7 @@ describe Puppet::Resource do
 
         it "should be able to set the name for non-builtin types" do
             resource = Puppet::Resource.new(:foo, "bar")
+            resource[:name] = "eh"
             lambda { resource[:name] = "eh" }.should_not raise_error
         end
 
diff --git a/spec/unit/resource/type.rb b/spec/unit/resource/type.rb
index 98b38c9b1..0a2f447ef 100755
--- a/spec/unit/resource/type.rb
+++ b/spec/unit/resource/type.rb
@@ -138,37 +138,37 @@ describe Puppet::Resource::Type do
 
         it "should set any provided arguments with the keys as symbols" do
             type = Puppet::Resource::Type.new(:hostclass, "foo", :arguments => {:foo => "bar", :baz => "biz"})
-            type.should be_validattr("foo")
-            type.should be_validattr("baz")
+            type.should be_valid_parameter("foo")
+            type.should be_valid_parameter("baz")
         end
 
         it "should set any provided arguments with they keys as strings" do
             type = Puppet::Resource::Type.new(:hostclass, "foo", :arguments => {"foo" => "bar", "baz" => "biz"})
-            type.should be_validattr(:foo)
-            type.should be_validattr(:baz)
+            type.should be_valid_parameter(:foo)
+            type.should be_valid_parameter(:baz)
         end
 
         it "should function if provided no arguments" do
             type = Puppet::Resource::Type.new(:hostclass, "foo")
-            type.should_not be_validattr(:foo)
+            type.should_not be_valid_parameter(:foo)
         end
     end
 
     describe "when testing the validity of an attribute" do
         it "should return true if the parameter was typed at initialization" do
-            Puppet::Resource::Type.new(:hostclass, "foo", :arguments => {"foo" => "bar"}).should be_validattr("foo")
+            Puppet::Resource::Type.new(:hostclass, "foo", :arguments => {"foo" => "bar"}).should be_valid_parameter("foo")
         end
 
         it "should return true if it is a metaparam" do
-            Puppet::Resource::Type.new(:hostclass, "foo").should be_validattr("require")
+            Puppet::Resource::Type.new(:hostclass, "foo").should be_valid_parameter("require")
         end
 
         it "should return true if the parameter is named 'name'" do
-            Puppet::Resource::Type.new(:hostclass, "foo").should be_validattr("name")
+            Puppet::Resource::Type.new(:hostclass, "foo").should be_valid_parameter("name")
         end
 
         it "should return false if it is not a metaparam and was not provided at initialization" do
-            Puppet::Resource::Type.new(:hostclass, "foo").should_not be_validattr("yayness")
+            Puppet::Resource::Type.new(:hostclass, "foo").should_not be_valid_parameter("yayness")
         end
     end
 
diff --git a/spec/unit/type.rb b/spec/unit/type.rb
index 97dc113a6..73f249faa 100755
--- a/spec/unit/type.rb
+++ b/spec/unit/type.rb
@@ -7,6 +7,18 @@ describe Puppet::Type do
         Puppet::Type.ancestors.should be_include(Puppet::Util::Cacher)
     end
 
+    it "should consider a parameter to be valid if it is a valid parameter" do
+        Puppet::Type.type(:mount).should be_valid_parameter(:path)
+    end
+
+    it "should consider a parameter to be valid if it is a valid property" do
+        Puppet::Type.type(:mount).should be_valid_parameter(:fstype)
+    end
+
+    it "should consider a parameter to be valid if it is a valid metaparam" do
+        Puppet::Type.type(:mount).should be_valid_parameter(:noop)
+    end
+
     it "should use its catalog as its expirer" do
         catalog = Puppet::Resource::Catalog.new
         resource = Puppet::Type.type(:mount).new(:name => "foo", :fstype => "bar", :pass => 1, :ensure => :present)
diff --git a/test/language/resource.rb b/test/language/resource.rb
index 69e82fde4..c1d116f9e 100755
--- a/test/language/resource.rb
+++ b/test/language/resource.rb
@@ -67,17 +67,17 @@ class TestResource < PuppetTest::TestCase
         res.instance_variable_set("@ref", ref)
         klass = mock("class")
         ref.expects(:typeclass).returns(klass).times(4)
-        klass.expects(:validattr?).with("good").returns(true)
+        klass.expects(:valid_parameter?).with("good").returns(true)
         assert(res.send(:paramcheck, :good), "Did not allow valid param")
 
         # It's name or title
-        klass.expects(:validattr?).with("name").returns(false)
+        klass.expects(:valid_parameter?).with("name").returns(false)
         assert(res.send(:paramcheck, :name), "Did not allow name")
-        klass.expects(:validattr?).with("title").returns(false)
+        klass.expects(:valid_parameter?).with("title").returns(false)
         assert(res.send(:paramcheck, :title), "Did not allow title")
 
         # It's not actually allowed
-        klass.expects(:validattr?).with("other").returns(false)
+        klass.expects(:valid_parameter?).with("other").returns(false)
         res.expects(:fail)
         ref.expects(:type)
         res.send(:paramcheck, :other)
diff --git a/test/lib/puppettest/fakes.rb b/test/lib/puppettest/fakes.rb
index db6ca4d80..0aedb59f6 100644
--- a/test/lib/puppettest/fakes.rb
+++ b/test/lib/puppettest/fakes.rb
@@ -35,7 +35,7 @@ module PuppetTest
 
         def []=(param, value)
             param = symbolize(param)
-            unless @realresource.validattr?(param)
+            unless @realresource.valid_parameter?(param)
                 raise Puppet::DevError, "Invalid attribute %s for %s" %
                     [param, @realresource.name]
             end