Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Feb 15, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -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*

@@ -67,7 +67,7 @@ def bind_attribute(name, value) # :nodoc:
# 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 @@ def create(attributes = nil, &block)
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 @@ def create!(attributes = nil, &block)
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 @@ def cache_key(timestamp_column = :updated_at)
# 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 @@ def reload

def reset
@delegate_to_klass = false
@_deprecated_scope_source = nil
@to_sql = @arel = @loaded = @should_eager_load = nil
@records = [].freeze
@offsets = {}
@@ -633,14 +636,35 @@ def preload_associations(records) # :nodoc:
end
end

attr_reader :_deprecated_scope_source # :nodoc:

protected
attr_writer :_deprecated_scope_source # :nodoc:

def load_records(records)
@records = records.freeze
@loaded = true
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
@@ -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.
@@ -30,14 +30,6 @@ def current_scope(skip_inherited_scope = false)
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:
@@ -27,6 +27,14 @@ def all
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`.

This comment has been minimized.

Copy link
@kamipo

kamipo Feb 15, 2019

Author Member

Reworded.

MSG
end

if self == scope.klass
scope.clone
else
@@ -180,7 +188,7 @@ def scope(name, body, &block)

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
@@ -1536,7 +1536,7 @@ def test_protected_environments_are_stored_as_an_array_of_string
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

@@ -1167,13 +1167,23 @@ def test_first_or_create_with_no_parameters
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 @@ def test_first_or_create_bang_with_no_parameters
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 @@ def test_first_or_initialize_with_no_parameters
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?
@@ -50,7 +50,7 @@ def test_delegates_finds_and_calculations_to_the_base_class

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 @@ def test_chaining_combines_conditions_when_searching
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
@@ -17,8 +17,8 @@ def cancel_save_callback_method
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
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.