Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

disable observers without cross-thread side effects #16

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions lib/rails/observers/active_model/disabled_observers_registry.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
module ActiveModel
class DisabledObserversRegistry
extend ActiveSupport::PerThreadRegistry

attr_accessor :disabled_observers_per_class
attr_accessor :disabled_observers_stacks_per_class

def initialize
@disabled_observers_per_class = {}
@disabled_observers_stacks_per_class = {}
end
end
end
10 changes: 7 additions & 3 deletions lib/rails/observers/active_model/observer_array.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,11 @@ def enable(*observers, &block)
protected

def disabled_observers #:nodoc:
@disabled_observers ||= Set.new
DisabledObserversRegistry.disabled_observers_per_class[model_class] ||= Set.new
end

def disabled_observers= observers #:nodoc:
DisabledObserversRegistry.disabled_observers_per_class[model_class] = observers
end

def observer_class_for(observer) #:nodoc:
Expand All @@ -95,11 +99,11 @@ def start_transaction #:nodoc:
end

def disabled_observer_stack #:nodoc:
@disabled_observer_stack ||= []
DisabledObserversRegistry.disabled_observers_stacks_per_class[model_class] ||= []
end

def end_transaction #:nodoc:
@disabled_observers = disabled_observer_stack.pop
self.disabled_observers = disabled_observer_stack.pop
each_subclass_array do |array|
array.end_transaction
end
Expand Down
1 change: 1 addition & 0 deletions lib/rails/observers/active_model/observing.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require 'singleton'
require 'rails/observers/active_model/disabled_observers_registry'
require 'rails/observers/active_model/observer_array'
require 'active_support/core_ext/module/aliasing'
require 'active_support/core_ext/module/remove_method'
Expand Down
8 changes: 7 additions & 1 deletion test/observer_array_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@ def assert_observer_not_notified(model_class, observer_class)

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
assert_observer_notified Budget, BudgetObserver
assert_observer_not_notified Widget, AuditTrail
Expand Down Expand Up @@ -162,6 +161,13 @@ def assert_observer_not_notified(model_class, observer_class)
assert_observer_notified Budget, AuditTrail
end

test "can enable observer without side effects on other threads" do
Thread.new do
Widget.observers.disable :audit_trail
end.join()
assert_observer_notified Widget, AuditTrail
end

test "raises an appropriate error when a developer accidentally enables or disables the wrong class (i.e. Widget instead of WidgetObserver)" do
assert_raise ArgumentError do
ORM.observers.enable :widget
Expand Down