diff options
author | Ryuta Kamizono <kamipo@gmail.com> | 2019-02-07 17:44:23 +0900 |
---|---|---|
committer | Ryuta Kamizono <kamipo@gmail.com> | 2019-02-07 21:04:01 +0900 |
commit | 22360534ac922c68fb0a28f584b48bc0f3633221 (patch) | |
tree | 634c8632646c2a4aa2560df46fad9f0a3e184ffb | |
parent | 2e018361c7c51e36d1d98bf770b7456d78dee68b (diff) | |
download | rails-22360534ac922c68fb0a28f584b48bc0f3633221.tar.gz rails-22360534ac922c68fb0a28f584b48bc0f3633221.tar.bz2 rails-22360534ac922c68fb0a28f584b48bc0f3633221.zip |
Fix `relation.create` to avoid leaking scope to initialization block and callbacks
`relation.create` populates scope attributes to new record by `scoping`,
it is necessary to assign the scope attributes to the record and to find
STI subclass from the scope attributes.
But the effect of `scoping` is class global, it was caused undesired
behavior that pollute all class level querying methods in initialization
block and callbacks (`after_initialize`, `before_validation`,
`before_save`, etc), which are user provided code.
To avoid the leaking scope issue, restore the original current scope
before initialization block and callbacks are invoked.
Fixes #9894.
Fixes #17577.
Closes #31526.
-rw-r--r-- | activerecord/CHANGELOG.md | 6 | ||||
-rw-r--r-- | activerecord/lib/active_record/relation.rb | 17 | ||||
-rw-r--r-- | activerecord/test/cases/base_test.rb | 2 | ||||
-rw-r--r-- | activerecord/test/cases/relations_test.rb | 21 | ||||
-rw-r--r-- | activerecord/test/models/bird.rb | 5 |
5 files changed, 45 insertions, 6 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index c412646cc9..e957361ec3 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* Fix `relation.create` to avoid leaking scope to initialization block and callbacks. + + Fixes #9894, #17577. + + *Ryuta Kamizono* + * Chaining named scope is no longer leaking to class level querying methods. Fixes #14003. diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index ab3d6f7222..32f0609798 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -67,6 +67,11 @@ module ActiveRecord # user = users.new { |user| user.name = 'Oscar' } # user.name # => Oscar def new(attributes = nil, &block) + current_scope = klass.current_scope(true) + block = -> record do + klass.current_scope = current_scope + yield record if block_given? + end scoping { klass.new(attributes, &block) } end @@ -92,7 +97,11 @@ module ActiveRecord # users.create(name: nil) # validation on name # # => #<User id: nil, name: nil, ...> def create(attributes = nil, &block) - scoping { klass.create(attributes, &block) } + if attributes.is_a?(Array) + attributes.collect { |attr| create(attr, &block) } + else + new(attributes, &block).tap(&:save) + end end # Similar to #create, but calls @@ -102,7 +111,11 @@ module ActiveRecord # Expects arguments in the same format as # {ActiveRecord::Base.create!}[rdoc-ref:Persistence::ClassMethods#create!]. def create!(attributes = nil, &block) - scoping { klass.create!(attributes, &block) } + if attributes.is_a?(Array) + attributes.collect { |attr| create!(attr, &block) } + else + new(attributes, &block).tap(&:save!) + end end def first_or_create(attributes = nil, &block) # :nodoc: diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 4938b6865f..363593ca19 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -1535,7 +1535,7 @@ class BasicsTest < ActiveRecord::TestCase Bird.create!(name: "Bluejay") ActiveRecord::Base.connection.while_preventing_writes do - assert_queries(2) { Bird.where(name: "Bluejay").explain } + assert_nothing_raised { Bird.where(name: "Bluejay").explain } end end diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index 6b5b877260..0ab0459c38 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -1167,7 +1167,12 @@ class RelationTest < ActiveRecord::TestCase end def test_first_or_create_with_block - parrot = Bird.where(color: "green").first_or_create { |bird| bird.name = "parrot" } + 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") + end + assert_equal 1, parrot.total_count assert_kind_of Bird, parrot assert_predicate parrot, :persisted? assert_equal "green", parrot.color @@ -1209,7 +1214,12 @@ class RelationTest < ActiveRecord::TestCase end def test_first_or_create_bang_with_valid_block - parrot = Bird.where(color: "green").first_or_create! { |bird| bird.name = "parrot" } + 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") + end + assert_equal 1, parrot.total_count assert_kind_of Bird, parrot assert_predicate parrot, :persisted? assert_equal "green", parrot.color @@ -1259,7 +1269,12 @@ class RelationTest < ActiveRecord::TestCase end def test_first_or_initialize_with_block - parrot = Bird.where(color: "green").first_or_initialize { |bird| bird.name = "parrot" } + 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") + 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/models/bird.rb b/activerecord/test/models/bird.rb index cfefa555b3..c9f6759c7d 100644 --- a/activerecord/test/models/bird.rb +++ b/activerecord/test/models/bird.rb @@ -16,4 +16,9 @@ class Bird < ActiveRecord::Base def cancel_save_callback_method throw(:abort) end + + attr_accessor :total_count + after_initialize do + self.total_count = Bird.count + end end |