Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Count returns 0 without querying if parent is not saved #6978

Merged
merged 1 commit into from

3 participants

@frodsan

Patches CollectionAssociation#count to return 0 without querying
if the parent record is new. Consider the following code:

class Account
  has_many :dossiers
end

class Dossier
  belongs_to :account
end

a = Account.new
a.dossiers.build

# before patch
a.dossiers.count
# SELECT COUNT(*) FROM "dossiers" WHERE "dossiers"."account_id" IS NULL
# => 0

# after
a.dosiers.count # fires without sql query
# => 0

Fixes #1856.

@rafaelfranca
Owner

Is not a good solution call length or size when owner is new_record?.

cc/ @tenderlove

@carlosantoniodasilva

Just want to point out that count may be only the tip of the iceberg, #5215 shows that all finders and scopes executed against an association actually execute a query, when they probably shouldn't.

@frodsan

@rafaelfranca I think size or length have a different behaviour:

>> a = User.new
=> #<User id: nil, name: nil, created_at: nil, updated_at: nil>
>> a.things.length
=> 0
>> a.things.new
=> #<Thing id: nil, user_id: nil, created_at: nil, updated_at: nil>
>> a.things.length
=> 1
>> a.things.count
   (0.3ms)  SELECT COUNT(*) FROM "things" WHERE "things"."user_id" = $1  [["user_id", nil]]
=> 0
@carlosantoniodasilva

@frodsan they have:

  • length with always load the objects and use Array#length
  • count will always do a SQL count
  • size will check whether the collection is loaded, and use it, otherwise will do a count.

size should give you the correct value in this case I think.

@rafaelfranca

Yes. I think count should always do a SQL count. In the case you pointed I think you should use size

@frodsan

updated :)

@rafaelfranca
Owner

I was thinking about this and I think is better to return 0.

size can return a different value and is expected to count do a SQL query, so if the we call count in a new record we should always return 0 because we don't have any data in the database.

@rafaelfranca
Owner

Good. Changelog entry please

@frodsan

done :)

activerecord/CHANGELOG.md
@@ -1,5 +1,22 @@
## Rails 4.0.0 (unreleased) ##
+* `CollectionAssociation#count` returns 0 without querying if the
+ parent record is new.
+
+ Before:
+
+ person.pets
+ # SELECT COUNT(*) FROM "pets" ...
@rafaelfranca Owner

is better to put the full sql with the WHERE people.id IS NULL (I don't know if this is the right SQL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Francesco Rodriguez Count returns 0 without querying if parent is not saved
Patches `CollectionAssociation#count` to return 0 without querying
if the parent record is new. Consider the following code:

    class Account
      has_many :dossiers
    end

    class Dossier
      belongs_to :account
    end

    a = Account.new
    a.dossiers.build

    # before patch
    a.dossiers.count
    # SELECT COUNT(*) FROM "dossiers" WHERE "dossiers"."account_id" IS NULL
    # => 0

    # after
    a.dosiers.count # fires without sql query
    # => 0

Fixes #1856.
aa202ad
@rafaelfranca rafaelfranca merged commit 3a63fe3 into rails:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 3, 2012
  1. Count returns 0 without querying if parent is not saved

    Francesco Rodriguez authored
    Patches `CollectionAssociation#count` to return 0 without querying
    if the parent record is new. Consider the following code:
    
        class Account
          has_many :dossiers
        end
    
        class Dossier
          belongs_to :account
        end
    
        a = Account.new
        a.dossiers.build
    
        # before patch
        a.dossiers.count
        # SELECT COUNT(*) FROM "dossiers" WHERE "dossiers"."account_id" IS NULL
        # => 0
    
        # after
        a.dosiers.count # fires without sql query
        # => 0
    
    Fixes #1856.
This page is out of date. Refresh to see the latest.
View
17 activerecord/CHANGELOG.md
@@ -1,5 +1,22 @@
## Rails 4.0.0 (unreleased) ##
+* `CollectionAssociation#count` returns 0 without querying if the
+ parent record is new.
+
+ Before:
+
+ person.pets
+ # SELECT COUNT(*) FROM "pets" WHERE "pets"."person_id" IS NULL
+ # => 0
+
+ After:
+
+ person.pets
+ # fires without sql query
+ # => 0
+
+ *Francesco Rodriguez*
+
* Fix `reset_counters` crashing on `has_many :through` associations.
Fix #7822.
View
2  activerecord/lib/active_record/associations/collection_association.rb
@@ -174,6 +174,8 @@ def sum(*args)
# association, it will be used for the query. Otherwise, construct options and pass them with
# scope to the target class's +count+.
def count(column_name = nil, count_options = {})
+ return 0 if owner.new_record?
+
column_name, count_options = nil, column_name if column_name.is_a?(Hash)
if options[:counter_sql] || options[:finder_sql]
View
6 activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
@@ -799,6 +799,12 @@ def test_count_with_counter_sql
assert_equal 1, developer.projects.count
end
+ def test_counting_should_not_fire_sql_if_parent_is_unsaved
+ assert_no_queries do
+ assert_equal 0, Developer.new.projects.count
+ end
+ end
+
unless current_adapter?(:PostgreSQLAdapter)
def test_count_with_finder_sql
assert_equal 3, projects(:active_record).developers_with_finder_sql.count
View
6 activerecord/test/cases/associations/has_many_associations_test.rb
@@ -262,6 +262,12 @@ def test_counting_with_association_limit
assert_equal firm.limited_clients.length, firm.limited_clients.count
end
+ def test_counting_should_not_fire_sql_if_parent_is_unsaved
+ assert_no_queries do
+ assert_equal 0, Person.new.readers.count
+ end
+ end
+
def test_finding
assert_equal 2, Firm.all.merge!(:order => "id").first.clients.length
end
View
6 activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -767,6 +767,12 @@ def test_count_has_many_through_with_named_scope
assert_equal 1, authors(:mary).categories.general.count
end
+ def test_counting_should_not_fire_sql_if_parent_is_unsaved
+ assert_no_queries do
+ assert_equal 0, Person.new.posts.count
+ end
+ end
+
def test_has_many_through_belongs_to_should_update_when_the_through_foreign_key_changes
post = posts(:eager_other)
Something went wrong with that request. Please try again.