diff options
author | Lamont Granquist <lamont@scriptkiddie.org> | 2017-03-15 11:00:20 -0700 |
---|---|---|
committer | GitHub <noreply@github.com> | 2017-03-15 11:00:20 -0700 |
commit | e2496911c433e6bd9f9b3506ab08a27c552436d4 (patch) | |
tree | c17712b7ea7ef927e52a9da777a63f56ee8afb9e | |
parent | 315a6f547bd6ca7486f66e5bc3082f2c7607259c (diff) | |
parent | d28d5c65d0074e6a5e3786555b5f31893447b157 (diff) | |
download | chef-e2496911c433e6bd9f9b3506ab08a27c552436d4.tar.gz |
Merge pull request #5896 from chef/lcg/deep-dup-attrs
Chef-13: properly deep dup Node#to_hash
-rw-r--r-- | lib/chef/node/immutable_collections.rb | 50 | ||||
-rw-r--r-- | spec/unit/node/attribute_spec.rb | 30 | ||||
-rw-r--r-- | spec/unit/node/immutable_collections_spec.rb | 105 |
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}) |