Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #2597 (closed enhancement: duplicate)

Opened 3 years ago

Last modified 2 years ago

kakalu.com

Reported by: daniel@itxl.nl Assigned to: David
Priority: normal Milestone: 1.0
Component: ActionPack Version: 0.14.1
Severity: major Keywords: kakalu.com
Cc: kakalu.com

Description

Table : contacts, customers contact belongs_to :customer

When using the following paginate :

@contact_pages, @contacts = paginate :contact, :conditions =>'contacts.name like "b%" and customers.name like "k%"' ,:include=>:customer,:per_page => 20, :order=>sort_clause

the :include is only used in fetching the data the count fails : Unknown table 'customers' in where clause: SELECT COUNT(*) FROM contacts WHERE (contacts.name like 'b%' and customers.name like 'k%')

But the documentation claims : :include: optional eager loading parameter passed to Model.find(:all, *params) and Model.count

Attachments

counting_and_limiting_for_named_associations.patch (12.2 kB) - added by jeremy@jthopple.com on 12/10/05 23:55:30.
counting_and_limiting_for_named_associations.2.patch (12.2 kB) - added by jeremy@jthopple.com on 12/12/05 16:41:53.
Minor change. AR unit tests pass in MySQL and Postgres.
counting_and_limiting_for_named_associations.3.patch (13.8 kB) - added by jeremy@jthopple.com on 12/12/05 22:58:20.
All unit tests now pass in MySQL, Postgres, and Sqlite3

Change History

12/10/05 23:54:20 changed by jeremy@jthopple.com

I have uploaded a patch that fixes paginate so that it works, as advertised, with named associations.

Paginate with named associations (eager loading) did now work for the following reasons:

1. Paginate uses Model.count to count the collection for pagination and Model.count did not support named associations.

2. Paginate relies on selecting a set of limited ids so that it can select the correct rows for each page. This method did not work when eager conditions were included.

I had to change the parameter list for Model.count, but I made sure that it works using the old (conditions=nil, joins=nil) parameters for backwards compatibility. One thing to note is that in order for this to work the following has to be true:

Authors.find(:all, :conditions => "books.category = 'rails'", :include => :books).size == Authors.count(:conditions => "books.category = 'rails'", :include => :books)

So, count_with_associations must return the number of distinct Authors matching the conditions and named associations so that it is consistent with find_with_associations.

Along the same lines, I had to change the selecting of limited ids to return a set of distinct ids when using eager conditions. Previously, this method threw an ArgumentError saying "Limited eager loads and conditions on the eager tables is incompatible". By returning a distinct set of ids (when using eager conditions), limited eager loads and conditions are no longer incompatible.

I have run all the active record unit tests and they all pass in MySQL 5.0.15-standard. The only other database I have to try is sqlite3, and the count_with_associations tests fail because sqlite does not support DISTINCT when using COUNT. Does anyone know if this syntax is problematic in any other databases? I'll try to install Postgres and re-run the tests later this weekend.

I think this patch is a great enhancement for rails, but we need to make sure that it's going to work everywhere. This may mean pushing the implementation of this functionality down into the connection_adapters. With the failures in sqlite, I'm not sure this patch is ready for prime-time yet, but I wanted to get it out there and see what everyone thinks. Any thoughts?

12/10/05 23:55:30 changed by jeremy@jthopple.com

  • attachment counting_and_limiting_for_named_associations.patch added.

12/11/05 14:57:55 changed by anonymous

  • summary changed from paginate :include isn't used in Model.count to [patch] paginate :include isn't used in Model.count.

12/12/05 16:41:53 changed by jeremy@jthopple.com

  • attachment counting_and_limiting_for_named_associations.2.patch added.

Minor change. AR unit tests pass in MySQL and Postgres.

12/12/05 16:43:41 changed by jeremy@jthopple.com

All unit tests pass for MySQL and Postgres.

12/12/05 17:46:47 changed by jeremy@jthopple.com

I created a RailsPlugin with these changes in case anyone is interested wants to use this now.

12/12/05 22:58:20 changed by jeremy@jthopple.com

  • attachment counting_and_limiting_for_named_associations.3.patch added.

All unit tests now pass in MySQL, Postgres, and Sqlite3

12/12/05 23:11:25 changed by jeremy@jthopple.com

  • type changed from defect to enhancement.

I had to do a bit of a workaround to get the unit tests to pass using sqlite3. Sqlite3 does not support COUNT DISTINCT syntax like: 

SELECT COUNT(DISTINCT posts.id) FROM posts LEFT JOIN authors on authors.post_id = posts.id where authors.name = 'david'

As a workaround, I added the following method to AbstractAdapter: 

# Does this adapter support using DISTINCT within COUNT?  This is +true+
# for all adapters except sqlite.
def supports_count_distinct?
  true
end

And override it so that it returns false in the SQLiteAdapter.

If the backend connection adapter returns false for supports_count_distinct, then we have to do the following sql instead of COUNT DISTINCT: 

SELECT COUNT(*) FROM (SELECT DISTINCT posts.id FROM posts LEFT JOIN authors on authors.post_id = posts.id where authors.name = 'david')

12/12/05 23:13:25 changed by anonymous

  • summary changed from [patch] paginate :include isn't used in Model.count to [patch] Add support for :include to ActiveRecord::Base.count (was paginate :include isn't used in Model.count).

12/12/05 23:13:55 changed by anonymous

  • keywords changed from paginate include count to paginate include Model.count.

12/13/05 00:33:59 changed by bitsweat

  • cc set to bitsweat.
  • milestone set to 1.0.

12/13/05 19:00:34 changed by anonymous

  • cc changed from bitsweat to bitsweat, jeremy@jthopple.com.

01/02/06 01:26:34 changed by anonymous

can it be added in the current trunk pleaaaaaaaaaaaaaase ? :)

01/25/06 23:44:56 changed by Jeremy Hopple

Any chance of getting a review of this patch??? A fair number of people have been using the plugin[1] I put together that wraps this patch and have been happy with it...

[1] http://wiki.rubyonrails.org/rails/pages/Count+Limit+Associations+Plugin

02/09/06 08:56:02 changed by kevinclark

  • status changed from new to closed.
  • resolution set to duplicate.

Supersceded by ticket 3779

07/24/06 03:12:08 changed by anonymous

boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke