Prefer name_property: true over default: nil

2 files changed, 32 insertions, 13 deletions

diff --git a/lib/chef/property.rb b/lib/chef/property.rb
index 7f42616bdd..6c9d4619b1 100644
--- a/lib/chef/property.rb
+++ b/lib/chef/property.rb
@@ -89,18 +89,19 @@ class Chef
 
       # Only pick the first of :default, :name_property and :name_attribute if
       # more than one is specified.
-      found_defaults = []
-      options.reject! do |k,v|
-        if [ :name_property, :name_attribute, :default ].include?(k)
-          found_defaults << k
-          # Reject all but the first default key you find
-          found_defaults.size > 1
-        else
-          false
-        end
-      end
+      found_defaults = options.keys.select { |k| [ :default, :name_attribute, :name_property ].include?(k) }
       if found_defaults.size > 1
-        Chef::Log.deprecation("Cannot specify keys #{found_defaults.join(", ")} together on property #{options[:name]}--only the first one (#{found_defaults[0]}) will be obeyed. Please pick one.", caller(5..5)[0])
+        preferred_default = found_defaults[0]
+        # We do *not* prefer `default: nil` even if it's first, because earlier
+        # versions of Chef (backcompat) treat specifying something as `nil` the
+        # same as not specifying it at all. In Chef 13 we can switch this behavior
+        # back to normal, since only one default will be specifiable.
+        if preferred_default == :default && options[:default].nil?
+          preferred_default = found_defaults[1]
+        end
+        Chef::Log.deprecation("Cannot specify keys #{found_defaults.join(", ")} together on property #{options[:name]}--only one (#{preferred_default}) will be obeyed. In Chef 13, specifying multiple defaults will become an error.", caller(5..5)[0])
+        # Only honor the preferred default
+        options.reject! { |k,v| found_defaults.include?(k) && k != preferred_default }
       end
       options[:name_property] = options.delete(:name_attribute) if options.has_key?(:name_attribute) && !options.has_key?(:name_property)
 
diff --git a/spec/unit/property_spec.rb b/spec/unit/property_spec.rb
index 369c065aaa..ebf94c1dd0 100644
--- a/spec/unit/property_spec.rb
+++ b/spec/unit/property_spec.rb
@@ -949,11 +949,19 @@ describe "Chef::Resource.property" do
       context "default ordering deprecation warnings" do
         it "emits a deprecation warning for property :x, default: 10, #{name}: true" do
           expect { resource_class.property :x, :default => 10, name.to_sym => true }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
-            /Cannot specify keys default, #{name} together on property x--only the first one \(default\) will be obeyed. Please pick one./
+            /Cannot specify keys default, #{name} together on property x--only one \(default\) will be obeyed./
+        end
+        it "emits a deprecation warning for property :x, default: nil, #{name}: true" do
+          expect { resource_class.property :x, :default => nil, name.to_sym => true }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
+            /Cannot specify keys default, #{name} together on property x--only one \(#{name}\) will be obeyed./
         end
         it "emits a deprecation warning for property :x, #{name}: true, default: 10" do
           expect { resource_class.property :x, name.to_sym => true, :default => 10 }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
-            /Cannot specify keys #{name}, default together on property x--only the first one \(#{name}\) will be obeyed. Please pick one./
+            /Cannot specify keys #{name}, default together on property x--only one \(#{name}\) will be obeyed./
+        end
+        it "emits a deprecation warning for property :x, #{name}: true, default: nil" do
+          expect { resource_class.property :x, name.to_sym => true, :default => nil }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
+            /Cannot specify keys #{name}, default together on property x--only one \(#{name}\) will be obeyed./
         end
       end
       context "default ordering" do
@@ -963,11 +971,21 @@ describe "Chef::Resource.property" do
             expect(resource.x).to eq 10
           end
         end
+        with_property ":x, default: nil, #{name}: true" do
+          it "chooses #{name} over default" do
+            expect(resource.x).to eq 'blah'
+          end
+        end
         with_property ":x, #{name}: true, default: 10" do
           it "chooses #{name} over default" do
             expect(resource.x).to eq 'blah'
           end
         end
+        with_property ":x, #{name}: true, default: nil" do
+          it "chooses #{name} over default" do
+            expect(resource.x).to eq 'blah'
+          end
+        end
       end
       with_property ":x, #{name}: true, required: true" do
         it "defaults x to resource.name" do