Chef 15 node attribute array fixeslcg/chef-15-attribute-array-fixes

Doing something like:

```
node.default["foo"] = []
node.default["foo"] << { "bar" => "baz }
```

Would result in a Hash instead of a VividMash inserted into the
AttrArray, so that:

```
node.default["foo"][0]["bar"] # gives the correct result
node.default["foo"][0][:bar]  # does not work due to the sub-Hash not
                              # converting keys
```

This fixes that behavior, but is likely a breaking change to someone
who expects to ingest hashes with symbol keys and get back out symbol
keys, they will now get converted to strings.  This breaking change
is fully 100% intentional.  This makes the node attribute structure
more consistent and predictable.

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

5 files changed, 186 insertions, 50 deletions

diff --git a/lib/chef/node/attribute_collections.rb b/lib/chef/node/attribute_collections.rb
index f62eceb646..382ecfba91 100644
--- a/lib/chef/node/attribute_collections.rb
+++ b/lib/chef/node/attribute_collections.rb
@@ -18,6 +18,9 @@
 
 require "chef/node/common_api"
 require "chef/node/mixin/state_tracking"
+require "chef/node/mixin/immutablize_array"
+require "chef/node/mixin/immutablize_hash"
+require "chef/node/mixin/mashy_array"
 
 class Chef
   class Node
@@ -26,36 +29,9 @@ class Chef
     # "root" (Chef::Node::Attribute) object, and will trigger a cache
     # invalidation on that object when mutated.
     class AttrArray < Array
-      MUTATOR_METHODS = [
-        :<<,
-        :[]=,
-        :clear,
-        :collect!,
-        :compact!,
-        :default=,
-        :default_proc=,
-        :delete_at,
-        :delete_if,
-        :fill,
-        :flatten!,
-        :insert,
-        :keep_if,
-        :map!,
-        :merge!,
-        :pop,
-        :push,
-        :update,
-        :reject!,
-        :reverse!,
-        :replace,
-        :select!,
-        :shift,
-        :slice!,
-        :sort!,
-        :sort_by!,
-        :uniq!,
-        :unshift,
-      ].freeze
+      include Chef::Node::Mixin::MashyArray
+
+      MUTATOR_METHODS = Chef::Node::Mixin::ImmutablizeArray::DISALLOWED_MUTATOR_METHODS
 
       # For all of the methods that may mutate an Array, we override them to
       # also invalidate the cached merged_attributes on the root
@@ -130,27 +106,11 @@ class Chef
       # Methods that mutate a VividMash. Each of them is overridden so that it
       # also invalidates the cached merged_attributes on the root Attribute
       # object.
-      MUTATOR_METHODS = [
-        :clear,
-        :delete_if,
-        :keep_if,
-        :merge!,
-        :update,
-        :reject!,
-        :replace,
-        :select!,
-        :shift,
-      ].freeze
+      MUTATOR_METHODS = Chef::Node::Mixin::ImmutablizeHash::DISALLOWED_MUTATOR_METHODS - [ :write, :write!, :unlink, :unlink! ]
 
       # For all of the mutating methods on Mash, override them so that they
       # also invalidate the cached `merged_attributes` on the root Attribute
       # object.
-
-      def delete(key, &block)
-        send_reset_cache(__path__, key)
-        super
-      end
-
       MUTATOR_METHODS.each do |mutator|
         define_method(mutator) do |*args, &block|
           send_reset_cache
@@ -158,6 +118,11 @@ class Chef
         end
       end
 
+      def delete(key, &block)
+        send_reset_cache(__path__, key)
+        super
+      end
+
       def initialize(data = {})
         super(data)
       end
diff --git a/lib/chef/node/mixin/immutablize_array.rb b/lib/chef/node/mixin/immutablize_array.rb
index 000a088410..b5e0993cfa 100644
--- a/lib/chef/node/mixin/immutablize_array.rb
+++ b/lib/chef/node/mixin/immutablize_array.rb
@@ -159,7 +159,6 @@ class Chef
           :sort_by!,
           :uniq!,
           :unshift,
-          :update,
         ].freeze
 
         # Redefine all of the methods that mutate a Hash to raise an error when called.
diff --git a/lib/chef/node/mixin/mashy_array.rb b/lib/chef/node/mixin/mashy_array.rb
new file mode 100644
index 0000000000..31506c5bbf
--- /dev/null
+++ b/lib/chef/node/mixin/mashy_array.rb
@@ -0,0 +1,68 @@
+#--
+# Copyright:: Copyright 2016-2018, Chef Software Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+class Chef
+  class Node
+    module Mixin
+      # missing methods for Arrays similar to Chef::Mash methods that call
+      # convert_value correctly.
+      module MashyArray
+        def <<(obj)
+          super(convert_value(obj))
+        end
+
+        def []=(*keys, value)
+          super(*keys, convert_value(value))
+        end
+
+        def push(*objs)
+          objs = objs.map { |obj| convert_value(obj) }
+          super(*objs)
+        end
+
+        def unshift(*objs)
+          objs = objs.map { |obj| convert_value(obj) }
+          super(*objs)
+        end
+
+        def insert(index, *objs)
+          objs = objs.map { |obj| convert_value(obj) }
+          super(index, *objs)
+        end
+
+        def collect!(&block)
+          super
+          map! { |x| convert_value(x) }
+        end
+
+        def map!(&block)
+          super
+          super { |x| convert_value(x) }
+        end
+
+        def fill(*args, &block)
+          super
+          map! { |x| convert_value(x) }
+        end
+
+        def replace(obj)
+          super(convert_value(obj))
+        end
+      end
+    end
+  end
+end
diff --git a/spec/unit/node/immutable_collections_spec.rb b/spec/unit/node/immutable_collections_spec.rb
index 273c3d2704..2208c45717 100644
--- a/spec/unit/node/immutable_collections_spec.rb
+++ b/spec/unit/node/immutable_collections_spec.rb
@@ -209,7 +209,6 @@ describe Chef::Node::ImmutableArray do
     :merge!,
     :pop,
     :push,
-    :update,
     :reject!,
     :reverse!,
     :replace,
diff --git a/spec/unit/node/vivid_mash_spec.rb b/spec/unit/node/vivid_mash_spec.rb
index e1021ba0c0..cfdc813b50 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-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -351,3 +351,108 @@ describe Chef::Node::VividMash do
     end
   end
 end
+
+describe Chef::Node::AttrArray do
+  let(:root) { instance_double(Chef::Node::Attribute) }
+
+  let(:array) do
+    Chef::Node::AttrArray.new(
+      %w{zero one two},
+      root
+    )
+  end
+
+  context "#<<" do
+    it "converts a Hash appended with #<< to a VividMash" do
+      array << { "three" => "four" }
+      expect(array[3].class).to eql(Chef::Node::VividMash)
+    end
+
+    it "deeply converts objects appended with #<<" do
+      array << [ { "three" => [ 0, 1] } ]
+      expect(array[3].class).to eql(Chef::Node::AttrArray)
+      expect(array[3][0].class).to eql(Chef::Node::VividMash)
+      expect(array[3][0]["three"].class).to eql(Chef::Node::AttrArray)
+    end
+  end
+
+  context "#[]=" do
+    it "assigning a Hash into an array converts it to VividMash" do
+      array[0] = { "zero" => "zero2" }
+      expect(array[0].class).to eql(Chef::Node::VividMash)
+    end
+  end
+
+  context "#push" do
+    it "pushing a Hash into an array converts it to VividMash" do
+      array.push({ "three" => "four" })
+      expect(array[3].class).to eql(Chef::Node::VividMash)
+    end
+  end
+
+  context "#unshift" do
+    it "unshifting a Hash into an array converts it to VividMash" do
+      array.unshift({ "zero" => "zero2" })
+      expect(array[0].class).to eql(Chef::Node::VividMash)
+    end
+  end
+
+  context "#insert" do
+    it "inserting a Hash into an array converts it to VividMash" do
+      array.insert(1, { "zero" => "zero2" })
+      expect(array[1].class).to eql(Chef::Node::VividMash)
+    end
+  end
+
+  context "#collect!" do
+    it "converts Hashes" do
+      array.collect! { |x| { "zero" => "zero2" } }
+      expect(array[1].class).to eql(Chef::Node::VividMash)
+    end
+  end
+
+  context "#map!" do
+    it "converts Hashes" do
+      array.map! { |x| { "zero" => "zero2" } }
+      expect(array[1].class).to eql(Chef::Node::VividMash)
+    end
+  end
+
+  context "#compact!" do
+    it "VividMashes remain VividMashes" do
+      array = Chef::Node::AttrArray.new(
+          [ nil, { "one" => "two" }, nil ],
+          root
+      )
+      expect(array[1].class).to eql(Chef::Node::VividMash)
+      array.compact!
+      expect(array[0].class).to eql(Chef::Node::VividMash)
+    end
+  end
+
+  context "#fill" do
+    it "inserts VividMashes for Hashes" do
+      array.fill({ "one" => "two" })
+      expect(array[0].class).to eql(Chef::Node::VividMash)
+    end
+  end
+
+  context "#flatten!" do
+    it "flattens sub-arrays maintaining VividMashes in them" do
+      array = Chef::Node::AttrArray.new(
+          [ [ { "one" => "two" } ], [ { "one" => "two" } ] ],
+          root
+      )
+      expect(array[0][0].class).to eql(Chef::Node::VividMash)
+      array.flatten!
+      expect(array[0].class).to eql(Chef::Node::VividMash)
+    end
+  end
+
+  context "#replace" do
+    it "replaces the array converting hashes to mashes" do
+      array.replace([ { "foo" => "bar" } ])
+      expect(array[0].class).to eql(Chef::Node::VividMash)
+    end
+  end
+end