From 3aeb98399c8dfaa90565bdf1b73f7fd9b0f7aeb8 Mon Sep 17 00:00:00 2001 From: Dave Armstrong Date: Mon, 10 Dec 2018 10:37:27 +0000 Subject: [PATCH] (maint) Validate Type Schema --- .rspec | 2 +- lib/puppet/resource_api.rb | 39 +------- lib/puppet/resource_api/type_definition.rb | 56 +++++++++-- spec/puppet/resource_api/base_context_spec.rb | 2 +- spec/puppet/resource_api/io_context_spec.rb | 2 +- .../resource_api/puppet_context_spec.rb | 2 +- .../resource_api/type_definition_spec.rb | 94 +++++++++++++++++-- spec/puppet/resource_api_spec.rb | 93 ++---------------- spec/spec_helper.rb | 5 + 9 files changed, 158 insertions(+), 137 deletions(-) diff --git a/.rspec b/.rspec index c895322c..fb8d3594 100644 --- a/.rspec +++ b/.rspec @@ -1,3 +1,3 @@ --format documentation --color ---order rand:123 \ No newline at end of file +--order rand \ No newline at end of file diff --git a/lib/puppet/resource_api.rb b/lib/puppet/resource_api.rb index 0f9ba788..07b97edf 100644 --- a/lib/puppet/resource_api.rb +++ b/lib/puppet/resource_api.rb @@ -19,33 +19,9 @@ class << self end def register_type(definition) - raise Puppet::DevError, 'requires a hash as definition, not `%{other_type}`' % { other_type: definition.class } unless definition.is_a? Hash - raise Puppet::DevError, 'requires a `:name`' unless definition.key? :name - raise Puppet::DevError, 'requires `:attributes`' unless definition.key? :attributes - raise Puppet::DevError, '`:attributes` must be a hash, not `%{other_type}`' % { other_type: definition[:attributes].class } unless definition[:attributes].is_a?(Hash) - [:title, :provider, :alias, :audit, :before, :consume, :export, :loglevel, :noop, :notify, :require, :schedule, :stage, :subscribe, :tag].each do |name| - raise Puppet::DevError, 'must not define an attribute called `%{name}`' % { name: name.inspect } if definition[:attributes].key? name - end - if definition.key?(:title_patterns) && !definition[:title_patterns].is_a?(Array) - raise Puppet::DevError, '`:title_patterns` must be an array, not `%{other_type}`' % { other_type: definition[:title_patterns].class } - end - - Puppet::ResourceApi::DataTypeHandling.validate_ensure(definition) - - definition[:features] ||= [] - supported_features = %w[supports_noop canonicalize remote_resource simple_get_filter].freeze - unknown_features = definition[:features] - supported_features - Puppet.warning("Unknown feature detected: #{unknown_features.inspect}") unless unknown_features.empty? - - # fixup any weird behavior ;-) - definition[:attributes].each do |name, attr| - next unless attr[:behavior] - if attr[:behaviour] - raise Puppet::DevError, "the '#{name}' attribute has both a `behavior` and a `behaviour`, only use one" - end - attr[:behaviour] = attr[:behavior] - attr.delete(:behavior) - end + # Attempt to create a TypeDefinition from the input hash + # This will validate and throw if its not right + type_def = TypeDefinition.new(definition) # prepare the ruby module for the provider # this has to happen before Puppet::Type.newtype starts autoloading providers @@ -57,6 +33,7 @@ def register_type(definition) Puppet::Type.newtype(definition[:name].to_sym) do @docs = definition[:docs] + @type_definition = type_def # Keeps a copy of the provider around. Weird naming to avoid clashes with puppet's own `provider` member define_singleton_method(:my_provider) do @@ -70,7 +47,7 @@ def my_provider end define_singleton_method(:type_definition) do - @type_definition ||= TypeDefinition.new(definition) + @type_definition end def type_definition @@ -195,14 +172,8 @@ def to_resource # https://puppet.com/docs/puppet/6.0/custom_types.html#reference-5883 # for details. send(param_or_property, name.to_sym, parent: parent) do - unless options[:type] - raise Puppet::DevError, "#{definition[:name]}.#{name} has no type" - end - if options[:desc] desc "#{options[:desc]} (a #{options[:type]})" - else - warn("#{definition[:name]}.#{name} has no docs") end # The initialize method is called when puppet core starts building up diff --git a/lib/puppet/resource_api/type_definition.rb b/lib/puppet/resource_api/type_definition.rb index 029c2ac3..e23a077a 100644 --- a/lib/puppet/resource_api/type_definition.rb +++ b/lib/puppet/resource_api/type_definition.rb @@ -3,8 +3,8 @@ class Puppet::ResourceApi::TypeDefinition attr_reader :definition def initialize(definition) - raise Puppet::DevError, 'TypeDefinition requires definition to be a Hash' unless definition.is_a?(Hash) - @definition = definition + @data_type_cache = {} + validate_schema(definition) end def name @@ -36,6 +36,53 @@ def feature?(feature) supported end + def validate_schema(definition) + raise Puppet::DevError, 'Type definition must be a Hash, not `%{other_type}`' % { other_type: definition.class } unless definition.is_a?(Hash) + raise Puppet::DevError, 'Type definition must have a name' unless definition.key? :name + raise Puppet::DevError, 'Type definition must have `:attributes`' unless definition.key? :attributes + unless definition[:attributes].is_a?(Hash) + raise Puppet::DevError, '`%{name}.attributes` must be a hash, not `%{other_type}`' % { + name: definition[:name], other_type: definition[:attributes].class + } + end + [:title, :provider, :alias, :audit, :before, :consume, :export, :loglevel, :noop, :notify, :require, :schedule, :stage, :subscribe, :tag].each do |name| + raise Puppet::DevError, 'must not define an attribute called `%{name}`' % { name: name.inspect } if definition[:attributes].key? name + end + if definition.key?(:title_patterns) && !definition[:title_patterns].is_a?(Array) + raise Puppet::DevError, '`:title_patterns` must be an array, not `%{other_type}`' % { other_type: definition[:title_patterns].class } + end + + Puppet::ResourceApi::DataTypeHandling.validate_ensure(definition) + + definition[:features] ||= [] + supported_features = %w[supports_noop canonicalize remote_resource simple_get_filter].freeze + unknown_features = definition[:features] - supported_features + Puppet.warning("Unknown feature detected: #{unknown_features.inspect}") unless unknown_features.empty? + + definition[:attributes].each do |key, attr| + raise Puppet::DevError, "`#{definition[:name]}.#{key}` must be a Hash, not a #{attr.class}" unless attr.is_a? Hash + raise Puppet::DevError, "`#{definition[:name]}.#{key}` has no type" unless attr.key? :type + Puppet.warning("`#{definition[:name]}.#{key}` has no docs") unless attr.key? :desc + + # validate the type by attempting to parse into a puppet type + @data_type_cache[definition[:attributes][key][:type]] ||= + Puppet::ResourceApi::DataTypeHandling.parse_puppet_type( + key, + definition[:attributes][key][:type], + ) + + # fixup any weird behavior ;-) + next unless attr[:behavior] + if attr[:behaviour] + raise Puppet::DevError, "the '#{key}' attribute has both a `behavior` and a `behaviour`, only use one" + end + attr[:behaviour] = attr[:behavior] + attr.delete(:behavior) + end + # store the validated definition + @definition = definition + end + # validates a resource hash against its type schema def check_schema(resource) namevars.each do |namevar| @@ -84,10 +131,7 @@ def check_schema_values(resource) bad_vals = {} resource.each do |key, value| next unless attributes[key] - type = Puppet::ResourceApi::DataTypeHandling.parse_puppet_type( - key, - attributes[key][:type], - ) + type = @data_type_cache[attributes[key][:type]] error_message = Puppet::ResourceApi::DataTypeHandling.try_validate( type, value, diff --git a/spec/puppet/resource_api/base_context_spec.rb b/spec/puppet/resource_api/base_context_spec.rb index b7f4f724..34869f75 100644 --- a/spec/puppet/resource_api/base_context_spec.rb +++ b/spec/puppet/resource_api/base_context_spec.rb @@ -13,7 +13,7 @@ def send_log(log, msg) TestContext.new(definition) end - let(:definition) { { name: 'some_resource', attributes: { name: 'some_resource' }, features: feature_support } } + let(:definition) { { name: 'some_resource', attributes: { name: { type: 'String', desc: 'message' } }, features: feature_support } } let(:feature_support) { [] } it { expect { described_class.new(nil) }.to raise_error ArgumentError, %r{BaseContext requires definition to be a Hash} } diff --git a/spec/puppet/resource_api/io_context_spec.rb b/spec/puppet/resource_api/io_context_spec.rb index 41425d23..266095d0 100644 --- a/spec/puppet/resource_api/io_context_spec.rb +++ b/spec/puppet/resource_api/io_context_spec.rb @@ -4,7 +4,7 @@ RSpec.describe Puppet::ResourceApi::IOContext do subject(:context) { described_class.new(definition, io) } - let(:definition) { { name: 'some_resource' } } + let(:definition) { { name: 'some_resource', attributes: {} } } let(:io) { StringIO.new('', 'w') } diff --git a/spec/puppet/resource_api/puppet_context_spec.rb b/spec/puppet/resource_api/puppet_context_spec.rb index f64b0119..96a3683e 100644 --- a/spec/puppet/resource_api/puppet_context_spec.rb +++ b/spec/puppet/resource_api/puppet_context_spec.rb @@ -3,7 +3,7 @@ RSpec.describe Puppet::ResourceApi::PuppetContext do subject(:context) { described_class.new(definition) } - let(:definition) { { name: 'some_resource' } } + let(:definition) { { name: 'some_resource', attributes: {} } } describe '#device' do context 'when a NetworkDevice is configured' do diff --git a/spec/puppet/resource_api/type_definition_spec.rb b/spec/puppet/resource_api/type_definition_spec.rb index 592f02ea..a2df5d99 100644 --- a/spec/puppet/resource_api/type_definition_spec.rb +++ b/spec/puppet/resource_api/type_definition_spec.rb @@ -23,7 +23,7 @@ end let(:feature_support) { [] } - it { expect { described_class.new(nil) }.to raise_error Puppet::DevError, %r{TypeDefinition requires definition to be a Hash} } + it { expect { described_class.new(nil) }.to raise_error Puppet::DevError, %r{Type definition must be a Hash} } describe '.name' do it { expect(type.name).to eq 'some_resource' } @@ -31,17 +31,17 @@ describe '#ensurable?' do context 'when type is ensurable' do - let(:definition) { { attributes: { ensure: true } } } + let(:definition) { { name: 'some_resource', attributes: { ensure: { type: 'Enum[absent, present]' } } } } it { expect(type).to be_ensurable } it { expect(type.attributes).to be_key(:ensure) } end context 'when type is not ensurable' do - let(:definition) { { attributes: { string: 'something' } } } + let(:definition) { { name: 'some_resource', attributes: { name: { type: 'String' } } } } it { expect(type).not_to be_ensurable } - it { expect(type.attributes).to be_key(:string) } + it { expect(type.attributes).to be_key(:name) } end end @@ -61,10 +61,9 @@ describe '#attributes' do context 'when type has attributes' do - let(:definition) { { attributes: { string: 'test_string' } } } + let(:definition) { { name: 'some_resource', attributes: { wibble: { type: 'String' } } } } - it { expect(type.attributes).to be_key(:string) } - it { expect(type.attributes[:string]).to eq('test_string') } + it { expect(type.attributes).to be_key(:wibble) } end end @@ -102,7 +101,7 @@ end end - describe '#check_schemas' do + describe '#check_schema' do context 'when resource does not contain its namevar' do let(:resource) { { nom: 'some_resource', prop: 1, ensure: 'present' } } @@ -183,4 +182,83 @@ end end end + + describe '#validate_schema' do + context 'when the type definition does not have a name' do + let(:definition) { { attributes: 'some_string' } } + + it { expect { type }.to raise_error Puppet::DevError, %r{Type definition must have a name} } + end + + context 'when attributes is not a hash' do + let(:definition) { { name: 'some_resource', attributes: 'some_string' } } + + it { expect { type }.to raise_error Puppet::DevError, %r{`some_resource.attributes` must be a hash} } + end + + context 'when the schema contains title_patterns and it is not an array' do + let(:definition) { { name: 'some_resource', title_patterns: {}, attributes: {} } } + + it { expect { type }.to raise_error Puppet::DevError, %r{`:title_patterns` must be an array} } + end + + context 'when an attribute is not a hash' do + let(:definition) { { name: 'some_resource', attributes: { name: 'some_string' } } } + + it { expect { type }.to raise_error Puppet::DevError, %r{`some_resource.name` must be a Hash} } + end + + context 'when an attribute has no type' do + let(:definition) { { name: 'some_resource', attributes: { name: { desc: 'message' } } } } + + it { expect { type }.to raise_error Puppet::DevError, %r{has no type} } + end + + context 'when an attribute has no descrption' do + let(:definition) { { name: 'some_resource', attributes: { name: { type: 'String' } } } } + + it 'Raises a warning message' do + expect(Puppet).to receive(:warning).with('`some_resource.name` has no docs') + type + end + end + + context 'when an attribute has an unsupported type' do + let(:definition) { { name: 'some_resource', attributes: { name: { type: 'basic' } } } } + + it { expect { type }.to raise_error %r{ is not a valid type specification} } + end + + context 'with both behavior and behaviour' do + let(:definition) do + { + name: 'bad_behaviour', + attributes: { + name: { + type: 'String', + behaviour: :namevar, + behavior: :namevar, + }, + }, + } + end + + it { expect { type }.to raise_error Puppet::DevError, %r{name.*attribute has both} } + end + + context 'when registering a type with badly formed attribute type' do + let(:definition) do + { + name: 'bad_syntax', + attributes: { + name: { + type: 'Optional[String', + }, + }, + } + end + + it { expect { type }.to raise_error Puppet::DevError, %r{The type of the `name` attribute `Optional\[String` could not be parsed:} } + end + end end diff --git a/spec/puppet/resource_api_spec.rb b/spec/puppet/resource_api_spec.rb index f383e72d..83fd72c3 100644 --- a/spec/puppet/resource_api_spec.rb +++ b/spec/puppet/resource_api_spec.rb @@ -22,28 +22,6 @@ expect(Puppet::ResourceApi::VERSION).not_to be nil end - context 'when registering a definition with missing keys' do - it { expect { described_class.register_type([]) }.to raise_error(Puppet::DevError, %r{requires a hash as definition}) } - it { expect { described_class.register_type({}) }.to raise_error(Puppet::DevError, %r{requires a `:name`}) } - it { expect { described_class.register_type(name: 'no attributes') }.to raise_error(Puppet::DevError, %r{requires `:attributes`}) } - it { expect { described_class.register_type(name: 'no hash attributes', attributes: []) }.to raise_error(Puppet::DevError, %r{`:attributes` must be a hash, not}) } - it { - expect { - described_class.register_type(name: 'with a title', attributes: { title: {} }) - }.to raise_error(Puppet::DevError, %r{must not define an attribute called `:title`}) - } - it { - expect { - described_class.register_type(name: 'with a provider', attributes: { provider: {} }) - }.to raise_error(Puppet::DevError, %r{must not define an attribute called `:provider`}) - } - it { - expect { - described_class.register_type(name: 'with no hash title_patterns', attributes: {}, title_patterns: {}) - }.to raise_error(Puppet::DevError, %r{`:title_patterns` must be an array, not}) - } - end - context 'when registering a minimal type' do let(:definition) { { name: 'minimal', attributes: {} } } @@ -772,17 +750,21 @@ def set(_context, _changes); end attributes: { ensure: { type: 'Enum[present, absent]', + desc: '', }, name: { type: 'String', + desc: '', behavior: :namevar, }, something_init_only: { type: 'String', + desc: '', behaviour: :init_only, }, mutable: { type: 'String', + desc: '', }, }, } @@ -937,69 +919,6 @@ def set(_context, _changes); end end end - context 'when registering a type with a malformed attributes' do - context 'without a type' do - let(:definition) do - { - name: 'no_type', - attributes: { - name: { - behaviour: :namevar, - }, - }, - } - end - - it { expect { described_class.register_type(definition) }.to raise_error Puppet::DevError, %r{name.*has no type} } - end - - context 'with both behavior and behaviour' do - let(:definition) do - { - name: 'bad_behaviour', - attributes: { - name: { - behaviour: :namevar, - behavior: :namevar, - }, - }, - } - end - - it { expect { described_class.register_type(definition) }.to raise_error Puppet::DevError, %r{name.*attribute has both} } - end - end - - context 'when registering a type with badly formed attribute type' do - let(:definition) do - { - name: 'bad_syntax', - attributes: { - name: { - type: 'Optional[String', - }, - }, - } - end - - it { expect { described_class.register_type(definition) }.to raise_error Puppet::DevError, %r{The type of the `name` attribute `Optional\[String` could not be parsed:} } - end - - context 'when registering a type with unknown attribute type' do - let(:definition) do - { - name: 'wibble', - attributes: { - name: { - type: 'wibble', - }, - }, - } - end - - it { expect { described_class.register_type(definition) }.to raise_error Puppet::DevError, %r{The type of the `name` attribute `wibble` is not recognised:} } - end - context 'when registering a namevar that is not called `name`' do let(:definition) do { @@ -1306,10 +1225,12 @@ class OtherDevice; end attributes: { name: { type: 'String', + desc: '', behaviour: :namevar, }, test_string: { type: 'String', + desc: '', }, }, features: ['canonicalize'], @@ -1795,11 +1716,13 @@ def set(_context, changes) end end it 'passes through the an empty array to `get`' do + expect { described_class.register_type(definition) }.not_to raise_error expect(provider).to receive(:get).with(anything, []).and_return([]) type.instances end it 'passes through the resource title to `get`' do + expect { described_class.register_type(definition) }.not_to raise_error instance = type.new(name: 'bar', ensure: 'present') expect(provider).to receive(:get).with(anything, ['bar']).and_return([]) instance.retrieve diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 07171f75..a267e5cb 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -45,6 +45,11 @@ # override legacy default from puppetlabs_spec_helper config.mock_with :rspec + + # reset the warning suppression count + config.before(:each) do + Puppet::ResourceApi.warning_count = 0 + end end require 'puppetlabs_spec_helper/module_spec_helper'