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.2k stars 179 forks source link

Avoid ActiveRecordExtensions affects AR's belongs_to method. #255

Closed machisuke closed 2 years ago

machisuke commented 2 years ago

Background

ActiveRecord#belongs_to accepts these args. ref

        def belongs_to(name, scope = nil, **options)
          reflection = Builder::BelongsTo.build(self, name, scope, options)
          Reflection.add_reflection self, name, reflection
        end

But , ActiveRecordExtensions drops scope arguments.

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

Changes

In this PR, by calling super with no arguments, all arguments including scope are passed to super.

machisuke commented 2 years ago

Resolved by #256

details ~~Wow.~~ ~~It turns out that running certain test cases together causes an error.~~ ⚠️ active-hash does not support testing with activerecord v7. You should change gemfile like this if you try the below code. ```diff $ git diff diff --git a/Gemfile b/Gemfile index 8a435c8..d88d4fb 100644 --- a/Gemfile +++ b/Gemfile @@ -16,4 +16,4 @@ platforms :ruby do gem 'sqlite3', '~> 1.4.0' end -gem 'activerecord', '>= 5.0.0' +gem 'activerecord', '>= 5.0.0', '< 7.0' ``` ```ruby $ ruby -v ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [arm64-darwin21] ``` ```ruby $ cat Gemfile.lock | grep activerecord activerecord (6.1.6.1) activerecord (>= 5.0.0) activerecord-jdbcsqlite3-adapter (>= 1.3.6) ``` ```ruby $ bundle exec rspec ./spec/associations/active_record_extensions_spec.rb:138 Run options: include {:locations=>{"./spec/associations/active_record_extensions_spec.rb"=>[138]}} . Finished in 1.2 seconds (files took 3.8 seconds to load) 1 example, 0 failures ``` ```ruby $ bundle exec rspec spec/active_hash/base_spec.rb:1287 ./spec/associations/active_record_extensions_spec.rb:138 Run options: include {:locations=>{"./spec/active_hash/base_spec.rb"=>[1287], "./spec/associations/active_record_extensions_spec.rb"=>[138]}} .F Failures: 1) ActiveHash::Base active record extensions ActiveHash::Associations::ActiveRecordExtensions#belongs_to doesn't interfere with AR's procs in belongs_to methods Failure/Error: return super unless respond_to? method_name NoMethodError: undefined method `current_scope' for Country:Class # ./lib/active_hash/base.rb:259:in `method_missing' # ./vendor/bundle/ruby/3.1.0/gems/activerecord-6.1.6.1/lib/active_record/associations/association.rb:100:in `scope' # ./vendor/bundle/ruby/3.1.0/gems/activerecord-6.1.6.1/lib/active_record/associations/association.rb:218:in `find_target' # ./vendor/bundle/ruby/3.1.0/gems/activerecord-6.1.6.1/lib/active_record/associations/singular_association.rb:39:in `find_target' # ./vendor/bundle/ruby/3.1.0/gems/activerecord-6.1.6.1/lib/active_record/associations/association.rb:174:in `load_target' # ./vendor/bundle/ruby/3.1.0/gems/activerecord-6.1.6.1/lib/active_record/associations/association.rb:67:in `reload' # ./vendor/bundle/ruby/3.1.0/gems/activerecord-6.1.6.1/lib/active_record/associations/singular_association.rb:9:in `reader' # ./vendor/bundle/ruby/3.1.0/gems/activerecord-6.1.6.1/lib/active_record/associations/builder/association.rb:103:in `country' # ./spec/associations/active_record_extensions_spec.rb:147:in `block (4 levels) in ' Finished in 0.16575 seconds (files took 0.43094 seconds to load) 2 examples, 1 failure Failed examples: rspec ./spec/associations/active_record_extensions_spec.rb:138 # ActiveHash::Base active record extensions ActiveHash::Associations::ActiveRecordExtensions#belongs_to doesn't interfere with AR's procs in belongs_to methods ```
kbrock commented 2 years ago

I'm trying to think about the use case here.

I've only used a where scope in those instances. In your code, what do you put into the scope (that we were dropping and you added back?)

kbrock commented 2 years ago

Thanks @machisuke I understand your use case. I worked on one project that was similar to that. (your example helped with my memory)

The where clauses on the scope makes a ton of sense. Wise to put that into the test case.

kbrock commented 2 years ago

Can you rebase the 256 PR code out of here? Or do you need that to make these tests pass?

machisuke commented 2 years ago

@kbrock Actually, this PR needs #256. Without #256, executing both spec/active_hash/base_spec.rb:1287 and. ./spec/associations/active_record_extensions_spec.rb:138 in a single process causes an error.

The detail is here

kbrock commented 2 years ago

@machisuke thanks

If you could rebase this on master (to remove #256) that would be great.

machisuke commented 2 years ago

@kbrock Sure.

kbrock commented 2 years ago

@machisuke thanks

I merge another and introduced a conflict for this. sorry.

Please let me know if I introduced a mistake while resolving this merge or introduced something that you did not want.