Fix Chef-13 action_class bug and cleanup

The old action_class code was doing some magical stuff with the provider
accessor in order to determine if the class was supposed to be a custom
resource or not and have the action_class autovivifying accessor return nil
in cases when the resource wasn't a custom resource and implementing
inheritance by walking back up the tree in ways that were difficult to
grok.

This removes the magic from the provider so that there's no longer any
accessor that magically short-circuits to nil if the resource is not
supposed to be a custom resource.

There is now a simple inherited API for `Chef::Resource.custom_resource?`
which just defines if the class is a custom resource or not.  Since both
`action` and `action_class` call `define_action_class` they both wind up
setting this boolean on the class, which is then inherited to subclasses
automatically, which eliminates the need to walk up the hierarchy.

The superclass.respond_to?(:action_class) checks have also been rendered
unnecessary by removing the code that walked up the inheritance
hierarchy and also because Chef::Resource is never going to be a
custom resource itself, so will never call `define_action_class` so from
inside of `define_action_class` you can always rely on the superclass
being a resource and implementing `custom_resource?` and `action_class`.

The wiring for picking the provider is now moved explicitly to the
ProviderResolver -- even though custom resources hardcode a 1:1
resource-to-provider mapping.  This reads much clearer to me than the
magical wiring to the provider accessor off of the instance.

The bug that this fixes was that the way the magical accessor
nil-or-action_class was implemented the old way of defining action
helpers with class_eval broke:

```ruby
action_class.class_eval << EOM
  def a
    "foo"
  end
EOM
```

If that came before any action_class-with-a-block or action declaration
and the resource did not inherit from another custom resource then the
action_class would not be created and it would return nil, which was an
API which the magical wiring in the provider accessor required.

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>

2 files changed, 54 insertions, 37 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 b0e3a372e8..f37f3a3989 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
 
     #