Permalink
Browse files

ActiveRecord::Base.all returns a Relation.

Previously it returned an Array.

If you want an array, call e.g. `Post.to_a` rather than `Post.all`. This
is more explicit.

In most cases this should not break existing code, since
Relations use method_missing to delegate unknown methods to #to_a
anyway.
  • Loading branch information...
1 parent f1afd77 commit 6a81ccd69d96f36f4322ef927191ab5a35e68d68 @jonleighton jonleighton committed Jul 27, 2012
Showing with 329 additions and 324 deletions.
  1. +1 −1 activemodel/lib/active_model/observing.rb
  2. +7 −7 activerecord/lib/active_record/querying.rb
  3. +8 −11 activerecord/lib/active_record/scoping/named.rb
  4. +1 −1 activerecord/test/cases/adapters/mysql/reserved_word_test.rb
  5. +1 −1 activerecord/test/cases/adapters/mysql2/reserved_word_test.rb
  6. +1 −1 activerecord/test/cases/associations/belongs_to_associations_test.rb
  7. +19 −19 activerecord/test/cases/associations/cascaded_eager_loading_test.rb
  8. +1 −1 activerecord/test/cases/associations/eager_load_nested_include_test.rb
  9. +7 −7 activerecord/test/cases/associations/eager_singularization_test.rb
  10. +76 −76 activerecord/test/cases/associations/eager_test.rb
  11. +7 −7 activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
  12. +14 −14 activerecord/test/cases/associations/has_many_associations_test.rb
  13. +2 −2 activerecord/test/cases/associations/has_many_through_associations_test.rb
  14. +1 −1 activerecord/test/cases/associations/has_one_associations_test.rb
  15. +7 −7 activerecord/test/cases/associations/has_one_through_associations_test.rb
  16. +17 −17 activerecord/test/cases/associations/join_model_test.rb
  17. +33 −25 activerecord/test/cases/base_test.rb
  18. +1 −1 activerecord/test/cases/batches_test.rb
  19. +1 −1 activerecord/test/cases/calculations_test.rb
  20. +10 −10 activerecord/test/cases/deprecated_dynamic_methods_test.rb
  21. +4 −4 activerecord/test/cases/explain_test.rb
  22. +17 −17 activerecord/test/cases/finder_test.rb
  23. +6 −6 activerecord/test/cases/inheritance_test.rb
  24. +6 −6 activerecord/test/cases/log_subscriber_test.rb
  25. +2 −2 activerecord/test/cases/migration/rename_column_test.rb
  26. +28 −28 activerecord/test/cases/named_scope_test.rb
  27. +1 −1 activerecord/test/cases/persistence_test.rb
  28. +2 −2 activerecord/test/cases/readonly_test.rb
  29. +23 −23 activerecord/test/cases/relation_scoping_test.rb
  30. +24 −24 activerecord/test/cases/relations_test.rb
  31. +1 −1 activerecord/test/cases/xml_serialization_test.rb
@@ -215,7 +215,7 @@ def observe(*models)
# end
# end
def observed_classes
- Array(observed_class)
+ [observed_class].compact.flatten
end
# The class observed by default is inferred from the observer's class name:
@@ -3,15 +3,15 @@
module ActiveRecord
module Querying
- delegate :find, :take, :take!, :first, :first!, :last, :last!, :all, :exists?, :any?, :many?, :to => :scoped
- delegate :first_or_create, :first_or_create!, :first_or_initialize, :to => :scoped
- delegate :find_by, :find_by!, :to => :scoped
- delegate :destroy, :destroy_all, :delete, :delete_all, :update, :update_all, :to => :scoped
- delegate :find_each, :find_in_batches, :to => :scoped
+ delegate :find, :take, :take!, :first, :first!, :last, :last!, :to_a, :exists?, :any?, :many?, :to => :all
+ delegate :first_or_create, :first_or_create!, :first_or_initialize, :to => :all
+ delegate :find_by, :find_by!, :to => :all
+ delegate :destroy, :destroy_all, :delete, :delete_all, :update, :update_all, :to => :all
+ delegate :find_each, :find_in_batches, :to => :all
delegate :select, :group, :order, :except, :reorder, :limit, :offset, :joins,
:where, :preload, :eager_load, :includes, :from, :lock, :readonly,
- :having, :create_with, :uniq, :references, :none, :to => :scoped
- delegate :count, :average, :minimum, :maximum, :sum, :calculate, :pluck, :ids, :to => :scoped
+ :having, :create_with, :uniq, :references, :none, :to => :all
+ delegate :count, :average, :minimum, :maximum, :sum, :calculate, :pluck, :ids, :to => :all
# Executes a custom SQL query against your database and returns all the results. The results will
# be returned as an array with columns requested encapsulated as attributes of the model you call
@@ -12,33 +12,30 @@ module Named
extend ActiveSupport::Concern
module ClassMethods
- # Returns an anonymous \scope.
+ # Returns an <tt>ActiveRecord::Relation</tt> scope object.
#
- # posts = Post.scoped
+ # posts = Post.all
# posts.size # Fires "select count(*) from posts" and returns the count
# posts.each {|p| puts p.name } # Fires "select * from posts" and loads post objects
#
- # fruits = Fruit.scoped
+ # fruits = Fruit.all
# fruits = fruits.where(:color => 'red') if options[:red_only]
# fruits = fruits.limit(10) if limited?
#
- # Anonymous \scopes tend to be useful when procedurally generating complex
- # queries, where passing intermediate values (\scopes) around as first-class
- # objects is convenient.
- #
# You can define a \scope that applies to all finders using
# ActiveRecord::Base.default_scope.
- def scoped(options = nil)
+ def all
if current_scope
- scope = current_scope.clone
+ current_scope.clone
else
scope = relation
scope.default_scoped = true
scope
end
+ end
- scope.merge!(options) if options
- scope
+ def scoped(options = nil)
+ options ? all.merge!(options) : all
end
##
@@ -130,7 +130,7 @@ def test_calculations_work_with_reserved_words
end
def test_associations_work_with_reserved_words
- assert_nothing_raised { Select.scoped(:includes => [:groups]).all }
+ assert_nothing_raised { Select.scoped(:includes => [:groups]).to_a }
end
#the following functions were added to DRY test cases
@@ -130,7 +130,7 @@ def test_calculations_work_with_reserved_words
end
def test_associations_work_with_reserved_words
- assert_nothing_raised { Select.scoped(:includes => [:groups]).all }
+ assert_nothing_raised { Select.scoped(:includes => [:groups]).to_a }
end
#the following functions were added to DRY test cases
@@ -396,7 +396,7 @@ def test_custom_counter_cache
def test_association_assignment_sticks
post = Post.first
- author1, author2 = Author.scoped(:limit => 2).all
+ author1, author2 = Author.scoped(:limit => 2).to_a
assert_not_nil author1
assert_not_nil author2
@@ -16,15 +16,15 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase
:categorizations, :people, :categories, :edges, :vertices
def test_eager_association_loading_with_cascaded_two_levels
- authors = Author.scoped(:includes=>{:posts=>:comments}, :order=>"authors.id").all
+ authors = Author.scoped(:includes=>{:posts=>:comments}, :order=>"authors.id").to_a
assert_equal 3, authors.size
assert_equal 5, authors[0].posts.size
assert_equal 3, authors[1].posts.size
assert_equal 10, authors[0].posts.collect{|post| post.comments.size }.inject(0){|sum,i| sum+i}
end
def test_eager_association_loading_with_cascaded_two_levels_and_one_level
- authors = Author.scoped(:includes=>[{:posts=>:comments}, :categorizations], :order=>"authors.id").all
+ authors = Author.scoped(:includes=>[{:posts=>:comments}, :categorizations], :order=>"authors.id").to_a
assert_equal 3, authors.size
assert_equal 5, authors[0].posts.size
assert_equal 3, authors[1].posts.size
@@ -35,16 +35,16 @@ def test_eager_association_loading_with_cascaded_two_levels_and_one_level
def test_eager_association_loading_with_hmt_does_not_table_name_collide_when_joining_associations
assert_nothing_raised do
- Author.joins(:posts).eager_load(:comments).where(:posts => {:taggings_count => 1}).all
+ Author.joins(:posts).eager_load(:comments).where(:posts => {:taggings_count => 1}).to_a
end
- authors = Author.joins(:posts).eager_load(:comments).where(:posts => {:taggings_count => 1}).all
+ authors = Author.joins(:posts).eager_load(:comments).where(:posts => {:taggings_count => 1}).to_a
assert_equal 1, assert_no_queries { authors.size }
assert_equal 10, assert_no_queries { authors[0].comments.size }
end
def test_eager_association_loading_grafts_stashed_associations_to_correct_parent
assert_nothing_raised do
- Person.eager_load(:primary_contact => :primary_contact).where('primary_contacts_people_2.first_name = ?', 'Susan').order('people.id').all
+ Person.eager_load(:primary_contact => :primary_contact).where('primary_contacts_people_2.first_name = ?', 'Susan').order('people.id').to_a
end
assert_equal people(:michael), Person.eager_load(:primary_contact => :primary_contact).where('primary_contacts_people_2.first_name = ?', 'Susan').order('people.id').first
end
@@ -54,67 +54,67 @@ def test_cascaded_eager_association_loading_with_join_for_count
assert_nothing_raised do
assert_equal 4, categories.count
- assert_equal 4, categories.all.count
+ assert_equal 4, categories.to_a.count
assert_equal 3, categories.count(:distinct => true)
- assert_equal 3, categories.all.uniq.size # Must uniq since instantiating with inner joins will get dupes
+ assert_equal 3, categories.to_a.uniq.size # Must uniq since instantiating with inner joins will get dupes
end
end
def test_cascaded_eager_association_loading_with_duplicated_includes
categories = Category.includes(:categorizations).includes(:categorizations => :author).where("categorizations.id is not null").references(:categorizations)
assert_nothing_raised do
assert_equal 3, categories.count
- assert_equal 3, categories.all.size
+ assert_equal 3, categories.to_a.size
end
end
def test_cascaded_eager_association_loading_with_twice_includes_edge_cases
categories = Category.includes(:categorizations => :author).includes(:categorizations => :post).where("posts.id is not null").references(:posts)
assert_nothing_raised do
assert_equal 3, categories.count
- assert_equal 3, categories.all.size
+ assert_equal 3, categories.to_a.size
end
end
def test_eager_association_loading_with_join_for_count
authors = Author.joins(:special_posts).includes([:posts, :categorizations])
assert_nothing_raised { authors.count }
- assert_queries(3) { authors.all }
+ assert_queries(3) { authors.to_a }
end
def test_eager_association_loading_with_cascaded_two_levels_with_two_has_many_associations
- authors = Author.scoped(:includes=>{:posts=>[:comments, :categorizations]}, :order=>"authors.id").all
+ authors = Author.scoped(:includes=>{:posts=>[:comments, :categorizations]}, :order=>"authors.id").to_a
assert_equal 3, authors.size
assert_equal 5, authors[0].posts.size
assert_equal 3, authors[1].posts.size
assert_equal 10, authors[0].posts.collect{|post| post.comments.size }.inject(0){|sum,i| sum+i}
end
def test_eager_association_loading_with_cascaded_two_levels_and_self_table_reference
- authors = Author.scoped(:includes=>{:posts=>[:comments, :author]}, :order=>"authors.id").all
+ authors = Author.scoped(:includes=>{:posts=>[:comments, :author]}, :order=>"authors.id").to_a
assert_equal 3, authors.size
assert_equal 5, authors[0].posts.size
assert_equal authors(:david).name, authors[0].name
assert_equal [authors(:david).name], authors[0].posts.collect{|post| post.author.name}.uniq
end
def test_eager_association_loading_with_cascaded_two_levels_with_condition
- authors = Author.scoped(:includes=>{:posts=>:comments}, :where=>"authors.id=1", :order=>"authors.id").all
+ authors = Author.scoped(:includes=>{:posts=>:comments}, :where=>"authors.id=1", :order=>"authors.id").to_a
assert_equal 1, authors.size
assert_equal 5, authors[0].posts.size
end
def test_eager_association_loading_with_cascaded_three_levels_by_ping_pong
- firms = Firm.scoped(:includes=>{:account=>{:firm=>:account}}, :order=>"companies.id").all
+ firms = Firm.scoped(:includes=>{:account=>{:firm=>:account}}, :order=>"companies.id").to_a
assert_equal 2, firms.size
assert_equal firms.first.account, firms.first.account.firm.account
assert_equal companies(:first_firm).account, assert_no_queries { firms.first.account.firm.account }
assert_equal companies(:first_firm).account.firm.account, assert_no_queries { firms.first.account.firm.account }
end
def test_eager_association_loading_with_has_many_sti
- topics = Topic.scoped(:includes => :replies, :order => 'topics.id').all
+ topics = Topic.scoped(:includes => :replies, :order => 'topics.id').to_a
first, second, = topics(:first).replies.size, topics(:second).replies.size
assert_no_queries do
assert_equal first, topics[0].replies.size
@@ -127,15 +127,15 @@ def test_eager_association_loading_with_has_many_sti_and_subclasses
silly.parent_id = 1
assert silly.save
- topics = Topic.scoped(:includes => :replies, :order => ['topics.id', 'replies_topics.id']).all
+ topics = Topic.scoped(:includes => :replies, :order => ['topics.id', 'replies_topics.id']).to_a
assert_no_queries do
assert_equal 2, topics[0].replies.size
assert_equal 0, topics[1].replies.size
end
end
def test_eager_association_loading_with_belongs_to_sti
- replies = Reply.scoped(:includes => :topic, :order => 'topics.id').all
+ replies = Reply.scoped(:includes => :topic, :order => 'topics.id').to_a
assert replies.include?(topics(:second))
assert !replies.include?(topics(:first))
assert_equal topics(:first), assert_no_queries { replies.first.topic }
@@ -151,7 +151,7 @@ def test_eager_association_loading_with_multiple_stis_and_order
end
def test_eager_association_loading_of_stis_with_multiple_references
- authors = Author.scoped(:includes => { :posts => { :special_comments => { :post => [ :special_comments, :very_special_comment ] } } }, :order => 'comments.body, very_special_comments_posts.body', :where => 'posts.id = 4').all
+ authors = Author.scoped(:includes => { :posts => { :special_comments => { :post => [ :special_comments, :very_special_comment ] } } }, :order => 'comments.body, very_special_comments_posts.body', :where => 'posts.id = 4').to_a
assert_equal [authors(:david)], authors
assert_no_queries do
authors.first.posts.first.special_comments.first.post.special_comments
@@ -160,7 +160,7 @@ def test_eager_association_loading_of_stis_with_multiple_references
end
def test_eager_association_loading_where_first_level_returns_nil
- authors = Author.scoped(:includes => {:post_about_thinking => :comments}, :order => 'authors.id DESC').all
+ authors = Author.scoped(:includes => {:post_about_thinking => :comments}, :order => 'authors.id DESC').to_a
assert_equal [authors(:bob), authors(:mary), authors(:david)], authors
assert_no_queries do
authors[2].post_about_thinking.comments.first
@@ -92,7 +92,7 @@ def generate_test_object_graphs
end
def test_include_query
- res = ShapeExpression.scoped(:includes => [ :shape, { :paint => :non_poly } ]).all
+ res = ShapeExpression.scoped(:includes => [ :shape, { :paint => :non_poly } ]).to_a
assert_equal NUM_SHAPE_EXPRESSIONS, res.size
assert_queries(0) do
res.each do |se|
@@ -103,43 +103,43 @@ def teardown
def test_eager_no_extra_singularization_belongs_to
return unless @have_tables
assert_nothing_raised do
- Virus.scoped(:includes => :octopus).all
+ Virus.scoped(:includes => :octopus).to_a
end
end
def test_eager_no_extra_singularization_has_one
return unless @have_tables
assert_nothing_raised do
- Octopus.scoped(:includes => :virus).all
+ Octopus.scoped(:includes => :virus).to_a
end
end
def test_eager_no_extra_singularization_has_many
return unless @have_tables
assert_nothing_raised do
- Bus.scoped(:includes => :passes).all
+ Bus.scoped(:includes => :passes).to_a
end
end
def test_eager_no_extra_singularization_has_and_belongs_to_many
return unless @have_tables
assert_nothing_raised do
- Crisis.scoped(:includes => :messes).all
- Mess.scoped(:includes => :crises).all
+ Crisis.scoped(:includes => :messes).to_a
+ Mess.scoped(:includes => :crises).to_a
end
end
def test_eager_no_extra_singularization_has_many_through_belongs_to
return unless @have_tables
assert_nothing_raised do
- Crisis.scoped(:includes => :successes).all
+ Crisis.scoped(:includes => :successes).to_a
end
end
def test_eager_no_extra_singularization_has_many_through_has_many
return unless @have_tables
assert_nothing_raised do
- Crisis.scoped(:includes => :compresses).all
+ Crisis.scoped(:includes => :compresses).to_a
end
end
end
Oops, something went wrong. Retry.

12 comments on commit 6a81ccd

@pixeltrix
Member

What's the reasoning behind this change?

@fxn
Member
fxn commented on 6a81ccd Jul 30, 2012

Yeah, we need more rationales in commit messages.

@jonleighton
Member

The rationale is there: #all returning an array is confusing. If you want an array call #to_a.

@fxn
Member
fxn commented on 6a81ccd Jul 30, 2012

Well, it has been used for about 8 years that way without a complain, I don't think "is confusing" just that way is enough for who inspects the history of the project 6 months later to understand that commit.

But anyway, the commit message does not explain a rationale, it just asserts how things are from this point on.

@pixeltrix
Member

How is #all confusing? It seems obvious to me that it returns all records matching the scope - after this commit there's no difference between Post.where(:foo => 'bar') and Post.where(:foo => 'bar').all. If for some reason you need to run the query before rendering you're going to be littering your controller with #to_a.

@jonleighton
Member

@fxn sorry you found the commit message to be lacking. I do try to make useful commit messages...

Before making this change, I brought up the issue in the rails core basecamp. This is what I said:

Currently the behaviour of Relation#all is to return an array.

In Rails 4, I would like to change it to return self. We could also add a Base.to_a method so e.g. Post.to_a would do what Post.all currently does.

Here are my reasons:

  • to_a is much more explicit about what is returned than all
  • basically every other relation method returns a relation rather than an array. it's surprising that #all returns an array, and essentially a historical relic of find(:all)
  • calling e.g. Post.scoped doesn't read nearly as well as Post.all to me
  • We have e.g. Post.delete_all and Post.destroy_all at the moment. I'd like it if we could do e.g. Post.all.destroy or Post.where(draft: true).destroy etc, and then just remove those extra methods.

I got 3 +1's, from David, Santiago and Aaron. I didn't hear any objections, so I went ahead and made the change.

@henning-koch

How will this "not break existing code"? Consciously choosing when to load a scope into an array is an important part of working with ActiveRecord. This change breaks a long-standing and commonly used API method for a personal and very debatable preference in naming.

@styx
styx commented on 6a81ccd Aug 8, 2012

I agree with @jonleighton, it is much more logical, when functionality related to the same scope works in the one common manner. Preciously in Rails if where was finding just one record we got a single object and if more than one then we got an array. That was confusing. all is also confusing.

@pixeltrix

If for some reason you need to run the query before rendering you're going to be littering your controller with #to_a.

Previously that was .all, now it will .to_a there is no regression here.

after this commit there's no difference between Post.where(:foo => 'bar') and Post.where(:foo => 'bar').all

And what's the problem?

@henning-koch

How will this "not break existing code"?

I do believe this can break the code only in one situation:

puts "Mu-Ha-Ha" if Post.all.is_a? Array
@henning-koch

You are arguing away the impact of this API-breaking change by saying that relations pretend to be arrays anyway. If required (e.g. someone calls relation[3]) the relation will just load scoped records into an internal array and delegate method calls to that. So it should all be transparent and not affect existing code, right?

Unfortunately the relation/array mimicry is a bad abstraction that leaks left and right. Just some examples:

  • subsequent calls of #first and #last on the same relation triggers multiple queries
  • Relation#count triggers a new database query every single call.
  • There are many methods that exist in both Array and ActiveRecord::Relation that do completely different things. Relation#sum and Array#sum have different arguments. Array#delete removes an object from the array, Relation#delete deletes a row in the database!

I agree that the simple "index action lists stuff" scenario is probably not affected by this change. But once you move away from this, you will see many instances where this change causes problems. If you look at the less trivial use cases for ActiveRecord you will see a pattern like the following over and over again:

  • Construct relation chains without touching the database for as long as possible
  • Explictely load the relation into an array (by calling #all)
  • Further process the array in Ruby-world
  • Return results

Code like this will probably break or cause heavy DB load as the result of this change. We shouldn't silently change the (significant) effect of a method call without raising an error or deprecation.

@JeanMertz

If this is a Rails 4 feature then I like this change. I would advice giving the users of 3.x ample warnings of this change, but I guess a deprecation warning wouldn't really fit here.

@korny
korny commented on 6a81ccd Oct 7, 2012

It seems we were all using all to say something like "load", "get_selected_rows", "make_the_query". While I agree that all is a bad name for this, and should be changed as proposed, I'm not sure that to_a is a better name. To me, to_* methods in Ruby are for converting without side effects. I guess this is one reason why people have been using all in favor of to_a.

How about introducing a new alias for to_a that makes it clear what it does? Returning an array is one thing, sure, of course it returns an array. But making a query is much more crucial (especially when thinking about caching and performance), so I would like to write something like Post.where(public: true).get.

@korny
korny commented on 6a81ccd Oct 7, 2012

Oops, I should have read newer commits, now there's load. Thanks ;-)

Please sign in to comment.