Merge pull request #6637 from chef/lcg/cleanup-node-map

simplify node_map logic

1 files changed, 33 insertions, 32 deletions

diff --git a/lib/chef/node_map.rb b/lib/chef/node_map.rb
index 0d7d986653..dde93a437d 100644
--- a/lib/chef/node_map.rb
+++ b/lib/chef/node_map.rb
@@ -20,21 +20,21 @@
 # example of a NodeMap entry for the user resource (as typed on the DSL):
 #
 #  :user=>
-#  [{:value=>Chef::Resource::User::AixUser, :filters=>{:os=>"aix"}},
-#   {:value=>Chef::Resource::User::DsclUser, :filters=>{:os=>"darwin"}},
-#   {:value=>Chef::Resource::User::PwUser, :filters=>{:os=>"freebsd"}},
-#   {:value=>Chef::Resource::User::LinuxUser, :filters=>{:os=>"linux"}},
-#   {:value=>Chef::Resource::User::SolarisUser,
-#    :filters=>{:os=>["omnios", "solaris2"]}},
-#   {:value=>Chef::Resource::User::WindowsUser, :filters=>{:os=>"windows"}}],
-#
-# if a block filter were to appear it would be a :block argument (XXX: turn into a filter?)
+#  [{:klass=>Chef::Resource::User::AixUser, :os=>"aix"},
+#   {:klass=>Chef::Resource::User::DsclUser, :os=>"darwin"},
+#   {:klass=>Chef::Resource::User::PwUser, :os=>"freebsd"},
+#   {:klass=>Chef::Resource::User::LinuxUser, :os=>"linux"},
+#   {:klass=>Chef::Resource::User::SolarisUser,
+#    :os=>["omnios", "solaris2"]},
+#   {:klass=>Chef::Resource::User::WindowsUser, :os=>"windows"}],
 #
 # the entries in the array are pre-sorted into priority order (blocks/platform_version/platform/platform_family/os/none) so that
-# the first entry's :value that matches the filter is returned when doing a get.
+# the first entry's :klass that matches the filter is returned when doing a get.
 #
 # note that as this examples show filter values may be a scalar string or an array of scalar strings.
 #
+# XXX: confusingly, in the *_priority_map the :klass may be an array of Strings of class names
+#
 class Chef
   class NodeMap
 
@@ -50,13 +50,12 @@ class Chef
     #
     # @return [NodeMap] Returns self for possible chaining
     #
-    def set(key, value, platform: nil, platform_version: nil, platform_family: nil, os: nil, canonical: nil, override: nil, &block)
-      filters = {}
-      filters[:platform] = platform if platform
-      filters[:platform_version] = platform_version if platform_version
-      filters[:platform_family] = platform_family if platform_family
-      filters[:os] = os if os
-      new_matcher = { value: value, filters: filters }
+    def set(key, klass, platform: nil, platform_version: nil, platform_family: nil, os: nil, canonical: nil, override: nil, &block)
+      new_matcher = { klass: klass }
+      new_matcher[:platform] = platform if platform
+      new_matcher[:platform_version] = platform_version if platform_version
+      new_matcher[:platform_family] = platform_family if platform_family
+      new_matcher[:os] = os if os
       new_matcher[:block] = block if block
       new_matcher[:canonical] = canonical if canonical
       new_matcher[:override] = override if override
@@ -90,13 +89,12 @@ class Chef
     # @param canonical [Boolean] `true` or `false` to match canonical or
     #   non-canonical values only. `nil` to ignore canonicality.  Default: `nil`
     #
-    # @return [Object] Value
+    # @return [Object] Class
     #
     def get(node, key, canonical: nil)
-      raise ArgumentError, "first argument must be a Chef::Node" unless node.is_a?(Chef::Node) || node.nil?
       return nil unless map.has_key?(key)
       map[key].map do |matcher|
-        return matcher[:value] if node_matches?(node, matcher) && canonical_matches?(canonical, matcher)
+        return matcher[:klass] if node_matches?(node, matcher) && canonical_matches?(canonical, matcher)
       end
       nil
     end
@@ -111,23 +109,22 @@ class Chef
     # @param canonical [Boolean] `true` or `false` to match canonical or
     #   non-canonical values only. `nil` to ignore canonicality.  Default: `nil`
     #
-    # @return [Object] Value
+    # @return [Object] Class
     #
     def list(node, key, canonical: nil)
-      raise ArgumentError, "first argument must be a Chef::Node" unless node.is_a?(Chef::Node) || node.nil?
       return [] unless map.has_key?(key)
       map[key].select do |matcher|
         node_matches?(node, matcher) && canonical_matches?(canonical, matcher)
-      end.map { |matcher| matcher[:value] }
+      end.map { |matcher| matcher[:klass] }
     end
 
     # Seriously, don't use this, it's nearly certain to change on you
     # @return remaining
     # @api private
-    def delete_canonical(key, value)
+    def delete_canonical(key, klass)
       remaining = map[key]
       if remaining
-        remaining.delete_if { |matcher| matcher[:canonical] && Array(matcher[:value]) == Array(value) }
+        remaining.delete_if { |matcher| matcher[:canonical] && Array(matcher[:klass]) == Array(klass) }
         if remaining.empty?
           map.delete(key)
           remaining = nil
@@ -187,7 +184,7 @@ class Chef
 
     def node_matches?(node, matcher)
       return true if !node
-      filters_match?(node, matcher[:filters]) && block_matches?(node, matcher[:block])
+      filters_match?(node, matcher) && block_matches?(node, matcher[:block])
     end
 
     def canonical_matches?(canonical, matcher)
@@ -199,13 +196,13 @@ class Chef
     def dispatch_compare_matchers(key, new_matcher, matcher)
       cmp = compare_matcher_properties(new_matcher[:block], matcher[:block])
       return cmp if cmp != 0
-      cmp = compare_matcher_properties(new_matcher[:filters][:platform_version], matcher[:filters][:platform_version])
+      cmp = compare_matcher_properties(new_matcher[:platform_version], matcher[:platform_version])
       return cmp if cmp != 0
-      cmp = compare_matcher_properties(new_matcher[:filters][:platform], matcher[:filters][:platform])
+      cmp = compare_matcher_properties(new_matcher[:platform], matcher[:platform])
       return cmp if cmp != 0
-      cmp = compare_matcher_properties(new_matcher[:filters][:platform_family], matcher[:filters][:platform_family])
+      cmp = compare_matcher_properties(new_matcher[:platform_family], matcher[:platform_family])
       return cmp if cmp != 0
-      cmp = compare_matcher_properties(new_matcher[:filters][:os], matcher[:filters][:os])
+      cmp = compare_matcher_properties(new_matcher[:os], matcher[:os])
       return cmp if cmp != 0
       cmp = compare_matcher_properties(new_matcher[:override], matcher[:override])
       return cmp if cmp != 0
@@ -222,8 +219,12 @@ class Chef
         # Sort by class name (ascending) as well, if all other properties
         # are exactly equal
         # XXX: remove this in Chef-14 and use last-writer-wins (prepend if they match)
-        if new_matcher[:value].is_a?(Class) && !new_matcher[:override]
-          cmp = compare_matcher_properties(new_matcher[:value].name, matcher[:value].name)
+        if !new_matcher[:override]
+          # we only sort classes, which only sorts the handler array, this magically does not sort
+          # the priority array via the invisible else here.
+          if new_matcher[:klass].is_a?(Class)
+            cmp = compare_matcher_properties(new_matcher[:klass].name, matcher[:klass].name)
+          end
         end
       end
       cmp