Fix regression in PR #39

[mvastola]

PR #39 (merged in bca5db9) includes a regression wherein observers are no longer automatically instantiated on application startup. This PR seeks to:

• Fix this bug (completed)
• Add a CI test to prevent this bug from recurring (completed)

Thanks to @jachenry for initially pointing out this bug.

[mvastola]

@jachenry Please try pointing to master on Partyista/rails-observers and confirm the error goes away.

Maintainer(s), this should be ready to merge as the new test I added (as well as all of the old ones) are passing. Also, a release was promised before I became aware of this bug, so I dropped that pursuit to make sure Rails 5 compatibility was working prior to a release enabling it. Now that I'm content that it is working, I'd like to re-request that this functionality be released to RubyGems after this PR is merged.

[mvastola]

@jachenry any news?

[jachenry]

@mvastola I'll try to take a look at this tomorrow and let you know what I find. Thanks for helping maintain the project.

[biow0lf]

@jachenry any news?

[mvastola]

Seeing now that other people ran have since run into this bug (see #45). Perhaps one or more of these people can test this fix and confirm it works in lieu of @jachenry so we can get this pushed ASAP, considering rails version 5 was just released today?

I'll go ahead and fix the merge conflicts on this.
Update: PR is now merge-conflict-free. (I have to head to bed and the CI tests are taking a while to start for whatever reason, but I'll check in tomorrow if there are any issues with those due to code changes.)

CC: @cannikin, @ZempTime, @bodrovis

[ericboehs]

Looks like Travis is failing.

rails/rails-observers/lib/rails/observers/active_model/observing.rb:19:in `inherited': uninitialized constant ActiveModel::Observing::ActiveRecord (NameError)

@mvastola could you take a look, please?

[CloCkWeRX]

Since this is in ActiveModel, not activerecord; should it be self.instantiate_observers?

Also in the railtie; there's a lot of loading and explicitly instantiate_observing going on; but nothing that specifically seems to target active_model itself

[kwent]

The issue is

/home/travis/build/rails/rails-observers/lib/rails/observers/active_model/observing.rb:19:in `inherited': uninitialized constant ActiveModel::Observing::ActiveRecord (NameError)

What about escaping the namespace ?

::ActiveRecord::Base.instantiate_observers

[enrico]

@mvastola , @rafaelfranca : Just took a look at this and and the Travis tests fail because in the context of the active_resource_observer_test, ActiveRecord::Base is not defined at all and therefore not found.
I'm not clear whether the gem can be used without activerecord or not. I'm also not sure if we can use Object.const_defined?('ActiveRecord::Base') to verify existence of ActiveRecord::Base...

Either way, this fixes all tests, you guys might have a more elegant solution to it...:

ActiveRecord::Base.instantiate_observers if Object.const_defined?('ActiveRecord::Base')

[panigrah]

@mvastola CI build fails - are you working to resolve the circular dependency issue in the test now?

[jnimety]

Any updates on this issue?

[rafaelfranca]

No updated, as you can see in the conversation in this PR.

[jnimety]

is the short term work around to add an initializer with ActiveRecord::Base.instantiate_observers?

[kayodeadeniyi]

@mvastola Can I take a stab at getting to see why this spec fails?

[mvastola]

@andela-kadeniyi I'm so sorry. I had already intimated that the task of fixing this was up for the taking, but I'm now realizing I did so only in a comment on the (merged/closed) PR #39 whose regression this PR fixes.

For convenience, I'm duplicating my comments below.

Hey guys,
The real issue here is in #41, so it would probably be best if all comments were redirected there. It would be even better if someone wanted to help work on that PR as my time is limited. (I'm happy to grant access to my fork.)

Long story short:
I introduced a regression in this PR (#39) which, after it was pointed out to me, I promptly fixed with #41 (which, at the time, passed all CI tests and was merge-conflict-free).

Unfortunately, by the time someone else verified the fix in #41, commits had already been made that resulted in merge conflicts between #41 and HEAD. While these conflicts were simple enough to resolve (and have been resolved in #41 as it stands), the CI tests no longer pass (suggesting the commits to master after #39 was merged made some incompatible changes) and thus far, I have not been able to successfully resolve the errors.

Basically, as you can see above, this PR worked (for me) at first, but due to a real-life race condition, I couldn't get someone to verify the PR prior to some conflicting commits being made.

Once they were, I resolved the merge conflicts, but for some reason (which, if I understood, I could resolve this) the commits that were made conflict with my PR on a functional level and cause Travis to fail. What you see now is as far as I've gotten. Personal time constraints prevent me from giving this more time at the moment, but I'm happy to assist in any way I can. Feel free to give me a prod over email if I don't reply for a few days.

[enrico]

@mvastola , did you see my comment above on why the test fails? (Travis tests fail because in the context of the active_resource_observer_test, ActiveRecord::Base is not defined and therefore not found) Hopefully this puts you in the right direction...

[mvastola]

@andela-kadeniyi, actually I think I might have this. Can you hold off a few hours?
Thanks!

[mvastola]

@enrico, I hadn't. I actually just saw it (right after I sent my first reply to @andela-kadeniyi). I'm testing it out, but it seems to be working. (I suspect I'll be thanking you a ton in a few.)

ActiveRecord is -- indeed -- a dependency, so I think the problem may have been that it wasn't initialized by the CI test, meaning my patch was correct and the test was wrong and this was a wild goose chase. 🤦‍♂️

[mvastola]

Alright, I'm going to get this thanks to @enrico. Hoping to have it done tomorrow.

[mvastola]

Still working on this btw. Sorting out a few things still.

[kgeoir]

Hello, any news about this issue?
I having my oberservers working fine in dev env but not at all in production....

[mvastola]

@kgeoir, yes. My crazy life has just been getting in the way. I made a bunch of changes while trying to find out the best way to implement this, many of which (however useful) ended up being irrelevant (and thus will be beyond the scope of this PR; expect other PRs once this is done). Several other conflicting projects have also been taking up my time.

I really ought to at least sort out everything I've done so far into discrete commits (expect that today or tmrw) and I truly am in a position to get this done very soon. I'm thinking Sunday.

Question though for @dhh, I have this repo on my RSS reader and you've been making some commits the past couple of days with comments that seem to be suggesting you're gearing up to release Rails 5.1 compatibility. Is that an allusion to this PR, or is someone else working on something instead because I've been admittedly lagging on this?

[dhh]

Mike, I was just trying to get this gem released for 5.1 compatibility. But if there are some concerns you need to address before that happens, please do!

…

On Thu, May 11, 2017 at 10:26 PM, Mike Vastola ***@***.***> wrote: @kgeoir <https://github.com/kgeoir>, yes. My crazy life has just been getting in the way. I made a bunch of changes while trying to find out the best way to implement this, many of which (however useful) ended up being irrelevant (and thus will be beyond the scope of this PR; expect other PRs once this is done). Several other conflicting projects have also been taking up my time. I really ought to at least sort out everything I've done so far into discrete commits (expect that today or tmrw) and I truly am in a position to get this done very soon. I'm thinking Sunday. Question though for @dhh <https://github.com/dhh>, I have this repo on my RSS reader and you've been making some commits the past couple of days with comments that seem to be suggesting you're gearing up to release Rails 5.1 compatibility. Is that an allusion to this PR, or is someone else working on something instead because I've been admittedly lagging on this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#41 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAKtf0qYQDVvQ5O5hEXr7M2Y3Psfko9ks5r4275gaJpZM4HTEFg> .

[mvastola]

@dhh, this PR is working on 5.1 compatibility. Does that mean you're working on it as well? How far have you gotten? Maybe we should collaborate?

I don't have any concerns, I just wanted to know if you were about to swoop in and supersede my work on this, in which case I wouldn't bother continuing. If you have time to spend on this though, it might be a good idea to work together.

[mvastola]

Er. "Swoop in" came out the wrong way. To be clear, I wouldn't begrudge you one bit. (This has taken far too long, and 5.2 is now out, which should also be supported). I just noticed that someone was making commits about this and hadn't said anything in the primary related PR.

[dhh]

I haven't done any actual work on the gem for compatibility. I only nixed the upper-bound. So please do proceed with any actual compatibility work. Thanks!

…

On Fri, May 12, 2017 at 9:07 PM, Mike Vastola ***@***.***> wrote: @dhh <https://github.com/dhh>, this PR is working on 5.1 compatibility. Does that mean you're working on it as well? How far have you gotten? Maybe we should collaborate? I don't have any concerns, I just wanted to know if you were about to swoop in and supersede my work on this, in which case I wouldn't bother continuing. If you have time to spend on this though, it might be a good idea to work together. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#41 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAKtU93pK8Pw2xI7M53PGp3gK5q8fyUks5r5K3UgaJpZM4HTEFg> .

[mvastola]

Okay, so this doesn't work yet, but lest people think nothing has been done on this, I wanted to push something.

(And, I know, I broke a major golden rule of coding by fixing about a gazillion things in these commits -- to the point that this is pretty much a total refactor -- so I anticipate this will be parsed out into a number of smaller PRs, but I saw a number of unintuitive things with the code organization at first and I couldn't be sure that they weren't the problem, and things just escalated from there.)

Things that are welcome:

• All constructive criticism
• General comments
• Offers to do any part of work that remains, including but not limited to
  • Adapting existing unit tests
  • Writing new unit tests for additional functionality/features/facets of the code
  • Simply pulling out a distinct function/feature/facet of the code into a separate PR for merging.

I'm sure someone industrious can probably also take about 20 lines from all the code I wrote and the steps I laid out and fix this one major bug, which would also be fine and simultaneously put me to shame. :-p

[kaspth]

Why can't we just add back the instantiate_observers line here? bca5db9#diff-9db46e46113d0c2eeb41f5d6f08bcac4L24

[mvastola]

@kaspth Heh. Because then you get this.

[kgeoir]

I am not sure to understand all the issue. As per my quick investigation it seems for me that the code in to_prepare is never call. It looks like an issue with the reloader. I tested it with Rails 5.0.2 and 5.0.3.

Meanwhile what I did as a dirty fix so I can move forward until the good fix is merged, is I added

    config.after_initialize do |app|
      ActiveSupport.on_load(:active_record) do
        ActiveRecord::Base.instantiate_observers
      end
    end

in my application.rb. it seems ok for now.

[mvastola]

Have you read these comments in particular? this and this

AFAIK, this problem isn't occurring in dev mode. Only in production. So reloaders don't apply. Although there could be an extra bug in reloading (or, I would imagine, those reloaders don't apply if the code wasn't loaded in the first place).

My understanding of the issue is/was that the issue is caused by eager loading in production.

Basically if the observers are instantiated at the correct time given the code as it stands (which they need to be in order to properly track changes that could be made by rails initializers and properly-configured ActiveSupport.on_load hooks) as of Rails 5, they will actually cause the load of the model class they observe, which then causes a circular dependency because the Observer immediately hooks in while the class is loading and then tries to reference the class that hasn't fully loaded. (See stack trace.)

[mvastola]

FWIW, I am aware that a quick and dirty workaround does exist involving instantiating the observers. The problem though is unless the observers hook in immediately upon loading, the observations really aren't continuous.

Also, I should note, all this discussion has been about AR, but theoretically rails-observers is ORM agnostic and can observe ActiveModel, ActiveResource or technically any old class that just does an include ActiveModel::Observing. Point though is that it's possible that this whole incomplete coverage/regression/circular dependency problem applies to that code too and those are just less common cases or they haven't brought it up here because they weren't using the code in a way that was officially supported.

[kaspth]

I've committed a fix for the "circular reference" root issue over here: #65

Appreciate the deep investigation and help with supporting newer Rails releases, @mvastola! ❤️

[mvastola]

@kaspth, you're my hero!!

Thanks so much for boiling this down into a simple fix for now -- I obviously hadn't had as much time as I'd expected to bring this PR into a working state.

So obviously this PR suffered from abominable levels of feature creep because I had no idea what would fix the problem (I thought it would have been more tedious to go through all the commits to rails and AR -- your approach -- but hindsight is 20/20).

In the end though, I implemented -- or mostly implemented -- many of the features I sought to (just all jumbled together). I'd hate to see this work and code go to waste though.

Is there any way you could take a minute and let me know your thoughts on the various features I began and particularly their suitability for incorporation (assuming they're submitted in their own PRs with appropriate tests and all that jazz)?

The features I'm referring to include:

• Regorganizing the directory structure and some module/class names
• Removed dependency on rails/railties (while adding a compatibility wrapper)
• Allow for lazy loading of observers, models they observe, and the ActiveRecord::Base class (by referencing observed by classes by symbol/string)

[kaspth]

I thought it would have been more tedious to go through all the commits to rails and AR -- your approach

Hardly inspected any commits. I reverted #39 to add Rails versions back manually. Rails 4.1 started exhibiting the circular reference bug. I slipped in a byebug statement around instantiate_observers and noticed the different backtraces. In Rails 4.0 the call came from before the user:count Rake task ran, while 4.1 had it from running class User < ActiveRecord::Base.

Regorganizing the directory structure and some module/class names

I don't care enough to review that 😊

Removed dependency on rails/railties (while adding a compatibility wrapper)

Not needed. Everything is wrapped in a railtie that's only loaded when Rails::Railtie is defined.

Allow for lazy loading of observers, models they observe, and the ActiveRecord::Base class (by referencing observed by classes by symbol/string)

Could be good. It would match Active Record associations' class_name: "User" option.

[mvastola]

Hardly inspected any commits. I reverted #39 to add Rails versions back manually

I guessed. I would have done that first if I knew what this would become. :-)

I don't care enough to review that 😊

There's an ancient PR (#13) regarding this. Maybe I'll try to do it by writing a script that does the re-org so that's the only thing that needs reviewing.

Not needed. Everything is wrapped in a railtie that's only loaded when Rails::Railtie is defined.

See what I did here and here. It's not done very robustly (I'd fix), and IMHO this should be part of AS, but the idea is to have an abstraction layer that can bootstrap the AS::LazyLoadHooks and the Reloader (i.e. everything rails-observers uses, without the rails dependency).

[kaspth]

I'll take a pass on those, thanks 😊