summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorLamont Granquist <lamont@scriptkiddie.org>2021-02-23 15:55:45 -0800
committerLamont Granquist <lamont@scriptkiddie.org>2021-02-25 16:54:23 -0800
commitba0a45460355014a2b7883bea0cabe5737f79fde (patch)
treef4308fa8063ded18060207e829423c1e8bb050e2
parent6fb4dae4f722ee28e17d6b4354c3b6f3fb01946e (diff)
downloadchef-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.rb18
-rw-r--r--lib/chef/resource/template.rb2
-rw-r--r--spec/unit/mixin/params_validate_spec.rb7
-rw-r--r--spec/unit/property_spec.rb45
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