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

Ticket #10480 (closed defect: fixed)

Opened 2 years ago

Last modified 1 year ago

[PATCH] has_many through count using group doesn't use distinct

Reported by: remvee Assigned to: core
Priority: normal Milestone: 2.1
Component: ActiveRecord Version: edge
Severity: normal Keywords:
Cc:

Description

A has_many through association including the group option doesn't use the group when counting. Meaning person.grouped_relations.count will return a different value than person.grouped_relations.length. This first will construct a query without taking group into account and the latter will properly fetch all relations and return the length of the result set.

I have attached a fix for this defect.

Attachments

group_has_many_through_should_use_group_for_count.diff (3.1 kB) - added by remvee on 12/12/07 08:32:48.
group_has_many_through_should_use_group_for_count2.diff (3.9 kB) - added by remvee on 12/12/07 10:19:46.
introduced custom error
group_has_many_through_should_use_group_for_count3.diff (3.8 kB) - added by remvee on 01/23/08 22:27:49.
new patch for current trunk (revision 8710)

Change History

12/12/07 08:32:48 changed by remvee

  • attachment group_has_many_through_should_use_group_for_count.diff added.

(follow-up: ↓ 3 ) 12/12/07 08:45:31 changed by evolving_jerk

+1 but I think it would be nice to change exception from ConfigurationError to something more consistent with existing HasManyThrough* exceptions.

12/12/07 09:13:50 changed by mdemare

+1

(in reply to: ↑ 1 ) 12/12/07 09:35:36 changed by remvee

Replying to evolving_jerk:

+1 but I think it would be nice to change exception from ConfigurationError to something more consistent with existing HasManyThrough* exceptions.

None of the existing HasManyThrough errors seem to apply and I don't dare to introduce a new one.

12/12/07 09:54:20 changed by evolving_jerk

But ConfigurationError is used for completely different purposes across the codebase. Consider adding a new exception, please.

12/12/07 10:19:46 changed by remvee

  • attachment group_has_many_through_should_use_group_for_count2.diff added.

introduced custom error

12/12/07 10:20:48 changed by remvee

I've updated the patch to introduce and use a custom error.

12/12/07 10:25:11 changed by evolving_jerk

Looks better, thanks!

01/03/08 13:36:48 changed by danger

  • keywords set to verified.

+1

Great work!

01/23/08 22:27:49 changed by remvee

  • attachment group_has_many_through_should_use_group_for_count3.diff added.

new patch for current trunk (revision 8710)

01/23/08 22:32:45 changed by remvee

Made a new patch to anticipate Changeset [8681].

(follow-up: ↓ 10 ) 01/25/08 08:00:24 changed by manfred

Maybe I'm a bit late to the ticket but isn't HasManyThroughCantCountOnColumnForGroupedAssociation a rather silly name for an exception? Doesn't a InvalidQuery or something with a good message in the exception work a lot better?

(in reply to: ↑ 9 ) 01/25/08 08:20:28 changed by remvee

Replying to manfred:

Maybe I'm a bit late to the ticket but isn't HasManyThroughCantCountOnColumnForGroupedAssociation a rather silly name for an exception? Doesn't a InvalidQuery or something with a good message in the exception work a lot better?

I agree it's a silly name but I tried to stay in style with other has-many-through exceptions like HasManyThroughAssociationPointlessSourceTypeError and HasManyThroughSourceAssociationNotFoundError.

On the other hand, a general query like you propose is harder to catch distinctly. Although I can't think of a common situation where you need to catch these to take exception type specific actions.

01/27/08 02:41:58 changed by nzkoz

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

(In [8742]) Make sure count works on has_many :through associations using :group. Closes #10480 [remvee]

01/28/08 18:39:29 changed by bitsweat

  • keywords deleted.
  • status changed from closed to reopened.
  • resolution deleted.
  • milestone changed from 2.x to 2.1.

fails on postgresql

01/29/08 08:34:12 changed by remvee

Can't get this construct to work on postgresql. Please revert this patch and close this ticket. Sorry for the inconvenience.

02/03/08 01:10:02 changed by bitsweat

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

(In [8790]) Revert r8742: remove has_many with :group option since it has sketchy sql support. Closes #10480.