diff options
author | Noah Kantrowitz <noah@coderanger.net> | 2017-04-04 12:23:59 -0700 |
---|---|---|
committer | GitHub <noreply@github.com> | 2017-04-04 12:23:59 -0700 |
commit | f38f957878057b0609d63dc1a735819c87e3c8c9 (patch) | |
tree | c8657656a3426ff6354f0fc44002d83a096f5e07 | |
parent | 373a4514806ae33c08fd6afd205a661cc6a18e19 (diff) | |
parent | 11a07953165b631eb3612a00560a24fa14d14606 (diff) | |
download | chef-f38f957878057b0609d63dc1a735819c87e3c8c9.tar.gz |
Merge pull request #6003 from coderanger/freeze-property-default
Chef-13: Freeze property defaults
-rw-r--r-- | RELEASE_NOTES.md | 16 | ||||
-rw-r--r-- | lib/chef/property.rb | 14 | ||||
-rw-r--r-- | lib/chef/resource/mount.rb | 2 | ||||
-rw-r--r-- | spec/unit/property_spec.rb | 78 |
4 files changed, 87 insertions, 23 deletions
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 75852adaf2..3a459b9a18 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -224,3 +224,19 @@ Accessing a provider class is a bit more complex, as you need a resource against ```ruby Chef::ProviderResolver.new(node, find_resource!("mycook_thing[name]"), :nothing).resolve ``` + +### Default values for resource properties are frozen + +A resource declaring something like: + +```ruby +property :x, default: {} +``` + +will now see the default value set to be immutable. This prevents cases of +modifying the default in one resource affecting others. If you want a per-resource +mutable default value, define it inside a `lazy{}` helper like: + +```ruby +property :x, default: lazy { {} } +``` diff --git a/lib/chef/property.rb b/lib/chef/property.rb index c6f72e15a7..a72e41a61e 100644 --- a/lib/chef/property.rb +++ b/lib/chef/property.rb @@ -110,6 +110,20 @@ class Chef raise ArgumentError, "Cannot specify both default and name_property/name_attribute together on property #{self}" end + # Recursively freeze the default if it isn't a lazy value. + unless default.is_a?(DelayedEvaluator) + visitor = lambda do |obj| + case obj + when Hash + obj.each_value { |value| visitor.call(value) } + when Array + obj.each { |value| visitor.call(value) } + end + obj.freeze + end + visitor.call(default) + end + # Validate the default early, so the user gets a good error message, and # cache it so we don't do it again if so begin diff --git a/lib/chef/resource/mount.rb b/lib/chef/resource/mount.rb index 2aca8432dd..7e601b861a 100644 --- a/lib/chef/resource/mount.rb +++ b/lib/chef/resource/mount.rb @@ -31,7 +31,7 @@ class Chef allowed_actions :mount, :umount, :unmount, :remount, :enable, :disable # this is a poor API please do not re-use this pattern - property :supports, Hash, default: { remount: false }, + property :supports, Hash, default: lazy { { remount: false } }, coerce: proc { |x| x.is_a?(Array) ? x.each_with_object({}) { |i, m| m[i] = true } : x } def initialize(name, run_context = nil) diff --git a/spec/unit/property_spec.rb b/spec/unit/property_spec.rb index 79ec05ea6d..b8cf7f5d1b 100644 --- a/spec/unit/property_spec.rb +++ b/spec/unit/property_spec.rb @@ -522,22 +522,41 @@ describe "Chef::Resource.property" do end end - context "hash default" do - context "(deprecations allowed)" do - before { Chef::Config[:treat_deprecation_warnings_as_errors] = false } + context "string default" do + with_property ":x, default: ''" 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(RuntimeError, "can't modify frozen String") + end + end - with_property ":x, default: {}" do - it "when x is not set, it returns {}" do - expect(resource.x).to eq({}) - 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) - 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) - end + with_property ":x, default: lazy { '' }" do + it "x is immutable" 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 + end + + context "hash default" do + with_property ":x, default: {}" 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(RuntimeError, "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) + 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) end end @@ -545,17 +564,34 @@ describe "Chef::Resource.property" do it "when x is not set, it returns {}" do expect(resource.x).to eq({}) end - # it "The value is different each time it is called" do - # value = resource.x - # expect(value).to eq({}) - # expect(resource.x.object_id).not_to eq(value.object_id) - # end + it "The value is the same each time it is called" do + value = resource.x + expect(value).to eq({}) + expect(resource.x.object_id).to eq(value.object_id) + end 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 + context "complex, nested default" do + with_property ":x, default: [{foo: 'bar'}]" 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(RuntimeError, "can't modify frozen Array") + end + it "x.first is immutable" do + expect { resource.x.first[:foo] = "other" }.to raise_error(RuntimeError, "can't modify frozen Hash") + end + it "x.first[:foo] is immutable" do + expect { resource.x.first[:foo] << "other" }.to raise_error(RuntimeError, "can't modify frozen String") + end + end + end + context "with a class with 'blah' as both class and instance methods" do before do resource_class.class_eval do @@ -576,7 +612,6 @@ describe "Chef::Resource.property" do it "x is run in the context of each instance it is run in" do expect(resource.x).to eq "blah1" expect(resource_class.new("another").x).to eq "another2" - # expect(resource.x).to eq "blah3" end end @@ -587,7 +622,6 @@ describe "Chef::Resource.property" do it "x is passed the value of each instance it is run in" do expect(resource.x).to eq "classblah1" expect(resource_class.new("another").x).to eq "classanother2" - # expect(resource.x).to eq "classblah3" end end end |