Merge pull request #11095 from chef/lcg/dup-property-defults

6 files changed, 50 insertions, 29 deletions

diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index 78dab7f76e..23987e8002 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -6,6 +6,7 @@ This section serves to track things we should later document here for 17.0
 
 - Dropped support for Ruby 2.6
 - Lazy attribute loading: https://github.com/chef/chef/pull/10861
+- Duping property defaults: https://github.com/chef/chef/pull/11095
 - Compliance Phase in GA: https://github.com/chef/chef/pull/10547
 - gem resource: assume rubygems 1.8+ now: https://github.com/chef/chef/pull/10379
 - remove support for RHEL 6 i386 / Ubuntu 16.04 / macOS 10.13
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
 
diff --git a/spec/unit/provider/group_spec.rb b/spec/unit/provider/group_spec.rb
index c96c29164e..b55d334f92 100644
--- a/spec/unit/provider/group_spec.rb
+++ b/spec/unit/provider/group_spec.rb
@@ -101,19 +101,19 @@ describe Chef::Provider::User do
     end
 
     it "should return false if append is true and the group member(s) already exists" do
-      @current_resource.members += [ "extra_user" ]
+      @current_resource.members << "extra_user"
       @new_resource.append(true)
       expect(@provider.compare_group).to be_falsey
     end
 
     it "should return true if append is true and the group member(s) do not already exist" do
-      @new_resource.members += [ "extra_user" ]
+      @new_resource.members << "extra_user"
       @new_resource.append(true)
       expect(@provider.compare_group).to be_truthy
     end
 
     it "should return false if append is true and excluded_members include a non existing member" do
-      @new_resource.excluded_members += [ "extra_user" ]
+      @new_resource.excluded_members << "extra_user"
       @new_resource.append(true)
       expect(@provider.compare_group).to be_falsey
     end