Clean up match code to get rid of TODO

5 files changed, 38 insertions, 69 deletions

diff --git a/lib/chef/node_map.rb b/lib/chef/node_map.rb
index 4e07f93593..bfa884d6d7 100644
--- a/lib/chef/node_map.rb
+++ b/lib/chef/node_map.rb
@@ -34,11 +34,11 @@ class Chef
     # @yield [node] Arbitrary node filter as a block which takes a node argument
     # @return [NodeMap] Returns self for possible chaining
     #
-    def set(key, value, platform: nil, platform_family: nil, os: nil, on_platform: nil, on_platforms: nil, &block)
+    def set(key, value, platform: nil, platform_version: nil, platform_family: nil, os: nil, on_platform: nil, on_platforms: nil, &block)
       Chef::Log.deprecation "The on_platform option to node_map has been deprecated" if on_platform
       Chef::Log.deprecation "The on_platforms option to node_map has been deprecated" if on_platforms
       platform ||= on_platform || on_platforms
-      filters = { platform: platform, platform_family: platform_family, os: os }
+      filters = { platform: platform, platform_version: platform_version, platform_family: platform_family, os: os }
       new_matcher = { filters: filters, block: block, value: value }
       @map[key] ||= []
       # Decide where to insert the matcher; the new value is preferred over
@@ -110,47 +110,32 @@ class Chef
       specificity
     end
 
-    # @todo: this works fine, but is probably hard to understand
-    def negative_match(filter, param)
-      # We support strings prefaced by '!' to mean 'not'.  In particular, this is most useful
-      # for os matching on '!windows'.
-      negative_matches = filter.select { |f| f[0] == '!' }
-      return true if !negative_matches.empty? && negative_matches.include?('!' + param)
+    #
+    # Succeeds if:
+    # - no negative matches (!value)
+    # - at least one positive match (value or :all), or no positive filters
+    #
+    def matches_black_white_list?(node, filters, attribute)
+      # It's super common for the filter to be nil.  Catch that so we don't
+      # spend any time here.
+      return true if !filters[attribute]
+      filter_values = Array(filters[attribute])
+      value = node[attribute]
+
+      # Split the blacklist and whitelist
+      blacklist, whitelist = filter_values.partition { |v| v.is_a?(String) && v.start_with?('!') }
 
-      # We support the symbol :all to match everything, for backcompat, but this can and should
-      # simply be ommitted.
-      positive_matches = filter.reject { |f| f[0] == '!' || f == :all }
-      return true if !positive_matches.empty? && !positive_matches.include?(param)
+      # If any blacklist value matches, we don't match
+      return false if blacklist.any? { |v| v[1..-1] == value }
 
-      # sorry double-negative: this means we pass this filter.
-      false
+      # If the whitelist is empty, or anything matches, we match.
+      whitelist.empty? || whitelist.any? { |v| v == :all || v == value }
     end
 
     def filters_match?(node, filters)
-      return true if filters.empty?
-
-      # each filter is applied in turn.  if any fail, then it shortcuts and returns false.
-      # if it passes or does not exist it succeeds and continues on.  so multiple filters are
-      # effectively joined by 'and'.  all filters can be single strings, or arrays which are
-      # effectively joined by 'or'.
-
-      os_filter = [ filters[:os] ].flatten.compact
-      unless os_filter.empty?
-        return false if negative_match(os_filter, node[:os])
-      end
-
-      platform_family_filter = [ filters[:platform_family] ].flatten.compact
-      unless platform_family_filter.empty?
-        return false if negative_match(platform_family_filter, node[:platform_family])
-      end
-
-      # :on_platform and :on_platforms here are synonyms which are deprecated
-      platform_filter = [ filters[:platform] || filters[:on_platform] || filters[:on_platforms] ].flatten.compact
-      unless platform_filter.empty?
-        return false if negative_match(platform_filter, node[:platform])
-      end
-
-      return true
+      matches_black_white_list?(node, filters, :os) &&
+      matches_black_white_list?(node, filters, :platform_family) &&
+      matches_black_white_list?(node, filters, :platform)
     end
 
     def block_matches?(node, block)
diff --git a/spec/unit/node_map_spec.rb b/spec/unit/node_map_spec.rb
index fe7372961b..9b5ff5e8c6 100644
--- a/spec/unit/node_map_spec.rb
+++ b/spec/unit/node_map_spec.rb
@@ -134,6 +134,10 @@ describe Chef::NodeMap do
   end
 
   describe "resource back-compat testing" do
+    before :each do
+      Chef::Config[:treat_deprecation_warnings_as_errors] = false
+    end
+
     it "should handle :on_platforms => :all" do
       node_map.set(:chef_gem, :foo, :on_platforms => :all)
       allow(node).to receive(:[]).with(:platform).and_return("windows")
@@ -152,4 +156,3 @@ describe Chef::NodeMap do
   end
 
 end
-
diff --git a/spec/unit/platform_spec.rb b/spec/unit/platform_spec.rb
index 0a4e9655d0..36325d5411 100644
--- a/spec/unit/platform_spec.rb
+++ b/spec/unit/platform_spec.rb
@@ -18,29 +18,6 @@
 
 require 'spec_helper'
 
-describe "Chef::Platform supports" do
-  [
-    :freebsd,
-    :ubuntu,
-    :debian,
-    :centos,
-    :fedora,
-    :suse,
-    :opensuse,
-    :redhat,
-    :oracle,
-    :gentoo,
-    :arch,
-    :solaris,
-    :gcel,
-    :ibm_powerkvm
-  ].each do |platform|
-    it "#{platform}" do
-      expect(Chef::Platform.platforms).to have_key(platform)
-    end
-  end
-end
-
 describe Chef::Platform do
 
   context "while testing with fake data" do
diff --git a/spec/unit/recipe_spec.rb b/spec/unit/recipe_spec.rb
index a4c0af882f..618c742998 100644
--- a/spec/unit/recipe_spec.rb
+++ b/spec/unit/recipe_spec.rb
@@ -122,7 +122,7 @@ describe Chef::Recipe do
         it "locate resource for particular platform" do
           ShaunTheSheep = Class.new(Chef::Resource)
           ShaunTheSheep.use_automatic_resource_name
-          ShaunTheSheep.provides :laughter, :on_platforms => ["television"]
+          ShaunTheSheep.provides :laughter, :platform => ["television"]
           node.automatic[:platform] = "television"
           node.automatic[:platform_version] = "123"
           res = recipe.laughter "timmy"
@@ -144,10 +144,8 @@ describe Chef::Recipe do
             node.automatic[:platform] = "nbc_sports"
             Sounders = Class.new(Chef::Resource)
             Sounders.use_automatic_resource_name
-            Sounders.provides :football, platform: "nbc_sports"
             TottenhamHotspur = Class.new(Chef::Resource)
             TottenhamHotspur.use_automatic_resource_name
-            TottenhamHotspur.provides :football, platform: "nbc_sports"
           end
 
           after do
@@ -155,9 +153,12 @@ describe Chef::Recipe do
             Object.send(:remove_const, :TottenhamHotspur)
           end
 
-          it "selects one if it is given priority" do
+          it "selects one if it is the last declared" do
             expect(Chef::Log).not_to receive(:warn)
-            Chef::Platform::ResourcePriorityMap.instance.send(:priority, :football, TottenhamHotspur, platform: "nbc_sports")
+
+            Sounders.provides :football, platform: "nbc_sports"
+            TottenhamHotspur.provides :football, platform: "nbc_sports"
+
             res1 = recipe.football "club world cup"
             expect(res1.name).to eql("club world cup")
             expect(res1).to be_a_kind_of(TottenhamHotspur)
@@ -165,7 +166,10 @@ describe Chef::Recipe do
 
           it "selects the other one if it is given priority" do
             expect(Chef::Log).not_to receive(:warn)
-            Chef::Platform::ResourcePriorityMap.instance.send(:priority, :football, Sounders, platform: "nbc_sports")
+
+            TottenhamHotspur.provides :football, platform: "nbc_sports"
+            Sounders.provides :football, platform: "nbc_sports"
+
             res1 = recipe.football "club world cup"
             expect(res1.name).to eql("club world cup")
             expect(res1).to be_a_kind_of(Sounders)
diff --git a/spec/unit/resource_spec.rb b/spec/unit/resource_spec.rb
index 95de09f5ce..1d20fcf604 100644
--- a/spec/unit/resource_spec.rb
+++ b/spec/unit/resource_spec.rb
@@ -864,7 +864,7 @@ describe Chef::Resource do
         @node.name("bumblebee")
         @node.automatic[:platform] = "autobots"
         @node.automatic[:platform_version] = "6.1"
-        klz2.provides :dinobot, :on_platforms => ['autobots']
+        klz2.provides :dinobot, :platform => ['autobots']
         Object.const_set('Grimlock', klz2)
         klz2.provides :grimlock
       end