Merge pull request #6003 from coderanger/freeze-property-default

Chef-13: Freeze property defaults

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