diff options
Diffstat (limited to 'activerecord')
| -rw-r--r-- | activerecord/CHANGELOG.md | 5 | ||||
| -rw-r--r-- | activerecord/lib/active_record/relation.rb | 36 | ||||
| -rw-r--r-- | activerecord/lib/active_record/relation/spawn_methods.rb | 2 | ||||
| -rw-r--r-- | activerecord/lib/active_record/scoping.rb | 8 | ||||
| -rw-r--r-- | activerecord/lib/active_record/scoping/named.rb | 10 | ||||
| -rw-r--r-- | activerecord/test/cases/base_test.rb | 2 | ||||
| -rw-r--r-- | activerecord/test/cases/relations_test.rb | 48 | ||||
| -rw-r--r-- | activerecord/test/cases/scoping/named_scoping_test.rb | 6 | ||||
| -rw-r--r-- | activerecord/test/models/bird.rb | 4 |
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 |