Add title consistency checks for multi-namevar providers

This ensures results returned from a multi-namevar provider
meet the following consistency checks:

* There must be a title attribute returned
* The title value must match one of the title patterns
* The values parsed from the title pattern match must match the
  values returned from the provider for the respective namevars.

Author: seanmil

lib/puppet/resource_api.rb

@@ -261,7 +261,7 @@ def refresh_current_state
 
         if @rsapi_current_state
           type_definition.check_schema(@rsapi_current_state)
-          strict_check(@rsapi_current_state) if type_definition.feature?('canonicalize')
+          strict_check(@rsapi_current_state)
         else
           @rsapi_current_state = if rsapi_title.is_a? Hash
                                    rsapi_title.dup
@@ -275,7 +275,7 @@ def refresh_current_state
       # Use this to set the current state from the `instances` method
       def cache_current_state(resource_hash)
         @rsapi_current_state = resource_hash
-        strict_check(@rsapi_current_state) if type_definition.feature?('canonicalize')
+        strict_check(@rsapi_current_state)
       end
 
       def retrieve
@@ -354,6 +354,22 @@ def raise_missing_params
       def strict_check(current_state)
         return if Puppet.settings[:strict] == :off
 
+        strict_check_canonicalize(current_state) if type_definition.feature?('canonicalize')
+        strict_check_title_parameter(current_state) if type_definition.namevars.size > 1
+
+        nil
+      end
+
+      def strict_message(message)
+        case Puppet.settings[:strict]
+        when :warning
+          Puppet.warning(message)
+        when :error
+          raise Puppet::DevError, message
+        end
+      end
+
+      def strict_check_canonicalize(current_state)
         # if strict checking is on we must notify if the values are changed by canonicalize
         # make a deep copy to perform the operation on and to compare against later
         state_clone = Marshal.load(Marshal.dump(current_state))
@@ -370,15 +386,43 @@ def strict_check(current_state)
 Canonicalized values:  #{state_clone.inspect}
 MESSAGE
         #:nocov:
+        strict_message(message)
+      end
 
-        case Puppet.settings[:strict]
-        when :warning
-          Puppet.warning(message)
-        when :error
-          raise Puppet::DevError, message
+      def strict_check_title_parameter(current_state)
+        unless current_state.key?(:title)
+          strict_message("#{type_definition.name}[#{@title}]#get has not provided a title attribute.")
+          return
         end
 
-        nil
+        # Logic borrowed from Puppet::Resource.parse_title
+        title_hash = {}
+        self.class.title_patterns.each do |regexp, symbols|
+          captures = regexp.match(current_state[:title])
+          next if captures.nil?
+          symbols.zip(captures[1..-1]).each do |symbol_and_lambda, capture|
+            # The Resource API does not support passing procs in title_patterns
+            # so, unlike Puppet::Resource, we do not need to handle that here.
+            symbol = symbol_and_lambda[0]
+            title_hash[symbol] = capture
+          end
+          break
+        end
+
+        return if title_hash == rsapi_title
+
+        namevars = type_definition.namevars.reject { |namevar| title_hash[namevar] == rsapi_title[namevar] }
+
+        #:nocov:
+        # codecov fails to register this multiline as covered, even though simplecov does.
+        message = <<MESSAGE.strip
+#{type_definition.name}[#{@title}]#get has provided a title attribute which does not match all namevars.
+Namevars which do not match: #{namevars.inspect}
+Returned parsed title hash:  #{title_hash.inspect}
+Expected hash:               #{rsapi_title.inspect}
+MESSAGE
+        #:nocov:
+        strict_message(message)
       end
 
       define_singleton_method(:context) do

spec/fixtures/test_module/lib/puppet/type/multiple_namevar.rb

@@ -5,6 +5,16 @@
   docs: <<-EOS,
     This type provides Puppet with the capabilities to manage ...
   EOS
+  title_patterns: [
+    {
+      pattern: %r{^(?<package>.*[^-])-(?<manager>.*)$},
+      desc: 'Package and manager with a hyphen seperator',
+    },
+    {
+      pattern: %r{^(?<package>.*)$},
+      desc: 'Package',
+    },
+  ],
   attributes:   {
     ensure:      {
       type:    'Enum[present, absent]',

spec/puppet/resource_api_spec.rb

@@ -1026,6 +1026,7 @@ def set(_context, changes) end
     let(:definition) do
       {
         name: 'composite',
+        desc: 'some desc',
         title_patterns: [
           {
             pattern: %r{^(?<package>.*[^/])/(?<manager>.*)$},
@@ -1039,13 +1040,16 @@ def set(_context, changes) end
         attributes: {
           ensure: {
             type: 'Enum[present, absent]',
+            desc: '',
           },
           package: {
             type: 'String',
+            desc: '',
             behavior: :namevar,
           },
           manager: {
             type: 'String',
+            desc: '',
             behavior: :namevar,
           },
         },
@@ -1123,6 +1127,120 @@ def set(_context, _changes); end
           type_class.new(title: 'php/yum', ensure: :absent).flush
         end
       end
+
+      context 'when no title is returned by get' do
+        let(:provider_class) do
+          Class.new do
+            def get(_context)
+              [{ package: 'php', manager: 'yum', ensure: 'present' }]
+            end
+
+            def set(_context, _changes); end
+          end
+        end
+        let(:type) { type_class.new(title: 'mytitle', package: 'php', manager: 'yum') }
+
+        context 'when Puppet strict setting is :off' do
+          let(:strict_level) { :off }
+
+          it 'instances will not log a warning' do
+            expect(Puppet).not_to receive(:warning)
+            type_class.instances
+          end
+
+          it 'refresh_current_state will not log a warning' do
+            expect(Puppet).not_to receive(:warning)
+            type.refresh_current_state
+          end
+        end
+
+        context 'when Puppet strict setting is :error' do
+          let(:strict_level) { :error }
+
+          it 'instances will throw an exception' do
+            expect {
+              type_class.instances
+            }.to raise_error(Puppet::DevError, %r{has not provided a title attribute})
+          end
+
+          it 'refresh_current_state will throw an exception' do
+            expect {
+              type.refresh_current_state
+            }.to raise_error(Puppet::DevError, %r{has not provided a title attribute})
+          end
+        end
+
+        context 'when Puppet strict setting is :warning' do
+          let(:strict_level) { :warning }
+
+          it 'instances will not log a warning' do
+            expect(Puppet).to receive(:warning).with(%r{has not provided a title attribute})
+            type_class.instances
+          end
+
+          it 'refresh_current_state will not log a warning' do
+            expect(Puppet).to receive(:warning).with(%r{has not provided a title attribute})
+            type.refresh_current_state
+          end
+        end
+      end
+
+      context 'when the title does not match a title pattern' do
+        let(:provider_class) do
+          Class.new do
+            def get(_context)
+              [{ title: 'Nomatch', package: 'php', manager: 'yum', ensure: 'present' }]
+            end
+
+            def set(_context, _changes); end
+          end
+        end
+        let(:type) { type_class.new(title: 'mytitle', package: 'php', manager: 'yum') }
+
+        context 'when Puppet strict setting is :off' do
+          let(:strict_level) { :off }
+
+          it 'instances will not log a warning' do
+            expect(Puppet).not_to receive(:warning)
+            type_class.instances
+          end
+
+          it 'refresh_current_state will not log a warning' do
+            expect(Puppet).not_to receive(:warning)
+            type.refresh_current_state
+          end
+        end
+
+        context 'when Puppet strict setting is :error' do
+          let(:strict_level) { :error }
+
+          it 'instances will throw an exception' do
+            expect {
+              type_class.instances
+            }.to raise_error(Puppet::DevError, %r{has provided a title attribute which does not match})
+          end
+
+          it 'refresh_current_state will throw an exception' do
+            expect {
+              type.refresh_current_state
+            }.to raise_error(Puppet::DevError, %r{has provided a title attribute which does not match})
+          end
+        end
+
+        context 'when Puppet strict setting is :warning' do
+          let(:strict_level) { :warning }
+
+          it 'instances will not log a warning' do
+            expect(Puppet).to receive(:warning).with(%r{has provided a title attribute which does not match})
+            type_class.instances
+          end
+
+          it 'refresh_current_state will not log a warning' do
+            expect(Puppet).to receive(:warning).with(%r{has provided a title attribute which does not match})
+            type.refresh_current_state
+          end
+        end
+      end
     end
   end