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

Fix instantiating observers on app boot. #65

Merged
merged 1 commit into from
Jul 16, 2017

Conversation

kaspth
Copy link
Contributor

@kaspth kaspth commented Jul 16, 2017

When adding support for Rails 4.1, 4.2 and 5.0, the instantiate_observers
call that happened after boot was erroneously removed.
https://github.com/rails/rails-observers/pull/39/files#diff-9db46e46113d0c2eeb41f5d6f08bcac4L24

This covered a regression in the test suite for the rake integration and
also broke the gem for apps.

The reason the test/rake_test.rb failed was that a require to active_record/base
had been removed which rails-observers depended on.
rails/rails@9e9793b

Without that we're this circular dependency error:

kaspers-macbook-pro:observer kasperhansen$ cat Rakefile

require_relative 'config/application'

namespace :user do
  task :count => :environment do
    puts User.count
  end
end

Rails.application.load_tasks

kaspers-macbook-pro:observer kasperhansen$ bin/rake user:count
rake aborted!
Circular dependency detected while autoloading constant User
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/active_model/observing.rb:334:in `observed_class'
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/active_model/observing.rb:325:in `observed_classes'
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/active_model/observing.rb:345:in `observed_classes'
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/activerecord/observer.rb:100:in `observed_classes'
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/active_model/observing.rb:341:in `initialize'
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/active_model/observing.rb:186:in `instantiate_observer'
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/active_model/observing.rb:108:in `block in instantiate_observers'
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/active_model/observing.rb:108:in `each'
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/active_model/observing.rb:108:in `instantiate_observers'
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/railtie.rb:42:in `block (2 levels) in <class:Railtie>'
/Users/kasperhansen/Documents/code/observer/app/models/application_record.rb:1:in `<top (required)>'
/Users/kasperhansen/Documents/code/observer/app/models/user.rb:1:in `<top (required)>'
/Users/kasperhansen/Documents/code/observer/Rakefile:8:in `block (2 levels) in <top (required)>'
Tasks: TOP => user:count
(See full trace by running task with --trace)

That's because User loads ActiveRecord::Base which then instantiates
the observers in its on_load hook, finally calling observed_class
which constantizes the very User constant we're loading!

One fix is to add a require to active_record/base manually here, because
we depend on the load hooks having been fired before model constants are
loaded.

(I didn't look into reorganizing the code such that observed_class isn't
needed on observer instantiation.)

This has the added benefit of not breaking the gem should Rails remove
the require's in console or runner blocks as well.

I can reliably reproduce the above circular dependency error by commenting
out the added require "active_record/base". And when it's in the rake
task works:

kaspers-macbook-pro:observer kasperhansen$ bin/rake user:count
1

When adding support for Rails 4.1, 4.2 and 5.0, the `instantiate_observers`
call that happened after boot was erroneously removed.
https://github.com/rails/rails-observers/pull/39/files#diff-9db46e46113d0c2eeb41f5d6f08bcac4L24

This covered a regression in the test suite for the rake integration and
also broke the gem for apps.

The reason the test/rake_test.rb failed was that a require to `active_record/base`
had been removed which rails-observers depended on.
rails/rails@9e9793b

Without that we're this circular dependency error:

```
kaspers-macbook-pro:observer kasperhansen$ cat Rakefile

require_relative 'config/application'

namespace :user do
  task :count => :environment do
    puts User.count
  end
end

Rails.application.load_tasks

kaspers-macbook-pro:observer kasperhansen$ bin/rake user:count
rake aborted!
Circular dependency detected while autoloading constant User
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/active_model/observing.rb:334:in `observed_class'
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/active_model/observing.rb:325:in `observed_classes'
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/active_model/observing.rb:345:in `observed_classes'
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/activerecord/observer.rb:100:in `observed_classes'
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/active_model/observing.rb:341:in `initialize'
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/active_model/observing.rb:186:in `instantiate_observer'
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/active_model/observing.rb:108:in `block in instantiate_observers'
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/active_model/observing.rb:108:in `each'
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/active_model/observing.rb:108:in `instantiate_observers'
/Users/kasperhansen/Documents/code/rails-observers/lib/rails/observers/railtie.rb:42:in `block (2 levels) in <class:Railtie>'
/Users/kasperhansen/Documents/code/observer/app/models/application_record.rb:1:in `<top (required)>'
/Users/kasperhansen/Documents/code/observer/app/models/user.rb:1:in `<top (required)>'
/Users/kasperhansen/Documents/code/observer/Rakefile:8:in `block (2 levels) in <top (required)>'
Tasks: TOP => user:count
(See full trace by running task with --trace)
```

That's because User loads ActiveRecord::Base which then instantiates
the observers in its on_load hook, finally calling observed_class
which constantizes the very User constant we're loading!

One fix is to add a require to active_record/base manually here, because
we depend on the load hooks having been fired before model constants are
loaded.

(I didn't look into reorganizing the code such that observed_class isn't
needed on observer instantiation.)

This has the added benefit of not breaking the gem should Rails remove
the require's in `console` or `runner` blocks as well.

I can reliably reproduce the above circular dependency error by commenting
out the added `require "active_record/base"`. And when it's in the rake
task works:

```
kaspers-macbook-pro:observer kasperhansen$ bin/rake user:count
1
```
@kaspth kaspth self-assigned this Jul 16, 2017
@kaspth
Copy link
Contributor Author

kaspth commented Jul 16, 2017

@rafaelfranca you worked on rails/rails@9e9793b, but don't the console and runner blocks have the same problem of triggering set_configs too soon?

https://github.com/rails/rails/blob/83f39a3bcf3f50d12d41bff4a110c57e2e4be605/activerecord/lib/active_record/railtie.rb#L52-L60

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant