Merge pull request #142 from da-ar/validate_schema

(maint) Validate Type Schema

.rspec

@@ -1,3 +1,3 @@
 --format documentation
 --color
---order rand:123
+--order rand

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

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,

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} }

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') }
 

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

spec/puppet/resource_api/type_definition_spec.rb

@@ -23,25 +23,25 @@
   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' }
   end
 
   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{<basic> 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