#1652 named_scope incorrectly uses SQL COUNT for empty?, size methods when :select option is in use - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#1652 ✓hold

named_scope incorrectly uses SQL COUNT for empty?, size methods when :select option is in use

Reported by Wolfram Arnold | December 28th, 2008 @ 12:48 AM | in 2.x

When using the :select option for named_scope, the implementation incorrectly assumes that an invocation of .empty? or .size should use the count method when no data has been loaded yet.

Consider this example:

class Post < ActiveRecord::Base ...

named_scope :most_commented,
            Proc.new { |limit, offset| { :select => 'posts.*, COUNT(*) AS comment_count',
                                         :joins => :comments, 
                                         :limit => limit,
                                         :group => 'posts.id',
                                         :order => 'comment_count DESC, comments.id ASC',
                                         :offset => offset
                                       } }
...

end

The intention here is to add a named scope that returns the N most commented posts.

If the first invocation is this (i.e. no data has been loaded yet):

Post.most_commented(3,0).empty?

then the current implementation will generate the following SQL statement:

SELECT count(*) AS count_all FROM posts INNER JOIN comments ON comments.post_id = posts.id LIMIT 0, 3

which is incorrect in that it ignores the :select option and yields the wrong result in this example.

I've added 3 new tests; one to verify that the :select option is honored, and two to check the described defect behavior.

I've then fixed this such that IF the :select option is set, then the .size and .empty? methods will not default to the count method if no data has been loaded. Patch is attached.

Comments and changes to this ticket

You flagged this item as spam.
Wolfram Arnold December 28th, 2008 @ 01:18 AM
- Tag changed from “2.2.2, named_scope” to “2.2.2, activerecord, named_scope, patch, tested”
Patch attached
Xavier Noria December 28th, 2008 @ 09:43 AM
In documentation a "named scope" (regular font, English, has a space) is the entity you build with the macro named_scope.

Also, the backslashes are needed in "\scope" because otherwise rdoc autolinks to the scope method. A recent rdoc is needed to interpret them.

I have not examined the rest of the patch by now, but would you please revert those doc changes except for the new stuff which is specific to :select?
Frederick Cheung December 28th, 2008 @ 10:53 AM
Would this not mean that your might unncessarily load the collection if you were just using the :select to piggy back an attribute ? Is the problem not also that the group by clause is dropped by the counting ?
You flagged this item as spam.
Wolfram Arnold December 28th, 2008 @ 06:21 PM
In response to Frederick Cheung's comment: The second question first: Yes, it is correct that the problem is also that the group clause gets dropped. This is taken care of, because by not running .count (which ignores the :group clause), the :group clause does take effect correctly in conjunction with the :select override.

Regarding the first question: In this case I am specifically intending to load the collection (:comments), because I want to display the comments on the same page that shows the counts (a sort of statistics/tracking page), but I realize that this intention is probably not clear from the example provided.
Frederick Cheung December 28th, 2008 @ 06:36 PM
What I was getting at was would it not be cleaner to make the :group option be used?
Frederick Cheung December 28th, 2008 @ 07:15 PM
- Assigned user changed from “Pratik” to “Frederick Cheung”
You flagged this item as spam.
Wolfram Arnold December 28th, 2008 @ 08:52 PM
In response to Xavier Noria: Thanks for pointing out the RDoc side-effects. I'm attaching a new patch that does NOT modify the existing documentation, it only adds a small note about :select and .count, and contains otherwise only the code changes and tests.

To Frederick Cheung: This is an interesting idea that I hadn't though about. I'll give it a try.
- patch_2008_12_28_named_scope_empty_fix_doc_fixes.diff 4.5 KB
You flagged this item as spam.
Wolfram Arnold December 28th, 2008 @ 10:50 PM
Frederick, I've pursued your idea. The real bug, I think was, that the size and empty? methods called count() without passing any of the @proxy_option arguments.

The latest patch, attached, passes the arguments except :select and :order. :select would fail because the count() method wraps the string passed to :select into a SELECT COUNT(). The :order clause fails in my example, because it makes reference to a string defined in the :select clause. Technically the :group clause could do the same. It's still not quite fool-proof, but tests pass and I think it's an improvement over the previous situation.
- patch_2008_12_28_named_scope_pass_options_to_count_for_empty_and_size.diff 4.4 KB
You flagged this item as spam.
Wolfram Arnold January 7th, 2009 @ 11:41 AM
Hey Frderick, any chance to review this yet? I've implemented your idea, it comes with tests and a sentence of documentation. I'd appreciate inclusion. The code as currently written has issues, because none of the arguments are passed to the count method. The previous path applied, to make the size behavior as with associations, caused this problem, and the problem didn't exist before that patch.
Frederick Cheung January 7th, 2009 @ 11:46 AM
I'm curious - what's with the checking that the result of count is a Numeric - why is this change necessary
You flagged this item as spam.
Wolfram Arnold January 7th, 2009 @ 04:05 PM
Good point--I should have put a source comment in. The result of count is not always a Numeric, it can be an "ordered Hash" (new in Rails 2.2, I think), when you use the group_by clause, see: http://api.rubyonrails.org/class...

For the present purposes we only care about the total length, not the groupings, hence the need to check for the return value.

The reason this change is necessary (I presume you are asking why the whole patch in its entirety is needed) is explained in the main part of this bug report, namely when you use :select or :group_by in named_scope, then the results returned by .size can be wrong. The behavior was correct prior to the optimization in commit: 7f179f8540ab92dbd9d3e650b465de5b694d93d4 by tomstuart on 8/29/08, http://github.com/rails/rails/co...

These optimizations are generally good, but they forgot to pass the query arguments to the count method that can have a material impact on the returned result.

Let me know if you have other concerns or if you have ideas how I can make this simpler still.
Frederick Cheung January 7th, 2009 @ 04:15 PM
I was only asking about the check on Numeric - that makes sense (the ordered hash stuff isn't new in 2.2 - that goes way back)
Frederick Cheung January 7th, 2009 @ 04:16 PM
I also think your tests could be a bit more direct: why not also assert that size/empty? methods return the correct value (after all, that is fairly important!)
You flagged this item as spam.
Wolfram Arnold January 7th, 2009 @ 05:29 PM
On the tests, I think I am checking that. There is one test the asserts !empty? and another test that assert size==3. Maybe I'm not understanding your question?
Frederick Cheung January 7th, 2009 @ 05:43 PM
Sorry, just me ready too quickly, didn;t see it was at the end of the assert_sql lines.
You flagged this item as spam.
Wolfram Arnold January 15th, 2009 @ 07:52 PM
Hey Frederick, just checking in with you. Is this patch good to submit or, if not, what is holding it up/what else do you want to see?

Thanks,

Wolf
Frederick Cheung January 15th, 2009 @ 07:58 PM
Something I was discussing with Pratik the other day was whether this was enough of an edge case that named_scope should just have a counter_sql option in the way that has_many has for example. Ultimately I'm not on the core team, so it's not my call :-) You may find that popping onto the rails-contrib IRC channel or the rubyonrails-core mailing list might gather some opnions, favourable or not.
You flagged this item as spam.
Wolfram Arnold January 16th, 2009 @ 07:04 PM
Oh you know, that's it. I hadn't run into that case. Thanks for pointing that out. I'll run this by the rails-contrib list and prepare a new patch, but I feel confident that this is the right solution. The problem arose in the first place by changes to make named_scope behave more like the associations. Thus borrowing more from the association semantics to fix the problems strikes me as the right move.
Pratik March 12th, 2009 @ 02:57 PM
- State changed from “new” to “hold”
You flagged this item as spam.
Anthony Richardson May 11th, 2009 @ 06:20 AM
I'm coming up against this bug as well. Changing count to support the select parameters, or adding :counter_sql would be greatly appreciated.
You flagged this item as spam.
Chris Hapgood June 3rd, 2010 @ 07:50 PM
Regarding the wisdom of having named scopes manage counting SQL independently...

Imagine a scope that handles pagination via :limit and :offset options

Shirts.red.page(3).all => array of ten red shirts
Shirts.red.page(3).count => 0

WFT? The problem is that using OFFSET in SQL results in a count(*) of zero, unconditionally (at least with SQLite3).

Without a separation of counting SQL from finding SQL, this kind of named scope (paginated) is a problem.

-Chris
Ryan Bigg October 9th, 2010 @ 10:13 PM
- Tag cleared.
- Importance changed from “” to “Low”
Automatic cleanup of spam.
Ryan Bigg October 21st, 2010 @ 03:35 AM
Automatic cleanup of spam.