Merge remote branch 'myron/am_disabling_fix_memory_leaks'

activemodel/lib/active_model/observer_array.rb

@@ -4,42 +4,16 @@ module ActiveModel
   # Stores the enabled/disabled state of individual observers for
   # a particular model classes.
   class ObserverArray < Array
-    INSTANCES = Hash.new do |hash, model_class|
-      hash[model_class] = new(model_class)
-    end
-
-    def self.for(model_class)
-      return nil unless model_class < ActiveModel::Observing
-      INSTANCES[model_class]
-    end
-
-    # returns false if:
-    #   - the ObserverArray for the given model's class has the given observer
-    #     in its disabled_observers set.
-    #   - or that is the case at any level of the model's superclass chain.
-    def self.observer_enabled?(observer, model)
-      klass = model.class
-      observer_class = observer.class
-
-      loop do
-        break unless array = self.for(klass)
-        return false if array.disabled_observers.include?(observer_class)
-        klass = klass.superclass
-      end
-
-      true # observers are enabled by default
-    end
-
-    def disabled_observers
-      @disabled_observers ||= Set.new
-    end
-
     attr_reader :model_class
     def initialize(model_class, *args)
       @model_class = model_class
       super(*args)
     end
 
+    def disabled_for?(observer)
+      disabled_observers.include?(observer.class)
+    end
+
     def disable(*observers, &block)
       set_enablement(false, observers, &block)
     end
@@ -48,7 +22,11 @@ def enable(*observers, &block)
       set_enablement(true, observers, &block)
     end
 
-    private
+    protected
+
+      def disabled_observers
+        @disabled_observers ||= Set.new
+      end
 
       def observer_class_for(observer)
         return observer if observer.is_a?(Class)
@@ -61,13 +39,37 @@ def observer_class_for(observer)
         end
       end
 
+      def start_transaction
+        disabled_observer_stack.push(disabled_observers.dup)
+        each_subclass_array do |array|
+          array.start_transaction
+        end
+      end
+
+      def disabled_observer_stack
+        @disabled_observer_stack ||= []
+      end
+
+      def end_transaction
+        @disabled_observers = disabled_observer_stack.pop
+        each_subclass_array do |array|
+          array.end_transaction
+        end
+      end
+
       def transaction
-        orig_disabled_observers = disabled_observers.dup
+        start_transaction
 
         begin
           yield
         ensure
-          @disabled_observers = orig_disabled_observers
+          end_transaction
+        end
+      end
+
+      def each_subclass_array
+        model_class.descendants.each do |subclass|
+          yield subclass.observers
         end
       end
 
@@ -78,7 +80,7 @@ def set_enablement(enabled, observers)
             yield
           end
         else
-          observers = ActiveModel::Observer.all_observers if observers == [:all]
+          observers = ActiveModel::Observer.descendants if observers == [:all]
           observers.each do |obs|
             klass = observer_class_for(obs)
 
@@ -92,6 +94,10 @@ def set_enablement(enabled, observers)
               disabled_observers << klass
             end
           end
+
+          each_subclass_array do |array|
+            array.set_enablement(enabled, observers)
+          end
         end
       end
   end

activemodel/lib/active_model/observing.rb

@@ -5,11 +5,16 @@
 require 'active_support/core_ext/module/remove_method'
 require 'active_support/core_ext/string/inflections'
 require 'active_support/core_ext/enumerable'
+require 'active_support/descendants_tracker'
 
 module ActiveModel
   module Observing
     extend ActiveSupport::Concern
 
+    included do
+      extend ActiveSupport::DescendantsTracker
+    end
+
     module ClassMethods
       # == Active Model Observers Activation
       #
@@ -37,7 +42,7 @@ def observers=(*values)
 
       # Gets the current observers.
       def observers
-        @observers ||= ObserverArray.for(self)
+        @observers ||= ObserverArray.new(self)
       end
 
       # Gets the current observer instances.
@@ -171,6 +176,7 @@ def notify_observers(method)
   #
   class Observer
     include Singleton
+    extend ActiveSupport::DescendantsTracker
 
     class << self
       # Attaches the observer to the supplied model classes.
@@ -203,23 +209,6 @@ def observed_class
           nil
         end
       end
-
-      def subclasses
-        @subclasses ||= []
-      end
-
-      # List of all observer subclasses, sub-subclasses, etc.
-      # Necessary so we can disable or enable all observers.
-      def all_observers
-        subclasses.each_with_object(subclasses.dup) do |subclass, array|
-          array.concat(subclass.all_observers)
-        end
-      end
-    end
-
-    def self.inherited(subclass)
-      subclasses << subclass
-      super
     end
 
     # Start observing the declared classes and their subclasses.
@@ -233,9 +222,9 @@ def observed_classes #:nodoc:
 
     # Send observed_method(object) if the method exists.
     def update(observed_method, object) #:nodoc:
-      if respond_to?(observed_method) && ObserverArray.observer_enabled?(self, object)
-        send(observed_method, object)
-      end
+      return unless respond_to?(observed_method)
+      return if disabled_for?(object)
+      send(observed_method, object)
     end
 
     # Special method sent by the observed class when it is inherited.
@@ -249,5 +238,11 @@ def observed_class_inherited(subclass) #:nodoc:
       def add_observer!(klass) #:nodoc:
         klass.add_observer(self)
       end
+
+      def disabled_for?(object)
+        klass = object.class
+        return false unless klass.respond_to?(:observers)
+        klass.observers.disabled_for?(self)
+      end
   end
 end

activemodel/test/cases/observer_array_test.rb

@@ -38,6 +38,16 @@ def assert_observer_not_notified(model_class, observer_class)
     assert_observer_notified     Budget, AuditTrail
   end
 
+  test "can enable individual observers using a class constant" do
+    ORM.observers.disable :all
+    ORM.observers.enable AuditTrail
+
+    assert_observer_not_notified Widget, WidgetObserver
+    assert_observer_not_notified Budget, BudgetObserver
+    assert_observer_notified     Widget, AuditTrail
+    assert_observer_notified     Budget, AuditTrail
+  end
+
   test "can disable individual observers using a symbol" do
     ORM.observers.disable :budget_observer
 
@@ -47,6 +57,35 @@ def assert_observer_not_notified(model_class, observer_class)
     assert_observer_notified     Budget, AuditTrail
   end
 
+  test "can enable individual observers using a symbol" do
+    ORM.observers.disable :all
+    ORM.observers.enable :audit_trail
+
+    assert_observer_not_notified Widget, WidgetObserver
+    assert_observer_not_notified Budget, BudgetObserver
+    assert_observer_notified     Widget, AuditTrail
+    assert_observer_notified     Budget, AuditTrail
+  end
+
+  test "can disable multiple observers at a time" do
+    ORM.observers.disable :widget_observer, :budget_observer
+
+    assert_observer_not_notified Widget, WidgetObserver
+    assert_observer_not_notified Budget, BudgetObserver
+    assert_observer_notified     Widget, AuditTrail
+    assert_observer_notified     Budget, AuditTrail
+  end
+
+  test "can enable multiple observers at a time" do
+    ORM.observers.disable :all
+    ORM.observers.enable :widget_observer, :budget_observer
+
+    assert_observer_notified     Widget, WidgetObserver
+    assert_observer_notified     Budget, BudgetObserver
+    assert_observer_not_notified Widget, AuditTrail
+    assert_observer_not_notified Budget, AuditTrail
+  end
+
   test "can disable all observers using :all" do
     ORM.observers.disable :all
 
@@ -56,7 +95,17 @@ def assert_observer_not_notified(model_class, observer_class)
     assert_observer_not_notified Budget, AuditTrail
   end
 
-  test "can disable observers on individual models without affecting observers on other models" do
+  test "can enable all observers using :all" do
+    ORM.observers.disable :all
+    ORM.observers.enable :all
+
+    assert_observer_notified Widget, WidgetObserver
+    assert_observer_notified Budget, BudgetObserver
+    assert_observer_notified Widget, AuditTrail
+    assert_observer_notified Budget, AuditTrail
+  end
+
+  test "can disable observers on individual models without affecting those observers on other models" do
     Widget.observers.disable :all
 
     assert_observer_not_notified Widget, WidgetObserver
@@ -65,6 +114,16 @@ def assert_observer_not_notified(model_class, observer_class)
     assert_observer_notified     Budget, AuditTrail
   end
 
+  test "can enable observers on individual models without affecting those observers on other models" do
+    ORM.observers.disable :all
+    Budget.observers.enable AuditTrail
+
+    assert_observer_not_notified Widget, WidgetObserver
+    assert_observer_not_notified Budget, BudgetObserver
+    assert_observer_not_notified Widget, AuditTrail
+    assert_observer_notified     Budget, AuditTrail
+  end
+
   test "can disable observers for the duration of a block" do
     yielded = false
     ORM.observers.disable :budget_observer do
@@ -118,5 +177,44 @@ def assert_observer_not_notified(model_class, observer_class)
       ORM.observers.disable Widget
     end
   end
+
+  test "allows #enable at the superclass level to override #disable at the subclass level when called last" do
+    Widget.observers.disable :all
+    ORM.observers.enable :all
+
+    assert_observer_notified Widget, WidgetObserver
+    assert_observer_notified Budget, BudgetObserver
+    assert_observer_notified Widget, AuditTrail
+    assert_observer_notified Budget, AuditTrail
+  end
+
+  test "allows #disable at the superclass level to override #enable at the subclass level when called last" do
+    Budget.observers.enable :audit_trail
+    ORM.observers.disable :audit_trail
+
+    assert_observer_notified     Widget, WidgetObserver
+    assert_observer_notified     Budget, BudgetObserver
+    assert_observer_not_notified Widget, AuditTrail
+    assert_observer_not_notified Budget, AuditTrail
+  end
+
+  test "can use the block form at different levels of the hierarchy" do
+    yielded = false
+    Widget.observers.disable :all
+
+    ORM.observers.enable :all do
+      yielded = true
+      assert_observer_notified Widget, WidgetObserver
+      assert_observer_notified Budget, BudgetObserver
+      assert_observer_notified Widget, AuditTrail
+      assert_observer_notified Budget, AuditTrail
+    end
+
+    assert yielded
+    assert_observer_not_notified Widget, WidgetObserver
+    assert_observer_notified     Budget, BudgetObserver
+    assert_observer_not_notified Widget, AuditTrail
+    assert_observer_notified     Budget, AuditTrail
+  end
 end