fix autovivification

3 files changed, 88 insertions, 33 deletions

diff --git a/lib/chef/node/attribute.rb b/lib/chef/node/attribute.rb
index 6ff20dc67f..3eb6449046 100644
--- a/lib/chef/node/attribute.rb
+++ b/lib/chef/node/attribute.rb
@@ -336,7 +336,7 @@ class Chef
        # equivalent to: force_default!['foo']['bar'].delete('baz')
        def rm_default(*args)
          reset
-         remove_from_precedence_level(force_default!, *args)
+         remove_from_precedence_level(force_default!(autovivify: false), *args)
        end
 
        # clears attributes from normal precedence
@@ -344,7 +344,7 @@ class Chef
        # equivalent to: normal!['foo']['bar'].delete('baz')
        def rm_normal(*args)
          reset
-         remove_from_precedence_level(normal!, *args)
+         remove_from_precedence_level(normal!(autovivify: false), *args)
        end
 
        # clears attributes from all override precedence levels
@@ -352,7 +352,7 @@ class Chef
        # equivalent to: force_override!['foo']['bar'].delete('baz')
        def rm_override(*args)
          reset
-         remove_from_precedence_level(force_override!, *args)
+         remove_from_precedence_level(force_override!(autovivify: false), *args)
        end
 
        #
@@ -360,28 +360,33 @@ class Chef
        #
 
        # sets default attributes without merging
-       def default!
-         MultiMash.new(@default)
+       def default!(opts={})
+         reset
+         MultiMash.new(self, @default, [], opts)
        end
 
        # sets normal attributes without merging
-       def normal!
-         MultiMash.new(@normal)
+       def normal!(opts={})
+         reset
+         MultiMash.new(self, @normal, [], opts)
        end
 
        # sets override attributes without merging
-       def override!
-         MultiMash.new(@override)
+       def override!(opts={})
+         reset
+         MultiMash.new(self, @override, [], opts)
        end
 
        # clears from all default precedence levels and then sets force_default
-       def force_default!
-         MultiMash.new(@default, @env_default, @role_default, @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!
-         MultiMash.new(@override, @env_override, @role_override, @force_override)
+       def force_override!(opts={})
+         reset
+         MultiMash.new(self, @force_override, [@override, @env_override, @role_override], opts)
        end
 
        #
diff --git a/lib/chef/node/attribute_collections.rb b/lib/chef/node/attribute_collections.rb
index 596e2b5586..c8bc618762 100644
--- a/lib/chef/node/attribute_collections.rb
+++ b/lib/chef/node/attribute_collections.rb
@@ -218,42 +218,66 @@ class Chef
     # by deleting the subtree and then assigning to the last mash which passed in
     # the initializer.
     #
+    # A lot of the complexity of this class comes from the fact that at any key
+    # value some or all of the mashes may walk off their ends and become nil or
+    # true or something.  The schema may change so that one precidence leve may
+    # be 'true' object and another may be a VividMash.  It is also possible that
+    # one or many of them may transition from VividMashes to Hashes or Arrays.
+    #
+    # It also supports the case where you may be deleting a key using node.rm
+    # in which case if intermediate keys all walk off into nil then you don't want
+    # to be autovivifying keys as you go.  On the other hand you may be using
+    # node.force_default! in which case you'll wind up with a []= operator at the
+    # end and you want autovivification, so we conditionally have to support either
+    # operation.
+    #
+    # @todo: can we have an autovivify class that decorates a class that doesn't
+    # autovivify or something so that the code is less awful?
+    #
     class MultiMash
+      attr_reader :root
       attr_reader :mashes
+      attr_reader :opts
+      attr_reader :primary_mash
 
       # Initialize with an array of mashes.  For the delete return value to work
       # properly the mashes must come from the same attribute level (i.e. all
       # override or all default, but not a mix of both).
-      def initialize(*mashes)
+      def initialize(root, primary_mash, mashes, opts={})
+        @root = root
+        @primary_mash = primary_mash
         @mashes = mashes
+        @opts = opts
+        @opts[:autovivify] = true if @opts[:autovivify].nil?
       end
 
       def [](key)
+        # handle the secondary mashes
         new_mashes = []
         mashes.each do |mash|
-          if mash.respond_to?(:[])
-            if mash.respond_to?(:has_key?)
-              if mash.has_key?(key)
-                new_mashes.push(mash[key]) if mash[key].respond_to?(:[])
-              end
-            elsif !mash[key].nil?
-              new_mashes.push(mash[key]) if mash[key].respond_to?(:[])
-            end
-          end
+          new_mash = safe_evalute_key(mash, key)
+          # secondary mashes never autovivify so once they fall into nil, we just stop tracking them
+          new_mashes.push(new_mash) unless new_mash.nil?
+        end
+
+        new_primary_mash = safe_evalute_key(primary_mash, key)
+
+        if new_primary_mash.nil? && @opts[:autovivify]
+          primary_mash[key] = VividMash.new(root)
+          new_primary_mash = primary_mash[key]
         end
-        MultiMash.new(*new_mashes)
+
+        MultiMash.new(root, new_primary_mash, new_mashes, opts)
       end
 
       def []=(key, value)
-        if mashes.count == 0
-          raise TypeError, "Intermediate attribute keys must be created before attempt to set/remove an attribute (no autovivification)"
+        if primary_mash.nil?
+          # This theoretically should never happen since node#force_default! setter methods will autovivify and
+          # node#rm methods do not end in #[]= operators.
+          raise TypeError, "No autovivification was specified initially on a method chain ending in assignment"
         end
         ret = delete(key)
-        unless mashes.last.respond_to?(:[]=)
-          mashes.pop
-          mashes << {}
-        end
-        mashes.last[key] = value
+        primary_mash[key] = value
         ret
       end
 
@@ -272,11 +296,29 @@ class Chef
         ret = mashes.inject(Mash.new) do |merged, mash|
           Chef::Mixin::DeepMerge.merge(merged, mash)
         end
+        ret = Chef::Mixin::DeepMerge.merge(ret, primary_mash)
         mashes.each do |mash|
           mash.delete(key) if mash.respond_to?(:delete)
         end
+        primary_mash.delete(key) if primary_mash.respond_to?(:delete)
         ret[key]
       end
+
+      private
+
+      def safe_evalute_key(mash, key)
+        if mash.respond_to?(:[])
+          if mash.respond_to?(:has_key?)
+            if mash.has_key?(key)
+              return mash[key] if mash[key].respond_to?(:[])
+            end
+          elsif !mash[key].nil?
+            return mash[key] if mash[key].respond_to?(:[])
+          end
+        end
+        return nil
+      end
+
     end
 
   end
diff --git a/spec/unit/node_spec.rb b/spec/unit/node_spec.rb
index 92d4422c3d..47605de863 100644
--- a/spec/unit/node_spec.rb
+++ b/spec/unit/node_spec.rb
@@ -566,6 +566,13 @@ describe Chef::Node do
         })
       end
 
+      it "will autovivify" do
+        node.force_default!["mysql"]["server"] = {
+          "data_dir" => "/my_raid_volume/lib/mysql",
+        }
+        expect( node["mysql"]["server"]["data_dir"] ).to eql("/my_raid_volume/lib/mysql")
+      end
+
       it "lower precedence levels aren't removed" do
         node.role_override["mysql"]["server"]["port"] = 1234
         node.override["mysql"]["server"]["port"] = 2345
@@ -587,9 +594,10 @@ describe Chef::Node do
       it "when overwriting a non-hash/array" do
         node.override["mysql"] = false
         node.force_override["mysql"] = true
-        expect { node.force_override!["mysql"]["server"] = {
+        node.force_override!["mysql"]["server"] = {
           "data_dir" => "/my_raid_volume/lib/mysql",
-        } }.to raise_error(TypeError)
+        }
+        expect( node["mysql"]["server"]["data_dir"] ).to eql("/my_raid_volume/lib/mysql")
       end
 
       it "when overwriting an array with a hash" do