Merge pull request #6424 from chef/lcg/deep-merge-cache-rewrite

Node attributes per-container lazy deep merge and deep merge performance improvements

9 files changed, 355 insertions, 159 deletions

diff --git a/lib/chef/node/attribute.rb b/lib/chef/node/attribute.rb
index 2998866bb2..389968f543 100644
--- a/lib/chef/node/attribute.rb
+++ b/lib/chef/node/attribute.rb
@@ -17,7 +17,6 @@
 # limitations under the License.
 #
 
-require "chef/node/mixin/deep_merge_cache"
 require "chef/node/mixin/immutablize_hash"
 require "chef/node/mixin/state_tracking"
 require "chef/node/immutable_collections"
@@ -45,7 +44,6 @@ class Chef
       # expects.  This include should probably be deleted?
       include Enumerable
 
-      include Chef::Node::Mixin::DeepMergeCache
       include Chef::Node::Mixin::StateTracking
       include Chef::Node::Mixin::ImmutablizeHash
 
@@ -187,6 +185,9 @@ class Chef
        # return the automatic level attribute component
       attr_reader :automatic
 
+      # return the immutablemash deep merge cache
+      attr_reader :deep_merge_cache
+
       def initialize(normal, default, override, automatic, node = nil)
         @default        = VividMash.new(default, self, node, :default)
         @env_default    = VividMash.new({}, self, node, :env_default)
@@ -202,7 +203,8 @@ class Chef
 
         @automatic      = VividMash.new(automatic, self, node, :automatic)
 
-        super(nil, self, node, :merged)
+        @deep_merge_cache = ImmutableMash.new({}, self, node, :merged)
+        @__node__ = node
       end
 
        # Debug what's going on with an attribute. +args+ is a path spec to the
@@ -226,6 +228,22 @@ class Chef
         end
       end
 
+      def reset
+        @deep_merge_cache = ImmutableMash.new({}, self, @__node__, :merged)
+      end
+
+      def reset_cache(*path)
+        if path.empty?
+          reset
+        else
+          container = read(*path)
+          case container
+          when Hash, Array
+            container.reset
+          end
+        end
+      end
+
        # Set the cookbook level default attribute component to +new_data+.
       def default=(new_data)
         reset
@@ -290,7 +308,7 @@ class Chef
 
        # clears attributes from all precedence levels
       def rm(*args)
-        with_deep_merged_return_value(self, *args) do
+        with_deep_merged_return_value(combined_all, *args) do
           rm_default(*args)
           rm_normal(*args)
           rm_override(*args)
@@ -337,6 +355,9 @@ class Chef
       def with_deep_merged_return_value(obj, *path, last)
         hash = obj.read(*path)
         return nil unless hash.is_a?(Hash)
+        # coerce from immutablemash/vividmash to plain-old Hash
+        # also de-immutablizes and dup's the return value correctly in chef-13
+        hash = hash.to_hash
         ret = hash[last]
         yield
         ret
@@ -398,16 +419,16 @@ class Chef
        # all of node['foo'] even if the user only requires node['foo']['bar']['baz'].
        #
 
-      def merged_attributes(*path)
-        merge_all(path)
+      def combined_override(*path)
+        merge_overrides(path)
       end
 
-      def combined_override(*path)
-        immutablize(merge_overrides(path))
+      def combined_all(*path)
+        path.empty? ? self : read(*path)
       end
 
       def combined_default(*path)
-        immutablize(merge_defaults(path))
+        merge_defaults(path)
       end
 
       def normal_unless(*args)
@@ -476,6 +497,14 @@ class Chef
         merged_attributes.to_s
       end
 
+      def [](key)
+        @deep_merge_cache[key]
+      end
+
+      def merged_attributes
+        @deep_merge_cache
+      end
+
       def inspect
         "#<#{self.class} " << (COMPONENTS + [:@merged_attributes, :@properties]).map do |iv|
           "#{iv}=#{instance_variable_get(iv).inspect}"
@@ -484,7 +513,14 @@ class Chef
 
       private
 
-       # Helper method for merge_all/merge_defaults/merge_overrides.
+      # For elements like Fixnums, true, nil...
+      def safe_dup(e)
+        e.dup
+      rescue TypeError
+        e
+      end
+
+       # Helper method for merge_defaults/merge_overrides.
        #
        # apply_path(thing, [ "foo", "bar", "baz" ]) = thing["foo"]["bar"]["baz"]
        #
@@ -514,34 +550,6 @@ class Chef
         end
       end
 
-       # 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),
-        ]
-
-        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.
        #
        # The path allows for selectively deep-merging a subtree of the node object.
@@ -613,38 +621,6 @@ class Chef
         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/attribute_collections.rb b/lib/chef/node/attribute_collections.rb
index a31b2d2b9b..bb32ae3b54 100644
--- a/lib/chef/node/attribute_collections.rb
+++ b/lib/chef/node/attribute_collections.rb
@@ -63,13 +63,13 @@ class Chef
       MUTATOR_METHODS.each do |mutator|
         define_method(mutator) do |*args, &block|
           ret = super(*args, &block)
-          send_reset_cache
+          send_reset_cache(__path__)
           ret
         end
       end
 
       def delete(key, &block)
-        send_reset_cache(__path__, key)
+        send_reset_cache(__path__)
         super
       end
 
@@ -147,13 +147,13 @@ class Chef
       # object.
 
       def delete(key, &block)
-        send_reset_cache(__path__, key)
+        send_reset_cache(__path__)
         super
       end
 
       MUTATOR_METHODS.each do |mutator|
         define_method(mutator) do |*args, &block|
-          send_reset_cache
+          send_reset_cache(__path__)
           super(*args, &block)
         end
       end
@@ -174,7 +174,7 @@ class Chef
 
       def []=(key, value)
         ret = super
-        send_reset_cache(__path__, key)
+        send_reset_cache(__path__)
         ret
       end
 
diff --git a/lib/chef/node/common_api.rb b/lib/chef/node/common_api.rb
index a703c1ef54..fc0ffc57a5 100644
--- a/lib/chef/node/common_api.rb
+++ b/lib/chef/node/common_api.rb
@@ -1,5 +1,5 @@
 #--
-# Copyright:: Copyright 2016, Chef Software, Inc.
+# Copyright:: Copyright 2016-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
diff --git a/lib/chef/node/immutable_collections.rb b/lib/chef/node/immutable_collections.rb
index 848e12d2df..6458db94e9 100644
--- a/lib/chef/node/immutable_collections.rb
+++ b/lib/chef/node/immutable_collections.rb
@@ -30,22 +30,16 @@ class Chef
         e
       end
 
-      def convert_value(value)
+      def convert_value(value, path = nil)
         case value
         when Hash
-          ImmutableMash.new(value, __root__, __node__, __precedence__)
+          ImmutableMash.new({}, __root__, __node__, __precedence__, path)
         when Array
-          ImmutableArray.new(value, __root__, __node__, __precedence__)
-        when ImmutableMash, ImmutableArray
-          value
+          ImmutableArray.new([], __root__, __node__, __precedence__, path)
         else
           safe_dup(value).freeze
         end
       end
-
-      def immutablize(value)
-        convert_value(value)
-      end
     end
 
     # == ImmutableArray
@@ -59,17 +53,51 @@ class Chef
     #   Chef::Node::Attribute's values, it overrides all reader methods to
     #   detect staleness and raise an error if accessed when stale.
     class ImmutableArray < Array
+      alias_method :internal_clear, :clear
+      alias_method :internal_replace, :replace
+      alias_method :internal_push, :<<
+      alias_method :internal_to_a, :to_a
+      alias_method :internal_each, :each
+      private :internal_push, :internal_replace, :internal_clear, :internal_each
+      protected :internal_to_a
+
       include Immutablize
 
-      alias :internal_push :<<
-      private :internal_push
+      methods = Array.instance_methods - Object.instance_methods +
+        [ :!, :!=, :<=>, :==, :===, :eql?, :to_s, :hash, :key, :has_key?, :inspect, :pretty_print, :pretty_print_inspect, :pretty_print_cycle, :pretty_print_instance_variables ]
 
-      def initialize(array_data = [])
-        array_data.each do |value|
-          internal_push(immutablize(value))
+      methods.each do |method|
+        define_method method do |*args, &block|
+          ensure_generated_cache!
+          super(*args, &block)
         end
       end
 
+      def each
+        ensure_generated_cache!
+        # aggressively pre generate the cache, works around ruby being too smart and fiddling with internals
+        internal_each { |i| i.ensure_generated_cache! if i.respond_to?(:ensure_generated_cache!) }
+        super
+      end
+
+      # because sometimes ruby gives us back Arrays or ImmutableArrays out of objects from things like #uniq or array slices
+      def return_normal_array(array)
+        if array.respond_to?(:internal_to_a, true)
+          array.internal_to_a
+        else
+          array.to_a
+        end
+      end
+
+      def uniq
+        ensure_generated_cache!
+        return_normal_array(super)
+      end
+
+      def initialize(array_data = [])
+        # Immutable collections no longer have initialized state
+      end
+
       # For elements like Fixnums, true, nil...
       def safe_dup(e)
         e.dup
@@ -96,8 +124,81 @@ class Chef
 
       alias_method :to_array, :to_a
 
+      def [](*args)
+        ensure_generated_cache!
+        args.length > 1 ? return_normal_array(super) : super # correctly handle array slices
+      end
+
+      def reset
+        @generated_cache = false
+        @short_circuit_attr_level = nil
+        internal_clear # redundant?
+      end
+
+      # @api private
+      def ensure_generated_cache!
+        generate_cache unless @generated_cache
+        @generated_cache = true
+      end
+
+      # This can be set to e.g. [ :@default ] by the parent container to cause this container
+      # to only use the default level and to bypass deep merging (the common case is either
+      # default-level or automatic-level and we aren't doing any deep merging).  Right now it
+      # "optimized" for the case where we're no longer merging anything and only tracking a
+      # single level, and setting this to anything other than a size=1 array would behave
+      # in a broken fashion.  That could be fixed, but the perf boost would likely not be
+      # that large in the typical case.
+      #
+      # @api private
+      attr_accessor :short_circuit_attr_levels
+
       private
 
+      # deep merging of array attribute within normal and override where they are merged together
+      def combined_components(components)
+        combined_values = nil
+        components.each do |component|
+          values = __node__.attributes.instance_variable_get(component).read(*__path__)
+          next unless values.is_a?(Array)
+          @tracked_components << component
+          combined_values ||= []
+          combined_values += values
+        end
+        combined_values
+      end
+
+      def get_array(component)
+        array = __node__.attributes.instance_variable_get(component).read(*__path__)
+        if array.is_a?(Array)
+          @tracked_components << component
+          array
+        end # else nil
+      end
+
+      def generate_cache
+        internal_clear
+        components = []
+        @tracked_components = []
+        if short_circuit_attr_levels
+          components << get_array(short_circuit_attr_levels.first)
+        else
+          components << combined_components(Attribute::DEFAULT_COMPONENTS)
+          components << get_array(:@normal)
+          components << combined_components(Attribute::OVERRIDE_COMPONENTS)
+          components << get_array(:@automatic)
+        end
+        highest = components.compact.last
+        if highest.is_a?(Array)
+          internal_replace( highest.each_with_index.map { |x, i| convert_value(x, __path__ + [ i ] ) } )
+        end
+        if @tracked_components.size == 1
+          # tracked_components is accurate enough to tell us if we're not really merging
+          internal_each do |key, value|
+            value.short_circuit_attr_levels = @tracked_components if value.respond_to?(:short_circuit_attr_levels)
+          end
+        end
+      end
+
       # needed for __path__
       def convert_key(key)
         key
@@ -120,19 +221,31 @@ class Chef
     #   it is stale.
     # * Values can be accessed in attr_reader-like fashion via method_missing.
     class ImmutableMash < Mash
+      alias_method :internal_clear, :clear
+      alias_method :internal_key?, :key? # FIXME: could bypass convert_key in Mash for perf
+      alias_method :internal_each, :each
+
       include Immutablize
       include CommonAPI
 
+      methods = Hash.instance_methods - Object.instance_methods +
+        [ :!, :!=, :<=>, :==, :===, :eql?, :to_s, :hash, :key, :has_key?, :inspect, :pretty_print, :pretty_print_inspect, :pretty_print_cycle, :pretty_print_instance_variables ]
+
+      methods.each do |method|
+        define_method method do |*args, &block|
+          ensure_generated_cache!
+          super(*args, &block)
+        end
+      end
+
       # 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))
+        regular_writer(key, convert_value(value, __path__ + [ key ]))
       end
 
       def initialize(mash_data = {})
-        mash_data.each do |key, value|
-          internal_set(key, value)
-        end
+        # Immutable collections no longer have initialized state
       end
 
       alias :attribute? :has_key?
@@ -168,11 +281,55 @@ class Chef
 
       alias_method :to_hash, :to_h
 
-      # For elements like Fixnums, true, nil...
-      def safe_dup(e)
-        e.dup
-      rescue TypeError
-        e
+      def [](key)
+        ensure_generated_cache!
+        super
+      end
+
+      alias_method :to_hash, :to_h
+
+      def reset
+        @generated_cache = false
+        @short_circuit_attr_level = nil
+        internal_clear # redundant?
+      end
+
+      # @api private
+      def ensure_generated_cache!
+        generate_cache unless @generated_cache
+        @generated_cache = true
+      end
+
+      # @api private
+      attr_accessor :short_circuit_attr_levels
+
+      private
+
+      def generate_cache
+        internal_clear
+        components = short_circuit_attr_levels ? short_circuit_attr_levels : Attribute::COMPONENTS.reverse
+        # tracked_components is not entirely accurate due to the short-circuit
+        tracked_components = []
+        components.each do |component|
+          subhash = __node__.attributes.instance_variable_get(component).read(*__path__)
+          unless subhash.nil? # FIXME: nil is used for not present
+            tracked_components << component
+            if subhash.kind_of?(Hash)
+              subhash.keys.each do |key|
+                next if internal_key?(key)
+                internal_set(key, subhash[key])
+              end
+            else
+              break
+            end
+          end
+        end
+        if tracked_components.size == 1
+          # tracked_components is accurate enough to tell us if we're not really merging
+          internal_each do |key, value|
+            value.short_circuit_attr_levels = tracked_components if value.respond_to?(:short_circuit_attr_levels)
+          end
+        end
       end
 
       prepend Chef::Node::Mixin::StateTracking
diff --git a/lib/chef/node/mixin/state_tracking.rb b/lib/chef/node/mixin/state_tracking.rb
index 5958973024..690d261df6 100644
--- a/lib/chef/node/mixin/state_tracking.rb
+++ b/lib/chef/node/mixin/state_tracking.rb
@@ -1,5 +1,5 @@
 #--
-# Copyright:: Copyright 2016, Chef Software, Inc.
+# Copyright:: Copyright 2016-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -24,11 +24,12 @@ class Chef
         attr_reader :__node__
         attr_reader :__precedence__
 
-        def initialize(data = nil, root = self, node = nil, precedence = nil)
+        def initialize(data = nil, root = self, node = nil, precedence = nil, path = nil)
           # __path__ and __root__ must be nil when we call super so it knows
           # to avoid resetting the cache on construction
           data.nil? ? super() : super(data)
-          @__path__ = []
+          @__path__ = path
+          @__path__ ||= []
           @__root__ = root
           @__node__ = node
           @__precedence__ = precedence
@@ -76,9 +77,8 @@ class Chef
           end
         end
 
-        def send_reset_cache(path = nil, key = nil)
-          next_path = [ path, key ].flatten.compact
-          __root__.reset_cache(next_path.first) if !__root__.nil? && __root__.respond_to?(:reset_cache) && !next_path.nil?
+        def send_reset_cache(path)
+          __root__.reset_cache(*path) if !__root__.nil? && __root__.respond_to?(:reset_cache) && !path.nil?
         end
 
         def copy_state_to(ret, next_path)
diff --git a/spec/unit/mixin/powershell_type_coercions_spec.rb b/spec/unit/mixin/powershell_type_coercions_spec.rb
index 6f52abccfb..159a0a8d1d 100644
--- a/spec/unit/mixin/powershell_type_coercions_spec.rb
+++ b/spec/unit/mixin/powershell_type_coercions_spec.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Jay Mundrawala (<jdm@chef.io>)
-# Copyright:: Copyright 2015-2016, Chef Software, Inc.
+# Copyright:: Copyright 2015-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -64,14 +64,15 @@ describe Chef::Mixin::PowershellTypeCoercions do
     end
 
     it "translates a Chef::Node::ImmutableMash like a hash" do
-      test_mash = Chef::Node::ImmutableMash.new({ "a" => 1, "b" => 1.2,
-                                                  "c" => false, "d" => true })
-      expect(test_class.translate_type(test_mash)).to eq("@{a=1;b=1.2;c=$false;d=$true}")
+      node = Chef::Node.new
+      node.default[:test] = { "a" => 1, "b" => 1.2, "c" => false, "d" => true }
+      expect(test_class.translate_type(node[:test])).to eq("@{a=1;b=1.2;c=$false;d=$true}")
     end
 
     it "translates a Chef::Node::ImmutableArray like an array" do
-      test_array = Chef::Node::ImmutableArray.new([true, false])
-      expect(test_class.translate_type(test_array)).to eq("@($true,$false)")
+      node = Chef::Node.new
+      node.default[:test] = [ true, false ]
+      expect(test_class.translate_type(node[:test])).to eq("@($true,$false)")
     end
 
     it "falls back :to_psobject if we have not defined at explicit rule" do
diff --git a/spec/unit/node/attribute_spec.rb b/spec/unit/node/attribute_spec.rb
index cf8d4d4a4f..90b3f1fe51 100644
--- a/spec/unit/node/attribute_spec.rb
+++ b/spec/unit/node/attribute_spec.rb
@@ -171,6 +171,7 @@ describe Chef::Node::Attribute do
     }
     @automatic_hash = { "week" => "friday" }
     @attributes = Chef::Node::Attribute.new(@attribute_hash, @default_hash, @override_hash, @automatic_hash, node)
+    allow(node).to receive(:attributes).and_return(@attributes)
   end
 
   describe "initialize" do
@@ -179,13 +180,14 @@ describe Chef::Node::Attribute do
     end
 
     it "should take an Automatioc, Normal, Default and Override hash" do
-      expect { Chef::Node::Attribute.new({}, {}, {}, {}) }.not_to raise_error
+      expect { Chef::Node::Attribute.new({}, {}, {}, {}, node) }.not_to raise_error
     end
 
     [ :normal, :default, :override, :automatic ].each do |accessor|
       it "should set #{accessor}" do
-        na = Chef::Node::Attribute.new({ :normal => true }, { :default => true }, { :override => true }, { :automatic => true })
-        expect(na.send(accessor)).to eq({ accessor.to_s => true })
+        @attributes = Chef::Node::Attribute.new({ :normal => true }, { :default => true }, { :override => true }, { :automatic => true }, node)
+        allow(node).to receive(:attributes).and_return(@attributes)
+        expect(@attributes.send(accessor)).to eq({ accessor.to_s => true })
       end
     end
 
@@ -330,7 +332,8 @@ describe Chef::Node::Attribute do
     end
 
     it "merges nested hashes between precedence levels" do
-      @attributes = Chef::Node::Attribute.new({}, {}, {}, {})
+      @attributes = Chef::Node::Attribute.new({}, {}, {}, {}, node)
+      allow(node).to receive(:attributes).and_return(@attributes)
       @attributes.env_default = { "a" => { "b" => { "default" => "default" } } }
       @attributes.normal = { "a" => { "b" => { "normal" => "normal" } } }
       @attributes.override = { "a" => { "override" => "role" } }
@@ -584,8 +587,10 @@ describe Chef::Node::Attribute do
           "one" => { "six" => "seven" },
           "snack" => "cookies",
         },
-        {}
+        {},
+        node
       )
+      allow(node).to receive(:attributes).and_return(@attributes)
     end
 
     it "should yield each top level key" do
@@ -632,8 +637,10 @@ describe Chef::Node::Attribute do
           "one" => "six",
           "snack" => "cookies",
         },
-        {}
+        {},
+        node
       )
+      allow(node).to receive(:attributes).and_return(@attributes)
     end
 
     it "should yield each top level key and value, post merge rules" do
@@ -670,8 +677,10 @@ describe Chef::Node::Attribute do
           "one" => "six",
           "snack" => "cookies",
         },
-        {}
+        {},
+        node
       )
+      allow(node).to receive(:attributes).and_return(@attributes)
     end
 
     it "should respond to each_key" do
@@ -706,8 +715,10 @@ describe Chef::Node::Attribute do
           "one" => "six",
           "snack" => "cookies",
         },
-        {}
+        {},
+        node
       )
+      allow(node).to receive(:attributes).and_return(@attributes)
     end
 
     it "should respond to each_pair" do
@@ -742,8 +753,10 @@ describe Chef::Node::Attribute do
           "one" => "six",
           "snack" => "cookies",
         },
-        {}
+        {},
+        node
       )
+      allow(node).to receive(:attributes).and_return(@attributes)
     end
 
     it "should respond to each_value" do
@@ -786,9 +799,10 @@ describe Chef::Node::Attribute do
           "one" => "six",
           "snack" => "cookies",
         },
-        {}
+        {},
+        node
       )
-      @empty = Chef::Node::Attribute.new({}, {}, {}, {})
+      allow(node).to receive(:attributes).and_return(@attributes)
     end
 
     it "should respond to empty?" do
@@ -796,7 +810,9 @@ describe Chef::Node::Attribute do
     end
 
     it "should return true when there are no keys" do
-      expect(@empty.empty?).to eq(true)
+      @attributes = Chef::Node::Attribute.new({}, {}, {}, {}, node)
+      allow(node).to receive(:attributes).and_return(@attributes)
+      expect(@attributes.empty?).to eq(true)
     end
 
     it "should return false when there are keys" do
@@ -820,8 +836,10 @@ describe Chef::Node::Attribute do
           "one" => "six",
           "snack" => "cookies",
         },
-        {}
+        {},
+        node
       )
+      allow(node).to receive(:attributes).and_return(@attributes)
     end
 
     it "should respond to fetch" do
@@ -877,8 +895,10 @@ describe Chef::Node::Attribute do
           "one" => "six",
           "snack" => "cookies",
         },
-        {}
+        {},
+        node
       )
+      allow(node).to receive(:attributes).and_return(@attributes)
     end
 
     it "should respond to has_value?" do
@@ -922,8 +942,10 @@ describe Chef::Node::Attribute do
           "one" => "six",
           "snack" => "cookies",
         },
-        {}
+        {},
+        node
       )
+      allow(node).to receive(:attributes).and_return(@attributes)
     end
 
     it "should respond to index" do
@@ -963,8 +985,10 @@ describe Chef::Node::Attribute do
           "one" => "six",
           "snack" => "cookies",
         },
-        {}
+        {},
+        node
       )
+      allow(node).to receive(:attributes).and_return(@attributes)
     end
 
     it "should respond to values" do
@@ -999,8 +1023,10 @@ describe Chef::Node::Attribute do
           "one" => "six",
           "snack" => "cookies",
         },
-        {}
+        {},
+        node
       )
+      allow(node).to receive(:attributes).and_return(@attributes)
     end
 
     it "should respond to select" do
@@ -1049,10 +1075,11 @@ describe Chef::Node::Attribute do
           "one" => "six",
           "snack" => "cookies",
         },
-        {}
+        {},
+        node
       )
+      allow(node).to receive(:attributes).and_return(@attributes)
 
-      @empty = Chef::Node::Attribute.new({}, {}, {}, {})
     end
 
     it "should respond to size" do
@@ -1064,7 +1091,9 @@ describe Chef::Node::Attribute do
     end
 
     it "should return 0 for an empty attribute" do
-      expect(@empty.size).to eq(0)
+      @attributes = Chef::Node::Attribute.new({}, {}, {}, {}, node)
+      allow(node).to receive(:attributes).and_return(@attributes)
+      expect(@attributes.size).to eq(0)
     end
 
     it "should return the number of pairs" do
@@ -1092,8 +1121,9 @@ describe Chef::Node::Attribute do
 
   describe "to_s" do
     it "should output simple attributes" do
-      attributes = Chef::Node::Attribute.new(nil, nil, nil, nil)
-      expect(attributes.to_s).to eq("{}")
+      @attributes = Chef::Node::Attribute.new(nil, nil, nil, nil, node)
+      allow(node).to receive(:attributes).and_return(@attributes)
+      expect(@attributes.to_s).to eq("{}")
     end
 
     it "should output merged attributes" do
@@ -1105,8 +1135,9 @@ describe Chef::Node::Attribute do
           "b" => 3,
           "c" => 4,
       }
-      attributes = Chef::Node::Attribute.new(nil, default_hash, override_hash, nil)
-      expect(attributes.to_s).to eq('{"a"=>1, "b"=>3, "c"=>4}')
+      @attributes = Chef::Node::Attribute.new(nil, default_hash, override_hash, nil, node)
+      allow(node).to receive(:attributes).and_return(@attributes)
+      expect(@attributes.to_s).to eq('{"b"=>3, "c"=>4, "a"=>1}')
     end
   end
 
diff --git a/spec/unit/node/immutable_collections_spec.rb b/spec/unit/node/immutable_collections_spec.rb
index 2d3392041c..96adedddcb 100644
--- a/spec/unit/node/immutable_collections_spec.rb
+++ b/spec/unit/node/immutable_collections_spec.rb
@@ -20,13 +20,18 @@ require "spec_helper"
 require "chef/node/immutable_collections"
 
 describe Chef::Node::ImmutableMash do
+
   before do
-    @data_in = { "top" => { "second_level" => "some value" },
-                 "top_level_2" => %w{array of values},
-                 "top_level_3" => [{ "hash_array" => 1, "hash_array_b" => 2 }],
-                 "top_level_4" => { "level2" => { "key" => "value" } },
+    @data_in = { "key" =>
+                 { "top" => { "second_level" => "some value" },
+                   "top_level_2" => %w{array of values},
+                   "top_level_3" => [{ "hash_array" => 1, "hash_array_b" => 2 }],
+                   "top_level_4" => { "level2" => { "key" => "value" } },
+                 },
     }
-    @immutable_mash = Chef::Node::ImmutableMash.new(@data_in)
+    @node = Chef::Node.new()
+    @node.attributes.default = @data_in
+    @immutable_mash = @node["key"]
   end
 
   it "element references like regular hash" do
@@ -57,9 +62,9 @@ describe Chef::Node::ImmutableMash do
   # we only ever absorb VividMashes from other precedence levels, which already have
   # been coerced to only have string keys, so we do not need to do that work twice (performance).
   it "does not call convert_value like Mash/VividMash" do
-    @mash = Chef::Node::ImmutableMash.new({ test: "foo", "test2" => "bar" })
-    expect(@mash[:test]).to eql("foo")
-    expect(@mash["test2"]).to eql("bar")
+    @node.attributes.default = { test: "foo", "test2" => "bar" }
+    expect(@node[:test]).to eql("foo")
+    expect(@node["test2"]).to eql("bar")
   end
 
   describe "to_hash" do
@@ -80,7 +85,7 @@ describe Chef::Node::ImmutableMash do
     end
 
     it "should create a mash with the same content" do
-      expect(@copy).to eq(@immutable_mash)
+      expect(@copy).to eql(@immutable_mash)
     end
 
     it "should allow mutation" do
@@ -175,9 +180,11 @@ end
 describe Chef::Node::ImmutableArray do
 
   before do
-    @immutable_array = Chef::Node::ImmutableArray.new(%w{foo bar baz} + Array(1..3) + [nil, true, false, [ "el", 0, nil ] ])
-    immutable_mash = Chef::Node::ImmutableMash.new({ "m" => "m" })
-    @immutable_nested_array = Chef::Node::ImmutableArray.new(["level1", @immutable_array, immutable_mash])
+    @node = Chef::Node.new()
+    @node.attributes.default = { "key" => ["level1", %w{foo bar baz} + Array(1..3) + [nil, true, false, [ "el", 0, nil ] ], { "m" => "m" }] }
+    @immutable_array = @node["key"][1]
+    @immutable_mash = @node["key"][2]
+    @immutable_nested_array = @node["key"]
   end
 
   ##
@@ -249,7 +256,7 @@ describe Chef::Node::ImmutableArray do
     end
 
     it "should create an array with the same content" do
-      expect(@copy).to eq(@immutable_nested_array)
+      expect(@immutable_nested_array).to eq(@copy)
     end
 
     it "should allow mutation" do
@@ -301,7 +308,7 @@ describe Chef::Node::ImmutableArray do
     end
 
     it "should create an array with the same content" do
-      expect(@copy).to eq(@immutable_nested_array)
+      expect(@immutable_nested_array).to eq(@copy)
     end
 
     it "should allow mutation" do
@@ -314,4 +321,11 @@ describe Chef::Node::ImmutableArray do
       expect(@immutable_array[1, 2]).to eql(%w{bar baz})
     end
   end
+
+  describe "uniq" do
+    it "works" do
+      @node.attributes.default = { "key" => %w{foo bar foo baz bar} }
+      expect(@node["key"].uniq).to eql(%w{foo bar baz})
+    end
+  end
 end
diff --git a/spec/unit/node/vivid_mash_spec.rb b/spec/unit/node/vivid_mash_spec.rb
index 4898c22380..651cfa0058 100644
--- a/spec/unit/node/vivid_mash_spec.rb
+++ b/spec/unit/node/vivid_mash_spec.rb
@@ -1,5 +1,5 @@
 #
-# Copyright:: Copyright 2016, Chef Software Inc.
+# Copyright:: Copyright 2016-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -60,7 +60,7 @@ describe Chef::Node::VividMash do
     end
 
     it "deep converts values through arrays" do
-      expect(root).to receive(:reset_cache).with("foo")
+      expect(root).to receive(:reset_cache).with(no_args)
       vivid["foo"] = [ { :bar => true } ]
       expect(vivid["foo"].class).to eql(Chef::Node::AttrArray)
       expect(vivid["foo"][0].class).to eql(Chef::Node::VividMash)
@@ -68,7 +68,7 @@ describe Chef::Node::VividMash do
     end
 
     it "deep converts values through nested arrays" do
-      expect(root).to receive(:reset_cache).with("foo")
+      expect(root).to receive(:reset_cache).with(no_args)
       vivid["foo"] = [ [ { :bar => true } ] ]
       expect(vivid["foo"].class).to eql(Chef::Node::AttrArray)
       expect(vivid["foo"][0].class).to eql(Chef::Node::AttrArray)
@@ -77,7 +77,7 @@ describe Chef::Node::VividMash do
     end
 
     it "deep converts values through hashes" do
-      expect(root).to receive(:reset_cache).with("foo")
+      expect(root).to receive(:reset_cache).with(no_args)
       vivid["foo"] = { baz: { :bar => true } }
       expect(vivid["foo"]).to be_an_instance_of(Chef::Node::VividMash)
       expect(vivid["foo"]["baz"]).to be_an_instance_of(Chef::Node::VividMash)
@@ -184,42 +184,55 @@ describe Chef::Node::VividMash do
 
     it "should deeply autovivify" do
       expect(root).to receive(:reset_cache).at_least(:once).with("one")
+      expect(root).to receive(:reset_cache).at_least(:once).with("one", "five")
+      expect(root).to receive(:reset_cache).at_least(:once).with("one", "five", "six")
+      expect(root).to receive(:reset_cache).at_least(:once).with("one", "five", "six", "seven")
+      expect(root).to receive(:reset_cache).at_least(:once).with("one", "five", "six", "seven", "eight")
       vivid.write("one", "five", "six", "seven", "eight", "nine", "ten")
       expect(vivid["one"]["five"]["six"]["seven"]["eight"]["nine"]).to eql("ten")
     end
 
     it "should raise an exception if you overwrite an array with a hash" do
+      expect(root).to receive(:reset_cache).at_least(:once).with(no_args)
       expect(root).to receive(:reset_cache).at_least(:once).with("array")
       vivid.write("array", "five", "six")
       expect(vivid).to eql({ "one" => { "two" => { "three" => "four" } }, "array" => { "five" => "six" }, "nil" => nil })
     end
 
     it "should raise an exception if you traverse through an array with a hash" do
+      expect(root).to receive(:reset_cache).at_least(:once).with(no_args)
       expect(root).to receive(:reset_cache).at_least(:once).with("array")
+      expect(root).to receive(:reset_cache).at_least(:once).with("array", "five")
       vivid.write("array", "five", "six", "seven")
       expect(vivid).to eql({ "one" => { "two" => { "three" => "four" } }, "array" => { "five" => { "six" => "seven" } }, "nil" => nil })
     end
 
     it "should raise an exception if you overwrite a string with a hash" do
-      expect(root).to receive(:reset_cache).at_least(:once).with("one")
+      expect(root).to receive(:reset_cache).at_least(:once).with("one", "two")
+      expect(root).to receive(:reset_cache).at_least(:once).with("one", "two", "three")
       vivid.write("one", "two", "three", "four", "five")
       expect(vivid).to eql({ "one" => { "two" => { "three" => { "four" => "five" } } }, "array" => [ 0, 1, 2 ], "nil" => nil })
     end
 
     it "should raise an exception if you traverse through a string with a hash" do
-      expect(root).to receive(:reset_cache).at_least(:once).with("one")
+      expect(root).to receive(:reset_cache).at_least(:once).with("one", "two")
+      expect(root).to receive(:reset_cache).at_least(:once).with("one", "two", "three")
+      expect(root).to receive(:reset_cache).at_least(:once).with("one", "two", "three", "four")
       vivid.write("one", "two", "three", "four", "five", "six")
       expect(vivid).to eql({ "one" => { "two" => { "three" => { "four" => { "five" => "six" } } } }, "array" => [ 0, 1, 2 ], "nil" => nil })
     end
 
     it "should raise an exception if you overwrite a nil with a hash" do
+      expect(root).to receive(:reset_cache).at_least(:once).with(no_args)
       expect(root).to receive(:reset_cache).at_least(:once).with("nil")
       vivid.write("nil", "one", "two")
       expect(vivid).to eql({ "one" => { "two" => { "three" => "four" } }, "array" => [ 0, 1, 2 ], "nil" => { "one" => "two" } })
     end
 
     it "should raise an exception if you traverse through a nil with a hash" do
+      expect(root).to receive(:reset_cache).at_least(:once).with(no_args)
       expect(root).to receive(:reset_cache).at_least(:once).with("nil")
+      expect(root).to receive(:reset_cache).at_least(:once).with("nil", "one")
       vivid.write("nil", "one", "two", "three")
       expect(vivid).to eql({ "one" => { "two" => { "three" => "four" } }, "array" => [ 0, 1, 2 ], "nil" => { "one" => { "two" => "three" } } })
     end
@@ -240,6 +253,10 @@ describe Chef::Node::VividMash do
 
     it "should deeply autovivify" do
       expect(root).to receive(:reset_cache).at_least(:once).with("one")
+      expect(root).to receive(:reset_cache).at_least(:once).with("one", "five")
+      expect(root).to receive(:reset_cache).at_least(:once).with("one", "five", "six")
+      expect(root).to receive(:reset_cache).at_least(:once).with("one", "five", "six", "seven")
+      expect(root).to receive(:reset_cache).at_least(:once).with("one", "five", "six", "seven", "eight")
       vivid.write!("one", "five", "six", "seven", "eight", "nine", "ten")
       expect(vivid["one"]["five"]["six"]["seven"]["eight"]["nine"]).to eql("ten")
     end
@@ -295,7 +312,7 @@ describe Chef::Node::VividMash do
     end
 
     it "should unlink hashes" do
-      expect(root).to receive(:reset_cache).at_least(:once).with("one")
+      expect(root).to receive(:reset_cache).at_least(:once).with(no_args)
       expect( vivid.unlink("one") ).to eql({ "two" => { "three" => "four" } })
       expect(vivid).to eql({ "array" => [ 0, 1, 2 ], "nil" => nil })
     end
@@ -307,7 +324,7 @@ describe Chef::Node::VividMash do
     end
 
     it "should unlink nil" do
-      expect(root).to receive(:reset_cache).at_least(:once).with("nil")
+      expect(root).to receive(:reset_cache).at_least(:once).with(no_args)
       expect(vivid.unlink("nil")).to eql(nil)
       expect(vivid).to eql({ "one" => { "two" => { "three" => "four" } }, "array" => [ 0, 1, 2 ] })
     end
@@ -327,7 +344,7 @@ describe Chef::Node::VividMash do
     end
 
     it "should unlink! hashes" do
-      expect(root).to receive(:reset_cache).at_least(:once).with("one")
+      expect(root).to receive(:reset_cache).at_least(:once).with(no_args)
       expect( vivid.unlink!("one") ).to eql({ "two" => { "three" => "four" } })
       expect(vivid).to eql({ "array" => [ 0, 1, 2 ], "nil" => nil })
     end
@@ -339,7 +356,7 @@ describe Chef::Node::VividMash do
     end
 
     it "should unlink! nil" do
-      expect(root).to receive(:reset_cache).at_least(:once).with("nil")
+      expect(root).to receive(:reset_cache).at_least(:once).with(no_args)
       expect(vivid.unlink!("nil")).to eql(nil)
       expect(vivid).to eql({ "one" => { "two" => { "three" => "four" } }, "array" => [ 0, 1, 2 ] })
     end