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 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Prev

Deprecate using class level querying methods if the receiver scope re…

…garded 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.
  • Loading branch information...
kamipo committed Feb 15, 2019
commit 4c6171d6056a346a953792a954853d14c000dc90
@@ -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
@@ -67,6 +67,7 @@ def bind_attribute(name, value) # :nodoc:
# 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 @@ def new(attributes = nil, &block)
# 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 @@ def create(attributes = nil, &block)
# 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 @@ 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
@@ -516,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 = {}
@@ -624,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.
@@ -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
@@ -1169,9 +1169,11 @@ def test_first_or_create_with_no_parameters

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 @@ def test_first_or_create_with_block
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 @@ def test_first_or_create_bang_with_no_parameters

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 @@ def test_first_or_create_bang_with_valid_block
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 @@ def test_first_or_initialize_with_no_parameters

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