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

Ticket #8801 (closed defect: fixed)

Opened 1 year ago

Last modified 1 year ago

[PATCH] has_many :through collections.count returns wrong count when :uniq => true

Reported by: lifofifo Assigned to: nzkoz
Priority: high Milestone: 1.x
Component: ActiveRecord Version: edge
Severity: normal Keywords: hmt verified needs_review
Cc:

Description

has_many :through collections.count ignores :uniq => true option and counts duplicate records too.

The patch fixes it and writes a test.

Thanks.

Attachments

hmt_collection_count_uniq.patch (1.2 kB) - added by lifofifo on 06/29/07 01:07:50.
has_many :through collections.count for :uniq => true
hmt_collection_count_uniq.2.patch (2.1 kB) - added by lifofifo on 07/15/07 18:51:39.
Use count(distinct id) query for hmt uniq collection count
has_many_through_uniq_count.patch (2.1 kB) - added by lifofifo on 07/19/07 14:58:04.
Updated against latest edge
1-2-stable.diff (2.1 kB) - added by court3nay on 08/17/07 20:22:09.
re-patched for STABLE

Change History

06/29/07 01:07:50 changed by lifofifo

  • attachment hmt_collection_count_uniq.patch added.

has_many :through collections.count for :uniq => true

07/01/07 23:52:06 changed by lifofifo

  • owner changed from core to nzkoz.

07/15/07 18:51:39 changed by lifofifo

  • attachment hmt_collection_count_uniq.2.patch added.

Use count(distinct id) query for hmt uniq collection count

07/15/07 18:52:31 changed by lifofifo

I've uploaded a new patch, getting rid of association loading for htm uniq count().

Thanks.

07/19/07 14:58:04 changed by lifofifo

  • attachment has_many_through_uniq_count.patch added.

Updated against latest edge

07/19/07 15:47:40 changed by norbert

The patch applies cleanly, the code looks good and the tests pass. +1

07/19/07 20:24:21 changed by alloy

Definitely looks good to me. +1

07/19/07 22:24:11 changed by mpalmer

  • keywords changed from hmt to hmt verified.

Patch applies cleanly, test case looks good and does what it should, code modification looks good and fixes the test. +1 from me.

07/20/07 00:10:10 changed by rick

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

(In [7199]) Fix #count on a has_many :through association so that it recognizes the :uniq option. Closes #8801 [lifofifo]

08/15/07 08:25:14 changed by matt

This patch seems to have created another problem.

Let's say you have that relationship in your model" 

has_many :unique_ips, :class_name => 'View', :select => "distinct ip", :conditions => "views.ip IS NOT NULL AND views.ip != '' "

@object.unique_ips will work fine but if you run @object.unique_ips.count before you call @object.unique_ips, the :select param is just skipped and you get a count(*)

I don't really have time now to create an example, failing tests and patch but I thought I should let you know.

08/17/07 20:08:40 changed by court3nay

  • keywords changed from hmt verified to hmt verified needs_review.
  • status changed from closed to reopened.
  • resolution deleted.

backport to STABLE pls?

08/17/07 20:22:09 changed by court3nay

  • attachment 1-2-stable.diff added.

re-patched for STABLE

08/17/07 20:27:17 changed by bryanl

+1 for the 1-2-stable.diff

08/17/07 20:30:51 changed by matt

+1 I guess, even if this patch isn't fully working, it should work in 80% of cases.

08/17/07 20:33:10 changed by lifofifo

matt: This is a has_many :through, :uniq => true related patch. Your example doesn't reflect that. You might wanna check it again.

08/20/07 04:26:14 changed by nzkoz

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [7347]) Merge 8801 fix to stable. Closes #8801