Merge pull request #35280 from kamipo/deprecate_leaking_scope

Deprecate using class level querying methods if the receiver scope regarded as leaked

9 files changed, 88 insertions, 33 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 47ae71f2a0..7ff4c743c6 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,6 +1,5 @@
-*   Fix `relation.create` to avoid leaking scope to initialization block and callbacks.
-
-    Fixes #9894, #17577.
+*   Deprecate using class level querying methods if the receiver scope
+    regarded as leaked. Use `klass.unscoped` to avoid the leaking scope.
 
     *Ryuta Kamizono*
 
diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb
index f98d9bb2c0..feaa20ccc6 100644
--- a/activerecord/lib/active_record/relation.rb
+++ b/activerecord/lib/active_record/relation.rb
@@ -67,7 +67,7 @@ module ActiveRecord
     #   user = users.new { |user| user.name = 'Oscar' }
     #   user.name # => Oscar
     def new(attributes = nil, &block)
-      block = klass.current_scope_restoring_block(&block)
+      block = _deprecated_scope_block("new", &block)
       scoping { klass.new(attributes, &block) }
     end
 
@@ -96,7 +96,8 @@ module ActiveRecord
       if attributes.is_a?(Array)
         attributes.collect { |attr| create(attr, &block) }
       else
-        new(attributes, &block).tap(&:save)
+        block = _deprecated_scope_block("create", &block)
+        scoping { klass.create(attributes, &block) }
       end
     end
 
@@ -110,7 +111,8 @@ module ActiveRecord
       if attributes.is_a?(Array)
         attributes.collect { |attr| create!(attr, &block) }
       else
-        new(attributes, &block).tap(&:save!)
+        block = _deprecated_scope_block("create!", &block)
+        scoping { klass.create!(attributes, &block) }
       end
     end
 
@@ -321,12 +323,12 @@ module ActiveRecord
     # Please check unscoped if you want to remove all previous scopes (including
     # the default_scope) during the execution of a block.
     def scoping
-      @delegate_to_klass ? yield : _scoping(self) { yield }
+      already_in_scope? ? yield : _scoping(self) { yield }
     end
 
-    def _exec_scope(*args, &block) # :nodoc:
+    def _exec_scope(name, *args, &block) # :nodoc:
       @delegate_to_klass = true
-      instance_exec(*args, &block) || self
+      _scoping(_deprecated_spawn(name)) { instance_exec(*args, &block) || self }
     ensure
       @delegate_to_klass = false
     end
@@ -525,6 +527,7 @@ module ActiveRecord
 
     def reset
       @delegate_to_klass = false
+      @_deprecated_scope_source = nil
       @to_sql = @arel = @loaded = @should_eager_load = nil
       @records = [].freeze
       @offsets = {}
@@ -633,7 +636,10 @@ module ActiveRecord
       end
     end
 
+    attr_reader :_deprecated_scope_source # :nodoc:
+
     protected
+      attr_writer :_deprecated_scope_source # :nodoc:
 
       def load_records(records)
         @records = records.freeze
@@ -641,6 +647,24 @@ module ActiveRecord
       end
 
     private
+      def already_in_scope?
+        @delegate_to_klass && begin
+          scope = klass.current_scope(true)
+          scope && !scope._deprecated_scope_source
+        end
+      end
+
+      def _deprecated_spawn(name)
+        spawn.tap { |scope| scope._deprecated_scope_source = name }
+      end
+
+      def _deprecated_scope_block(name, &block)
+        -> record do
+          klass.current_scope = _deprecated_spawn(name)
+          yield record if block_given?
+        end
+      end
+
       def _scoping(scope)
         previous, klass.current_scope = klass.current_scope(true), scope
         yield
diff --git a/activerecord/lib/active_record/relation/spawn_methods.rb b/activerecord/lib/active_record/relation/spawn_methods.rb
index 7874c4c35a..efc4b447aa 100644
--- a/activerecord/lib/active_record/relation/spawn_methods.rb
+++ b/activerecord/lib/active_record/relation/spawn_methods.rb
@@ -8,7 +8,7 @@ module ActiveRecord
   module SpawnMethods
     # This is overridden by Associations::CollectionProxy
     def spawn #:nodoc:
-      @delegate_to_klass ? klass.all : clone
+      already_in_scope? ? klass.all : clone
     end
 
     # Merges in the conditions from <tt>other</tt>, if <tt>other</tt> is an ActiveRecord::Relation.
diff --git a/activerecord/lib/active_record/scoping.rb b/activerecord/lib/active_record/scoping.rb
index 1142a87d25..35e9dcbffc 100644
--- a/activerecord/lib/active_record/scoping.rb
+++ b/activerecord/lib/active_record/scoping.rb
@@ -30,14 +30,6 @@ module ActiveRecord
       def current_scope=(scope)
         ScopeRegistry.set_value_for(:current_scope, self, scope)
       end
-
-      def current_scope_restoring_block(&block)
-        current_scope = self.current_scope(true)
-        -> *args do
-          self.current_scope = current_scope
-          yield(*args) if block_given?
-        end
-      end
     end
 
     def populate_with_current_scope_attributes # :nodoc:
diff --git a/activerecord/lib/active_record/scoping/named.rb b/activerecord/lib/active_record/scoping/named.rb
index 5278eb29a9..681a5c6250 100644
--- a/activerecord/lib/active_record/scoping/named.rb
+++ b/activerecord/lib/active_record/scoping/named.rb
@@ -27,6 +27,14 @@ module ActiveRecord
           scope = current_scope
 
           if scope
+            if scope._deprecated_scope_source
+              ActiveSupport::Deprecation.warn(<<~MSG.squish)
+                Class level methods will no longer inherit scoping from `#{scope._deprecated_scope_source}`
+                in Rails 6.1. To continue using the scoped relation, pass it into the block directly.
+                To instead access the full set of models, as Rails 6.1 will, use `#{name}.unscoped`.
+              MSG
+            end
+
             if self == scope.klass
               scope.clone
             else
@@ -180,7 +188,7 @@ module ActiveRecord
 
           if body.respond_to?(:to_proc)
             singleton_class.define_method(name) do |*args|
-              scope = all._exec_scope(*args, &body)
+              scope = all._exec_scope(name, *args, &body)
               scope = scope.extending(extension) if extension
               scope
             end
diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb
index 1b8f748bad..d560b4e519 100644
--- a/activerecord/test/cases/base_test.rb
+++ b/activerecord/test/cases/base_test.rb
@@ -1536,7 +1536,7 @@ class BasicsTest < ActiveRecord::TestCase
     Bird.create!(name: "Bluejay")
 
     ActiveRecord::Base.connection.while_preventing_writes do
-      assert_nothing_raised { Bird.where(name: "Bluejay").explain }
+      assert_queries(2) { Bird.where(name: "Bluejay").explain }
     end
   end
 
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index 857d743605..2ac81ba425 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -1167,13 +1167,23 @@ class RelationTest < ActiveRecord::TestCase
     assert_equal "green", parrot.color
   end
 
+  def test_first_or_create_with_after_initialize
+    Bird.create!(color: "yellow", name: "canary")
+    parrot = assert_deprecated do
+      Bird.where(color: "green").first_or_create do |bird|
+        bird.name = "parrot"
+        bird.enable_count = true
+      end
+    end
+    assert_equal 0, parrot.total_count
+  end
+
   def test_first_or_create_with_block
-    canary = Bird.create!(color: "yellow", name: "canary")
+    Bird.create!(color: "yellow", name: "canary")
     parrot = Bird.where(color: "green").first_or_create do |bird|
       bird.name = "parrot"
-      assert_equal canary, Bird.find_by!(name: "canary")
+      assert_deprecated { assert_equal 0, Bird.count }
     end
-    assert_equal 1, parrot.total_count
     assert_kind_of Bird, parrot
     assert_predicate parrot, :persisted?
     assert_equal "green", parrot.color
@@ -1214,13 +1224,23 @@ class RelationTest < ActiveRecord::TestCase
     assert_raises(ActiveRecord::RecordInvalid) { Bird.where(color: "green").first_or_create! }
   end
 
+  def test_first_or_create_bang_with_after_initialize
+    Bird.create!(color: "yellow", name: "canary")
+    parrot = assert_deprecated do
+      Bird.where(color: "green").first_or_create! do |bird|
+        bird.name = "parrot"
+        bird.enable_count = true
+      end
+    end
+    assert_equal 0, parrot.total_count
+  end
+
   def test_first_or_create_bang_with_valid_block
-    canary = Bird.create!(color: "yellow", name: "canary")
+    Bird.create!(color: "yellow", name: "canary")
     parrot = Bird.where(color: "green").first_or_create! do |bird|
       bird.name = "parrot"
-      assert_equal canary, Bird.find_by!(name: "canary")
+      assert_deprecated { assert_equal 0, Bird.count }
     end
-    assert_equal 1, parrot.total_count
     assert_kind_of Bird, parrot
     assert_predicate parrot, :persisted?
     assert_equal "green", parrot.color
@@ -1269,13 +1289,23 @@ class RelationTest < ActiveRecord::TestCase
     assert_equal "green", parrot.color
   end
 
+  def test_first_or_initialize_with_after_initialize
+    Bird.create!(color: "yellow", name: "canary")
+    parrot = assert_deprecated do
+      Bird.where(color: "green").first_or_initialize do |bird|
+        bird.name = "parrot"
+        bird.enable_count = true
+      end
+    end
+    assert_equal 0, parrot.total_count
+  end
+
   def test_first_or_initialize_with_block
-    canary = Bird.create!(color: "yellow", name: "canary")
+    Bird.create!(color: "yellow", name: "canary")
     parrot = Bird.where(color: "green").first_or_initialize do |bird|
       bird.name = "parrot"
-      assert_equal canary, Bird.find_by!(name: "canary")
+      assert_deprecated { assert_equal 0, Bird.count }
     end
-    assert_equal 1, parrot.total_count
     assert_kind_of Bird, parrot
     assert_not_predicate parrot, :persisted?
     assert_predicate parrot, :valid?
diff --git a/activerecord/test/cases/scoping/named_scoping_test.rb b/activerecord/test/cases/scoping/named_scoping_test.rb
index 1a3a95f168..8b08e40468 100644
--- a/activerecord/test/cases/scoping/named_scoping_test.rb
+++ b/activerecord/test/cases/scoping/named_scoping_test.rb
@@ -50,7 +50,7 @@ class NamedScopingTest < ActiveRecord::TestCase
 
   def test_calling_merge_at_first_in_scope
     Topic.class_eval do
-      scope :calling_merge_at_first_in_scope, Proc.new { merge(Topic.replied) }
+      scope :calling_merge_at_first_in_scope, Proc.new { merge(Topic.unscoped.replied) }
     end
     assert_equal Topic.calling_merge_at_first_in_scope.to_a, Topic.replied.to_a
   end
@@ -448,7 +448,9 @@ class NamedScopingTest < ActiveRecord::TestCase
   end
 
   def test_class_method_in_scope
-    assert_equal [topics(:second)], topics(:first).approved_replies.ordered
+    assert_deprecated do
+      assert_equal [topics(:second)], topics(:first).approved_replies.ordered
+    end
   end
 
   def test_nested_scoping
diff --git a/activerecord/test/models/bird.rb b/activerecord/test/models/bird.rb
index c9f6759c7d..20af7c6122 100644
--- a/activerecord/test/models/bird.rb
+++ b/activerecord/test/models/bird.rb
@@ -17,8 +17,8 @@ class Bird < ActiveRecord::Base
     throw(:abort)
   end
 
-  attr_accessor :total_count
+  attr_accessor :total_count, :enable_count
   after_initialize do
-    self.total_count = Bird.count
+    self.total_count = Bird.count if enable_count
   end
 end