aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorRyuta Kamizono <kamipo@gmail.com>2019-02-15 11:34:54 +0900
committerRyuta Kamizono <kamipo@gmail.com>2019-02-15 17:40:15 +0900
commit4c6171d6056a346a953792a954853d14c000dc90 (patch)
tree6828ab3c21482a8a81bc0c9229e2028e9e26bdc0
parentcdb8697b4a654aad2e65590d58c5f581a53d6b33 (diff)
downloadrails-4c6171d6056a346a953792a954853d14c000dc90.tar.gz
rails-4c6171d6056a346a953792a954853d14c000dc90.tar.bz2
rails-4c6171d6056a346a953792a954853d14c000dc90.zip
Deprecate using class level querying methods if the receiver scope regarded as leaked
This deprecates using class level querying methods if the receiver scope regarded as leaked, since #32380 and #35186 may cause that silently leaking information when people upgrade the app. We need deprecation first before making those.
-rw-r--r--activerecord/CHANGELOG.md5
-rw-r--r--activerecord/lib/active_record/relation.rb43
-rw-r--r--activerecord/lib/active_record/relation/spawn_methods.rb2
-rw-r--r--activerecord/lib/active_record/scoping/named.rb10
-rw-r--r--activerecord/test/cases/relations_test.rb30
-rw-r--r--activerecord/test/cases/scoping/named_scoping_test.rb6
6 files changed, 75 insertions, 21 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 63205368c7..7ff4c743c6 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,8 @@
+* Deprecate using class level querying methods if the receiver scope
+ regarded as leaked. Use `klass.unscoped` to avoid the leaking scope.
+
+ *Ryuta Kamizono*
+
* Allow applications to automatically switch connections.
Adds a middleware and configuration options that can be used in your
diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb
index ecca833847..feaa20ccc6 100644
--- a/activerecord/lib/active_record/relation.rb
+++ b/activerecord/lib/active_record/relation.rb
@@ -67,6 +67,7 @@ module ActiveRecord
# user = users.new { |user| user.name = 'Oscar' }
# user.name # => Oscar
def new(attributes = nil, &block)
+ block = _deprecated_scope_block("new", &block)
scoping { klass.new(attributes, &block) }
end
@@ -92,7 +93,12 @@ 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
+ block = _deprecated_scope_block("create", &block)
+ scoping { klass.create(attributes, &block) }
+ end
end
# Similar to #create, but calls
@@ -102,7 +108,12 @@ 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
+ block = _deprecated_scope_block("create!", &block)
+ scoping { klass.create!(attributes, &block) }
+ end
end
def first_or_create(attributes = nil, &block) # :nodoc:
@@ -312,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
@@ -516,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 = {}
@@ -624,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
@@ -632,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/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/relations_test.rb b/activerecord/test/cases/relations_test.rb
index 98ba47f1b8..2ac81ba425 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -1169,9 +1169,11 @@ class RelationTest < ActiveRecord::TestCase
def test_first_or_create_with_after_initialize
Bird.create!(color: "yellow", name: "canary")
- parrot = Bird.where(color: "green").first_or_create do |bird|
- bird.name = "parrot"
- bird.enable_count = true
+ 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
@@ -1180,7 +1182,7 @@ class RelationTest < ActiveRecord::TestCase
Bird.create!(color: "yellow", name: "canary")
parrot = Bird.where(color: "green").first_or_create do |bird|
bird.name = "parrot"
- assert_equal 0, Bird.count
+ assert_deprecated { assert_equal 0, Bird.count }
end
assert_kind_of Bird, parrot
assert_predicate parrot, :persisted?
@@ -1224,9 +1226,11 @@ class RelationTest < ActiveRecord::TestCase
def test_first_or_create_bang_with_after_initialize
Bird.create!(color: "yellow", name: "canary")
- parrot = Bird.where(color: "green").first_or_create! do |bird|
- bird.name = "parrot"
- bird.enable_count = true
+ 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
@@ -1235,7 +1239,7 @@ class RelationTest < ActiveRecord::TestCase
Bird.create!(color: "yellow", name: "canary")
parrot = Bird.where(color: "green").first_or_create! do |bird|
bird.name = "parrot"
- assert_equal 0, Bird.count
+ assert_deprecated { assert_equal 0, Bird.count }
end
assert_kind_of Bird, parrot
assert_predicate parrot, :persisted?
@@ -1287,9 +1291,11 @@ class RelationTest < ActiveRecord::TestCase
def test_first_or_initialize_with_after_initialize
Bird.create!(color: "yellow", name: "canary")
- parrot = Bird.where(color: "green").first_or_initialize do |bird|
- bird.name = "parrot"
- bird.enable_count = true
+ 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
@@ -1298,7 +1304,7 @@ class RelationTest < ActiveRecord::TestCase
Bird.create!(color: "yellow", name: "canary")
parrot = Bird.where(color: "green").first_or_initialize do |bird|
bird.name = "parrot"
- assert_equal 0, Bird.count
+ assert_deprecated { assert_equal 0, Bird.count }
end
assert_kind_of Bird, parrot
assert_not_predicate parrot, :persisted?
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