Merge pull request #2477 from opscode/sersut/port-immut-merge

Merge pull request #2462 from opscode/lcg/remove-knockout-merge

8 files changed, 109 insertions, 94 deletions

diff --git a/lib/chef/mixin/deep_merge.rb b/lib/chef/mixin/deep_merge.rb
index a8a4737758..825406a93e 100644
--- a/lib/chef/mixin/deep_merge.rb
+++ b/lib/chef/mixin/deep_merge.rb
@@ -27,17 +27,6 @@ class Chef
     #   http://trac.misuse.org/science/wiki/DeepMerge
     module DeepMerge
 
-      class InvalidSubtractiveMerge < ArgumentError; end
-
-
-      OLD_KNOCKOUT_PREFIX = "!merge:".freeze
-
-      # Regex to match the "knockout prefix" that was used to indicate
-      # subtractive merging in Chef 10.x and previous. Subtractive merging is
-      # removed as of Chef 11, but we detect attempted use of it and raise an
-      # error (see: raise_if_knockout_used!)
-      OLD_KNOCKOUT_MATCH = %r[!merge].freeze
-
       extend self
 
       def merge(first, second)
@@ -47,15 +36,6 @@ class Chef
         DeepMerge.deep_merge(second, first)
       end
 
-      # Inherited roles use the knockout_prefix array subtraction functionality
-      # This is likely to go away in Chef >= 0.11
-      def role_merge(first, second)
-        first  = Mash.new(first)  unless first.kind_of?(Mash)
-        second = Mash.new(second) unless second.kind_of?(Mash)
-
-        DeepMerge.deep_merge(second, first)
-      end
-
       class InvalidParameter < StandardError; end
 
       # Deep Merge core documentation.
@@ -78,8 +58,6 @@ class Chef
           dest = source; return dest
         end
 
-        raise_if_knockout_used!(source)
-        raise_if_knockout_used!(dest)
         case source
         when nil
           dest
@@ -89,7 +67,6 @@ class Chef
               if dest[src_key]
                 dest[src_key] = deep_merge!(src_value, dest[src_key])
               else # dest[src_key] doesn't exist so we take whatever source has
-                raise_if_knockout_used!(src_value)
                 dest[src_key] = src_value
               end
             end
@@ -128,11 +105,19 @@ class Chef
         # If there are two Hashes, recursively merge.
         if merge_onto.kind_of?(Hash) && merge_with.kind_of?(Hash)
           merge_with.each do |key, merge_with_value|
-            merge_onto[key] = if merge_onto.has_key?(key)
-                                hash_only_merge(merge_onto[key], merge_with_value)
-                              else
-                                merge_with_value
-                              end
+            value =
+              if merge_onto.has_key?(key)
+                hash_only_merge(merge_onto[key], merge_with_value)
+              else
+                merge_with_value
+              end
+
+            if merge_onto.respond_to?(:public_method_that_only_deep_merge_should_use)
+              # we can't call ImmutableMash#[]= because its immutable, but we need to mutate it to build it in-place
+              merge_onto.public_method_that_only_deep_merge_should_use(key, value)
+            else
+              merge_onto[key] = value
+            end
           end
           merge_onto
 
@@ -146,27 +131,6 @@ class Chef
         end
       end
 
-      # Checks for attempted use of subtractive merge, which was removed for
-      # Chef 11.0. If subtractive merge use is detected, will raise an
-      # InvalidSubtractiveMerge exception.
-      def raise_if_knockout_used!(obj)
-        if uses_knockout?(obj)
-          raise InvalidSubtractiveMerge, "subtractive merge with !merge is no longer supported"
-        end
-      end
-
-      # Checks for attempted use of subtractive merge in +obj+.
-      def uses_knockout?(obj)
-        case obj
-        when String
-          obj =~ OLD_KNOCKOUT_MATCH
-        when Array
-          obj.any? {|element| element.respond_to?(:gsub) && element =~ OLD_KNOCKOUT_MATCH }
-        else
-          false
-        end
-      end
-
       def deep_merge(source, dest)
         deep_merge!(safe_dup(source), safe_dup(dest))
       end
diff --git a/lib/chef/node.rb b/lib/chef/node.rb
index dbb7852586..6055f0e1e9 100644
--- a/lib/chef/node.rb
+++ b/lib/chef/node.rb
@@ -127,6 +127,7 @@ class Chef
     # Set a normal attribute of this node, but auto-vivify any Mashes that
     # might be missing
     def normal
+      attributes.top_level_breadcrumb = nil
       attributes.set_unless_value_present = false
       attributes.normal
     end
@@ -136,14 +137,17 @@ class Chef
     # Set a normal attribute of this node, auto-vivifying any mashes that are
     # missing, but if the final value already exists, don't set it
     def normal_unless
+      attributes.top_level_breadcrumb = nil
       attributes.set_unless_value_present = true
       attributes.normal
     end
+
     alias_method :set_unless, :normal_unless
 
     # Set a default of this node, but auto-vivify any Mashes that might
     # be missing
     def default
+      attributes.top_level_breadcrumb = nil
       attributes.set_unless_value_present = false
       attributes.default
     end
@@ -151,6 +155,7 @@ class Chef
     # Set a default attribute of this node, auto-vivifying any mashes that are
     # missing, but if the final value already exists, don't set it
     def default_unless
+      attributes.top_level_breadcrumb = nil
       attributes.set_unless_value_present = true
       attributes.default
     end
@@ -158,6 +163,7 @@ class Chef
     # Set an override attribute of this node, but auto-vivify any Mashes that
     # might be missing
     def override
+      attributes.top_level_breadcrumb = nil
       attributes.set_unless_value_present = false
       attributes.override
     end
@@ -165,35 +171,30 @@ class Chef
     # Set an override attribute of this node, auto-vivifying any mashes that
     # are missing, but if the final value already exists, don't set it
     def override_unless
+      attributes.top_level_breadcrumb = nil
       attributes.set_unless_value_present = true
       attributes.override
     end
 
-    def override_attrs
-     attributes.override
-    end
+    alias :override_attrs :override
+    alias :default_attrs :default
+    alias :normal_attrs :normal
 
     def override_attrs=(new_values)
       attributes.override = new_values
     end
 
-    def default_attrs
-      attributes.default
-    end
-
     def default_attrs=(new_values)
       attributes.default = new_values
     end
 
-    def normal_attrs
-      attributes.normal
-    end
-
     def normal_attrs=(new_values)
       attributes.normal = new_values
     end
 
     def automatic_attrs
+      attributes.top_level_breadcrumb = nil
+      attributes.set_unless_value_present = false
       attributes.automatic
     end
 
diff --git a/lib/chef/node/attribute.rb b/lib/chef/node/attribute.rb
index 6130925aea..3c48f653eb 100644
--- a/lib/chef/node/attribute.rb
+++ b/lib/chef/node/attribute.rb
@@ -175,6 +175,19 @@ class Chef
        # return the automatic level attribute component
        attr_reader :automatic
 
+       # This is used to track the top level key as we descend through method chaining into
+       # a precedence level (e.g. node.default['foo']['bar']['baz']= results in 'foo' here).  We
+       # need this so that when we hit the end of a method chain which results in a mutator method
+       # that we can invalidate the whole top-level deep merge cache for the top-level key.  It is
+       # the responsibility of the accessor on the Chef::Node object to reset this to nil, and then
+       # the first VividMash#[] call can ||= and set this to the first key we encounter.
+       attr_accessor :top_level_breadcrumb
+
+       # Cache of deep merged values by top-level key.  This is a simple hash which has keys that are the
+       # top-level keys of the node object, and we save the computed deep-merge for that key here.  There is
+       # no cache of subtrees.
+       attr_accessor :deep_merge_cache
+
        def initialize(normal, default, override, automatic)
          @set_unless_present = false
 
@@ -195,6 +208,8 @@ class Chef
          @merged_attributes = nil
          @combined_override = nil
          @combined_default = nil
+         @top_level_breadcrumb = nil
+         @deep_merge_cache = {}
        end
 
        # Debug what's going on with an attribute. +args+ is a path spec to the
@@ -230,51 +245,75 @@ class Chef
          @set_unless_present = setting
        end
 
+       # Invalidate a key in the deep_merge_cache.  If called with nil, or no arg, this will invalidate
+       # the entire deep_merge cache.  In the case of the user doing node.default['foo']['bar']['baz']=
+       # that eventually results in a call to reset_cache('foo') here.  A node.default=hash_thing call
+       # must invalidate the entire cache and re-deep-merge the entire node object.
+       def reset_cache(path = nil)
+         if path.nil?
+           @deep_merge_cache = {}
+         else
+           deep_merge_cache.delete(path)
+         end
+       end
+
+       alias :reset :reset_cache
+
        # Set the cookbook level default attribute component to +new_data+.
        def default=(new_data)
+         reset
          @default = VividMash.new(self, new_data)
        end
 
        # Set the role level default attribute component to +new_data+
        def role_default=(new_data)
+         reset
          @role_default = VividMash.new(self, new_data)
        end
 
        # Set the environment level default attribute component to +new_data+
        def env_default=(new_data)
+         reset
          @env_default = VividMash.new(self, new_data)
        end
 
        # Set the force_default (+default!+) level attributes to +new_data+
        def force_default=(new_data)
+         reset
          @force_default = VividMash.new(self, new_data)
        end
 
        # Set the normal level attribute component to +new_data+
        def normal=(new_data)
+         reset
          @normal = VividMash.new(self, new_data)
        end
 
        # Set the cookbook level override attribute component to +new_data+
        def override=(new_data)
+         reset
          @override = VividMash.new(self, new_data)
        end
 
        # Set the role level override attribute component to +new_data+
        def role_override=(new_data)
+         reset
          @role_override = VividMash.new(self, new_data)
        end
 
        # Set the environment level override attribute component to +new_data+
        def env_override=(new_data)
+         reset
          @env_override = VividMash.new(self, new_data)
        end
 
        def force_override=(new_data)
+         reset
          @force_override = VividMash.new(self, new_data)
        end
 
        def automatic=(new_data)
+         reset
          @automatic = VividMash.new(self, new_data)
        end
 
@@ -284,6 +323,7 @@ class Chef
 
        # clears attributes from all precedence levels
        def rm(*args)
+         reset(args[0])
          # just easier to compute our retval, rather than collect+merge sub-retvals
          ret = args.inject(merged_attributes) do |attr, arg|
            if attr.nil? || !attr.respond_to?(:[])
@@ -314,6 +354,7 @@ class Chef
        #
        # equivalent to: force_default!['foo']['bar'].delete('baz')
        def rm_default(*args)
+         reset(args[0])
          remove_from_precedence_level(force_default!(autovivify: false), *args)
        end
 
@@ -321,6 +362,7 @@ class Chef
        #
        # equivalent to: normal!['foo']['bar'].delete('baz')
        def rm_normal(*args)
+         reset(args[0])
          remove_from_precedence_level(normal!(autovivify: false), *args)
        end
 
@@ -328,6 +370,7 @@ class Chef
        #
        # equivalent to: force_override!['foo']['bar'].delete('baz')
        def rm_override(*args)
+         reset(args[0])
          remove_from_precedence_level(force_override!(autovivify: false), *args)
        end
 
@@ -337,35 +380,51 @@ class Chef
 
        # sets default attributes without merging
        def default!(opts={})
+         # FIXME: do not flush whole cache
+         reset
          MultiMash.new(self, @default, [], opts)
        end
 
        # sets normal attributes without merging
        def normal!(opts={})
+         # FIXME: do not flush whole cache
+         reset
          MultiMash.new(self, @normal, [], opts)
        end
 
        # sets override attributes without merging
        def override!(opts={})
+         # FIXME: do not flush whole cache
+         reset
          MultiMash.new(self, @override, [], opts)
        end
 
        # clears from all default precedence levels and then sets force_default
        def force_default!(opts={})
+         # FIXME: do not flush whole cache
+         reset
          MultiMash.new(self, @force_default, [@default, @env_default, @role_default], opts)
        end
 
        # clears from all override precedence levels and then sets force_override
        def force_override!(opts={})
+         # FIXME: do not flush whole cache
+         reset
          MultiMash.new(self, @force_override, [@override, @env_override, @role_override], opts)
        end
 
        #
-       # Accessing merged attributes
+       # Accessing merged attributes.
+       #
+       # Note that merged_attributes('foo', 'bar', 'baz') can be called to compute only the
+       # deep merge of node['foo']['bar']['baz'], but in practice we currently always compute
+       # all of node['foo'] even if the user only requires node['foo']['bar']['baz'].
        #
 
        def merged_attributes(*path)
-         immutablize(merge_all(path))
+        # immutablize(
+           merge_all(path)
+        # )
        end
 
        def combined_override(*path)
@@ -377,7 +436,13 @@ class Chef
        end
 
        def [](key)
-         merged_attributes(key)
+         if deep_merge_cache.has_key?(key)
+           # return the cache of the deep merged values by top-level key
+           deep_merge_cache[key]
+         else
+           # save all the work of computing node[key]
+           deep_merge_cache[key] = merged_attributes(key)
+         end
        end
 
        def []=(key, value)
@@ -480,7 +545,9 @@ class Chef
            safe_dup(component)
          end
 
-         components.inject(nil) do |merged, component|
+         return nil if components.compact.empty?
+
+         components.inject(ImmutableMash.new({})) do |merged, component|
            Chef::Mixin::DeepMerge.hash_only_merge!(merged, component)
          end
        end
diff --git a/lib/chef/node/attribute_collections.rb b/lib/chef/node/attribute_collections.rb
index 3b19a14d1c..333f4864c6 100644
--- a/lib/chef/node/attribute_collections.rb
+++ b/lib/chef/node/attribute_collections.rb
@@ -63,6 +63,7 @@ class Chef
       MUTATOR_METHODS.each do |mutator|
         class_eval(<<-METHOD_DEFN, __FILE__, __LINE__)
           def #{mutator}(*args, &block)
+            root.reset_cache(root.top_level_breadcrumb)
             super
           end
         METHOD_DEFN
@@ -127,6 +128,7 @@ class Chef
       MUTATOR_METHODS.each do |mutator|
         class_eval(<<-METHOD_DEFN, __FILE__, __LINE__)
           def #{mutator}(*args, &block)
+            root.reset_cache(root.top_level_breadcrumb)
             super
           end
         METHOD_DEFN
@@ -138,6 +140,7 @@ class Chef
       end
 
       def [](key)
+        root.top_level_breadcrumb ||= key
         value = super
         if !key?(key)
           value = self.class.new(root)
@@ -148,9 +151,11 @@ class Chef
       end
 
       def []=(key, value)
+        root.top_level_breadcrumb ||= key
         if set_unless? && key?(key)
           self[key]
         else
+          root.reset_cache(root.top_level_breadcrumb)
           super
         end
       end
diff --git a/lib/chef/node/immutable_collections.rb b/lib/chef/node/immutable_collections.rb
index af04ef26d4..56b8fed3b7 100644
--- a/lib/chef/node/immutable_collections.rb
+++ b/lib/chef/node/immutable_collections.rb
@@ -155,6 +155,10 @@ class Chef
         end
       end
 
+      def public_method_that_only_deep_merge_should_use(key, value)
+        internal_set(key, immutablize(value))
+      end
+
       alias :attribute? :has_key?
 
       # Redefine all of the methods that mutate a Hash to raise an error when called.
diff --git a/lib/chef/provider_resolver.rb b/lib/chef/provider_resolver.rb
index 247102f191..d83a3e0468 100644
--- a/lib/chef/provider_resolver.rb
+++ b/lib/chef/provider_resolver.rb
@@ -34,7 +34,7 @@ class Chef
 
     # return a deterministically sorted list of Chef::Provider subclasses
     def providers
-      @providers ||= Chef::Provider.descendants.sort {|a,b| a.to_s <=> b.to_s }
+      @providers ||= Chef::Provider.descendants
     end
 
     def resolve
@@ -48,7 +48,7 @@ class Chef
       @enabled_handlers ||=
         providers.select do |klass|
           klass.provides?(node, resource)
-        end
+        end.sort {|a,b| a.to_s <=> b.to_s }
     end
 
     # this cut looks at if the provider can handle the specific resource and action
diff --git a/lib/chef/run_list/run_list_expansion.rb b/lib/chef/run_list/run_list_expansion.rb
index 73665f39e7..46b45f1d9e 100644
--- a/lib/chef/run_list/run_list_expansion.rb
+++ b/lib/chef/run_list/run_list_expansion.rb
@@ -96,8 +96,8 @@ class Chef
       end
 
       def apply_role_attributes(role)
-        @default_attrs = Chef::Mixin::DeepMerge.role_merge(@default_attrs, role.default_attributes)
-        @override_attrs = Chef::Mixin::DeepMerge.role_merge(@override_attrs, role.override_attributes)
+        @default_attrs = Chef::Mixin::DeepMerge.merge(@default_attrs, role.default_attributes)
+        @override_attrs = Chef::Mixin::DeepMerge.merge(@override_attrs, role.override_attributes)
       end
 
       def applied_role?(role_name)
diff --git a/spec/unit/mixin/deep_merge_spec.rb b/spec/unit/mixin/deep_merge_spec.rb
index 76f5c68a29..d024acccdd 100644
--- a/spec/unit/mixin/deep_merge_spec.rb
+++ b/spec/unit/mixin/deep_merge_spec.rb
@@ -290,32 +290,6 @@ describe Chef::Mixin::DeepMerge do
 
   end
 
-  describe "role_merge" do
-    it "errors out if knockout merge use is detected in an array" do
-      hash_dst = {"property" => ["2","4"]}
-      hash_src = {"property" => ["1","!merge:4"]}
-      lambda {@dm.role_merge(hash_dst, hash_src)}.should raise_error(Chef::Mixin::DeepMerge::InvalidSubtractiveMerge)
-    end
-
-    it "errors out if knockout merge use is detected in an array (reversed merge order)" do
-      hash_dst = {"property" => ["1","!merge:4"]}
-      hash_src = {"property" => ["2","4"]}
-      lambda {@dm.role_merge(hash_dst, hash_src)}.should raise_error(Chef::Mixin::DeepMerge::InvalidSubtractiveMerge)
-    end
-
-    it "errors out if knockout merge use is detected in a string" do
-      hash_dst = {"property" => ["2","4"]}
-      hash_src = {"property" => "!merge"}
-      lambda {@dm.role_merge(hash_dst, hash_src)}.should raise_error(Chef::Mixin::DeepMerge::InvalidSubtractiveMerge)
-    end
-
-    it "errors out if knockout merge use is detected in a string (reversed merge order)" do
-      hash_dst = {"property" => "!merge"}
-      hash_src= {"property" => ["2","4"]}
-      lambda {@dm.role_merge(hash_dst, hash_src)}.should raise_error(Chef::Mixin::DeepMerge::InvalidSubtractiveMerge)
-    end
-  end
-
   describe "hash-only merging" do
     it "merges Hashes like normal deep merge" do
       merge_ee_hash = {"top_level_a" => {"1_deep_a" => "1-a-merge-ee", "1_deep_b" => "1-deep-b-merge-ee"}, "top_level_b" => "top-level-b-merge-ee"}