immutablize on the fly and reduce dupinglcg/deep-merge-immutablizing

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>

2 files changed, 96 insertions, 29 deletions

diff --git a/lib/chef/node/attribute.rb b/lib/chef/node/attribute.rb
index a6443df62c..2998866bb2 100644
--- a/lib/chef/node/attribute.rb
+++ b/lib/chef/node/attribute.rb
@@ -399,7 +399,7 @@ class Chef
        #
 
       def merged_attributes(*path)
-        immutablize(merge_all(path))
+        merge_all(path)
       end
 
       def combined_override(*path)
@@ -536,11 +536,10 @@ class Chef
           apply_path(@automatic, path),
         ]
 
-        return nil if components.compact.empty?
-
-        components.inject(ImmutableMash.new({}, self, __node__, :merged)) do |merged, component|
-          Chef::Mixin::DeepMerge.hash_only_merge!(merged, component)
+        ret = components.inject(NIL) do |merged, component|
+          hash_only_merge!(merged, component)
         end
+        ret == NIL ? nil : ret
       end
 
        # Deep merge the default attribute levels with array merging.
@@ -550,10 +549,11 @@ class Chef
        # @param path [Array] Array of args to method chain to descend into the node object
        # @return [attr] Deep Merged values (may be VividMash, Hash, Array, etc) from the node object
       def merge_defaults(path)
-        DEFAULT_COMPONENTS.inject(nil) do |merged, component_ivar|
+        ret = DEFAULT_COMPONENTS.inject(NIL) do |merged, component_ivar|
           component_value = apply_path(instance_variable_get(component_ivar), path)
-          Chef::Mixin::DeepMerge.deep_merge(component_value, merged)
+          deep_merge!(merged, component_value)
         end
+        ret == NIL ? nil : ret
       end
 
        # Deep merge the override attribute levels with array merging.
@@ -563,10 +563,11 @@ class Chef
        # @param path [Array] Array of args to method chain to descend into the node object
        # @return [attr] Deep Merged values (may be VividMash, Hash, Array, etc) from the node object
       def merge_overrides(path)
-        OVERRIDE_COMPONENTS.inject(nil) do |merged, component_ivar|
+        ret = OVERRIDE_COMPONENTS.inject(NIL) do |merged, component_ivar|
           component_value = apply_path(instance_variable_get(component_ivar), path)
-          Chef::Mixin::DeepMerge.deep_merge(component_value, merged)
+          deep_merge!(merged, component_value)
         end
+        ret == NIL ? nil : ret
       end
 
        # needed for __path__
@@ -574,7 +575,76 @@ class Chef
         key.kind_of?(Symbol) ? key.to_s : key
       end
 
-    end
+      NIL = Object.new
+
+      # @api private
+      def deep_merge!(merge_onto, merge_with)
+        # 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|
+            value =
+              if merge_onto.has_key?(key)
+                deep_merge!(safe_dup(merge_onto[key]), merge_with_value)
+              else
+                merge_with_value
+              end
+
+            # internal_set bypasses converting keys, does convert values and allows writing to immutable mashes
+            merge_onto.internal_set(key, value)
+          end
+          merge_onto
+
+        elsif merge_onto.kind_of?(Array) && merge_with.kind_of?(Array)
+          merge_onto |= merge_with
+
+        # If merge_with is nil, don't replace merge_onto
+        elsif merge_with.nil?
+          merge_onto
+
+        # In all other cases, replace merge_onto with merge_with
+        else
+          if merge_with.kind_of?(Hash)
+            Chef::Node::VividMash.new(merge_with)
+          elsif merge_with.kind_of?(Array)
+            Chef::Node::AttrArray.new(merge_with)
+          else
+            merge_with
+          end
+        end
+      end
 
+      # @api private
+      def hash_only_merge!(merge_onto, merge_with)
+        # 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|
+            value =
+              if merge_onto.has_key?(key)
+                hash_only_merge!(safe_dup(merge_onto[key]), merge_with_value)
+              else
+                merge_with_value
+              end
+
+            # internal_set bypasses converting keys, does convert values and allows writing to immutable mashes
+            merge_onto.internal_set(key, value)
+          end
+          merge_onto
+
+        # If merge_with is nil, don't replace merge_onto
+        elsif merge_with.nil?
+          merge_onto
+
+        # In all other cases, replace merge_onto with merge_with
+        else
+          if merge_with.kind_of?(Hash)
+            Chef::Node::ImmutableMash.new(merge_with)
+          elsif merge_with.kind_of?(Array)
+            Chef::Node::ImmutableArray.new(merge_with)
+          else
+            merge_with
+          end
+        end
+      end
+    end
   end
 end
diff --git a/lib/chef/node/immutable_collections.rb b/lib/chef/node/immutable_collections.rb
index 5ac560b4d5..848e12d2df 100644
--- a/lib/chef/node/immutable_collections.rb
+++ b/lib/chef/node/immutable_collections.rb
@@ -30,16 +30,22 @@ class Chef
         e
       end
 
-      def immutablize(value)
+      def convert_value(value)
         case value
         when Hash
           ImmutableMash.new(value, __root__, __node__, __precedence__)
         when Array
           ImmutableArray.new(value, __root__, __node__, __precedence__)
+        when ImmutableMash, ImmutableArray
+          value
         else
           safe_dup(value).freeze
         end
       end
+
+      def immutablize(value)
+        convert_value(value)
+      end
     end
 
     # == ImmutableArray
@@ -90,7 +96,9 @@ class Chef
 
       alias_method :to_array, :to_a
 
-      # for consistency's sake -- integers 'converted' to integers
+      private
+
+      # needed for __path__
       def convert_key(key)
         key
       end
@@ -115,31 +123,20 @@ class Chef
       include Immutablize
       include CommonAPI
 
-      alias :internal_set :regular_writer
-      private :internal_set
+      # this is for deep_merge usage, chef users must never touch this API
+      # @api private
+      def internal_set(key, value)
+        regular_writer(key, convert_value(value))
+      end
 
       def initialize(mash_data = {})
         mash_data.each do |key, value|
-          internal_set(key, immutablize(value))
+          internal_set(key, value)
         end
       end
 
-      def public_method_that_only_deep_merge_should_use(key, value)
-        internal_set(key, immutablize(value))
-      end
-
       alias :attribute? :has_key?
 
-      # Mash uses #convert_value to mashify values on input.
-      # Since we're handling this ourselves, override it to be a no-op
-      #
-      # FIXME?  this seems wrong to do and i think is responsible for
-      # #dup needing to be more complicated than Mash.new(self)?
-      #
-      def convert_value(value)
-        value
-      end
-
       # NOTE: #default and #default= are likely to be pretty confusing. For a
       # regular ruby Hash, they control what value is returned for, e.g.,
       #   hash[:no_such_key] #=> hash.default