Merge pull request #5946 from chef/lcg/fix-action-class

Fix action class weirdness in Chef-13

3 files changed, 88 insertions, 64 deletions

diff --git a/lib/chef/provider_resolver.rb b/lib/chef/provider_resolver.rb
index 439a7e9f5f..16e57b00b8 100644
--- a/lib/chef/provider_resolver.rb
+++ b/lib/chef/provider_resolver.rb
@@ -58,6 +58,7 @@ class Chef
 
     def resolve
       maybe_explicit_provider(resource) ||
+        maybe_custom_resource(resource) ||
         maybe_dynamic_provider_resolution(resource, action) ||
         raise(Chef::Exceptions::ProviderNotFound, "Cannot find a provider for #{resource} on #{node["platform"]} version #{node["platform_version"]}")
     end
@@ -105,10 +106,14 @@ class Chef
       end
     end
 
+    # if its a custom resource, just grab the action class
+    def maybe_custom_resource(resource)
+      resource.class.action_class if resource.class.custom_resource?
+    end
+
     # if resource.provider is set, just return one of those objects
     def maybe_explicit_provider(resource)
-      return nil unless resource.provider
-      resource.provider
+      resource.provider if resource.provider
     end
 
     # try dynamically finding a provider based on querying the providers to see what they support
diff --git a/lib/chef/resource.rb b/lib/chef/resource.rb
index f0d816cd89..8992594933 100644
--- a/lib/chef/resource.rb
+++ b/lib/chef/resource.rb
@@ -752,8 +752,7 @@ class Chef
               else
                 arg
               end
-      set_or_return(:provider, klass, kind_of: [ Class ]) ||
-        self.class.action_class
+      set_or_return(:provider, klass, kind_of: [ Class ])
     end
 
     def provider=(arg)
@@ -1122,53 +1121,66 @@ class Chef
     end
 
     #
-    # The action class is an automatic `Provider` created to handle
-    # actions declared by `action :x do ... end`.
+    # The action class is a `Chef::Provider` which is created at Resource
+    # class evaluation time when the Custom Resource is being constructed.
     #
-    # This class will be returned by `resource.provider` if `resource.provider`
-    # is not set. `provider_for_action` will also use this instead of calling
-    # out to `Chef::ProviderResolver`.
-    #
-    # If the user has not declared actions on this class or its superclasses
-    # using `action :x do ... end`, then there is no need for this class and
-    # `action_class` will be `nil`.
+    # This happens the first time the ruby parser hits an `action` or an
+    # `action_class` method, the presence of either indiates that this is
+    # going to be a Chef-12.5 custom resource.  If we never see one of these
+    # directives then we are constructing an old-style Resource+Provider or
+    # LWRP or whatevs.
     #
     # If a block is passed, the action_class is always created and the block is
     # run inside it.
     #
-    # @api private
-    #
     def self.action_class(&block)
-      return @action_class if @action_class && !block
-      # If the superclass needed one, then we need one as well.
-      if block || (superclass.respond_to?(:action_class) && superclass.action_class)
-        @action_class = declare_action_class(&block)
-      end
+      @action_class ||= declare_action_class
+      @action_class.class_eval(&block) if block
       @action_class
     end
 
+    # Returns true or false based on if the resource is a custom resource.  The
+    # top-level Chef::Resource is not a chef resource.  This value is inherited.
+    #
+    # @return [Boolean] if the resource is a custom_resource
+    def self.custom_resource?
+      false
+    end
+
+    # This sets the resource to being a custom resource, and does so in a way
+    # that automatically inherits to all subclasses via defining a method on
+    # the class (class variables and class instance variables don't have the
+    # correct semantics here, this is a poor man's activesupport class_attribute)
+    #
+    # @api private
+    def self.is_custom_resource!
+      define_singleton_method :custom_resource? do
+        true
+      end
+    end
+
     #
     # Ensure the action class actually gets created. This is called
     # when the user does `action :x do ... end`.
     #
-    # If a block is passed, it is run inside the action_class.
-    #
     # @api private
-    def self.declare_action_class(&block)
-      @action_class ||= begin
-                          if superclass.respond_to?(:action_class)
-                            base_provider = superclass.action_class
-                          end
-                          base_provider ||= Chef::Provider
-
-                          resource_class = self
-                          Class.new(base_provider) do
-                            include ActionClass
-                            self.resource_class = resource_class
-                          end
-                        end
-      @action_class.class_eval(&block) if block
-      @action_class
+    def self.declare_action_class
+      @action_class ||=
+        begin
+          is_custom_resource!
+          base_provider =
+            if superclass.custom_resource?
+              superclass.action_class
+            else
+              Chef::Provider
+            end
+
+          resource_class = self
+          Class.new(base_provider) do
+            include ActionClass
+            self.resource_class = resource_class
+          end
+        end
     end
 
     #
diff --git a/spec/integration/recipes/resource_action_spec.rb b/spec/integration/recipes/resource_action_spec.rb
index 7a68164d41..03331c8625 100644
--- a/spec/integration/recipes/resource_action_spec.rb
+++ b/spec/integration/recipes/resource_action_spec.rb
@@ -502,40 +502,28 @@ module ResourceActionSpec
       end
     end
 
-    context "When a resource declares methods in action_class and declare_action_class" do
+    context "When a resource declares methods in action_class" do
       class DeclaresActionClassMethods < Chef::Resource
         use_automatic_resource_name
         property :x
-        action :create do
-          new_resource.x = a + b + c + d
-        end
         action_class do
           def a
             1
           end
         end
-        declare_action_class do
-          def b
-            2
-          end
-        end
-        action_class do
+        action_class.class_eval <<-EOM
           def c
             3
           end
-        end
-        declare_action_class do
-          def d
-            4
-          end
+        EOM
+        action :create do
+          new_resource.x = a + c
         end
       end
 
       it "the methods are not available on the resource" do
         expect { DeclaresActionClassMethods.new("hi").a }.to raise_error(NameError)
-        expect { DeclaresActionClassMethods.new("hi").b }.to raise_error(NameError)
         expect { DeclaresActionClassMethods.new("hi").c }.to raise_error(NameError)
-        expect { DeclaresActionClassMethods.new("hi").d }.to raise_error(NameError)
       end
 
       it "the methods are available to the action" do
@@ -543,17 +531,14 @@ module ResourceActionSpec
         expect_recipe do
           r = declares_action_class_methods "hi"
         end.to emit_no_warnings_or_errors
-        expect(r.x).to eq(10)
+        expect(r.x).to eq(4)
       end
 
-      context "And a subclass also creates a method" do
+      context "And a subclass overrides a method with an action_class block" do
         class DeclaresActionClassMethodsToo < DeclaresActionClassMethods
           use_automatic_resource_name
-          action :create do
-            new_resource.x a + b + c + d + e
-          end
           action_class do
-            def e
+            def a
               5
             end
           end
@@ -561,10 +546,7 @@ module ResourceActionSpec
 
         it "the methods are not available on the resource" do
           expect { DeclaresActionClassMethods.new("hi").a }.to raise_error(NameError)
-          expect { DeclaresActionClassMethods.new("hi").b }.to raise_error(NameError)
           expect { DeclaresActionClassMethods.new("hi").c }.to raise_error(NameError)
-          expect { DeclaresActionClassMethods.new("hi").d }.to raise_error(NameError)
-          expect { DeclaresActionClassMethods.new("hi").e }.to raise_error(NameError)
         end
 
         it "the methods are available to the action" do
@@ -572,7 +554,32 @@ module ResourceActionSpec
           expect_recipe do
             r = declares_action_class_methods_too "hi"
           end.to emit_no_warnings_or_errors
-          expect(r.x).to eq(15)
+          expect(r.x).to eq(8)
+        end
+      end
+
+      context "And a subclass overrides a method with class_eval" do
+        # this tests inheritance with *only* an action_class accessor that does not declare a block
+        class DeclaresActionClassMethodsToo < DeclaresActionClassMethods
+          use_automatic_resource_name
+          action_class.class_eval <<-EOM
+            def a
+              5
+            end
+          EOM
+        end
+
+        it "the methods are not available on the resource" do
+          expect { DeclaresActionClassMethods.new("hi").a }.to raise_error(NameError)
+          expect { DeclaresActionClassMethods.new("hi").c }.to raise_error(NameError)
+        end
+
+        it "the methods are available to the action" do
+          r = nil
+          expect_recipe do
+            r = declares_action_class_methods_too "hi"
+          end.to emit_no_warnings_or_errors
+          expect(r.x).to eq(8)
         end
       end
     end