diff options
author | Lamont Granquist <lamont@scriptkiddie.org> | 2021-02-23 15:55:45 -0800 |
---|---|---|
committer | Lamont Granquist <lamont@scriptkiddie.org> | 2021-02-25 16:54:23 -0800 |
commit | ba0a45460355014a2b7883bea0cabe5737f79fde (patch) | |
tree | f4308fa8063ded18060207e829423c1e8bb050e2 | |
parent | 6fb4dae4f722ee28e17d6b4354c3b6f3fb01946e (diff) | |
download | chef-ba0a45460355014a2b7883bea0cabe5737f79fde.tar.gz |
Dup default property values
This largely makes frozen default value errors go away. Each
copy of a resource will get a dup of the default. Defaults are
still frozen so we have defense-in-depth against them being
mutated. This eliminates the need to `default: lazy{ {} }`
things like Hashes or Arrays.
Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>
-rw-r--r-- | lib/chef/property.rb | 18 | ||||
-rw-r--r-- | lib/chef/resource/template.rb | 2 | ||||
-rw-r--r-- | spec/unit/mixin/params_validate_spec.rb | 7 | ||||
-rw-r--r-- | spec/unit/property_spec.rb | 45 |
4 files changed, 46 insertions, 26 deletions
diff --git a/lib/chef/property.rb b/lib/chef/property.rb index bbf06854d6..0813005845 100644 --- a/lib/chef/property.rb +++ b/lib/chef/property.rb @@ -412,6 +412,7 @@ class Chef # Otherwise, we have to validate it now. value = input_to_stored_value(resource, default, is_default: true) end + value = deep_dup(value) value = stored_value_to_output(resource, value) # If the value is mutable (non-frozen), we set it on the instance @@ -748,5 +749,22 @@ class Chef result end + + # recursively dup the value + def deep_dup(value) + return value if value.is_a?(DelayedEvaluator) + + visitor = lambda do |obj| + obj = obj.dup rescue obj + case obj + when Hash + obj.each { |k, v| obj[k] = visitor.call(v) } + when Array + obj.each_with_index { |v, i| obj[i] = visitor.call(v) } + end + obj + end + visitor.call(value) + end end end diff --git a/lib/chef/resource/template.rb b/lib/chef/resource/template.rb index e3f266740d..60b431ab8e 100644 --- a/lib/chef/resource/template.rb +++ b/lib/chef/resource/template.rb @@ -61,7 +61,7 @@ class Chef property :variables, Hash, description: "The variables property of the template resource can be used to reference a partial template file by using a Hash.", - default: lazy { {} } + default: {} property :cookbook, String, description: "The cookbook in which a file is located (if it is not located in the current cookbook). The default value is the current cookbook.", diff --git a/spec/unit/mixin/params_validate_spec.rb b/spec/unit/mixin/params_validate_spec.rb index 459c2ce237..1b00d9ee09 100644 --- a/spec/unit/mixin/params_validate_spec.rb +++ b/spec/unit/mixin/params_validate_spec.rb @@ -359,10 +359,11 @@ describe Chef::Mixin::ParamsValidate do expect(@vo.set_or_return(:test, nil, {}).object_id).to eq(value.object_id) end - it "should set and return a default value when the argument is nil, then return the same value" do + it "should set and return a default value when the argument is nil, then return a dup of the value" do value = "meow" - expect(@vo.set_or_return(:test, nil, { default: value }).object_id).to eq(value.object_id) - expect(@vo.set_or_return(:test, nil, {}).object_id).to eq(value.object_id) + expect(@vo.set_or_return(:test, nil, { default: value }).object_id).not_to eq(value.object_id) + expect(@vo.set_or_return(:test, nil, {}).object_id).not_to eq(value.object_id) + expect(@vo.set_or_return(:test, nil, {})).to eql(value) end it "should raise an ArgumentError when argument is nil and required is true" do diff --git a/spec/unit/property_spec.rb b/spec/unit/property_spec.rb index 6c64a961b4..aa75710ffb 100644 --- a/spec/unit/property_spec.rb +++ b/spec/unit/property_spec.rb @@ -541,13 +541,16 @@ describe "Chef::Resource.property" do it "when x is not set, it returns ''" do expect(resource.x).to eq "" end - it "x is immutable" do - expect { resource.x << "foo" }.to raise_error(FrozenError, /can't modify frozen String/) + it "setting x does not mutate the default" do + expect(resource.x).to eq "" + resource.x << "foo" + expect(resource.x).to eq "foo" + expect(resource_class.new("other").x).to eq "" end end with_property ":x, default: lazy { '' }" do - it "x is immutable" do + it "setting x does not mutate the default" do expect(resource.x).to eq "" resource.x << "foo" expect(resource.x).to eq "foo" @@ -561,16 +564,14 @@ describe "Chef::Resource.property" do it "when x is not set, it returns {}" do expect(resource.x).to eq({}) end - it "x is immutable" do - expect { resource.x["foo"] = "bar" }.to raise_error(FrozenError, /can't modify frozen Hash/) - end - it "The same exact value is returned multiple times in a row" do - value = resource.x - expect(value).to eq({}) - expect(resource.x.object_id).to eq(value.object_id) + it "setting x does not mutate the default" do + expect(resource.x).to eq({}) + resource.x["plants"] = "zombies" + expect(resource.x).to eq({ "plants" => "zombies" }) + expect(resource_class.new("other").x).to eq({}) end - it "Multiple instances of x receive the exact same value" do - expect(resource.x.object_id).to eq(resource_class.new("blah2").x.object_id) + it "Multiple instances of x receive different values" do + expect(resource.x.object_id).not_to eq(resource_class.new("blah2").x.object_id) end end @@ -594,14 +595,14 @@ describe "Chef::Resource.property" do it "when x is not set, it returns [{foo: 'bar'}]" do expect(resource.x).to eq([{ foo: "bar" }]) end - it "x is immutable" do - expect { resource.x << :other }.to raise_error(FrozenError, /can't modify frozen Array/) - end - it "x.first is immutable" do - expect { resource.x.first[:foo] = "other" }.to raise_error(FrozenError, /can't modify frozen Hash/) + it "setting x does not mutate the default" do + expect(resource.x).to eq([{ foo: "bar" }]) + resource.x[0][:foo] << "baz" + expect(resource.x).to eq([{ foo: "barbaz" }]) + expect(resource_class.new("other").x).to eq([{ foo: "bar" }]) end - it "x.first[:foo] is immutable" do - expect { resource.x.first[:foo] << "other" }.to raise_error(FrozenError, /can't modify frozen String/) + it "Multiple instances of x receive different values" do + expect(resource.x.object_id).not_to eq(resource_class.new("blah2").x.object_id) end end end @@ -708,10 +709,10 @@ describe "Chef::Resource.property" do expect(resource.x).to eq "hi1" expect(Namer.current_index).to eq 1 end - it "when x is retrieved, coercion is run each time" do + it "when x is retrieved, coercion is run exactly once" do expect(resource.x).to eq "101" - expect(resource.x).to eq "102" - expect(Namer.current_index).to eq 2 + expect(resource.x).to eq "101" + expect(Namer.current_index).to eq 1 end end |