Avoid ActiveRecordExtensions affects AR's belongs_to method. by machisuke · Pull Request #255 · active-hash/active_hash

machisuke · 2022-07-22T00:09:21Z

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.

active_hash/lib/associations/associations.rb

Lines 6 to 21 in b11cb2e

    
           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 · 2022-07-23T13:24:44Z

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

kbrock · 2022-07-29T00:18:27Z

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 · 2022-07-29T00:19:33Z

spec/associations/active_record_extensions_spec.rb

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


does this still work if someone puts a where here?

@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

kbrock · 2022-08-08T14:21:33Z

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

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

Anyone else have an opinion?

kbrock · 2022-08-10T17:45:14Z

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

machisuke · 2022-08-11T23:07:40Z

@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 · 2022-08-11T23:32:46Z

@machisuke thanks

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

machisuke · 2022-08-12T00:15:14Z

@kbrock
Sure.

kbrock · 2022-08-22T21:46:13Z

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

machisuke force-pushed the fix_scope branch from 348c555 to b980d09 Compare July 22, 2022 00:40

kbrock added the bug label Jul 22, 2022

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

kbrock reviewed Jul 29, 2022

View reviewed changes

machisuke requested a review from kbrock August 1, 2022 14:54

kbrock approved these changes Aug 9, 2022

View reviewed changes

kbrock mentioned this pull request Aug 10, 2022

isolate tests with temporary classes #256

Merged

machisuke force-pushed the fix_scope branch from 20cb9bc to 2b7cdbc Compare August 12, 2022 00:13

machisuke added 2 commits August 12, 2022 09:13

Avoid ActiveRecordExtensions affects AR's belongs_to method.

986a535

use where in scope because it is more common use case

95d8ad5

machisuke force-pushed the fix_scope branch from 2b7cdbc to 95d8ad5 Compare August 12, 2022 00:13

Merge branch 'master' into fix_scope

1e7ce0f

kbrock merged commit 819dff7 into active-hash:master Aug 22, 2022

machisuke deleted the fix_scope branch December 9, 2022 02:32

kbrock mentioned this pull request Jan 27, 2023

Avoid interfering with AR's belongs_to arguments. #270

Merged

pfeiffer mentioned this pull request Mar 10, 2023

Refactor Relation and conditions to fix mutating scopes issues #268

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid ActiveRecordExtensions affects AR's belongs_to method.#255

Avoid ActiveRecordExtensions affects AR's belongs_to method.#255
kbrock merged 3 commits intoactive-hash:masterfrom
machisuke:fix_scope

machisuke commented Jul 22, 2022

Uh oh!

machisuke commented Jul 23, 2022 •

edited

Loading

Uh oh!

kbrock commented Jul 29, 2022

Uh oh!

kbrock Jul 29, 2022

Uh oh!

machisuke Jul 30, 2022

Uh oh!

kbrock commented Aug 8, 2022

Uh oh!

kbrock left a comment

Uh oh!

kbrock commented Aug 10, 2022

Uh oh!

machisuke commented Aug 11, 2022

Uh oh!

kbrock commented Aug 11, 2022

Uh oh!

machisuke commented Aug 12, 2022

Uh oh!

kbrock commented Aug 22, 2022 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	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

Conversation

machisuke commented Jul 22, 2022

Background

Changes

Uh oh!

machisuke commented Jul 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kbrock commented Jul 29, 2022

Uh oh!

kbrock Jul 29, 2022

Choose a reason for hiding this comment

Uh oh!

machisuke Jul 30, 2022

Choose a reason for hiding this comment

Uh oh!

kbrock commented Aug 8, 2022

Uh oh!

kbrock left a comment

Choose a reason for hiding this comment

Uh oh!

kbrock commented Aug 10, 2022

Uh oh!

machisuke commented Aug 11, 2022

Uh oh!

kbrock commented Aug 11, 2022

Uh oh!

machisuke commented Aug 12, 2022

Uh oh!

kbrock commented Aug 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

machisuke commented Jul 23, 2022 •

edited

Loading

kbrock commented Aug 22, 2022 •

edited

Loading