Skip to content

Conversation

@machisuke
Copy link
Contributor

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.

def belongs_to(name, scope = nil, **options)
options = {:class_name => name.to_s.camelize }.merge(options)
klass =
begin
options[:class_name].constantize
rescue
nil
rescue LoadError
nil
end
if klass && klass < ActiveHash::Base
belongs_to_active_hash(name, options)
else
super(name, **options)
end
end

Changes

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

@machisuke
Copy link
Contributor Author

machisuke commented Jul 23, 2022

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.

$ 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 -v
ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [arm64-darwin21]
$ cat Gemfile.lock | grep activerecord
    activerecord (6.1.6.1)
  activerecord (>= 5.0.0)
  activerecord-jdbcsqlite3-adapter (>= 1.3.6)
$ 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
$ 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 <top (required)>'

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

@machisuke machisuke force-pushed the fix_scope branch 3 times, most recently from d71db68 to 1804ac1 Compare July 27, 2022 23:38
@kbrock
Copy link
Collaborator

kbrock commented Jul 29, 2022

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?)

if ActiveRecord::VERSION::MAJOR > 3
it "doesn't interfere with AR's procs in belongs_to methods" do
School.belongs_to :country, lambda { where() }
School.belongs_to :country, lambda { select(:id) }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this still work if someone puts a where here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kbrock

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?)

In our use case, we have a feature that automatically decrypts the contents when a column is loaded. That process can take a long time, and we are optimizing performance by skipping that process by not loading unnecessary columns. We use select to achieve this.

does this still work if someone puts a where here?

Sure. I searched and thought it would be more common for use cases to use "where" in scope, so I modified the test to use "where" as well. What do you think? 20cb9bc

This is the search results.
https://github.com/search?l=Ruby&q=belongs_to%3A+lambda&type=Code

@machisuke machisuke requested a review from kbrock August 1, 2022 14:54
@kbrock
Copy link
Collaborator

kbrock commented Aug 8, 2022

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.

Copy link
Collaborator

@kbrock kbrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is minor and the tests suggest that it does not have many (any?) side effects.

Anyone else have an opinion?

@kbrock
Copy link
Collaborator

kbrock commented Aug 10, 2022

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

@machisuke
Copy link
Contributor Author

@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
Copy link
Collaborator

kbrock commented Aug 11, 2022

@machisuke thanks

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

@machisuke
Copy link
Contributor Author

@kbrock
Sure.

@kbrock
Copy link
Collaborator

kbrock commented Aug 22, 2022

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants