Merge pull request #2454 from opscode/sersut/port-attr-perf

Merge pull request #2447 from opscode/lcg/lazy-deep-merge2

5 files changed, 132 insertions, 107 deletions

diff --git a/lib/chef/exceptions.rb b/lib/chef/exceptions.rb
index 93fdd414e4..c8d26dbed2 100644
--- a/lib/chef/exceptions.rb
+++ b/lib/chef/exceptions.rb
@@ -162,7 +162,12 @@ class Chef
     # Node::Attribute computes the merged version of of attributes
     # and makes it read-only. Attempting to modify a read-only
     # attribute will cause this error.
-    class ImmutableAttributeModification < NoMethodError; end
+    class ImmutableAttributeModification < NoMethodError
+      def initialize
+        super "Node attributes are read-only when you do not specify which precedence level to set. " +
+          %Q(To set an attribute use code like `node.default["key"] = "value"')
+      end
+    end
 
     # Merged node attributes are invalidated when the component
     # attributes are updated. Attempting to read from a stale copy
diff --git a/lib/chef/node/attribute.rb b/lib/chef/node/attribute.rb
index 3eb6449046..6130925aea 100644
--- a/lib/chef/node/attribute.rb
+++ b/lib/chef/node/attribute.rb
@@ -230,72 +230,51 @@ class Chef
          @set_unless_present = setting
        end
 
-       # Clears merged_attributes, which will cause it to be recomputed on the
-       # next access.
-       def reset_cache
-         @merged_attributes = nil
-         @combined_default  = nil
-         @combined_override = nil
-         @set_unless_present = false
-       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
 
@@ -335,7 +314,6 @@ class Chef
        #
        # equivalent to: force_default!['foo']['bar'].delete('baz')
        def rm_default(*args)
-         reset
          remove_from_precedence_level(force_default!(autovivify: false), *args)
        end
 
@@ -343,7 +321,6 @@ class Chef
        #
        # equivalent to: normal!['foo']['bar'].delete('baz')
        def rm_normal(*args)
-         reset
          remove_from_precedence_level(normal!(autovivify: false), *args)
        end
 
@@ -351,7 +328,6 @@ class Chef
        #
        # equivalent to: force_override!['foo']['bar'].delete('baz')
        def rm_override(*args)
-         reset
          remove_from_precedence_level(force_override!(autovivify: false), *args)
        end
 
@@ -361,31 +337,26 @@ class Chef
 
        # sets default attributes without merging
        def default!(opts={})
-         reset
          MultiMash.new(self, @default, [], opts)
        end
 
        # sets normal attributes without merging
        def normal!(opts={})
-         reset
          MultiMash.new(self, @normal, [], opts)
        end
 
        # sets override attributes without merging
        def override!(opts={})
-         reset
          MultiMash.new(self, @override, [], opts)
        end
 
        # clears from all default precedence levels and then sets force_default
        def force_default!(opts={})
-         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={})
-         reset
          MultiMash.new(self, @force_override, [@override, @env_override, @role_override], opts)
        end
 
@@ -393,30 +364,24 @@ class Chef
        # Accessing merged attributes
        #
 
-       def merged_attributes
-         @merged_attributes ||= begin
-                                  components = [merge_defaults, @normal, merge_overrides, @automatic]
-                                  resolved_attrs = components.inject(Mash.new) do |merged, component|
-                                    Chef::Mixin::DeepMerge.hash_only_merge(merged, component)
-                                  end
-                                  immutablize(resolved_attrs)
-                                end
+       def merged_attributes(*path)
+         immutablize(merge_all(path))
        end
 
-       def combined_override
-         @combined_override ||= immutablize(merge_overrides)
+       def combined_override(*path)
+         immutablize(merge_overrides(path))
        end
 
-       def combined_default
-         @combined_default ||= immutablize(merge_defaults)
+       def combined_default(*path)
+         immutablize(merge_defaults(path))
        end
 
        def [](key)
-         merged_attributes[key]
+         merged_attributes(key)
        end
 
        def []=(key, value)
-         merged_attributes[key] = value
+         raise Exceptions::ImmutableAttributeModification
        end
 
        def has_key?(key)
@@ -459,17 +424,90 @@ class Chef
 
        private
 
-       def merge_defaults
-         DEFAULT_COMPONENTS.inject(Mash.new) do |merged, component_ivar|
-           component_value = instance_variable_get(component_ivar)
-           Chef::Mixin::DeepMerge.merge(merged, component_value)
+       # Helper method for merge_all/merge_defaults/merge_overrides.
+       #
+       # apply_path(thing, [ "foo", "bar", "baz" ]) = thing["foo"]["bar"]["baz"]
+       #
+       # The path value can be nil in which case the whole component is returned.
+       #
+       # It returns nil (does not raise an exception) if it walks off the end of an Mash/Hash/Array, it does not
+       # raise any TypeError if it attempts to apply a hash key to an Integer/String/TrueClass, and just returns
+       # nil in that case.
+       #
+       def apply_path(component, path)
+         path ||= []
+         path.inject(component) do |val, path_arg|
+           if val.respond_to?(:[])
+             # Have an Array-like or Hash-like thing
+             if !val.respond_to?(:has_key?)
+               # Have an Array-like thing
+               val[path_arg]
+             elsif val.has_key?(path_arg)
+               # Hash-like thing (must check has_key? first to protect against Autovivification)
+               val[path_arg]
+             else
+               nil
+             end
+           else
+             nil
+           end
          end
        end
 
-       def merge_overrides
-         OVERRIDE_COMPONENTS.inject(Mash.new) do |merged, component_ivar|
-           component_value = instance_variable_get(component_ivar)
-           Chef::Mixin::DeepMerge.merge(merged, component_value)
+       # For elements like Fixnums, true, nil...
+       def safe_dup(e)
+         e.dup
+       rescue TypeError
+         e
+       end
+
+       # Deep merge all attribute levels using hash-only merging between different precidence
+       # levels (so override arrays completely replace arrays set at any default level).
+       #
+       # The path allows for selectively deep-merging a subtree of the node object.
+       #
+       # @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_all(path)
+         components = [
+           merge_defaults(path),
+           apply_path(@normal, path),
+           merge_overrides(path),
+           apply_path(@automatic, path),
+         ]
+
+         components.map! do |component|
+           safe_dup(component)
+         end
+
+         components.inject(nil) do |merged, component|
+           Chef::Mixin::DeepMerge.hash_only_merge!(merged, component)
+         end
+       end
+
+       # Deep merge the default attribute levels with array merging.
+       #
+       # The path allows for selectively deep-merging a subtree of the node object.
+       #
+       # @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|
+           component_value = apply_path(instance_variable_get(component_ivar), path)
+           Chef::Mixin::DeepMerge.deep_merge(component_value, merged)
+         end
+       end
+
+       # Deep merge the override attribute levels with array merging.
+       #
+       # The path allows for selectively deep-merging a subtree of the node object.
+       #
+       # @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|
+           component_value = apply_path(instance_variable_get(component_ivar), path)
+           Chef::Mixin::DeepMerge.deep_merge(component_value, merged)
          end
        end
 
diff --git a/lib/chef/node/attribute_collections.rb b/lib/chef/node/attribute_collections.rb
index c8bc618762..3b19a14d1c 100644
--- a/lib/chef/node/attribute_collections.rb
+++ b/lib/chef/node/attribute_collections.rb
@@ -63,7 +63,6 @@ class Chef
       MUTATOR_METHODS.each do |mutator|
         class_eval(<<-METHOD_DEFN, __FILE__, __LINE__)
           def #{mutator}(*args, &block)
-            root.reset_cache
             super
           end
         METHOD_DEFN
@@ -128,7 +127,6 @@ class Chef
       MUTATOR_METHODS.each do |mutator|
         class_eval(<<-METHOD_DEFN, __FILE__, __LINE__)
           def #{mutator}(*args, &block)
-            root.reset_cache
             super
           end
         METHOD_DEFN
@@ -153,7 +151,6 @@ class Chef
         if set_unless? && key?(key)
           self[key]
         else
-          root.reset_cache
           super
         end
       end
diff --git a/lib/chef/node/immutable_collections.rb b/lib/chef/node/immutable_collections.rb
index 3558ba3a86..af04ef26d4 100644
--- a/lib/chef/node/immutable_collections.rb
+++ b/lib/chef/node/immutable_collections.rb
@@ -78,9 +78,7 @@ class Chef
         # Ruby 1.8 blocks can't have block arguments, so we must use string eval:
         class_eval(<<-METHOD_DEFN, __FILE__, __LINE__)
           def #{mutator_method_name}(*args, &block)
-            msg = "Node attributes are read-only when you do not specify which precedence level to set. " +
-            %Q(To set an attribute use code like `node.default["key"] = "value"')
-            raise Exceptions::ImmutableAttributeModification, msg
+            raise Exceptions::ImmutableAttributeModification
           end
         METHOD_DEFN
       end
@@ -165,9 +163,7 @@ class Chef
         # Ruby 1.8 blocks can't have block arguments, so we must use string eval:
         class_eval(<<-METHOD_DEFN, __FILE__, __LINE__)
         def #{mutator_method_name}(*args, &block)
-          msg = "Node attributes are read-only when you do not specify which precedence level to set. " +
-          %Q(To set an attribute use code like `node.default["key"] = "value"')
-          raise Exceptions::ImmutableAttributeModification, msg
+          raise Exceptions::ImmutableAttributeModification
         end
         METHOD_DEFN
       end
diff --git a/spec/unit/node/attribute_spec.rb b/spec/unit/node/attribute_spec.rb
index 3b3a81f3a6..1eddab00ba 100644
--- a/spec/unit/node/attribute_spec.rb
+++ b/spec/unit/node/attribute_spec.rb
@@ -554,8 +554,7 @@ describe Chef::Node::Attribute do
 
     it "should allow the last method to set a value if it has an = sign on the end" do
       @attributes.normal.music.mastodon = [ "dream", "still", "shining" ]
-      @attributes.reset
-      @attributes.normal.music.mastodon.should == [ "dream", "still", "shining" ]
+      expect(@attributes.normal.music.mastodon).to eq([ "dream", "still", "shining" ])
     end
   end
 
@@ -1091,50 +1090,6 @@ describe Chef::Node::Attribute do
     end
   end
 
-  # For expedience, this test is implementation-heavy.
-  describe "when a component attribute is mutated" do
-    [
-      :clear,
-      :shift
-    ].each do |mutator|
-      it "resets the cache when the mutator #{mutator} is called" do
-        @attributes.should_receive(:reset_cache)
-        @attributes.default.send(mutator)
-      end
-    end
-
-    it "resets the cache when the mutator delete is called" do
-      @attributes.should_receive(:reset_cache)
-      @attributes.default.delete(:music)
-    end
-
-    [
-      :merge!,
-      :update,
-      :replace
-    ].each do |mutator|
-      it "resets the cache when the mutator #{mutator} is called" do
-        # Implementation of Mash means that this could get called many times. That's okay.
-        @attributes.should_receive(:reset_cache).at_least(1).times
-        @attributes.default.send(mutator, {:foo => :bar})
-      end
-    end
-
-    [
-      :delete_if,
-      :keep_if,
-      :reject!,
-      :select!,
-    ].each do |mutator|
-      it "resets the cache when the mutator #{mutator} is called" do
-        # Implementation of Mash means that this could get called many times. That's okay.
-        @attributes.should_receive(:reset_cache).at_least(1).times
-        block = lambda {|k,v| true }
-        @attributes.default.send(mutator, &block)
-      end
-    end
-  end
-
   describe "when not mutated" do
 
     it "does not reset the cache when dup'd [CHEF-3680]" do
@@ -1172,6 +1127,40 @@ describe Chef::Node::Attribute do
     end
   end
 
+  describe "when deep-merging between precedence levels" do
+    it "correctly deep merges hashes and preserves the original contents" do
+      @attributes.default = { "arglebargle" => { "foo" => "bar" } }
+      @attributes.override = { "arglebargle" => { "fizz" => "buzz" } }
+      expect(@attributes.merged_attributes[:arglebargle]).to eq({ "foo" => "bar", "fizz" => "buzz" })
+      expect(@attributes.default[:arglebargle]).to eq({ "foo" => "bar" })
+      expect(@attributes.override[:arglebargle]).to eq({ "fizz" => "buzz" })
+    end
+
+    it "does not deep merge arrays, and preserves the original contents" do
+      @attributes.default = { "arglebargle" => [ 1, 2, 3 ] }
+      @attributes.override = { "arglebargle" => [ 4, 5, 6 ] }
+      expect(@attributes.merged_attributes[:arglebargle]).to eq([ 4, 5, 6 ])
+      expect(@attributes.default[:arglebargle]).to eq([ 1, 2, 3 ])
+      expect(@attributes.override[:arglebargle]).to eq([ 4, 5, 6 ])
+    end
+
+    it "correctly deep merges hashes and preserves the original contents when merging default and role_default" do
+      @attributes.default = { "arglebargle" => { "foo" => "bar" } }
+      @attributes.role_default = { "arglebargle" => { "fizz" => "buzz" } }
+      expect(@attributes.merged_attributes[:arglebargle]).to eq({ "foo" => "bar", "fizz" => "buzz" })
+      expect(@attributes.default[:arglebargle]).to eq({ "foo" => "bar" })
+      expect(@attributes.role_default[:arglebargle]).to eq({ "fizz" => "buzz" })
+    end
+
+    it "correctly deep merges arrays, and preserves the original contents when merging default and role_default" do
+      @attributes.default = { "arglebargle" => [ 1, 2, 3 ] }
+      @attributes.role_default = { "arglebargle" => [ 4, 5, 6 ] }
+      expect(@attributes.merged_attributes[:arglebargle]).to eq([ 1, 2, 3, 4, 5, 6 ])
+      expect(@attributes.default[:arglebargle]).to eq([ 1, 2, 3 ])
+      expect(@attributes.role_default[:arglebargle]).to eq([ 4, 5, 6 ])
+    end
+  end
+
   describe "when attemping to write without specifying precedence" do
     it "raises an error when using []=" do
       lambda { @attributes[:new_key] = "new value" }.should raise_error(Chef::Exceptions::ImmutableAttributeModification)