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 <[email protected]> --- lib/puppet/parser/resource.rb | 2 +- lib/puppet/provider/ldap.rb | 2 +- lib/puppet/provider/nameservice.rb | 2 +- lib/puppet/resource.rb | 56 +++++++++++++++++--- lib/puppet/resource/type.rb | 4 +- lib/puppet/transportable.rb | 2 +- lib/puppet/type.rb | 5 ++ lib/puppet/type/component.rb | 4 +- spec/unit/provider/ldap.rb | 6 +- spec/unit/resource.rb | 102 ++++++++++++++++++++++++++++++++++-- spec/unit/resource/type.rb | 18 +++--- spec/unit/type.rb | 12 ++++ test/language/resource.rb | 8 ++-- test/lib/puppettest/fakes.rb | 2 +- 14 files changed, 187 insertions(+), 38 deletions(-) diff --git a/lib/puppet/parser/resource.rb b/lib/puppet/parser/resource.rb index 63d028c..428b9df 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 be66838..38668e5 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 cc517ee..57441dd 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 ab03bfd..35676ba 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 ATTRIBUTES = [:file, :line, :exported] @@ -77,6 +81,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 @@ -109,7 +114,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 @@ -131,6 +142,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 @@ -227,8 +247,33 @@ class Puppet::Resource return result 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 @@ -248,11 +293,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 f5e0eeb..cadd034 100644 --- a/lib/puppet/resource/type.rb +++ b/lib/puppet/resource/type.rb @@ -145,7 +145,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 68977dc..1970d9f 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 2f7b57a..f2b1d5c 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 5fed176..bf9007a 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 fd5d1bd..6c58208 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 32b0017..df632e1 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 a5fb679..41ea8e1 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 cfc061f..6bdbe37 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 69e82fd..c1d116f 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 db6ca4d..0aedb59 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 -- 1.6.1 -- 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.
