Merge pull request #7892 from chef/lcg/chef-15-nillable-deep-merge

Add nillability to attribute deep merging

3 files changed, 62 insertions, 12 deletions

diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index 098c91c318..a83367df59 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -6,6 +6,24 @@ Chef 15 release notes will be added here as development progresses.
 
 ## Breaking Changes
 
+### Node Attributes deep merge nil values
+
+Writing a nil to a precedence level in the node object now acts like any other value and can be used to override values back to nil.
+
+For example:
+
+```
+chef (15.0.53)> node.default["foo"] = "bar"
+ => "bar"
+chef (15.0.53)> node.override["foo"] = nil
+ => nil
+chef (15.0.53)> node["foo"]
+ => nil
+```
+
+In prior versions of chef-client the nil set in the override level would be completely ignored and the value of `node["foo"]` would have
+been "bar".
+
 ### http_disable_auth_on_redirect now enabled
 
 The Chef config ``http_disable_auth_on_redirect`` has been changed from `false` to `true`. In Chef 16 this config option will be removed alltogether and Chef will always disable auth on redirect.
diff --git a/lib/chef/node/attribute.rb b/lib/chef/node/attribute.rb
index 69cb327d70..c5731b65a0 100644
--- a/lib/chef/node/attribute.rb
+++ b/lib/chef/node/attribute.rb
@@ -406,11 +406,13 @@ class Chef
       end
 
       def combined_override(*path)
-        merge_overrides(path)
+        ret = merge_overrides(path)
+        ret == NIL ? nil : ret
       end
 
       def combined_default(*path)
-        merge_defaults(path)
+        ret = merge_defaults(path)
+        ret == NIL ? nil : ret
       end
 
       def normal_unless(*args)
@@ -505,10 +507,10 @@ class Chef
               # Hash-like thing (must check has_key? first to protect against Autovivification)
               val[path_arg]
             else
-              nil
+              NIL
             end
           else
-            nil
+            NIL
           end
         end
       end
@@ -552,7 +554,7 @@ class Chef
           component_value = apply_path(instance_variable_get(component_ivar), path)
           deep_merge!(merged, component_value)
         end
-        ret == NIL ? nil : ret
+        ret
       end
 
       # Deep merge the override attribute levels with array merging.
@@ -566,7 +568,7 @@ class Chef
           component_value = apply_path(instance_variable_get(component_ivar), path)
           deep_merge!(merged, component_value)
         end
-        ret == NIL ? nil : ret
+        ret
       end
 
       # needed for __path__
@@ -596,11 +598,11 @@ class Chef
         elsif merge_onto.kind_of?(Array) && merge_with.kind_of?(Array)
           merge_onto |= merge_with
 
-          # If merge_with is nil, don't replace merge_onto
-        elsif merge_with.nil?
+        # If merge_with is NIL, don't replace merge_onto
+        elsif merge_with == NIL
           merge_onto
 
-          # In all other cases, replace merge_onto with merge_with
+        # In all other cases, replace merge_onto with merge_with
         else
           if merge_with.kind_of?(Hash)
             Chef::Node::ImmutableMash.new(merge_with)
@@ -629,11 +631,11 @@ class Chef
           end
           merge_onto
 
-          # If merge_with is nil, don't replace merge_onto
-        elsif merge_with.nil?
+        # If merge_with is NIL, don't replace merge_onto
+        elsif merge_with == NIL
           merge_onto
 
-          # In all other cases, replace merge_onto with merge_with
+        # In all other cases, replace merge_onto with merge_with
         else
           if merge_with.kind_of?(Hash)
             Chef::Node::ImmutableMash.new(merge_with)
diff --git a/spec/unit/node/attribute_spec.rb b/spec/unit/node/attribute_spec.rb
index 36827215f0..c6fdf1e1c2 100644
--- a/spec/unit/node/attribute_spec.rb
+++ b/spec/unit/node/attribute_spec.rb
@@ -1273,4 +1273,34 @@ describe Chef::Node::Attribute do
       expect { @attributes["foo"]["bar"][0] << "buzz" }.to raise_error(RuntimeError, "can't modify frozen String")
     end
   end
+
+  describe "deep merging with nils" do
+    it "nils when deep merging between default levels knocks out values" do
+      @attributes.default["foo"] = "bar"
+      expect(@attributes["foo"]).to eql("bar")
+      @attributes.force_default["foo"] = nil
+      expect(@attributes["foo"]).to be nil
+    end
+
+    it "nils when deep merging between override levels knocks out values" do
+      @attributes.override["foo"] = "bar"
+      expect(@attributes["foo"]).to eql("bar")
+      @attributes.force_override["foo"] = nil
+      expect(@attributes["foo"]).to be nil
+    end
+
+    it "nils when deep merging between default+override levels knocks out values" do
+      @attributes.default["foo"] = "bar"
+      expect(@attributes["foo"]).to eql("bar")
+      @attributes.override["foo"] = nil
+      expect(@attributes["foo"]).to be nil
+    end
+
+    it "nils when deep merging between normal+automatic levels knocks out values" do
+      @attributes.normal["foo"] = "bar"
+      expect(@attributes["foo"]).to eql("bar")
+      @attributes.automatic["foo"] = nil
+      expect(@attributes["foo"]).to be nil
+    end
+  end
 end