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

[jaynetics]

Steps to reproduce

# define a convenient wrapper so we don't always have to use the clunky syntax
def ApplicationRecord.replica_find_each(...)
  ActiveRecord::Base.connected_to(role: :replica) do
    find_each(...)
  end
end

# send birthday invitations
User.where(birth_date: Date.today).replica_find_each do |user|
  User.where.not(birth_date: Date.today).invite(to: user)
end

Expected behavior

The inner query is WHERE birth_date != <today> and finds some users.

Actual behavior

The inner query is WHERE birth_date = <today> AND WHERE birth_date != <today> and returns an empty relation.

This is because the outer scoping (where(birth_date: Date.today)) is mixed into the inner query by this code.

Ideas

The automatic scoping usually makes sense, e.g. you would want methods like this one to adapt correctly to Relations:

def User.exaggerated_count
  count * 2
end

User.exaggerated_count # => 2000
User.where(created_at: 10.days.ago..).exaggerated_count # => 200

When doing inner queries, however, the resulting bugs can be quite subtle and hard to track down!

Idea 1: Warn for access to the same model within any given block in delegated relation methods?

Idea 2: Revert to the surrounding scoping inside any given block?

Idea 3: Deprecate delegation of class methods (non-scopes) through relations and add new helper(s) to define such methods instead - helper(s) that make it obvious whether or not scoping will be applied?

[rafaelfranca]

This is the expected behavior.

[rafaelfranca]

Just to be clear #50396 might change that, but right now this is the expected behavior. If that PR changes it we will have to document it, but at the moment this isn't an issue.

[xijo]

Edit: side note of an issue that popped up along with it:

It might be a good idea to document that passing a relation will result in immediate database access in connected_to

How about adding a sentence like:

Please be aware, that passing a relation in a connected_to block will immediately load the entire relation

to https://guides.rubyonrails.org/active_record_multiple_databases.html#using-manual-connection-switching?

[jaynetics]

For anyone encountering this issue in the future, a workaround is to define the methods on ActiveRecord::Relation instead of on your model:

-class ApplicationRecord
-  def self.replica_find_each(...)
+class ActiveRecord::Relation
+  def replica_find_each(...)
     ActiveRecord::Base.connected_to(role: :replica) do
       find_each(...)
     end
   end
 end
+ApplicationRecord.delegate :replica_find_each, to: :all

This way, the method itself still uses the correct scope, but the scope does not leak into the method body or passed block.