ActiveHash::Associations::ActiveRecordExtensions is too intrusive

[hunterae]

&tldr;
By overriding the belongs_to method, ActiveHash forces autoloading of Rails model classes potentially before they would normally be loaded, which can cause unintentional side-effects. By swallowing the load errors, ActiveHash makes it hard to detect or resolve such side-effects. It also makes it more confusing for people trying to understand somebody else's code and not knowing that a ActiveHash relationship is being used instead of ActiveRecord, the latter of which they are likely more used to. Please log load errors as well as provide a configuration option to disable the overriding of belongs_to when extending ActiveHash::Associations::ActiveRecordExtensions

I encountered an extraordinarily rare set of circumstances today as I traced my way through multiple layers of Rails autoloading and constant resolution. I was getting an error trying to do a rails join to an association and joining that association to another association.

It was something like:
Form.joins(keyword: :campaign)

This triggered an error if I had cache_classes turned on, which is the case in our production.rb. The error was that the Campaign association could not be found on Keyword, even though turning off cache_classes made the problem go away. I found out today that when you run rake tasks against any environment, it actually automatically turns off eager_load even if it is turned on. So cache_classes on, eager_load off.

Through a process of trail and error, I was able to to determine that the reason for the error was due to the fact that when one extends ActiveHash::Associations::ActiveRecordExtensions, one is automatically overriding the "belongs_to" method that is normally provided by ActiveRecord. The new implementation for belongs_to then attempts to constantize the class associated with the belongs_to (which has the effect of autoloading the class if it has not yet been loaded) in order to determine if it is an ActiveHash or ActiveRecord class. It then catches and swallows any errors that occur with the process of loading said class.

In development mode, this is likely fine, as cache_classes will likely be turned off. However, when running rake tasks like "rake db:migrate" in production, cache_classes will likely be turned on (while eager_load will be automatically turned off as previously mentioned).

In my above mentioned scenario, I was running a migration against the production db. The Form class happened to be extending ActiveHash::Associations::ActiveRecordExtensions. It also had a "belongs_to :keyword" in it, which is overridden by ActiveHash's implementation. So then ActiveHash attempted to load Keyword. Due to some constants being in the wrong place in another file, it eventually caused a LoadError to occur, and Keyword was only loaded up to the point that the triggered error had occurred.

I would have been able to easily identify the triggered error but the LoadError was swallowed by ActiveHash.

So a couple of suggestions. At the very least, could you add some logging to the place where you catch the LoadError (https://github.com/zilkey/active_hash/blob/master/lib/associations/associations.rb#L17).
But more particularly, I would say I am not a fan of overriding the belongs_to method from ActiveRecord. It is changing behavior and magical and unexpected ways.

I would rather always explicitly use belongs_to_active_hash and never use or be forced to use the new version of belongs_to. Perhaps a configuration option to enable or disable the overriding of that method?

[hunterae]

Also, if it would help, i can see about created a simplified version of the issue. Let me know.

[hunterae]

@zilkey ?

[zilkey]

Ugh - I’m sorry you had to go through that!

Logging errors seems totally reasonable, and wouldn’t affect existing users.

If we could somehow just respect the cache_classes / eager load behavior of rails, would that solve it?

[hunterae]

@zilkey , I'm not sure that would help matters. The ActiveHash implementation for belongs_to seems to be dependent on the autoloading / constantizing of the association class in order to determine if Rails or ActiveHash is responsible for managing the relationship.

While it would be possible to not resolve the class until the association is accessed, I don't think that this would be a good solution as it might start to mess with any optimizations that Rails is doing when defining associations and their accessor methods.

Logging would definitely help, but I do think ultimately, it would be nice to be able to opt out of the overriding of belongs_to. While it's neat to be able to use the same syntax, I do worry about unintended side-effects that become that much harder to diagnose.

[hunterae]

Plus, if there is a config option or some other means to opt out of the behavior, you could turn the default for opt out to false so as to preserve existing functionality for any users who upgrade.