ActiveRecord::Observer.observed_classes doesn't get redefined properly #3505

Closed
pivotal-casebook opened this Issue Nov 3, 2011 · 12 comments

Comments

Projects
None yet
4 participants
@pivotal-casebook
Contributor

pivotal-casebook commented Nov 3, 2011

This is an intentional duplicate of #1034, which was just closed without any solution.

class Foo < ActiveRecord::Base
end

class BarObserver < ActiveRecord::Observer
  observe :foo
end

ruby-1.8.7-p174 > BarObserver.observed_classes
NameError: uninitialized constant Bar
    from /Users/pivotal/.rvm/gems/ruby-1.8.7-p174/gems/activesupport-3.0.6/lib/active_support/inflector/methods.rb:113:in `constantize'
    from /Users/pivotal/.rvm/gems/ruby-1.8.7-p174/gems/activesupport-3.0.6/lib/active_support/inflector/methods.rb:112:in `each'
    from /Users/pivotal/.rvm/gems/ruby-1.8.7-p174/gems/activesupport-3.0.6/lib/active_support/inflector/methods.rb:112:in `constantize'
    from /Users/pivotal/.rvm/gems/ruby-1.8.7-p174/gems/activesupport-3.0.6/lib/active_support/core_ext/string/inflections.rb:43:in `constantize'
    from /Users/pivotal/.rvm/gems/ruby-1.8.7-p174/gems/activemodel-3.0.6/lib/active_model/observing.rb:182:in `observed_class'
    from /Users/pivotal/.rvm/gems/ruby-1.8.7-p174/gems/activemodel-3.0.6/lib/active_model/observing.rb:175:in `observed_classes'
    from (irb):1

By reading the implementation at https://github.com/rails/rails/blob/v3.0.6/activemodel/lib/active_model/observing.rb#L158-163 I would expect BarObserver's observed_classes method to have been redefined to return [Foo] without trying to find the non-existant Bar class.

@frodsan

This comment has been minimized.

Show comment Hide comment
@frodsan

frodsan Apr 29, 2012

Contributor

@pivotal-casebook I have this error with master too. cc @rafaelfranca

Contributor

frodsan commented Apr 29, 2012

@pivotal-casebook I have this error with master too. cc @rafaelfranca

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Apr 29, 2012

Member

@frodsan thanks. I tagged this issue.

Member

rafaelfranca commented Apr 29, 2012

@frodsan thanks. I tagged this issue.

@frodsan

This comment has been minimized.

Show comment Hide comment
@frodsan

frodsan Apr 29, 2012

Contributor

@rafaelfranca can you help me with this? I change this in the rails code:

def observe(*models)
    models.flatten!
    models.collect! { |model| model.respond_to?(:to_sym) ? model.to_s.camelize.constantize : model }
    remove_possible_method(:observed_classes)
    define_singleton_method(:observed_classes) { models }
 end

and now works fine:

irb(main):001:0> class Foo < ActiveRecord::Base
irb(main):002:1> end
=> nil
irb(main):003:0> 
irb(main):004:0* class BarObserver < ActiveRecord::Observer
irb(main):005:1>   observe :foo
irb(main):006:1> end
=> #<Proc:0x007fe86bb482b8@/Users/frodsan/Code/rails/activemodel/lib/active_model/observing.rb:198 (lambda)>
irb(main):007:0> BarObserver.observed_classes
=> [Foo(Table doesn't exist)]
Contributor

frodsan commented Apr 29, 2012

@rafaelfranca can you help me with this? I change this in the rails code:

def observe(*models)
    models.flatten!
    models.collect! { |model| model.respond_to?(:to_sym) ? model.to_s.camelize.constantize : model }
    remove_possible_method(:observed_classes)
    define_singleton_method(:observed_classes) { models }
 end

and now works fine:

irb(main):001:0> class Foo < ActiveRecord::Base
irb(main):002:1> end
=> nil
irb(main):003:0> 
irb(main):004:0* class BarObserver < ActiveRecord::Observer
irb(main):005:1>   observe :foo
irb(main):006:1> end
=> #<Proc:0x007fe86bb482b8@/Users/frodsan/Code/rails/activemodel/lib/active_model/observing.rb:198 (lambda)>
irb(main):007:0> BarObserver.observed_classes
=> [Foo(Table doesn't exist)]
@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@frodsan

This comment has been minimized.

Show comment Hide comment
@frodsan

frodsan Apr 29, 2012

Contributor

@rafaelfranca ok, i will do the pull request and let you know :)

Contributor

frodsan commented Apr 29, 2012

@rafaelfranca ok, i will do the pull request and let you know :)

@frodsan

This comment has been minimized.

Show comment Hide comment
@frodsan

frodsan Apr 29, 2012

Contributor

@pivotal-casebook @rafaelfranca is redefining the observed_classes instance method:

FooObserver.observe 'observed_model'
instance = FooObserver.new
assert instance.send(:observed_classes).include?(ObservedModel)

When must be redefining the observed_classes class method, so it can be called in the instance method too:

def observed_classes #:nodoc:
  self.class.observed_classes
end

any ideas?

Contributor

frodsan commented Apr 29, 2012

@pivotal-casebook @rafaelfranca is redefining the observed_classes instance method:

FooObserver.observe 'observed_model'
instance = FooObserver.new
assert instance.send(:observed_classes).include?(ObservedModel)

When must be redefining the observed_classes class method, so it can be called in the instance method too:

def observed_classes #:nodoc:
  self.class.observed_classes
end

any ideas?

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@frodsan

This comment has been minimized.

Show comment Hide comment
@frodsan

frodsan Apr 29, 2012

Contributor

No, because when observe is called, it redefines this instance method in ActiveModel::Observer:

def observed_classes #:nodoc:
  self.class.observed_classes
end

instead of the class method:

def observed_classes
  Array(observed_class)
end

@rafaelfranca I'm very close. I change to define_singleton_method. But i want to know how to undef this because i'm having some warnings :S

def observe(*models)
  models.flatten!
  models.collect! { |model| model.respond_to?(:to_sym) ? model.to_s.camelize.constantize : model }
  # i have to undef this singleton method :S 
  define_singleton_method(:observed_classes) { models }
end

These are the warnings:

rails/activemodel/lib/active_model/observing.rb:197: warning: previous definition of observed_classes was here
rails/activemodel/lib/active_model/observing.rb:197: warning: method redefined; discarding old observed_classes
Contributor

frodsan commented Apr 29, 2012

No, because when observe is called, it redefines this instance method in ActiveModel::Observer:

def observed_classes #:nodoc:
  self.class.observed_classes
end

instead of the class method:

def observed_classes
  Array(observed_class)
end

@rafaelfranca I'm very close. I change to define_singleton_method. But i want to know how to undef this because i'm having some warnings :S

def observe(*models)
  models.flatten!
  models.collect! { |model| model.respond_to?(:to_sym) ? model.to_s.camelize.constantize : model }
  # i have to undef this singleton method :S 
  define_singleton_method(:observed_classes) { models }
end

These are the warnings:

rails/activemodel/lib/active_model/observing.rb:197: warning: previous definition of observed_classes was here
rails/activemodel/lib/active_model/observing.rb:197: warning: method redefined; discarding old observed_classes
@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Apr 29, 2012

Member

Or you can try to check if it is already defined. If so you dont define.

unless self.singleton_class.respond_to?(:observed_classes)
  define_singleton_method(:observed_classes) { models }
end
Member

rafaelfranca commented Apr 29, 2012

Or you can try to check if it is already defined. If so you dont define.

unless self.singleton_class.respond_to?(:observed_classes)
  define_singleton_method(:observed_classes) { models }
end
@frodsan

This comment has been minimized.

Show comment Hide comment
@frodsan

frodsan Apr 29, 2012

Contributor

@rafaelfranca I made the change. But i'm having those warnings from https://github.com/rails/rails/blob/master/activemodel/test/cases/observing_test.rb#L90. I think i have to check subclasses too or am i wrong?

Contributor

frodsan commented Apr 29, 2012

@rafaelfranca I made the change. But i'm having those warnings from https://github.com/rails/rails/blob/master/activemodel/test/cases/observing_test.rb#L90. I think i have to check subclasses too or am i wrong?

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Apr 29, 2012

Member

Well I don't know. I never looked at the observers code. You can try. Someone can point a better implementation when you open the pull request.

Member

rafaelfranca commented Apr 29, 2012

Well I don't know. I never looked at the observers code. You can try. Someone can point a better implementation when you open the pull request.

@frodsan

This comment has been minimized.

Show comment Hide comment
@frodsan

frodsan Apr 30, 2012

Contributor

@rafaelfranca ok, i sent the pull request. muito obrigado! :)

Contributor

frodsan commented Apr 30, 2012

@rafaelfranca ok, i sent the pull request. muito obrigado! :)

@jeremy jeremy closed this in 0393c7c Apr 30, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment