Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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
  • Loading branch information
jonleighton committed Mar 21, 2012
1 parent fd68bd2 commit 0a12a5f
Show file tree
Hide file tree
Showing 20 changed files with 159 additions and 83 deletions.
31 changes: 31 additions & 0 deletions 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
Expand Down
25 changes: 15 additions & 10 deletions activerecord/lib/active_record/scoping/default.rb
@@ -1,4 +1,5 @@
require 'active_support/concern'
require 'active_support/deprecation'

module ActiveRecord
module Scoping
Expand Down Expand Up @@ -51,7 +52,7 @@ def before_remove_const #:nodoc:
# 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
Expand All @@ -62,21 +63,15 @@ def before_remove_const #:nodoc:
# Article.new.published # => true
# Article.create.published # => true
#
# You can also use <tt>default_scope</tt> 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 <tt>call</tt> to the <tt>default_scope</tt>
# macro, and it will be called when building the default scope.)
#
# If you use multiple <tt>default_scope</tt> declarations in your model then they will
# 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'
Expand All @@ -94,6 +89,16 @@ def before_remove_const #:nodoc:
# 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

Expand All @@ -106,7 +111,7 @@ def build_default_scope #:nodoc:
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
Expand Down
18 changes: 16 additions & 2 deletions activerecord/lib/active_record/scoping/named.rb
Expand Up @@ -3,6 +3,7 @@
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
Expand Down Expand Up @@ -171,11 +172,24 @@ def scope_attributes? # :nodoc:
# 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)
Expand Down
45 changes: 35 additions & 10 deletions activerecord/test/cases/named_scope_test.rb
Expand Up @@ -123,16 +123,6 @@ def test_scope_with_object
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?
Expand Down Expand Up @@ -464,6 +454,41 @@ def test_scoped_are_lazy_loaded_if_table_still_does_not_exist
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
Expand Down
4 changes: 2 additions & 2 deletions activerecord/test/models/author.rb
Expand Up @@ -140,8 +140,8 @@ def testing_proxy_target
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
Expand Down
2 changes: 1 addition & 1 deletion 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
Expand Down
12 changes: 6 additions & 6 deletions activerecord/test/models/car.rb
Expand Up @@ -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
2 changes: 1 addition & 1 deletion activerecord/test/models/categorization.rb
Expand Up @@ -12,7 +12,7 @@ class Categorization < ActiveRecord::Base

class SpecialCategorization < ActiveRecord::Base
self.table_name = 'categorizations'
default_scope where(:special => true)
default_scope { where(:special => true) }

belongs_to :author
belongs_to :category
Expand Down
2 changes: 1 addition & 1 deletion activerecord/test/models/category.rb
Expand Up @@ -27,7 +27,7 @@ def self.what_are_you
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
Expand Down
14 changes: 6 additions & 8 deletions 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
Expand All @@ -25,7 +23,7 @@ def self.search_by_type(q)
def self.all_as_method
all
end
scope :all_as_scope, {}
scope :all_as_scope, -> { scoped }
end

class SpecialComment < Comment
Expand Down
32 changes: 16 additions & 16 deletions activerecord/test/models/developer.rb
Expand Up @@ -45,7 +45,7 @@ def find_least_recent

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
Expand Down Expand Up @@ -88,20 +88,20 @@ def raise_if_projects_empty!

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
Expand All @@ -112,7 +112,7 @@ def self.all_ordered_by_name

class DeveloperCalledDavid < ActiveRecord::Base
self.table_name = 'developers'
default_scope where("name = 'David'")
default_scope { where("name = 'David'") }
end

class LazyLambdaDeveloperCalledDavid < ActiveRecord::Base
Expand Down Expand Up @@ -140,7 +140,7 @@ def self.default_scope

class ClassMethodReferencingScopeDeveloperCalledDavid < ActiveRecord::Base
self.table_name = 'developers'
scope :david, where(:name => 'David')
scope :david, -> { where(:name => 'David') }

def self.default_scope
david
Expand All @@ -149,40 +149,40 @@ def self.default_scope

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
Expand All @@ -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
Expand Down

7 comments on commit 0a12a5f

@drewblas
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the concept of the change that scopes should be encapsulated in a proc/block in order to prevent errors from execution However, why is the method signature of 'scope' and 'default_scope' so different? They should both either take a block or a lambda (or both). But instead, scope takes a block and does something totally different with it. Either they should both do the same thing with the block or not take a block at all. Otherwise it becomes very confusing to track which method accepts which format.

@jonleighton
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment scope takes a proxy extension as its block argument. I plan to deprecate that too, after which we can make it use the block as its definition as an alternative.

Note that that requires parens around the args so it's a matter of style:

scope :foo, -> { ... }
scope(:foo) { ... }

@exviva
Copy link
Contributor

@exviva exviva commented on 0a12a5f Mar 28, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about taking a hash:

scope foo: -> { ... }

@drewblas
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, +1 for deprecating the proxy extension.

@fxn
Copy link
Member

@fxn fxn commented on 0a12a5f Mar 29, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonleighton could you please update the finders guide? There's a section about scopes. Documentation needs to reflect the current ways to do things. There is no need to add content about the deprecated idioms, guides rarely mention deprecated APIs, modernizing the current coverage is enough.

@jonleighton
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fxn
Copy link
Member

@fxn fxn commented on 0a12a5f Mar 30, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonleighton awesome

Please sign in to comment.