search

active-hash / active_hash

A readonly ActiveRecord-esque base class that lets you use a hash, a Yaml file or a custom file as the datasource
MIT License
1.19k stars 178 forks source link

Avoid interfering with AR's belongs_to arguments. #270

Closed koyo-miyamura closed 1 year ago

koyo-miyamura commented 1 year ago

Background

In active_hash v3.1.0, ActiveHash::Associations::ActiveRecordExtensions#belongs_to copies arguments and calls super with no arguments. It doesn't interfere with AR's belongs_to arguments.

https://github.com/active-hash/active_hash/blob/d952d67a0a52ee78f899366925757a79e4de6c37/lib/associations/associations.rb#L6-L24

However, after active_hash v3.1.1, it doesn't copy arguments and updates options from arguments and calls super with no arguments. It interferes with AR's belongs_to arguments (please reference rspec in this PR).

Changes

In this PR, avoid ActiveHash::Associations::ActiveRecordExtensions#belongs_to interfering with AR's belongs_to arguments. (In addition, refactored the error handling as minor fix.)

koyo-miyamura commented 1 year ago

@kbrock Thanks to launch CI ! I fixed to pass test in ruby 2.7. Please run again at your leisure.

kbrock commented 1 year ago

this is going to be tricky. different versions of active record have different interfaces into this method.

The original implementation #73 of this behavior used super like it is now Then to work around some keyword issues, we modified the super call in #251 Then we went back to super in #255

So it looks like we have always been modifying options. But I can appreciate your change and it looks good.

I don't think checking the parameters coming into belongs_to is the best way to test. Possibly just testing that passing :class_name works?

To be honest, I'm not totally sure why you are trying to change this behavior. Maybe writing a test that is protecting you from this behavior may be the best bet?

koyo-miyamura commented 1 year ago

@kbrock

Thank you for your advice and analyzing my PR.

To be honest, I'm not totally sure why you are trying to change this behavior. Maybe writing a test that is protecting you from this behavior may be the best bet?

No wonder you think so because the problem I am facing now is complicated. Let me try to explain.

In a product I develop now, it also overrides AR's belongs_to, and the order of method call is after ActiveHash::Associations::ActiveRecordExtensions#belongs_to.

Therefore, ActiveHash::Associations::ActiveRecordExtensions#belongs_to adding :class_name to options causes trouble because the method can't distinguish whether options[:class_name] is originally included or not.

That's why I created this PR.

koyo-miyamura commented 1 year ago

@kbrock

Sorry for waiting you, the fix is ready, please review.

koyo-miyamura commented 1 year ago

@kbrock Could you review this PR ?