Chef-13: properly deep dup Node#to_hash

previously to_hash allowed mutating non-container elements, now
they get properly dup'd.

fixes a 2.5 year old pending spec test.

also fills out the API so that there is to_h/to_hash/to_a/to_array
instead of the weird mix-and-match we had before.

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

3 files changed, 78 insertions, 8 deletions

diff --git a/lib/chef/node/immutable_collections.rb b/lib/chef/node/immutable_collections.rb
index 12ee2e5dd8..08bd9c9c77 100644
--- a/lib/chef/node/immutable_collections.rb
+++ b/lib/chef/node/immutable_collections.rb
@@ -77,14 +77,16 @@ class Chef
             when ImmutableArray
               v.to_a
             when ImmutableMash
-              v.to_hash
+              v.to_h
             else
-              v
+              safe_dup(v)
             end
         end
         a
       end
 
+      alias_method :to_array, :to_a
+
       # for consistency's sake -- integers 'converted' to integers
       def convert_key(key)
         key
@@ -140,22 +142,31 @@ class Chef
         Mash.new(self)
       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..25594ca20b 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
diff --git a/spec/unit/node/immutable_collections_spec.rb b/spec/unit/node/immutable_collections_spec.rb
index 81dd771df3..a8078f1093 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,32 @@ describe Chef::Node::ImmutableMash do
     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 +223,32 @@ describe Chef::Node::ImmutableArray do
     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})