Merge pull request #174 from DavidS/maint-fix-composite-simple_get_fi…

…lter

(MODULES-9428) make the composite namevar implementation usable

Rakefile

@@ -14,22 +14,30 @@ end
 require 'rspec/core/rake_task'
 
 RSpec::Core::RakeTask.new(:spec) do |t|
-  Rake::Task[:spec_prep].invoke
   # thanks to the fixtures/modules/ symlinks this needs to exclude fixture modules explicitely
   excludes = ['fixtures/**/*.rb,fixtures/modules/*/**/*.rb']
   if RUBY_PLATFORM == 'java'
     excludes += ['acceptance/**/*.rb', 'integration/**/*.rb', 'puppet/resource_api/*_context_spec.rb', 'puppet/util/network_device/simple/device_spec.rb']
     t.rspec_opts = '--tag ~agent_test'
+    t.rspec_opts << ' --tag ~j17_exclude' if Gem::Version.new(RUBY_VERSION) < Gem::Version.new('2.0.0')
   end
   t.exclude_pattern = "spec/{#{excludes.join ','}}"
 end
 
+task :spec => :spec_prep
+
 namespace :spec do
   desc 'Run RSpec code examples with coverage collection'
   task :coverage do
     ENV['SIMPLECOV'] = 'yes'
     Rake::Task['spec'].execute
   end
+
+  RSpec::Core::RakeTask.new(:unit) do |t|
+    t.pattern = "spec/puppet/**/*_spec.rb,spec/integration/**/*_spec.rb"
+  end
+
+  task :unit => :spec_prep
 end
 
 #### LICENSE_FINDER ####

lib/puppet/resource_api.rb

@@ -91,7 +91,7 @@ def type_definition
         apply_to_device
       end
 
-      define_method(:initialize) do |attributes|
+      def initialize(attributes)
         # $stderr.puts "A: #{attributes.inspect}"
         if attributes.is_a? Puppet::Resource
           @title = attributes.title
@@ -119,7 +119,7 @@ def type_definition
         # the `Puppet::Resource::Ral.find` method, when `instances` does not return a match, uses a Hash with a `:name` key to create
         # an "absent" resource. This is often hit by `puppet resource`. This needs to work, even if the namevar is not called `name`.
         # This bit here relies on the default `title_patterns` (see below) to match the title back to the first (and often only) namevar
-        if definition[:attributes][:name].nil? && attributes[:title].nil?
+        if type_definition.attributes[:name].nil? && attributes[:title].nil?
           attributes[:title] = attributes.delete(:name)
           if attributes[:title].nil? && !type_definition.namevars.empty?
             attributes[:title] = @title
@@ -133,11 +133,25 @@ def name
         title
       end
 
+      def self.build_title(type_definition, resource_hash)
+        if type_definition.namevars.size > 1
+          # use a MonkeyHash to allow searching in Puppet's RAL
+          Puppet::ResourceApi::MonkeyHash[type_definition.namevars.map { |attr| [attr, resource_hash[attr]] }]
+        else
+          resource_hash[type_definition.namevars[0]]
+        end
+      end
+
+      def rsapi_title
+        @rsapi_title ||= self.class.build_title(type_definition, self)
+        @rsapi_title
+      end
+
       def to_resource
         to_resource_shim(super)
       end
 
-      define_method(:to_resource_shim) do |resource|
+      def to_resource_shim(resource)
         resource_hash = Hash[resource.keys.map { |k| [k, resource[k]] }]
         resource_hash[:title] = resource.title
         ResourceShim.new(resource_hash, type_definition.name, type_definition.namevars, type_definition.attributes, catalog)
@@ -244,7 +258,7 @@ def to_resource
         end
       end
 
-      define_singleton_method(:instances) do
+      def self.instances
         # puts 'instances'
         # force autoloading of the provider
         provider(type_definition.name)
@@ -261,16 +275,16 @@ def to_resource
           result = if resource_hash.key? :title
                      new(title: resource_hash[:title])
                    else
-                     new(title: resource_hash[type_definition.namevars.first])
+                     new(title: build_title(type_definition, resource_hash))
                    end
           result.cache_current_state(resource_hash)
           result
         end
       end
 
-      define_method(:refresh_current_state) do
+      def refresh_current_state
         @rsapi_current_state = if type_definition.feature?('simple_get_filter')
-                                 my_provider.get(context, [title]).find { |h| namevar_match?(h) }
+                                 my_provider.get(context, [rsapi_title]).find { |h| namevar_match?(h) }
                                else
                                  my_provider.get(context).find { |h| namevar_match?(h) }
                                end
@@ -279,7 +293,11 @@ def to_resource
           type_definition.check_schema(@rsapi_current_state)
           strict_check(@rsapi_current_state) if type_definition.feature?('canonicalize')
         else
-          @rsapi_current_state = { title: title }
+          @rsapi_current_state = if rsapi_title.is_a? Hash
+                                   rsapi_title.dup
+                                 else
+                                   { title: rsapi_title }
+                                 end
           @rsapi_current_state[:ensure] = :absent if type_definition.ensurable?
         end
       end
@@ -290,7 +308,7 @@ def cache_current_state(resource_hash)
         strict_check(@rsapi_current_state) if type_definition.feature?('canonicalize')
       end
 
-      define_method(:retrieve) do
+      def retrieve
         refresh_current_state unless @rsapi_current_state
 
         Puppet.debug("Current State: #{@rsapi_current_state.inspect}")
@@ -304,13 +322,13 @@ def cache_current_state(resource_hash)
         result
       end
 
-      define_method(:namevar_match?) do |item|
+      def namevar_match?(item)
         context.type.namevars.all? do |namevar|
           item[namevar] == @parameters[namevar].value if @parameters[namevar].respond_to? :value
         end
       end
 
-      define_method(:flush) do
+      def flush
         raise_missing_attrs
 
         # puts 'flush'
@@ -328,7 +346,7 @@ def cache_current_state(resource_hash)
         # enforce init_only attributes
         if Puppet.settings[:strict] != :off && @rsapi_current_state && (@rsapi_current_state[:ensure] == 'present' && target_state[:ensure] == 'present')
           target_state.each do |name, value|
-            next unless definition[:attributes][name][:behaviour] == :init_only && value != @rsapi_current_state[name]
+            next unless type_definition.attributes[name][:behaviour] == :init_only && value != @rsapi_current_state[name]
             message = "Attempting to change `#{name}` init_only attribute value from `#{@rsapi_current_state[name]}` to `#{value}`"
             case Puppet.settings[:strict]
             when :warning
@@ -340,27 +358,27 @@ def cache_current_state(resource_hash)
         end
 
         if type_definition.feature?('supports_noop')
-          my_provider.set(context, { title => { is: @rsapi_current_state, should: target_state } }, noop: noop?)
+          my_provider.set(context, { rsapi_title => { is: @rsapi_current_state, should: target_state } }, noop: noop?)
         else
-          my_provider.set(context, title => { is: @rsapi_current_state, should: target_state }) unless noop?
+          my_provider.set(context, rsapi_title => { is: @rsapi_current_state, should: target_state }) unless noop?
         end
         raise 'Execution encountered an error' if context.failed?
 
         # remember that we have successfully reached our desired state
         @rsapi_current_state = target_state
       end
 
-      define_method(:raise_missing_attrs) do
+      def raise_missing_attrs
         error_msg = "The following mandatory attributes were not provided:\n    *  " + @missing_attrs.join(", \n    *  ")
         raise Puppet::ResourceError, error_msg if @missing_attrs.any? && (value(:ensure) != :absent && !value(:ensure).nil?)
       end
 
-      define_method(:raise_missing_params) do
+      def raise_missing_params
         error_msg = "The following mandatory parameters were not provided:\n    *  " + @missing_params.join(", \n    *  ")
         raise Puppet::ResourceError, error_msg
       end
 
-      define_method(:strict_check) do |current_state|
+      def strict_check(current_state)
         return if Puppet.settings[:strict] == :off
 
         # if strict checking is on we must notify if the values are changed by canonicalize
@@ -374,7 +392,7 @@ def cache_current_state(resource_hash)
         #:nocov:
         # codecov fails to register this multiline as covered, even though simplecov does.
         message = <<MESSAGE.strip
-#{definition[:name]}[#{@title}]#get has not provided canonicalized values.
+#{type_definition.name}[#{@title}]#get has not provided canonicalized values.
 Returned values:       #{current_state.inspect}
 Canonicalized values:  #{state_clone.inspect}
 MESSAGE
@@ -387,7 +405,7 @@ def cache_current_state(resource_hash)
           raise Puppet::DevError, message
         end
 
-        return nil
+        nil
       end
 
       define_singleton_method(:context) do
@@ -398,9 +416,9 @@ def context
         self.class.context
       end
 
-      define_singleton_method(:title_patterns) do
-        @title_patterns ||= if definition.key? :title_patterns
-                              parse_title_patterns(definition[:title_patterns])
+      def self.title_patterns
+        @title_patterns ||= if type_definition.definition.key? :title_patterns
+                              parse_title_patterns(type_definition.definition[:title_patterns])
                             else
                               [[%r{(.*)}m, [[type_definition.namevars.first]]]]
                             end

lib/puppet/resource_api/glue.rb

@@ -57,4 +57,19 @@ def filtered_keys
       values.keys.reject { |k| k == :title || !attr_def[k] || (attr_def[k][:behaviour] == :namevar && @namevars.size == 1) }
     end
   end
+
+  # this hash allows key-value based ordering comparisons between instances of this and instances of this and other classes
+  # this is required for `lib/puppet/indirector/resource/ral.rb`'s `search` method which expects all titles to be comparable
+  class MonkeyHash < Hash
+    def <=>(other)
+      result = self.class.name <=> other.class.name
+      if result.zero?
+        result = keys.sort <=> other.keys.sort
+      end
+      if result.zero?
+        result = keys.sort.map { |k| self[k] } <=> other.keys.sort.map { |k| other[k] }
+      end
+      result
+    end
+  end
 end

lib/puppet/resource_api/simple_provider.rb

@@ -1,4 +1,5 @@
 module Puppet; end # rubocop:disable Style/Documentation
+
 module Puppet::ResourceApi
   # This class provides a default implementation for set(), when your resource does not benefit from batching.
   # Instead of processing changes yourself, the `create`, `update`, and `delete` functions, are called for you,
@@ -19,12 +20,12 @@ def set(context, changes)
 
         raise 'SimpleProvider cannot be used with a Type that is not ensurable' unless context.type.ensurable?
 
-        is = { name: name, ensure: 'absent' } if is.nil?
-        should = { name: name, ensure: 'absent' } if should.nil?
+        is = SimpleProvider.create_absent(:name, name) if is.nil?
+        should = SimpleProvider.create_absent(:name, name) if should.nil?
 
         name_hash = if context.type.namevars.length > 1
                       # pass a name_hash containing the values of all namevars
-                      name_hash = { title: name }
+                      name_hash = {}
                       context.type.namevars.each do |namevar|
                         name_hash[namevar] = change[:should][namevar]
                       end
@@ -60,5 +61,16 @@ def update(_context, _name, _should)
     def delete(_context, _name)
       raise "#{self.class} has not implemented `delete`"
     end
+
+    # @api private
+    def self.create_absent(namevar, title)
+      result = if title.is_a? Hash
+                 title.dup
+               else
+                 { namevar => title }
+               end
+      result[:ensure] = 'absent'
+      result
+    end
   end
 end