Merge pull request #5606 from chef/adamleff/warn-on-dangerous-property-names

Deprecate creating properties whose names are already methods

9 files changed, 69 insertions, 7 deletions

diff --git a/Gemfile.lock b/Gemfile.lock
index cd47dfbba7..7eb8ca4723 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -178,7 +178,7 @@ GEM
       mixlib-log (~> 1.3)
       rack (~> 2.0)
       uuidtools (~> 2.1)
-    cheffish (4.0.0)
+    cheffish (4.1.0)
       chef-zero (~> 5.0)
       net-ssh
     chefspec (5.3.0)
diff --git a/lib/chef/chef_class.rb b/lib/chef/chef_class.rb
index 3a395d979d..e61fd5e1d2 100644
--- a/lib/chef/chef_class.rb
+++ b/lib/chef/chef_class.rb
@@ -30,6 +30,7 @@ require "chef/platform/provider_priority_map"
 require "chef/platform/resource_priority_map"
 require "chef/platform/provider_handler_map"
 require "chef/platform/resource_handler_map"
+require "chef/deprecated"
 require "chef/event_dispatch/dsl"
 require "chef/deprecated"
 
diff --git a/lib/chef/deprecated.rb b/lib/chef/deprecated.rb
index 1cadacec98..76f66f723b 100644
--- a/lib/chef/deprecated.rb
+++ b/lib/chef/deprecated.rb
@@ -156,6 +156,26 @@ class Chef
       end
     end
 
+    class PropertyNameCollision < Base
+      def id
+        11
+      end
+
+      def target
+        "property_name_collision.html"
+      end
+    end
+
+    class LaunchdHashProperty < Base
+      def id
+        12
+      end
+
+      def target
+        "launchd_hash_property.html"
+      end
+    end
+
     class ChefPlatformMethods < Base
       def id
         13
diff --git a/lib/chef/property.rb b/lib/chef/property.rb
index 8d8ed352b3..8fa290251a 100644
--- a/lib/chef/property.rb
+++ b/lib/chef/property.rb
@@ -518,6 +518,13 @@ class Chef
       # be using the existing getter/setter to manipulate it instead.
       return if !instance_variable_name
 
+      # We deprecate any attempt to create a property that already exists as a
+      # method in some Classes that we know would cause our users problems.
+      # For example, creating a `hash` property could cause issues when adding
+      # a Chef::Resource instance to an data structure that expects to be able
+      # to call the `#hash` method and get back an appropriate Fixnum.
+      emit_property_redefinition_deprecations
+
       # We prefer this form because the property name won't show up in the
       # stack trace if you use `define_method`.
       declared_in.class_eval <<-EOM, __FILE__, __LINE__ + 1
@@ -632,6 +639,30 @@ class Chef
 
     private
 
+    def emit_property_redefinition_deprecations
+      # We only emit deprecations if this property already exists as an instance method.
+      # Weeding out class methods avoids unnecessary deprecations such Chef::Resource
+      # defining a `name` property when there's an already-existing `name` method
+      # for a Module.
+      return unless declared_in.instance_methods.include?(name)
+
+      # Only emit deprecations for some well-known classes. This will still
+      # allow more advanced users to subclass their own custom resources and
+      # override their own properties.
+      return unless [ Object, BasicObject, Kernel, Chef::Resource ].include?(declared_in.instance_method(name).owner)
+
+      # Allow top-level Chef::Resource proprties, such as `name`, to be overridden.
+      # As of this writing, `name` is the only Chef::Resource property created with the
+      # `property` definition, but this will allow for future properties to be extended
+      # as needed.
+      return if Chef::Resource.properties.keys.include?(name)
+
+      # Emit the deprecation.
+      resource_name = declared_in.respond_to?(:resource_name) ? declared_in.resource_name : declared_in
+      Chef.deprecated(:property_name_collision, "Property `#{name}` of resource `#{resource_name}` overwrites an existing method. " \
+        "Please use a different property name. This will raise an exception in Chef 13.")
+    end
+
     def exec_in_resource(resource, proc, *args)
       if resource
         if proc.arity > args.size
diff --git a/lib/chef/provider/launchd.rb b/lib/chef/provider/launchd.rb
index 0ec8d0cfe1..f3e0a00758 100644
--- a/lib/chef/provider/launchd.rb
+++ b/lib/chef/provider/launchd.rb
@@ -150,7 +150,7 @@ class Chef
       end
 
       def content
-        plist_hash = new_resource.hash || gen_hash
+        plist_hash = new_resource.plist_hash || gen_hash
         Plist::Emit.dump(plist_hash) unless plist_hash.nil?
       end
 
diff --git a/lib/chef/resource/apt_repository.rb b/lib/chef/resource/apt_repository.rb
index 8b87371824..b38bd1c8ec 100644
--- a/lib/chef/resource/apt_repository.rb
+++ b/lib/chef/resource/apt_repository.rb
@@ -38,7 +38,6 @@ class Chef
 
       property :cookbook, [String, nil, false], default: nil, desired_state: false, nillable: true, coerce: proc { |x| x ? x : nil }
       property :cache_rebuild, [TrueClass, FalseClass], default: true, desired_state: false
-      property :sensitive, [TrueClass, FalseClass], default: false, desired_state: false
 
       default_action :add
       allowed_actions :add, :remove
diff --git a/lib/chef/resource/launchd.rb b/lib/chef/resource/launchd.rb
index 6427b13f84..c78ffa3f0e 100644
--- a/lib/chef/resource/launchd.rb
+++ b/lib/chef/resource/launchd.rb
@@ -33,7 +33,7 @@ class Chef
       property :backup, [Integer, FalseClass]
       property :cookbook, String
       property :group, [String, Integer]
-      property :hash, Hash
+      property :plist_hash, Hash
       property :mode, [String, Integer]
       property :owner, [String, Integer]
       property :path, String
@@ -139,6 +139,18 @@ class Chef
       property :wait_for_debugger, [ TrueClass, FalseClass ]
       property :watch_paths, Array
       property :working_directory, String
+
+      # hash is an instance method on Object and needs to return a Fixnum.
+      def hash(arg = nil)
+        Chef.deprecated(:launchd_hash_property, "Property `hash` on the `launchd` resource has changed to `plist_hash`." \
+          "Please use `plist_hash` instead. This will raise an exception in Chef 13.")
+
+        set_or_return(
+          :plist_hash,
+          arg,
+          :kind_of => Hash
+        )
+      end
     end
   end
 end
diff --git a/lib/chef/resource/yum_repository.rb b/lib/chef/resource/yum_repository.rb
index 0d6c3472c0..f59ad56d16 100644
--- a/lib/chef/resource/yum_repository.rb
+++ b/lib/chef/resource/yum_repository.rb
@@ -58,7 +58,6 @@ class Chef
       property :repo_gpgcheck, [TrueClass, FalseClass]
       property :report_instanceid, [TrueClass, FalseClass]
       property :repositoryid, String, regex: /.*/, name_property: true
-      property :sensitive, [TrueClass, FalseClass], default: false
       property :skip_if_unavailable, [TrueClass, FalseClass]
       property :source, String, regex: /.*/
       property :sslcacert, String, regex: /.*/
diff --git a/spec/unit/provider/launchd_spec.rb b/spec/unit/provider/launchd_spec.rb
index 3e45433c62..693801f99b 100644
--- a/spec/unit/provider/launchd_spec.rb
+++ b/spec/unit/provider/launchd_spec.rb
@@ -185,8 +185,8 @@ XML
       end
 
       describe "hash is passed" do
-        it "should produce the test_plist from the hash" do
-          new_resource.hash test_hash
+        it "should produce the test_plist content from the plist_hash property" do
+          new_resource.plist_hash test_hash
           expect(provider.content?).to be_truthy
           expect(provider.content).to eql(test_plist)
         end