Make the Relation -> Model delegation stricter #50396

casperisfine · 2023-12-19T10:50:01Z

In #50395 I noticed lots of methods are delegated from Relation to the model. The intent of this code is to allow using use defined class methods like scopes.

But because of this autmated delegation it allowed calling any ActiveRecord::Base class method on a Relation, which in itself may be desireable, however we very wastefully define the delegator on the first call, and worse we wrap it with a current scope setter.

So I think we should be more strict about it.

NB: this of course breaks a tons of test, so I need to find more time to finish this, and it likely need a config flag and a deprecation notice of some sort. Or alternatively we can just accept this API, but delegate all the AR::Base class methods eagerly and without the scoping overhead. TBD.

casperisfine · 2024-01-15T16:57:38Z

Note to self:

It's hard to know for sure the list of methods in AR::Base that require this scoped delegation
We should make sure that if we get this wrong it end up in NoMethodError or in a scoped delegate, never in an unscoped-delegate, as it could lead to unscoped queries leaking or deleting data by accident (would be super bad).

casperisfine · 2024-01-15T17:40:58Z

activerecord/lib/active_record/relation/delegation.rb

+             :sanitize_sql_like, :unscoped, to: :klass
+
+    # TODO: scoped delegate
+    [:find_signed, :find_signed!, :delete, :find_by_token_for, :find_by_token_for!, :upsert_all, :insert_all, :insert_all!].each do |method|


This list is very likely incomplete.

So I rebased, and this is still the main blocker. We have a number of ActiveRecord::Base class methods that rely on this scoped delegation.

I need to find a good alternative.

rafaelfranca · 2024-01-15T19:39:15Z

Makes sense

jaynetics · 2024-01-15T21:08:50Z

perhaps relevant: #50760

casperisfine · 2024-01-16T07:40:57Z

perhaps relevant: #50760

Yeah, it's in #50395, which in a way is similar, that I noticed this problem.

Fix: rails#51775 This us us being bitten by rails#50396 once more. We should really make this delegation much stricter.

casperisfine · 2024-05-10T08:04:33Z

Got bit by this again: #51776, I really need to finish this PR somehow.

Ref: rails#50396 Ref: rails#51776 `ActiveRecord::Relation` automatically delegates missing methods to the model class wrapped in a `scoping { }` block. This is to support scoping in user defined class methods. The problem however is that it's very error prone for the framework, because we can mistakenly call model methods from inside `Relation` and not realized we're applying a global scope. In the best case scenario it's just a waste of performance, but it can also lead to bugs like rails#51775 I'm planning to restrict this automatic delegation to methods defined in childs of `ActiveRecord::Base` only: rails#50396 but for this to work we must first refactor any Rails code that rely on it.

activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb

Ref: rails#50396

Ref: rails#50396 As well as related methods.

In rails#50395 I noticed lots of methods are delegated from `Relation` to the model. The intent of this code is to allow using use defined class methods like scopes. But because of this autmated delegation it allowed calling any `ActiveRecord::Base` class method on a `Relation`, which in itself may be desireable, however we very wastefully define the delegator on the first call, and worse we wrap it with a current scope setter. So I think we should be more strict about it.

casperisfine · 2024-05-13T08:57:19Z

activerecord/lib/active_record/relation/delegation.rb

@@ -100,7 +100,7 @@ def #{method}(...)
             :to_sentence, :to_fs, :to_formatted_s, :as_json,
             :shuffle, :split, :slice, :index, :rindex, to: :records

-    delegate :primary_key, :lease_connection, :connection, :with_connection, :transaction, to: :klass
+    delegate :primary_key, :with_connection, :connection, :table_name, :transaction, :sanitize_sql_like, :unscoped, to: :klass


Not quite sure whether we should keep any of these.

Fix: rails#51775 This us us being bitten by rails#50396 once more. We should really make this delegation much stricter.

Ref: rails#50396 Ref: rails#51776 `ActiveRecord::Relation` automatically delegates missing methods to the model class wrapped in a `scoping { }` block. This is to support scoping in user defined class methods. The problem however is that it's very error prone for the framework, because we can mistakenly call model methods from inside `Relation` and not realized we're applying a global scope. In the best case scenario it's just a waste of performance, but it can also lead to bugs like rails#51775 I'm planning to restrict this automatic delegation to methods defined in childs of `ActiveRecord::Base` only: rails#50396 but for this to work we must first refactor any Rails code that rely on it.

Ref: rails#50396

Ref: rails#50396 As well as related methods.

casperisfine · 2024-05-13T23:34:38Z

I slept on this one. I think long term we should deprecate most of these delegations, but short term we can simply make it so that we still delegate for methods present in ActiveRecord::Base, but without the scoping around it.

I also want to ship the call site cleanup part.

Ref: #50396 Ref: #51776 `ActiveRecord::Relation` automatically delegates missing methods to the model class wrapped in a `scoping { }` block. This is to support scoping in user defined class methods. The problem however is that it's very error prone for the framework, because we can mistakenly call model methods from inside `Relation` and not realized we're applying a global scope. In the best case scenario it's just a waste of performance, but it can also lead to bugs like #51775 I'm planning to restrict this automatic delegation to methods defined in childs of `ActiveRecord::Base` only: #50396 but for this to work we must first refactor any Rails code that rely on it.

Ref: #50396

Ref: #50396 As well as related methods.

rails-bot bot added the activerecord label Dec 19, 2023

casperisfine force-pushed the stricter-relation-delegation branch from 323e59a to d3990e8 Compare December 19, 2023 10:55

casperisfine marked this pull request as draft December 19, 2023 10:56

casperisfine force-pushed the stricter-relation-delegation branch from d3990e8 to 189e334 Compare December 22, 2023 12:32

casperisfine force-pushed the stricter-relation-delegation branch 2 times, most recently from 0fe6cde to 5c84152 Compare January 15, 2024 16:52

casperisfine commented Jan 15, 2024

View reviewed changes

rafaelfranca mentioned this pull request Jan 15, 2024

ActiveRecord::Delegation leaks scope to inner queries on the same model #50760

Closed

casperisfine pushed a commit to Shopify/rails that referenced this pull request May 10, 2024

Fix .with_connection to not set current scope

4bd4fcc

Fix: rails#51775 This us us being bitten by rails#50396 once more. We should really make this delegation much stricter.

casperisfine mentioned this pull request May 10, 2024

Fix .with_connection to not set current scope #51776

Merged

casperisfine force-pushed the stricter-relation-delegation branch 4 times, most recently from 8127973 to fcde614 Compare May 10, 2024 08:40

byroot marked this pull request as ready for review May 10, 2024 08:51

casperisfine mentioned this pull request May 13, 2024

Refactor ActiveRecord::SignedId to not rely on relation delegation #51800

Merged

casperisfine mentioned this pull request May 13, 2024

Refactor ActiveRecord::TokenFor to not rely on relation delegation #51801

Merged

casperisfine force-pushed the stricter-relation-delegation branch from fcde614 to 9c09de8 Compare May 13, 2024 02:39

bogdan reviewed May 13, 2024

View reviewed changes

activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb Outdated Show resolved Hide resolved

casperisfine pushed a commit to Shopify/rails that referenced this pull request May 13, 2024

Move ActiveRecord::Base.delete in Relation

5e1d07d

Ref: rails#50396

casperisfine pushed a commit to Shopify/rails that referenced this pull request May 13, 2024

Move ActiveRecord::Base.destroy in Relation

8f419e4

Ref: rails#50396

casperisfine pushed a commit to Shopify/rails that referenced this pull request May 13, 2024

Move ActiveRecord::Base.insert in Relation

ed2c15b

Ref: rails#50396 As well as related methods.

casperisfine mentioned this pull request May 13, 2024

Refactor ActiveRecord::Persistence class methods into Relation methods #51805

Merged

casperisfine force-pushed the stricter-relation-delegation branch 3 times, most recently from 53b00de to c28d7a9 Compare May 13, 2024 07:40

casperisfine force-pushed the stricter-relation-delegation branch from c28d7a9 to f7c6343 Compare May 13, 2024 07:46

casperisfine commented May 13, 2024

View reviewed changes

fractaledmind pushed a commit to fractaledmind/rails that referenced this pull request May 13, 2024

$@fractaledmind$

Fix .with_connection to not set current scope

be6b6bf

Fix: rails#51775 This us us being bitten by rails#50396 once more. We should really make this delegation much stricter.

fractaledmind pushed a commit to fractaledmind/rails that referenced this pull request May 13, 2024

$@fractaledmind$

Move ActiveRecord::Base.delete in Relation

a188035

Ref: rails#50396

fractaledmind pushed a commit to fractaledmind/rails that referenced this pull request May 13, 2024

$@fractaledmind$

Move ActiveRecord::Base.destroy in Relation

ff022d0

Ref: rails#50396

fractaledmind pushed a commit to fractaledmind/rails that referenced this pull request May 13, 2024

$@fractaledmind$

Move ActiveRecord::Base.insert in Relation

de3f3bf

Ref: rails#50396 As well as related methods.

dhh pushed a commit that referenced this pull request May 14, 2024

Move ActiveRecord::Base.delete in Relation

b1381c3

Ref: #50396

dhh pushed a commit that referenced this pull request May 14, 2024

Move ActiveRecord::Base.destroy in Relation

565b348

Ref: #50396

dhh pushed a commit that referenced this pull request May 14, 2024

Move ActiveRecord::Base.insert in Relation

5624c4e

Ref: #50396 As well as related methods.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the Relation -> Model delegation stricter #50396

Make the Relation -> Model delegation stricter #50396

casperisfine commented Dec 19, 2023

casperisfine commented Jan 15, 2024

casperisfine Jan 15, 2024

casperisfine May 10, 2024

rafaelfranca commented Jan 15, 2024

jaynetics commented Jan 15, 2024

casperisfine commented Jan 16, 2024

casperisfine commented May 10, 2024

casperisfine May 13, 2024

casperisfine commented May 13, 2024

Make the Relation -> Model delegation stricter #50396

Are you sure you want to change the base?

Make the Relation -> Model delegation stricter #50396

Conversation

casperisfine commented Dec 19, 2023

casperisfine commented Jan 15, 2024

casperisfine Jan 15, 2024

Choose a reason for hiding this comment

casperisfine May 10, 2024

Choose a reason for hiding this comment

rafaelfranca commented Jan 15, 2024

jaynetics commented Jan 15, 2024

casperisfine commented Jan 16, 2024

casperisfine commented May 10, 2024

casperisfine May 13, 2024

Choose a reason for hiding this comment

casperisfine commented May 13, 2024