diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 5a289a5aac..46031e7c13 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,5 +1,36 @@ ## Rails 4.0.0 (unreleased) ## +* Deprecate eager-evaluated scopes. + + Don't use this: + + scope :red, where(color: 'red') + default_scope where(color: 'red') + + Use this: + + scope :red, -> { where(color: 'red') } + default_scope { where(color: 'red') } + + The former has numerous issues. It is a common newbie gotcha to do + the following: + + scope :recent, where(published_at: Time.now - 2.weeks) + + Or a more subtle variant: + + scope :recent, -> { where(published_at: Time.now - 2.weeks) } + scope :recent_red, recent.where(color: 'red') + + Eager scopes are also very complex to implement within Active + Record, and there are still bugs. For example, the following does + not do what you expect: + + scope :remove_conditions, except(:where) + where(...).remove_conditions # => still has conditions + + *Jon Leighton* + * Remove IdentityMap IdentityMap has never graduated to be an "enabled-by-default" feature, due diff --git a/activerecord/lib/active_record/scoping/default.rb b/activerecord/lib/active_record/scoping/default.rb index 5f05d146f2..b0609a8c08 100644 --- a/activerecord/lib/active_record/scoping/default.rb +++ b/activerecord/lib/active_record/scoping/default.rb @@ -1,4 +1,5 @@ require 'active_support/concern' +require 'active_support/deprecation' module ActiveRecord module Scoping @@ -51,7 +52,7 @@ module ActiveRecord # the model. # # class Article < ActiveRecord::Base - # default_scope where(:published => true) + # default_scope { where(:published => true) } # end # # Article.all # => SELECT * FROM articles WHERE published = true @@ -62,12 +63,6 @@ module ActiveRecord # Article.new.published # => true # Article.create.published # => true # - # You can also use default_scope with a block, in order to have it lazily evaluated: - # - # class Article < ActiveRecord::Base - # default_scope { where(:published_at => Time.now - 1.week) } - # end - # # (You can also pass any object which responds to call to the default_scope # macro, and it will be called when building the default scope.) # @@ -75,8 +70,8 @@ module ActiveRecord # be merged together: # # class Article < ActiveRecord::Base - # default_scope where(:published => true) - # default_scope where(:rating => 'G') + # default_scope { where(:published => true) } + # default_scope { where(:rating => 'G') } # end # # Article.all # => SELECT * FROM articles WHERE published = true AND rating = 'G' @@ -94,6 +89,16 @@ module ActiveRecord # end def default_scope(scope = {}) scope = Proc.new if block_given? + + if scope.is_a?(Relation) || !scope.respond_to?(:call) + ActiveSupport::Deprecation.warn( + "Calling #default_scope without a block is deprecated. For example instead " \ + "of `default_scope where(color: 'red')`, please use " \ + "`default_scope { where(color: 'red') }`. (Alternatively you can just redefine " \ + "self.default_scope.)" + ) + end + self.default_scopes = default_scopes + [scope] end @@ -106,7 +111,7 @@ module ActiveRecord if scope.is_a?(Hash) default_scope.apply_finder_options(scope) elsif !scope.is_a?(Relation) && scope.respond_to?(:call) - default_scope.merge(scope.call) + default_scope.merge(unscoped { scope.call }) else default_scope.merge(scope) end diff --git a/activerecord/lib/active_record/scoping/named.rb b/activerecord/lib/active_record/scoping/named.rb index 2f266eff79..077e2d067e 100644 --- a/activerecord/lib/active_record/scoping/named.rb +++ b/activerecord/lib/active_record/scoping/named.rb @@ -3,6 +3,7 @@ require 'active_support/core_ext/hash/except' require 'active_support/core_ext/kernel/singleton_class' require 'active_support/core_ext/object/blank' require 'active_support/core_ext/class/attribute' +require 'active_support/deprecation' module ActiveRecord # = Active Record Named \Scopes @@ -171,11 +172,24 @@ module ActiveRecord # Article.published.featured.latest_article # Article.featured.titles - def scope(name, scope_options = {}, &block) + def scope(name, body = {}, &block) extension = Module.new(&block) if block + # Check body.is_a?(Relation) to prevent the relation actually being + # loaded by respond_to? + if body.is_a?(Relation) || !body.respond_to?(:call) + ActiveSupport::Deprecation.warn( + "Using #scope without passing a callable object is deprecated. For " \ + "example `scope :red, where(color: 'red')` should be changed to " \ + "`scope :red, -> { where(color: 'red') }`. There are numerous gotchas " \ + "in the former usage and it makes the implementation more complicated " \ + "and buggy. (If you prefer, you can just define a class method named " \ + "`self.red`.)" + ) + end + singleton_class.send(:define_method, name) do |*args| - options = scope_options.respond_to?(:call) ? unscoped { scope_options.call(*args) } : scope_options + options = body.respond_to?(:call) ? unscoped { body.call(*args) } : body options = scoped.apply_finder_options(options) if options.is_a?(Hash) relation = scoped.merge(options) diff --git a/activerecord/test/cases/named_scope_test.rb b/activerecord/test/cases/named_scope_test.rb index 6c063bf57c..0d3c0b20a4 100644 --- a/activerecord/test/cases/named_scope_test.rb +++ b/activerecord/test/cases/named_scope_test.rb @@ -123,16 +123,6 @@ class NamedScopeTest < ActiveRecord::TestCase assert objects.all?(&:approved?), 'all objects should be approved' end - def test_extensions - assert_equal 1, Topic.anonymous_extension.one - assert_equal 2, Topic.named_extension.two - end - - def test_multiple_extensions - assert_equal 2, Topic.multiple_extensions.extension_two - assert_equal 1, Topic.multiple_extensions.extension_one - end - def test_has_many_associations_have_access_to_scopes assert_not_equal Post.containing_the_letter_a, authors(:david).posts assert !Post.containing_the_letter_a.empty? @@ -464,6 +454,41 @@ class NamedScopeTest < ActiveRecord::TestCase require "models/without_table" end end + + def test_eager_scopes_are_deprecated + klass = Class.new(ActiveRecord::Base) + klass.table_name = 'posts' + + assert_deprecated do + klass.scope :welcome, { :conditions => { :id => posts(:welcome).id } } + end + assert_equal [posts(:welcome).title], klass.welcome.map(&:title) + + assert_deprecated do + klass.scope :welcome_2, klass.where(:id => posts(:welcome).id) + end + assert_equal [posts(:welcome).title], klass.welcome_2.map(&:title) + end + + def test_eager_default_scope_hashes_are_deprecated + klass = Class.new(ActiveRecord::Base) + klass.table_name = 'posts' + + assert_deprecated do + klass.send(:default_scope, :conditions => { :id => posts(:welcome).id }) + end + assert_equal [posts(:welcome).title], klass.all.map(&:title) + end + + def test_eager_default_scope_relations_are_deprecated + klass = Class.new(ActiveRecord::Base) + klass.table_name = 'posts' + + assert_deprecated do + klass.send(:default_scope, klass.where(:id => posts(:welcome).id)) + end + assert_equal [posts(:welcome).title], klass.all.map(&:title) + end end class DynamicScopeMatchTest < ActiveRecord::TestCase diff --git a/activerecord/test/models/author.rb b/activerecord/test/models/author.rb index d50e11d6c9..14444a0092 100644 --- a/activerecord/test/models/author.rb +++ b/activerecord/test/models/author.rb @@ -140,8 +140,8 @@ class Author < ActiveRecord::Base has_many :posts_with_default_include, :class_name => 'PostWithDefaultInclude' has_many :comments_on_posts_with_default_include, :through => :posts_with_default_include, :source => :comments - scope :relation_include_posts, includes(:posts) - scope :relation_include_tags, includes(:tags) + scope :relation_include_posts, -> { includes(:posts) } + scope :relation_include_tags, -> { includes(:tags) } attr_accessor :post_log after_initialize :set_post_log diff --git a/activerecord/test/models/bulb.rb b/activerecord/test/models/bulb.rb index 888afc7604..640e57555d 100644 --- a/activerecord/test/models/bulb.rb +++ b/activerecord/test/models/bulb.rb @@ -1,5 +1,5 @@ class Bulb < ActiveRecord::Base - default_scope where(:name => 'defaulty') + default_scope { where(:name => 'defaulty') } belongs_to :car attr_protected :car_id, :frickinawesome diff --git a/activerecord/test/models/car.rb b/activerecord/test/models/car.rb index 6ff1329d8e..42ac81690f 100644 --- a/activerecord/test/models/car.rb +++ b/activerecord/test/models/car.rb @@ -11,18 +11,18 @@ class Car < ActiveRecord::Base has_many :engines, :dependent => :destroy has_many :wheels, :as => :wheelable, :dependent => :destroy - scope :incl_tyres, includes(:tyres) - scope :incl_engines, includes(:engines) + scope :incl_tyres, -> { includes(:tyres) } + scope :incl_engines, -> { includes(:engines) } - scope :order_using_new_style, order('name asc') - scope :order_using_old_style, :order => 'name asc' + scope :order_using_new_style, -> { order('name asc') } + scope :order_using_old_style, -> { { :order => 'name asc' } } end class CoolCar < Car - default_scope :order => 'name desc' + default_scope { order('name desc') } end class FastCar < Car - default_scope :order => 'name desc' + default_scope { order('name desc') } end diff --git a/activerecord/test/models/categorization.rb b/activerecord/test/models/categorization.rb index 4bd980e606..6588531de6 100644 --- a/activerecord/test/models/categorization.rb +++ b/activerecord/test/models/categorization.rb @@ -12,7 +12,7 @@ end class SpecialCategorization < ActiveRecord::Base self.table_name = 'categorizations' - default_scope where(:special => true) + default_scope { where(:special => true) } belongs_to :author belongs_to :category diff --git a/activerecord/test/models/category.rb b/activerecord/test/models/category.rb index 02b85fd38a..ab3139680c 100644 --- a/activerecord/test/models/category.rb +++ b/activerecord/test/models/category.rb @@ -27,7 +27,7 @@ class Category < ActiveRecord::Base has_many :authors, :through => :categorizations has_many :authors_with_select, :through => :categorizations, :source => :author, :select => 'authors.*, categorizations.post_id' - scope :general, :conditions => { :name => 'General' } + scope :general, -> { where(:name => 'General') } end class SpecialCategory < Category diff --git a/activerecord/test/models/comment.rb b/activerecord/test/models/comment.rb index 88b139d931..00f5a74070 100644 --- a/activerecord/test/models/comment.rb +++ b/activerecord/test/models/comment.rb @@ -1,12 +1,10 @@ class Comment < ActiveRecord::Base scope :limit_by, lambda {|l| limit(l) } - scope :containing_the_letter_e, :conditions => "comments.body LIKE '%e%'" - scope :not_again, where("comments.body NOT LIKE '%again%'") - scope :for_first_post, :conditions => { :post_id => 1 } - scope :for_first_author, - :joins => :post, - :conditions => { "posts.author_id" => 1 } - scope :created + scope :containing_the_letter_e, -> { where("comments.body LIKE '%e%'") } + scope :not_again, -> { where("comments.body NOT LIKE '%again%'") } + scope :for_first_post, -> { where(:post_id => 1) } + scope :for_first_author, -> { joins(:post).where("posts.author_id" => 1) } + scope :created, -> { scoped } belongs_to :post, :counter_cache => true has_many :ratings @@ -25,7 +23,7 @@ class Comment < ActiveRecord::Base def self.all_as_method all end - scope :all_as_scope, {} + scope :all_as_scope, -> { scoped } end class SpecialComment < Comment diff --git a/activerecord/test/models/developer.rb b/activerecord/test/models/developer.rb index 4dc9fff9fd..4cc4947e3b 100644 --- a/activerecord/test/models/developer.rb +++ b/activerecord/test/models/developer.rb @@ -45,7 +45,7 @@ class Developer < ActiveRecord::Base has_many :audit_logs - scope :jamises, :conditions => {:name => 'Jamis'} + scope :jamises, -> { where(:name => 'Jamis') } validates_inclusion_of :salary, :in => 50000..200000 validates_length_of :name, :within => 3..20 @@ -88,20 +88,20 @@ end class DeveloperWithSelect < ActiveRecord::Base self.table_name = 'developers' - default_scope select('name') + default_scope { select('name') } end class DeveloperWithIncludes < ActiveRecord::Base self.table_name = 'developers' has_many :audit_logs, :foreign_key => :developer_id - default_scope includes(:audit_logs) + default_scope { includes(:audit_logs) } end class DeveloperOrderedBySalary < ActiveRecord::Base self.table_name = 'developers' - default_scope :order => 'salary DESC' + default_scope { order('salary DESC') } - scope :by_name, order('name DESC') + scope :by_name, -> { order('name DESC') } def self.all_ordered_by_name with_scope(:find => { :order => 'name DESC' }) do @@ -112,7 +112,7 @@ end class DeveloperCalledDavid < ActiveRecord::Base self.table_name = 'developers' - default_scope where("name = 'David'") + default_scope { where("name = 'David'") } end class LazyLambdaDeveloperCalledDavid < ActiveRecord::Base @@ -140,7 +140,7 @@ end class ClassMethodReferencingScopeDeveloperCalledDavid < ActiveRecord::Base self.table_name = 'developers' - scope :david, where(:name => 'David') + scope :david, -> { where(:name => 'David') } def self.default_scope david @@ -149,40 +149,40 @@ end class LazyBlockReferencingScopeDeveloperCalledDavid < ActiveRecord::Base self.table_name = 'developers' - scope :david, where(:name => 'David') + scope :david, -> { where(:name => 'David') } default_scope { david } end class DeveloperCalledJamis < ActiveRecord::Base self.table_name = 'developers' - default_scope where(:name => 'Jamis') - scope :poor, where('salary < 150000') + default_scope { where(:name => 'Jamis') } + scope :poor, -> { where('salary < 150000') } end class PoorDeveloperCalledJamis < ActiveRecord::Base self.table_name = 'developers' - default_scope where(:name => 'Jamis', :salary => 50000) + default_scope -> { where(:name => 'Jamis', :salary => 50000) } end class InheritedPoorDeveloperCalledJamis < DeveloperCalledJamis self.table_name = 'developers' - default_scope where(:salary => 50000) + default_scope -> { where(:salary => 50000) } end class MultiplePoorDeveloperCalledJamis < ActiveRecord::Base self.table_name = 'developers' - default_scope where(:name => 'Jamis') - default_scope where(:salary => 50000) + default_scope -> { where(:name => 'Jamis') } + default_scope -> { where(:salary => 50000) } end module SalaryDefaultScope extend ActiveSupport::Concern - included { default_scope where(:salary => 50000) } + included { default_scope { where(:salary => 50000) } } end class ModuleIncludedPoorDeveloperCalledJamis < DeveloperCalledJamis @@ -195,7 +195,7 @@ class EagerDeveloperWithDefaultScope < ActiveRecord::Base self.table_name = 'developers' has_and_belongs_to_many :projects, :foreign_key => 'developer_id', :join_table => 'developers_projects', :order => 'projects.id' - default_scope includes(:projects) + default_scope { includes(:projects) } end class EagerDeveloperWithClassMethodDefaultScope < ActiveRecord::Base diff --git a/activerecord/test/models/organization.rb b/activerecord/test/models/organization.rb index 4a4111833f..72e7bade68 100644 --- a/activerecord/test/models/organization.rb +++ b/activerecord/test/models/organization.rb @@ -8,5 +8,5 @@ class Organization < ActiveRecord::Base has_one :author, :primary_key => :name has_one :author_owned_essay_category, :through => :author, :source => :owned_essay_category - scope :clubs, { :from => 'clubs' } + scope :clubs, -> { from('clubs') } end diff --git a/activerecord/test/models/person.rb b/activerecord/test/models/person.rb index 84bc901b5e..b7d5dabc4f 100644 --- a/activerecord/test/models/person.rb +++ b/activerecord/test/models/person.rb @@ -27,8 +27,8 @@ class Person < ActiveRecord::Base has_many :agents_posts, :through => :agents, :source => :posts has_many :agents_posts_authors, :through => :agents_posts, :source => :author - scope :males, :conditions => { :gender => 'M' } - scope :females, :conditions => { :gender => 'F' } + scope :males, -> { where(:gender => 'M') } + scope :females, -> { where(:gender => 'F') } end class PersonWithDependentDestroyJobs < ActiveRecord::Base diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb index 0fc22ac6a3..5002ab9ff8 100644 --- a/activerecord/test/models/post.rb +++ b/activerecord/test/models/post.rb @@ -5,8 +5,8 @@ class Post < ActiveRecord::Base end end - scope :containing_the_letter_a, where("body LIKE '%a%'") - scope :ranked_by_comments, order("comments_count DESC") + scope :containing_the_letter_a, -> { where("body LIKE '%a%'") } + scope :ranked_by_comments, -> { order("comments_count DESC") } scope :limit_by, lambda {|l| limit(l) } scope :with_authors_at_address, lambda { |address| { @@ -30,8 +30,8 @@ class Post < ActiveRecord::Base has_one :first_comment, :class_name => 'Comment', :order => 'id ASC' has_one :last_comment, :class_name => 'Comment', :order => 'id desc' - scope :with_special_comments, :joins => :comments, :conditions => {:comments => {:type => 'SpecialComment'} } - scope :with_very_special_comments, joins(:comments).where(:comments => {:type => 'VerySpecialComment'}) + scope :with_special_comments, -> { joins(:comments).where(:comments => {:type => 'SpecialComment'}) } + scope :with_very_special_comments, -> { joins(:comments).where(:comments => {:type => 'VerySpecialComment'}) } scope :with_post, lambda {|post_id| { :joins => :comments, :conditions => {:comments => {:post_id => post_id} } } } @@ -171,7 +171,7 @@ end class FirstPost < ActiveRecord::Base self.table_name = 'posts' - default_scope where(:id => 1) + default_scope { where(:id => 1) } has_many :comments, :foreign_key => :post_id has_one :comment, :foreign_key => :post_id @@ -179,16 +179,16 @@ end class PostWithDefaultInclude < ActiveRecord::Base self.table_name = 'posts' - default_scope includes(:comments) + default_scope { includes(:comments) } has_many :comments, :foreign_key => :post_id end class PostWithDefaultScope < ActiveRecord::Base self.table_name = 'posts' - default_scope :order => :title + default_scope { order(:title) } end class SpecialPostWithDefaultScope < ActiveRecord::Base self.table_name = 'posts' - default_scope where(:id => [1, 5,6]) + default_scope { where(:id => [1, 5,6]) } end diff --git a/activerecord/test/models/project.rb b/activerecord/test/models/project.rb index efe1ce67da..32ce164995 100644 --- a/activerecord/test/models/project.rb +++ b/activerecord/test/models/project.rb @@ -32,7 +32,7 @@ class Project < ActiveRecord::Base def self.all_as_method all end - scope :all_as_scope, {} + scope :all_as_scope, -> { scoped } end class SpecialProject < Project diff --git a/activerecord/test/models/reference.rb b/activerecord/test/models/reference.rb index c5af0b5d5f..561b431766 100644 --- a/activerecord/test/models/reference.rb +++ b/activerecord/test/models/reference.rb @@ -19,5 +19,5 @@ end class BadReference < ActiveRecord::Base self.table_name = 'references' - default_scope where(:favourite => false) + default_scope { where(:favourite => false) } end diff --git a/activerecord/test/models/reply.rb b/activerecord/test/models/reply.rb index 6adfe0ae3c..53bc95e5f2 100644 --- a/activerecord/test/models/reply.rb +++ b/activerecord/test/models/reply.rb @@ -1,7 +1,7 @@ require 'models/topic' class Reply < Topic - scope :base + scope :base, -> { scoped } belongs_to :topic, :foreign_key => "parent_id", :counter_cache => true belongs_to :topic_with_primary_key, :class_name => "Topic", :primary_key => "title", :foreign_key => "parent_title", :counter_cache => "replies_count" diff --git a/activerecord/test/models/topic.rb b/activerecord/test/models/topic.rb index 8bcb9df8a8..785839be75 100644 --- a/activerecord/test/models/topic.rb +++ b/activerecord/test/models/topic.rb @@ -1,21 +1,24 @@ class Topic < ActiveRecord::Base - scope :base + scope :base, -> { scoped } scope :written_before, lambda { |time| if time { :conditions => ['written_on < ?', time] } end } - scope :approved, :conditions => {:approved => true} - scope :rejected, :conditions => {:approved => false} + scope :approved, -> { where(:approved => true) } + scope :rejected, -> { where(:approved => false) } scope :scope_with_lambda, lambda { scoped } - scope :by_lifo, :conditions => {:author_name => 'lifo'} + scope :by_lifo, -> { where(:author_name => 'lifo') } - scope :approved_as_hash_condition, :conditions => {:topics => {:approved => true}} - scope 'approved_as_string', :conditions => {:approved => true} - scope :replied, :conditions => ['replies_count > 0'] - scope :anonymous_extension do + ActiveSupport::Deprecation.silence do + scope :approved_as_hash_condition, :conditions => {:topics => {:approved => true}} + scope :replied, :conditions => ['replies_count > 0'] + end + + scope 'approved_as_string', -> { where(:approved => true) } + scope :anonymous_extension, -> { scoped } do def one 1 end @@ -42,8 +45,8 @@ class Topic < ActiveRecord::Base 2 end end - scope :named_extension, :extend => NamedExtension - scope :multiple_extensions, :extend => [MultipleExtensionTwo, MultipleExtensionOne] + scope :named_extension, -> { { :extend => NamedExtension } } + scope :multiple_extensions, -> { { :extend => [MultipleExtensionTwo, MultipleExtensionOne] } } has_many :replies, :dependent => :destroy, :foreign_key => "parent_id" has_many :replies_with_primary_key, :class_name => "Reply", :dependent => :destroy, :primary_key => "title", :foreign_key => "parent_title" diff --git a/activerecord/test/models/toy.rb b/activerecord/test/models/toy.rb index 0377e50011..ddc7048a56 100644 --- a/activerecord/test/models/toy.rb +++ b/activerecord/test/models/toy.rb @@ -2,5 +2,5 @@ class Toy < ActiveRecord::Base self.primary_key = :toy_id belongs_to :pet - scope :with_pet, joins(:pet) + scope :with_pet, -> { joins(:pet) } end diff --git a/activerecord/test/models/without_table.rb b/activerecord/test/models/without_table.rb index 184ab1649e..50c824e4ac 100644 --- a/activerecord/test/models/without_table.rb +++ b/activerecord/test/models/without_table.rb @@ -1,3 +1,3 @@ class WithoutTable < ActiveRecord::Base - default_scope where(:published => true) + default_scope -> { where(:published => true) } end