If both name_attribute and name_property are specified, raise an error.jk/property-default-nil

2 files changed, 29 insertions, 28 deletions

diff --git a/lib/chef/property.rb b/lib/chef/property.rb
index d699f16a9d..f6160d6954 100644
--- a/lib/chef/property.rb
+++ b/lib/chef/property.rb
@@ -87,27 +87,28 @@ class Chef
     def initialize(**options)
       options.each { |k,v| options[k.to_sym] = v if k.is_a?(String) }
 
+      # Replace name_attribute with name_property
+      if options.has_key?(:name_attribute)
+        # If we have both name_attribute and name_property and they differ, raise an error
+        if options.has_key?(:name_property)
+          raise ArgumentError, "Cannot specify both name_property and name_attribute together on property #{options[:name]}#{options[:declared_in] ? " of resource #{options[:declared_in].resource_name}" : ""}."
+        end
+        # replace name_property with name_attribute in place
+        options = options.map { |k,v| k == :name_attribute ? [ :name_property, v ] : [ k,v ] }.to_h
+      end
+
       # Only pick the first of :default, :name_property and :name_attribute if
       # more than one is specified.
-      found_defaults = options.keys.select do |k|
-         # Only treat name_property or name_attribute as a default if they are `true`
-        k == :default ||
-        ((k == :name_property || k == :name_attribute) && options[k])
-      end
-      if found_defaults.size > 1
-        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]
+      if options.has_key?(:default) && options[:name_property]
+        if options[:default].nil? || options.keys.index(:name_property) < options.keys.index(:default)
+          options.delete(:default)
+          preferred_default = :name_property
+        else
+          options.delete(:name_property)
+          preferred_default = :default
         end
-        Chef.log_deprecation("Cannot specify keys #{found_defaults.join(", ")} together on property #{options[:name]}#{options[:resource_class] ? " of resource #{options[:resource_class].resource_name}" : ""}. Only one (#{preferred_default}) will be obeyed. In Chef 13, specifying multiple defaults will become an error.")
-        # Only honor the preferred default
-        options.reject! { |k,v| found_defaults.include?(k) && k != preferred_default }
+        Chef.log_deprecation("Cannot specify both default and name_property together on property #{options[:name]}#{options[:declared_in] ? " of resource #{options[:declared_in].resource_name}" : ""}. Only one (#{preferred_default}) will be obeyed. In Chef 13, this will become an error.")
       end
-      options[:name_property] = options.delete(:name_attribute) if options.has_key?(:name_attribute) && !options.has_key?(:name_property)
 
       @options = options
 
@@ -446,10 +447,10 @@ class Chef
       EOM
     rescue SyntaxError
       # If the name is not a valid ruby name, we use define_method.
-      resource_class.define_method(name) do |value=NOT_PASSED|
+      declared_in.define_method(name) do |value=NOT_PASSED|
         self.class.properties[name].call(self, value)
       end
-      resource_class.define_method("#{name}=") do |value|
+      declared_in.define_method("#{name}=") do |value|
         self.class.properties[name].set(self, value)
       end
     end
diff --git a/spec/unit/property_spec.rb b/spec/unit/property_spec.rb
index de8d493026..64638d9be9 100644
--- a/spec/unit/property_spec.rb
+++ b/spec/unit/property_spec.rb
@@ -962,19 +962,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 one \(default\) will be obeyed./
+            /Cannot specify both default and name_property together on property x of resource chef_resource_property_spec_(\d+). 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./
+            /Cannot specify both default and name_property together on property x of resource chef_resource_property_spec_(\d+). Only one \(name_property\) 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 one \(#{name}\) will be obeyed./
+            /Cannot specify both default and name_property together on property x of resource chef_resource_property_spec_(\d+). Only one \(name_property\) 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./
+            /Cannot specify both default and name_property together on property x of resource chef_resource_property_spec_(\d+). Only one \(name_property\) will be obeyed./
         end
       end
 
@@ -1028,11 +1028,11 @@ describe "Chef::Resource.property" do
         end
       end
 
-      with_property ":x, #{name}: true, required: true" do
-        it "defaults x to resource.name" do
-          expect(resource.x).to eq 'blah'
-        end
-      end
     end
   end
+
+  it "raises an error if both name_property and name_attribute are specified (even if they are false or nil)" do
+    expect { resource_class.property :x, :name_property => false, :name_attribute => true }.to raise_error ArgumentError,
+      /Cannot specify both name_property and name_attribute together on property x of resource chef_resource_property_spec_(\d+)./
+  end
 end