First pass at fixing broken notifications

8 files changed, 301 insertions, 115 deletions

diff --git a/lib/chef/dsl/recipe.rb b/lib/chef/dsl/recipe.rb
index 120497d56e..d70266c14c 100644
--- a/lib/chef/dsl/recipe.rb
+++ b/lib/chef/dsl/recipe.rb
@@ -81,7 +81,7 @@ class Chef
 
         resource = build_resource(type, name, created_at, &resource_attrs_block)
 
-        run_context.resource_collection.insert(resource, resource_type:type, instance_name:name)
+        run_context.resource_collection.insert(resource, resource_type: type, instance_name: name)
         resource
       end
 
@@ -101,6 +101,7 @@ class Chef
 
         resource = resource_class.new(name, run_context)
         resource.source_line = created_at
+        resource.declared_type = type
         # If we have a resource like this one, we want to steal its state
         # This behavior is very counter-intuitive and should be removed.
         # See CHEF-3694, https://tickets.opscode.com/browse/CHEF-3694
diff --git a/lib/chef/provider/package/homebrew.rb b/lib/chef/provider/package/homebrew.rb
index 1b407a1901..342c3fd952 100644
--- a/lib/chef/provider/package/homebrew.rb
+++ b/lib/chef/provider/package/homebrew.rb
@@ -26,7 +26,7 @@ class Chef
     class Package
       class Homebrew < Chef::Provider::Package
 
-        provides :homebrew_package, os: "mac_os_x"
+        provides :homebrew_package, os: ["mac_os_x", "darwin"]
 
         include Chef::Mixin::HomebrewUser
 
diff --git a/lib/chef/resource.rb b/lib/chef/resource.rb
index d2718c859e..81642b8915 100644
--- a/lib/chef/resource.rb
+++ b/lib/chef/resource.rb
@@ -30,97 +30,13 @@ require 'chef/resource_collection'
 require 'chef/node_map'
 require 'chef/node'
 require 'chef/platform'
+require 'chef/resource_notification'
 
 require 'chef/mixin/deprecation'
 require 'chef/mixin/descendants_tracker'
 
 class Chef
   class Resource
-    class Notification < Struct.new(:resource, :action, :notifying_resource)
-
-      def duplicates?(other_notification)
-        unless other_notification.respond_to?(:resource) && other_notification.respond_to?(:action)
-          msg = "only duck-types of Chef::Resource::Notification can be checked for duplication "\
-                "you gave #{other_notification.inspect}"
-          raise ArgumentError, msg
-        end
-        other_notification.resource == resource && other_notification.action == action
-      end
-
-      # If resource and/or notifying_resource is not a resource object, this will look them up in the resource collection
-      # and fix the references from strings to actual Resource objects.
-      def resolve_resource_reference(resource_collection)
-        return resource if resource.kind_of?(Chef::Resource) && notifying_resource.kind_of?(Chef::Resource)
-
-        if not(resource.kind_of?(Chef::Resource))
-          fix_resource_reference(resource_collection)
-        end
-
-        if not(notifying_resource.kind_of?(Chef::Resource))
-          fix_notifier_reference(resource_collection)
-        end
-      end
-
-      # This will look up the resource if it is not a Resource Object.  It will complain if it finds multiple
-      # resources, can't find a resource, or gets invalid syntax.
-      def fix_resource_reference(resource_collection)
-        matching_resource = resource_collection.find(resource)
-        if Array(matching_resource).size > 1
-          msg = "Notification #{self} from #{notifying_resource} was created with a reference to multiple resources, "\
-          "but can only notify one resource. Notifying resource was defined on #{notifying_resource.source_line}"
-          raise Chef::Exceptions::InvalidResourceReference, msg
-        end
-        self.resource = matching_resource
-
-      rescue Chef::Exceptions::ResourceNotFound => e
-        err = Chef::Exceptions::ResourceNotFound.new(<<-FAIL)
-resource #{notifying_resource} is configured to notify resource #{resource} with action #{action}, \
-but #{resource} cannot be found in the resource collection. #{notifying_resource} is defined in \
-#{notifying_resource.source_line}
-FAIL
-        err.set_backtrace(e.backtrace)
-        raise err
-      rescue Chef::Exceptions::InvalidResourceSpecification => e
-          err = Chef::Exceptions::InvalidResourceSpecification.new(<<-F)
-Resource #{notifying_resource} is configured to notify resource #{resource} with action #{action}, \
-but #{resource.inspect} is not valid syntax to look up a resource in the resource collection. Notification \
-is defined near #{notifying_resource.source_line}
-F
-          err.set_backtrace(e.backtrace)
-        raise err
-      end
-
-      # This will look up the notifying_resource if it is not a Resource Object.  It will complain if it finds multiple
-      # resources, can't find a resource, or gets invalid syntax.
-      def fix_notifier_reference(resource_collection)
-        matching_notifier = resource_collection.find(notifying_resource)
-        if Array(matching_notifier).size > 1
-          msg = "Notification #{self} from #{notifying_resource} was created with a reference to multiple notifying "\
-          "resources, but can only originate from one resource.  Destination resource was defined "\
-          "on #{resource.source_line}"
-          raise Chef::Exceptions::InvalidResourceReference, msg
-        end
-        self.notifying_resource = matching_notifier
-
-      rescue Chef::Exceptions::ResourceNotFound => e
-        err = Chef::Exceptions::ResourceNotFound.new(<<-FAIL)
-Resource #{resource} is configured to receive notifications from #{notifying_resource} with action #{action}, \
-but #{notifying_resource} cannot be found in the resource collection. #{resource} is defined in \
-#{resource.source_line}
-FAIL
-        err.set_backtrace(e.backtrace)
-        raise err
-      rescue Chef::Exceptions::InvalidResourceSpecification => e
-          err = Chef::Exceptions::InvalidResourceSpecification.new(<<-F)
-Resource #{resource} is configured to receive notifications from  #{notifying_resource} with action #{action}, \
-but #{notifying_resource.inspect} is not valid syntax to look up a resource in the resource collection. Notification \
-is defined near #{resource.source_line}
-F
-          err.set_backtrace(e.backtrace)
-        raise err
-      end
-
-    end
 
     FORBIDDEN_IVARS = [:@run_context, :@not_if, :@only_if, :@enclosing_provider]
     HIDDEN_IVARS = [:@allowed_actions, :@resource_name, :@source_line, :@run_context, :@name, :@not_if, :@only_if, :@elapsed_time, :@enclosing_provider]
@@ -211,6 +127,7 @@ F
     attr_accessor :source_line
     attr_accessor :retries
     attr_accessor :retry_delay
+    attr_accessor :declared_type
 
     attr_reader :updated
 
@@ -298,7 +215,7 @@ F
 
     def load_prior_resource(resource_type, instance_name)
       begin
-        key = ::Chef::ResourceCollection::ResourceSet.create_key(resource_type, instance_name)
+        key = "#{resource_type}[#{instance_name}]"
         prior_resource = run_context.resource_collection.lookup(key)
         # if we get here, there is a prior resource (otherwise we'd have jumped
         # to the rescue clause).
@@ -425,16 +342,16 @@ F
     def notifies(action, resource_spec, timing=:delayed)
       # when using old-style resources(:template => "/foo.txt") style, you
       # could end up with multiple resources.
+      validate_resource_spec!(resource_spec)
+
       resources = [ resource_spec ].flatten
       resources.each do |resource|
 
-        validate_resource_spec!(resource_spec)
-
         case timing.to_s
         when 'delayed'
-          notifies_delayed(action, resource)
+          run_context.notifies_delayed(Notification.new(resource_spec, action, self))
         when 'immediate', 'immediately'
-          notifies_immediately(action, resource)
+          run_context.notifies_immediately(Notification.new(resource_spec, action, self))
         else
           raise ArgumentError,  "invalid timing: #{timing} for notifies(#{action}, #{resources.inspect}, #{timing}) resource #{self} "\
                                 "Valid timings are: :delayed, :immediate, :immediately"
@@ -448,16 +365,12 @@ F
     # resolve_resource_reference on each in turn, causing them to
     # resolve lazy/forward references.
     def resolve_notification_references
-      run_context.immediate_notifications(self).each { |n| n.resolve_resource_reference(run_context.resource_collection) }
-      run_context.delayed_notifications(self).each {|n| n.resolve_resource_reference(run_context.resource_collection) }
-    end
-
-    def notifies_immediately(action, resource_spec)
-      run_context.notifies_immediately(Notification.new(resource_spec, action, self))
-    end
-
-    def notifies_delayed(action, resource_spec)
-      run_context.notifies_delayed(Notification.new(resource_spec, action, self))
+      run_context.immediate_notifications(self).each { |n|
+        n.resolve_resource_reference(run_context.resource_collection)
+      }
+      run_context.delayed_notifications(self).each {|n|
+        n.resolve_resource_reference(run_context.resource_collection)
+      }
     end
 
     def immediate_notifications
@@ -498,6 +411,14 @@ F
       end
     end
 
+    # We usually want to store and reference resources by their declared type and not the actual type that
+    # was looked up by the Resolver (IE, "package" becomes YumPackage class).  If we have not been provided
+    # the declared key we want to fall back on the old to_s key.
+    def declared_key
+      return to_s if declared_type.nil?
+      "#{declared_type}[#{@name}]"
+    end
+
     def to_s
       "#{@resource_name}[#{@name}]"
     end
diff --git a/lib/chef/resource/homebrew_package.rb b/lib/chef/resource/homebrew_package.rb
index 952552e3a8..3bd5dc16dc 100644
--- a/lib/chef/resource/homebrew_package.rb
+++ b/lib/chef/resource/homebrew_package.rb
@@ -25,7 +25,8 @@ class Chef
   class Resource
     class HomebrewPackage < Chef::Resource::Package
 
-      provides :homebrew_package, os: "mac_os_x"
+      provides :homebrew_package
+      provides :package, os: ["mac_os_x", "darwin"]
 
       def initialize(name, run_context=nil)
         super
diff --git a/lib/chef/resource_collection/resource_set.rb b/lib/chef/resource_collection/resource_set.rb
index 6425c2ab08..1b39298cb4 100644
--- a/lib/chef/resource_collection/resource_set.rb
+++ b/lib/chef/resource_collection/resource_set.rb
@@ -44,7 +44,7 @@ class Chef
         is_chef_resource!(resource)
         resource_type ||= resource.resource_name
         instance_name ||= resource.name
-        key = ResourceSet.create_key(resource_type, instance_name)
+        key = create_key(resource_type, instance_name)
         @resources_by_key[key] = resource
       end
 
@@ -53,7 +53,7 @@ class Chef
           when key.kind_of?(String)
             lookup_by = key
           when key.kind_of?(Chef::Resource)
-            lookup_by = ResourceSet.create_key(key.resource_name, key.name)
+            lookup_by = create_key(key.resource_name, key.name)
           else
             raise ArgumentError, "Must pass a Chef::Resource or String to lookup"
         end
@@ -128,18 +128,18 @@ class Chef
         end
       end
 
-      def self.create_key(resource_type, instance_name)
+      private
+
+      def create_key(resource_type, instance_name)
         "#{resource_type}[#{instance_name}]"
       end
 
-      private
-
       def find_resource_by_hash(arg)
         results = Array.new
         arg.each do |resource_type, name_list|
           instance_names = name_list.kind_of?(Array) ? name_list : [ name_list ]
           instance_names.each do |instance_name|
-            results << lookup(ResourceSet.create_key(resource_type, instance_name))
+            results << lookup(create_key(resource_type, instance_name))
           end
         end
         return results
@@ -153,12 +153,12 @@ class Chef
             arg =~ /^.+\[(.+)\]$/
             resource_list = $1
             resource_list.split(",").each do |instance_name|
-              results << lookup(ResourceSet.create_key(resource_type, instance_name))
+              results << lookup(create_key(resource_type, instance_name))
             end
           when SINGLE_RESOURCE_MATCH
             resource_type = $1
             name = $2
-            results << lookup(ResourceSet.create_key(resource_type, name))
+            results << lookup(create_key(resource_type, name))
           else
             raise ArgumentError, "Bad string format #{arg}, you must have a string like resource_type[name]!"
         end
diff --git a/lib/chef/resource_notification.rb b/lib/chef/resource_notification.rb
new file mode 100644
index 0000000000..c02c268709
--- /dev/null
+++ b/lib/chef/resource_notification.rb
@@ -0,0 +1,91 @@
+require 'chef/resource'
+
+class Chef
+  class Resource
+    class Notification < Struct.new(:resource, :action, :notifying_resource)
+
+      def duplicates?(other_notification)
+        unless other_notification.respond_to?(:resource) && other_notification.respond_to?(:action)
+          msg = "only duck-types of Chef::Resource::Notification can be checked for duplication "\
+                "you gave #{other_notification.inspect}"
+          raise ArgumentError, msg
+        end
+        other_notification.resource == resource && other_notification.action == action
+      end
+
+      # If resource and/or notifying_resource is not a resource object, this will look them up in the resource collection
+      # and fix the references from strings to actual Resource objects.
+      def resolve_resource_reference(resource_collection)
+        return resource if resource.kind_of?(Chef::Resource) && notifying_resource.kind_of?(Chef::Resource)
+
+        if not(resource.kind_of?(Chef::Resource))
+          fix_resource_reference(resource_collection)
+        end
+
+        if not(notifying_resource.kind_of?(Chef::Resource))
+          fix_notifier_reference(resource_collection)
+        end
+      end
+
+      # This will look up the resource if it is not a Resource Object.  It will complain if it finds multiple
+      # resources, can't find a resource, or gets invalid syntax.
+      def fix_resource_reference(resource_collection)
+        matching_resource = resource_collection.find(resource)
+        if Array(matching_resource).size > 1
+          msg = "Notification #{self} from #{notifying_resource} was created with a reference to multiple resources, "\
+          "but can only notify one resource. Notifying resource was defined on #{notifying_resource.source_line}"
+          raise Chef::Exceptions::InvalidResourceReference, msg
+        end
+        self.resource = matching_resource
+
+      rescue Chef::Exceptions::ResourceNotFound => e
+        err = Chef::Exceptions::ResourceNotFound.new(<<-FAIL)
+resource #{notifying_resource} is configured to notify resource #{resource} with action #{action}, \
+but #{resource} cannot be found in the resource collection. #{notifying_resource} is defined in \
+#{notifying_resource.source_line}
+        FAIL
+        err.set_backtrace(e.backtrace)
+        raise err
+      rescue Chef::Exceptions::InvalidResourceSpecification => e
+        err = Chef::Exceptions::InvalidResourceSpecification.new(<<-F)
+Resource #{notifying_resource} is configured to notify resource #{resource} with action #{action}, \
+but #{resource.inspect} is not valid syntax to look up a resource in the resource collection. Notification \
+is defined near #{notifying_resource.source_line}
+        F
+        err.set_backtrace(e.backtrace)
+        raise err
+      end
+
+      # This will look up the notifying_resource if it is not a Resource Object.  It will complain if it finds multiple
+      # resources, can't find a resource, or gets invalid syntax.
+      def fix_notifier_reference(resource_collection)
+        matching_notifier = resource_collection.find(notifying_resource)
+        if Array(matching_notifier).size > 1
+          msg = "Notification #{self} from #{notifying_resource} was created with a reference to multiple notifying "\
+          "resources, but can only originate from one resource.  Destination resource was defined "\
+          "on #{resource.source_line}"
+          raise Chef::Exceptions::InvalidResourceReference, msg
+        end
+        self.notifying_resource = matching_notifier
+
+      rescue Chef::Exceptions::ResourceNotFound => e
+        err = Chef::Exceptions::ResourceNotFound.new(<<-FAIL)
+Resource #{resource} is configured to receive notifications from #{notifying_resource} with action #{action}, \
+but #{notifying_resource} cannot be found in the resource collection. #{resource} is defined in \
+#{resource.source_line}
+        FAIL
+        err.set_backtrace(e.backtrace)
+        raise err
+      rescue Chef::Exceptions::InvalidResourceSpecification => e
+        err = Chef::Exceptions::InvalidResourceSpecification.new(<<-F)
+Resource #{resource} is configured to receive notifications from  #{notifying_resource} with action #{action}, \
+but #{notifying_resource.inspect} is not valid syntax to look up a resource in the resource collection. Notification \
+is defined near #{resource.source_line}
+        F
+        err.set_backtrace(e.backtrace)
+        raise err
+      end
+
+    end
+  end
+end
diff --git a/lib/chef/run_context.rb b/lib/chef/run_context.rb
index 1a2d7ba3a3..e1ba87b258 100644
--- a/lib/chef/run_context.rb
+++ b/lib/chef/run_context.rb
@@ -100,10 +100,13 @@ class Chef
       if nr.instance_of?(Chef::Resource)
         @immediate_notification_collection[nr.name] << notification
       else
-        @immediate_notification_collection[nr.to_s] << notification
+        @immediate_notification_collection[nr.declared_key] << notification
       end
     end
 
+    # TODO notification.notifying_resource either needs to be a real resource or "type[name]" which matches declaration
+    # TODO otherwords, @collection needs to always be keyed by "defined_type[name]"
+
     # Adds a delayed notification to the +delayed_notification_collection+. The
     # notification should be a Chef::Resource::Notification or duck type.
     def notifies_delayed(notification)
@@ -111,7 +114,7 @@ class Chef
       if nr.instance_of?(Chef::Resource)
         @delayed_notification_collection[nr.name] << notification
       else
-        @delayed_notification_collection[nr.to_s] << notification
+        @delayed_notification_collection[nr.declared_key] << notification
       end
     end
 
@@ -119,7 +122,7 @@ class Chef
       if resource.instance_of?(Chef::Resource)
         return @immediate_notification_collection[resource.name]
       else
-        return @immediate_notification_collection[resource.to_s]
+        return @immediate_notification_collection[resource.declared_key]
       end
     end
 
@@ -127,7 +130,7 @@ class Chef
       if resource.instance_of?(Chef::Resource)
         return @delayed_notification_collection[resource.name]
       else
-        return @delayed_notification_collection[resource.to_s]
+        return @delayed_notification_collection[resource.declared_key]
       end
     end
 
diff --git a/spec/functional/notifications_spec.rb b/spec/functional/notifications_spec.rb
new file mode 100644
index 0000000000..a02fdffe5e
--- /dev/null
+++ b/spec/functional/notifications_spec.rb
@@ -0,0 +1,169 @@
+require 'spec_helper'
+require 'chef/recipe'
+
+
+# The goal of these tests is to make sure that loading resources from a file creates the necessary notifications.
+# Then once converge has started, both immediate and delayed notifications are called as the resources are converged.
+# We want to do this WITHOUT actually converging any resources - we don't want to take time changing the system,
+# we just want to make sure the run_context, the notification DSL and the converge hooks are working together
+# to perform notifications.
+
+# This test is extremely fragile since it mocks MANY different systems at once - any of them changes, this test
+# breaks
+describe "Notifications" do
+
+  # We always pretend we are on OSx because that has a specific provider (HomebrewProvider) so it
+  # tests the translation from Provider => HomebrewProvider
+  let(:node) {
+    n = Chef::Node.new
+    n.override[:os] = "darwin"
+    n
+  }
+  let(:cookbook_collection) { double("Chef::CookbookCollection").as_null_object }
+  let(:events) { double("Chef::EventDispatch::Dispatcher").as_null_object }
+  let(:run_context) { Chef::RunContext.new(node, cookbook_collection, events) }
+  let(:recipe) { Chef::Recipe.new("notif", "test", run_context) }
+  let(:runner) { Chef::Runner.new(run_context) }
+
+  before do
+    # By default, every provider will do nothing
+    p = Chef::Provider.new(nil, run_context)
+    allow_any_instance_of(Chef::Resource).to receive(:provider_for_action).and_return(p)
+    allow(p).to receive(:run_action)
+  end
+
+  it "should subscribe from one resource to another" do
+    log_resource = recipe.declare_resource(:log, "subscribed-log") do
+      message "This is a log message"
+      action :nothing
+      subscribes :write, "package[vim]", :immediately
+    end
+
+    package_resource = recipe.declare_resource(:package, "vim") do
+      action :install
+    end
+
+    expect(log_resource).to receive(:run_action).with(:nothing, nil, nil).and_call_original
+
+    expect(package_resource).to receive(:run_action).with(:install, nil, nil).and_call_original
+    update_action(package_resource)
+
+    expect(log_resource).to receive(:run_action).with(:write, :immediate, package_resource).and_call_original
+
+    runner.converge
+  end
+
+  it "should notify from one resource to another immediately" do
+    log_resource = recipe.declare_resource(:log, "log") do
+      message "This is a log message"
+      action :write
+      notifies :install, "package[vim]", :immediately
+    end
+
+    package_resource = recipe.declare_resource(:package, "vim") do
+      action :nothing
+    end
+
+    expect(log_resource).to receive(:run_action).with(:write, nil, nil).and_call_original
+    update_action(log_resource)
+
+    expect(package_resource).to receive(:run_action).with(:install, :immediate, log_resource).ordered.and_call_original
+
+    expect(package_resource).to receive(:run_action).with(:nothing, nil, nil).ordered.and_call_original
+
+    runner.converge
+  end
+
+  it "should notify from one resource to another delayed" do
+    log_resource = recipe.declare_resource(:log, "log") do
+      message "This is a log message"
+      action :write
+      notifies :install, "package[vim]", :delayed
+    end
+
+    package_resource = recipe.declare_resource(:package, "vim") do
+      action :nothing
+    end
+
+    expect(log_resource).to receive(:run_action).with(:write, nil, nil).and_call_original
+    update_action(log_resource)
+
+    expect(package_resource).to receive(:run_action).with(:nothing, nil, nil).ordered.and_call_original
+
+    expect(package_resource).to receive(:run_action).with(:install, :delayed, nil).ordered.and_call_original
+
+    runner.converge
+  end
+  
+  describe "when one resource is defined lazily" do
+
+    it "subscribes to a resource defined in a ruby block" do
+      r = recipe
+      t = self
+      ruby_block = recipe.declare_resource(:ruby_block, "rblock") do
+        block do
+          log_resource = r.declare_resource(:log, "log") do
+            message "This is a log message"
+            action :write
+          end
+          t.expect(log_resource).to t.receive(:run_action).with(:write, nil, nil).and_call_original
+          t.update_action(log_resource)
+        end
+      end
+
+      package_resource = recipe.declare_resource(:package, "vim") do
+        action :nothing
+        subscribes :install, "log[log]", :delayed
+      end
+
+      # RubyBlock needs to be able to run for our lazy examples to work - and it alone cannot affect the system
+      expect(ruby_block).to receive(:provider_for_action).and_call_original
+
+      expect(package_resource).to receive(:run_action).with(:nothing, nil, nil).ordered.and_call_original
+
+      expect(package_resource).to receive(:run_action).with(:install, :delayed, nil).ordered.and_call_original
+
+      runner.converge
+    end
+
+    it "notifies from inside a ruby_block to a resource defined outside" do
+      r = recipe
+      t = self
+      ruby_block = recipe.declare_resource(:ruby_block, "rblock") do
+        block do
+          log_resource = r.declare_resource(:log, "log") do
+            message "This is a log message"
+            action :write
+            notifies :install, "package[vim]", :immediately
+          end
+          t.expect(log_resource).to t.receive(:run_action).with(:write, nil, nil).and_call_original
+          t.update_action(log_resource)
+        end
+      end
+
+      package_resource = recipe.declare_resource(:package, "vim") do
+        action :nothing
+      end
+
+      # RubyBlock needs to be able to run for our lazy examples to work - and it alone cannot affect the system
+      expect(ruby_block).to receive(:provider_for_action).and_call_original
+
+      expect(package_resource).to receive(:run_action).with(:install, :immediate, instance_of(Chef::Resource::Log)).ordered.and_call_original
+
+      expect(package_resource).to receive(:run_action).with(:nothing, nil, nil).ordered.and_call_original
+
+      runner.converge
+    end
+
+  end
+
+  # Mocks having the provider run successfully and update the resource
+  def update_action(resource)
+    p = Chef::Provider.new(resource, run_context)
+    expect(resource).to receive(:provider_for_action).and_return(p)
+    expect(p).to receive(:run_action) {
+      resource.updated_by_last_action(true)
+    }
+  end
+
+end