From b9be64cc3e2959c974c3036a0eb5a37fc8c7d580 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Tue, 27 Mar 2018 19:31:34 +0900 Subject: [PATCH 1/2] Bring back private class methods accessibility in named scope The receiver in a scope was changed from `klass` to `relation` itself for all scopes (named scope, default_scope, and association scope) behaves consistently. In addition. Before 5.2, if both an AR model class and a Relation instance have same named methods (e.g. `arel_attribute`, `predicate_builder`, etc), named scope doesn't respect relation instance information. For example: ```ruby class Post < ActiveRecord::Base has_many :comments1, class_name: "RecentComment1" has_many :comments2, class_name: "RecentComment2" end class RecentComment1 < ActiveRecord::Base self.table_name = "comments" default_scope { where(arel_attribute(:created_at).gteq(2.weeks.ago)) } end class RecentComment2 < ActiveRecord::Base self.table_name = "comments" default_scope { recent_updated } scope :recent_updated, -> { where(arel_attribute(:updated_at).gteq(2.weeks.ago)) } end ``` If eager loading `Post.eager_load(:comments1, :comments2).to_a`, `:comments1` (default_scope) respects aliased table name, but `:comments2` (using named scope) may not work correctly since named scope doesn't respect relation instance information. See also 801ccab. But this is a breaking change between releases without deprecation. I decided to bring back private class methods accessibility in named scope. Fixes #31740. Fixes #32331. --- activerecord/lib/active_record/relation.rb | 7 +++++++ activerecord/lib/active_record/relation/delegation.rb | 3 +++ activerecord/lib/active_record/scoping/named.rb | 2 +- activerecord/test/models/topic.rb | 9 ++++++++- 4 files changed, 19 insertions(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 34e643b2de..38b720ffc8 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -313,6 +313,13 @@ module ActiveRecord klass.current_scope = previous end + def _exec_scope(*args, &block) # :nodoc: + @delegate_to_klass = true + instance_exec(*args, &block) || self + ensure + @delegate_to_klass = false + end + # Updates all records in the current relation with details given. This method constructs a single SQL UPDATE # statement and sends it straight to the database. It does not instantiate the involved models and it does not # trigger Active Record callbacks or validations. However, values passed to #update_all will still go through diff --git a/activerecord/lib/active_record/relation/delegation.rb b/activerecord/lib/active_record/relation/delegation.rb index 4b16b49cdf..27450ab8b3 100644 --- a/activerecord/lib/active_record/relation/delegation.rb +++ b/activerecord/lib/active_record/relation/delegation.rb @@ -82,6 +82,9 @@ module ActiveRecord if @klass.respond_to?(method) self.class.delegate_to_scoped_klass(method) scoping { @klass.public_send(method, *args, &block) } + elsif defined?(@delegate_to_klass) && + @delegate_to_klass && @klass.respond_to?(method, true) + @klass.send(method, *args, &block) elsif arel.respond_to?(method) ActiveSupport::Deprecation.warn \ "Delegating #{method} to arel is deprecated and will be removed in Rails 6.0." diff --git a/activerecord/lib/active_record/scoping/named.rb b/activerecord/lib/active_record/scoping/named.rb index 752655aa05..a784001587 100644 --- a/activerecord/lib/active_record/scoping/named.rb +++ b/activerecord/lib/active_record/scoping/named.rb @@ -183,7 +183,7 @@ module ActiveRecord if body.respond_to?(:to_proc) singleton_class.send(:define_method, name) do |*args| scope = all - scope = scope.instance_exec(*args, &body) || scope + scope = scope._exec_scope(*args, &body) scope = scope.extending(extension) if extension scope end diff --git a/activerecord/test/models/topic.rb b/activerecord/test/models/topic.rb index 8cd4dc352a..a924cf86ae 100644 --- a/activerecord/test/models/topic.rb +++ b/activerecord/test/models/topic.rb @@ -12,9 +12,16 @@ class Topic < ActiveRecord::Base scope :scope_with_lambda, lambda { all } - scope :by_lifo, -> { where(author_name: "lifo") } + scope :by_lifo, -> { where(author_name: author_name) } scope :replied, -> { where "replies_count > 0" } + class << self + private + def author_name + "lifo" + end + end + scope "approved_as_string", -> { where(approved: true) } scope :anonymous_extension, -> {} do def one From 0bfeb481a0aa35eeca1c5230938fa61fee0faef2 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Fri, 30 Mar 2018 09:28:12 +0900 Subject: [PATCH 2/2] Deprecate accessibility of private/protected class methods in named scope --- activerecord/lib/active_record/relation.rb | 2 ++ activerecord/lib/active_record/relation/delegation.rb | 6 ++++-- activerecord/test/cases/scoping/named_scoping_test.rb | 7 +++++++ activerecord/test/models/topic.rb | 5 +++-- 4 files changed, 16 insertions(+), 4 deletions(-) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 38b720ffc8..38a364ff88 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -30,6 +30,7 @@ module ActiveRecord @offsets = {} @loaded = false @predicate_builder = predicate_builder + @delegate_to_klass = false end def initialize_copy(other) @@ -453,6 +454,7 @@ module ActiveRecord end def reset + @delegate_to_klass = false @to_sql = @arel = @loaded = @should_eager_load = nil @records = [].freeze @offsets = {} diff --git a/activerecord/lib/active_record/relation/delegation.rb b/activerecord/lib/active_record/relation/delegation.rb index 27450ab8b3..488f71cdde 100644 --- a/activerecord/lib/active_record/relation/delegation.rb +++ b/activerecord/lib/active_record/relation/delegation.rb @@ -82,8 +82,10 @@ module ActiveRecord if @klass.respond_to?(method) self.class.delegate_to_scoped_klass(method) scoping { @klass.public_send(method, *args, &block) } - elsif defined?(@delegate_to_klass) && - @delegate_to_klass && @klass.respond_to?(method, true) + elsif @delegate_to_klass && @klass.respond_to?(method, true) + ActiveSupport::Deprecation.warn \ + "Delegating missing #{method} method to #{@klass}. " \ + "Accessibility of private/protected class methods in :scope is deprecated and will be removed in Rails 6.0." @klass.send(method, *args, &block) elsif arel.respond_to?(method) ActiveSupport::Deprecation.warn \ diff --git a/activerecord/test/cases/scoping/named_scoping_test.rb b/activerecord/test/cases/scoping/named_scoping_test.rb index ea71a5ce28..03f8a4f7c9 100644 --- a/activerecord/test/cases/scoping/named_scoping_test.rb +++ b/activerecord/test/cases/scoping/named_scoping_test.rb @@ -303,6 +303,13 @@ class NamedScopingTest < ActiveRecord::TestCase assert_equal "lifo", topic.author_name end + def test_deprecated_delegating_private_method + assert_deprecated do + scope = Topic.all.by_private_lifo + assert_not scope.instance_variable_get(:@delegate_to_klass) + end + end + def test_reserved_scope_names klass = Class.new(ActiveRecord::Base) do self.table_name = "topics" diff --git a/activerecord/test/models/topic.rb b/activerecord/test/models/topic.rb index a924cf86ae..fa50eeb6a4 100644 --- a/activerecord/test/models/topic.rb +++ b/activerecord/test/models/topic.rb @@ -12,12 +12,13 @@ class Topic < ActiveRecord::Base scope :scope_with_lambda, lambda { all } - scope :by_lifo, -> { where(author_name: author_name) } + scope :by_private_lifo, -> { where(author_name: private_lifo) } + scope :by_lifo, -> { where(author_name: "lifo") } scope :replied, -> { where "replies_count > 0" } class << self private - def author_name + def private_lifo "lifo" end end