Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Merge branch 'rm-fix-13648'

Includes #14711 and some cleanup
commits.

Fixes #13648

Conflicts:
	activerecord/CHANGELOG.md
  • Loading branch information...
commit 968c581ea34b5236af14805e6a77913b1cb36238 1 parent 7f6bd0a
@rafaelfranca rafaelfranca authored
View
7 activerecord/CHANGELOG.md
@@ -1,3 +1,10 @@
+* Fixed error for aggregate methods (`empty?`, `any?`, `count`) with `select`
+ which created invalid SQL.
+
+ Fixes #13648.
+
+ *Simon Woker*
+
* Fix insertion of records via `has_many :through` association with scope.
Fixes #3548.
View
8 activerecord/lib/active_record/association_relation.rb
@@ -9,6 +9,14 @@ def proxy_association
@association
end
+ def size
+ @association.size
+ end
+
+ def empty?
+ @association.empty?
+ end
+
private
def exec_queries
View
5 activerecord/lib/active_record/relation.rb
@@ -238,7 +238,7 @@ def as_json(options = nil) #:nodoc:
# Returns size of the records.
def size
- loaded? ? @records.length : count
+ loaded? ? @records.length : count(:all)
end
# Returns true if there are no records.
@@ -248,8 +248,7 @@ def empty?
if limit_value == 0
true
else
- # FIXME: This count is not compatible with #select('authors.*') or other select narrows
- c = count
+ c = count(:all)
c.respond_to?(:zero?) ? c.zero? : c.empty?
end
end
View
8 activerecord/test/cases/associations/eager_test.rb
@@ -1198,6 +1198,14 @@ def test_deep_including_through_habtm
assert_equal authors(:bob), author
end
+ test "preloading with a polymorphic association and using the existential predicate but also using a select" do
+ assert_equal authors(:david), authors(:david).essays.includes(:writer).first.writer
+
+ assert_nothing_raised do
+ authors(:david).essays.includes(:writer).select(:name).any?
+ end
+ end
+
test "preloading with a polymorphic association and using the existential predicate" do
assert_equal authors(:david), authors(:david).essays.includes(:writer).first.writer
View
10 activerecord/test/cases/relations_test.rb
@@ -800,6 +800,16 @@ def test_delete_all_limit_error
assert_raises(ActiveRecord::ActiveRecordError) { Author.limit(10).delete_all }
end
+ def test_select_with_aggregates
+ posts = Post.select(:title, :body)
+
+ assert_equal 11, posts.count(:all)
+ assert_equal 11, posts.size
+ assert posts.any?
+ assert posts.many?
+ assert_not posts.empty?
+ end
+
def test_select_takes_a_variable_list_of_args
david = developers(:david)

1 comment on commit 968c581

@laurocaetano
Collaborator

I think it is not the proper fix, because it will break the AssociationRelation behavior. See #14744
Also, the test test_select_with_aggregates is not testing this scenario:

posts = Post.select(:title, :body).where(title: 'Some inexistent Title')
assert_equal 0, posts.count

Then :bomb:

RelationTest#test_select_with_aggregates:
ActiveRecord::StatementInvalid: SQLite3::SQLException: wrong number of arguments to function COUNT(): SELECT COUNT(title, body) FROM "posts"  WHERE "posts"."title" = 'Some inexistent Title'

Also, the count and empty methods defined in association_relation are doing the wrong thing. Lets say I have this scenario:

author = Author.last
author.posts.where(title: 'Some Title').count

What would be the expected sql?

SELECT COUNT(*) FROM "posts"  WHERE "posts"."user_id" = ? AND "posts"."title" = "Some Title"

but instead, it is doing:

SELECT COUNT(*) FROM "posts"  WHERE "posts"."user_id" = ?

because the method count does the calculation on the@association, which in this case will be posts and this will ignore the where clause.

Btw, I'm trying to figure out how this could work for both cases.

cc/ @rafaelfranca

Please sign in to comment.
Something went wrong with that request. Please try again.