Merge pull request #5896 from chef/lcg/deep-dup-attrs

Chef-13:  properly deep dup Node#to_hash

3 files changed, 165 insertions, 20 deletions

diff --git a/lib/chef/node/immutable_collections.rb b/lib/chef/node/immutable_collections.rb
index 12ee2e5dd8..13a8aefe97 100644
--- a/lib/chef/node/immutable_collections.rb
+++ b/lib/chef/node/immutable_collections.rb
@@ -70,21 +70,20 @@ class Chef
       end
 
       def to_a
-        a = Array.new
-        each do |v|
-          a <<
-            case v
-            when ImmutableArray
-              v.to_a
-            when ImmutableMash
-              v.to_hash
-            else
-              v
-            end
-        end
-        a
+        Array.new(map do |v|
+          case v
+          when ImmutableArray
+            v.to_a
+          when ImmutableMash
+            v.to_h
+          else
+            safe_dup(v)
+          end
+        end)
       end
 
+      alias_method :to_array, :to_a
+
       # for consistency's sake -- integers 'converted' to integers
       def convert_key(key)
         key
@@ -127,6 +126,10 @@ class Chef
 
       # 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
@@ -137,25 +140,38 @@ class Chef
       # Of course, 'default' has a specific meaning in Chef-land
 
       def dup
-        Mash.new(self)
+        h = Mash.new
+        each_pair do |k, v|
+          h[k] = safe_dup(v)
+        end
+        h
       end
 
-      def to_hash
+      def to_h
         h = Hash.new
         each_pair do |k, v|
           h[k] =
             case v
             when ImmutableMash
-              v.to_hash
+              v.to_h
             when ImmutableArray
               v.to_a
             else
-              v
+              safe_dup(v)
             end
         end
         h
       end
 
+      alias_method :to_hash, :to_h
+
+      # For elements like Fixnums, true, nil...
+      def safe_dup(e)
+        e.dup
+      rescue TypeError
+        e
+      end
+
       prepend Chef::Node::Mixin::StateTracking
       prepend Chef::Node::Mixin::ImmutablizeHash
     end
diff --git a/spec/unit/node/attribute_spec.rb b/spec/unit/node/attribute_spec.rb
index e9515ff3b4..0869b83e43 100644
--- a/spec/unit/node/attribute_spec.rb
+++ b/spec/unit/node/attribute_spec.rb
@@ -459,8 +459,7 @@ describe Chef::Node::Attribute do
       expect(@attributes.default["foo"]).to eql({ "bar" => [ "fizz" ] })
     end
 
-    it "mutating strings should not mutate the attributes" do
-      pending "this is a bug that should be fixed"
+    it "mutating strings should not mutate the attributes in a hash" do
       @attributes.default["foo"]["bar"]["baz"] = "fizz"
       hash = @attributes["foo"].to_hash
       expect(hash).to eql({ "bar" => { "baz" => "fizz" } })
@@ -468,6 +467,15 @@ describe Chef::Node::Attribute do
       expect(hash).to eql({ "bar" => { "baz" => "fizzbuzz" } })
       expect(@attributes.default["foo"]).to eql({ "bar" => { "baz" => "fizz" } })
     end
+
+    it "mutating array elements should not mutate the attributes" do
+      @attributes.default["foo"]["bar"] = [ "fizz" ]
+      hash = @attributes["foo"].to_hash
+      expect(hash).to eql({ "bar" => [ "fizz" ] })
+      hash["bar"][0] << "buzz"
+      expect(hash).to eql({ "bar" => [ "fizzbuzz" ] })
+      expect(@attributes.default["foo"]).to eql({ "bar" => [ "fizz" ] })
+    end
   end
 
   describe "dup" do
@@ -475,6 +483,24 @@ describe Chef::Node::Attribute do
       @attributes.default[:foo] = %w{foo bar baz} + Array(1..3) + [nil, true, false, [ "el", 0, nil ] ]
       @attributes.default[:foo].dup
     end
+
+    it "mutating strings should not mutate the attributes in a hash" do
+      @attributes.default["foo"]["bar"]["baz"] = "fizz"
+      hash = @attributes["foo"].dup
+      expect(hash).to eql({ "bar" => { "baz" => "fizz" } })
+      hash["bar"]["baz"] << "buzz"
+      expect(hash).to eql({ "bar" => { "baz" => "fizzbuzz" } })
+      expect(@attributes.default["foo"]).to eql({ "bar" => { "baz" => "fizz" } })
+    end
+
+    it "mutating array elements should not mutate the attributes" do
+      @attributes.default["foo"]["bar"] = [ "fizz" ]
+      hash = @attributes["foo"].dup
+      expect(hash).to eql({ "bar" => [ "fizz" ] })
+      hash["bar"][0] << "buzz"
+      expect(hash).to eql({ "bar" => [ "fizzbuzz" ] })
+      expect(@attributes.default["foo"]).to eql({ "bar" => [ "fizz" ] })
+    end
   end
 
   describe "has_key?" do
diff --git a/spec/unit/node/immutable_collections_spec.rb b/spec/unit/node/immutable_collections_spec.rb
index 81dd771df3..520bc1ba42 100644
--- a/spec/unit/node/immutable_collections_spec.rb
+++ b/spec/unit/node/immutable_collections_spec.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Daniel DeLeo (<dan@chef.io>)
-# Copyright:: Copyright 2012-2016, Chef Software Inc.
+# Copyright:: Copyright 2012-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -78,7 +78,58 @@ describe Chef::Node::ImmutableMash do
     it "should allow mutation" do
       expect { @copy["m"] = "m" }.not_to raise_error
     end
+  end
+
+  describe "dup" do
+    before do
+      @copy = @immutable_mash.dup
+    end
+
+    it "converts an immutable mash to a new mutable hash" do
+      expect(@copy).to be_instance_of(Mash)
+    end
+
+    it "converts an immutable nested mash to a new mutable hash" do
+      expect(@copy["top_level_4"]["level2"]).to be_instance_of(Mash)
+    end
+
+    it "converts an immutable nested array to a new mutable array" do
+      expect(@copy["top_level_2"]).to be_instance_of(Array)
+    end
+
+    it "should create a mash with the same content" do
+      expect(@copy).to eq(@immutable_mash)
+    end
+
+    it "should allow mutation" do
+      expect { @copy["m"] = "m" }.not_to raise_error
+    end
+  end
 
+  describe "to_h" do
+    before do
+      @copy = @immutable_mash.to_h
+    end
+
+    it "converts an immutable mash to a new mutable hash" do
+      expect(@copy).to be_instance_of(Hash)
+    end
+
+    it "converts an immutable nested mash to a new mutable hash" do
+      expect(@copy["top_level_4"]["level2"]).to be_instance_of(Hash)
+    end
+
+    it "converts an immutable nested array to a new mutable array" do
+      expect(@copy["top_level_2"]).to be_instance_of(Array)
+    end
+
+    it "should create a mash with the same content" do
+      expect(@copy).to eq(@immutable_mash)
+    end
+
+    it "should allow mutation" do
+      expect { @copy["m"] = "m" }.not_to raise_error
+    end
   end
 
   [
@@ -198,6 +249,58 @@ describe Chef::Node::ImmutableArray do
     end
   end
 
+  describe "dup" do
+    before do
+      @copy = @immutable_nested_array.dup
+    end
+
+    it "converts an immutable array to a new mutable array" do
+      expect(@copy).to be_instance_of(Array)
+    end
+
+    it "converts an immutable nested array to a new mutable array" do
+      expect(@copy[1]).to be_instance_of(Array)
+    end
+
+    it "converts an immutable nested mash to a new mutable hash" do
+      expect(@copy[2]).to be_instance_of(Mash)
+    end
+
+    it "should create an array with the same content" do
+      expect(@copy).to eq(@immutable_nested_array)
+    end
+
+    it "should allow mutation" do
+      expect { @copy << "m" }.not_to raise_error
+    end
+  end
+
+  describe "to_array" do
+    before do
+      @copy = @immutable_nested_array.to_array
+    end
+
+    it "converts an immutable array to a new mutable array" do
+      expect(@copy).to be_instance_of(Array)
+    end
+
+    it "converts an immutable nested array to a new mutable array" do
+      expect(@copy[1]).to be_instance_of(Array)
+    end
+
+    it "converts an immutable nested mash to a new mutable hash" do
+      expect(@copy[2]).to be_instance_of(Hash)
+    end
+
+    it "should create an array with the same content" do
+      expect(@copy).to eq(@immutable_nested_array)
+    end
+
+    it "should allow mutation" do
+      expect { @copy << "m" }.not_to raise_error
+    end
+  end
+
   describe "#[]" do
     it "works with array slices" do
       expect(@immutable_array[1, 2]).to eql(%w{bar baz})