This project is archived and is in readonly mode.

#831 ✓resolved
Mike Champion

count on HABTMs broken by Ruby 1.8.7 Array.count

Reported by Mike Champion | August 14th, 2008 @ 05:06 PM

Ruby 1.8.7 introduces several new methods including Array.count (through Enumerable). This appears to break using the count method on HABTM relationships, although not has_many relationships.

I have a test rails app that I have tested with ruby 1.8.7p22 against Rails 2.1 and edge Rails. Both exhibit this problem.

The models are:


class Author < AR::Base
  has_and_belongs_to_many :posts
end

class Post < AR::Base
  has_and_belongs_to_many :authors
end

In the unit test, there is:


# This author has 2 posts
mike = authors(:mike)

# Appears to be correct, but looking at logs it is not
# doing a "count(*)" but instead is doing full "select (*)" via
# Array.count
assert_equal 2, mike.posts.count

# This generates:
# ArgumentError: wrong number of arguments (2 for 1)
# because it calls Array.count instead of AR calculations.
assert_equal 2, mike.posts(true).count(:all, :conditions => ["posts.created_at < ?", Time.utc('2020')])

I haven't dug into the Rails sources much to see why this is only happening for HABTM associations, but was wondering if others had encountered this and hopefully had a fix.

Comments and changes to this ticket

  • Ernie Miller

    Ernie Miller August 14th, 2008 @ 09:14 PM

    • Tag changed from “2.1, activerecord, edge” to “2.1, activerecord, bug, edge, patch”

    There's actually no count method defined on the habtm association class, which would help explain the issue. ;)

    After a quick look through the association code, I think what's happening is that without an Array#count (1.8.6) AssociationCollection#method_missing is calling the reflection class's count method scoped to its owner.

    With an Array#count (1.8.7), however, the first condition of AssociationCollection#method_missing is being met and instead the call is passed up the line to AssociationProxy, which in turn passes it to the @target directly, after loading it.

    There are two possible ways this could be resolved. It seems that the way that most keeps in line with the other association types is to implement HasAndBelongsToManyAssociation#count. Another shortcut would be to change AssociationCollection#method_missing such that the first condition won't pass if the method is :count.

    The former isn't very DRY, and the latter feels awfully hackish to me.

    So instead, I've opted too try something much more dangerous, and refactored the handling of count so that it's implemented inside AssociationCollection instead of its descendants.

    This is really rough, but could you give the attached patch a try and see if it resolves the issue? I'm not running 1.8.7 here, because I'm stuck on a Windows box at the moment with a One-Click-Installer Ruby installation. I can at least confirm that nothing new in the ActiveRecord MySQL test suite is failing from this patch, but I'm not sure there's ample test coverage for all of the various :counter_sql and :finder_sql possibilities in all 3 affected association macros.

    Assuming positive feedback, I'll update docs and add any tests that might be needed as well, since :counter_sql doesn't seem to be documented everywhere, and wasn't being used on HABTM associations before.

  • Mike Champion

    Mike Champion August 14th, 2008 @ 09:27 PM

    Thanks. I think you're right as to the cause. The method_missing in association_collection looks at the class level methods, where "count" is defined by calculations.rb. It works for HasMany relationships because that defines a count method on the instance and overrides the Array.count, I believe, whereas HABTM doesn't define it.

    I started down the path of adding a count method similar to what is in has_many_association to has_and_belongs_to_many_association. Although maybe I will just try to have it restore the behavior of what it was doing for ruby 1.8.6.

  • Ernie Miller

    Ernie Miller August 14th, 2008 @ 09:38 PM

    If you get the chance, try the patch I attached and let me know how it works for you... and if it breaks anything new and exotic! ;)

  • Ernie Miller

    Ernie Miller August 15th, 2008 @ 03:30 AM

    Updated patch to add a bit of documentation.

    I'm a little concerned about the handling of a has_many association's :conditions option if passed to count. Before the refactor, it was pulling out the conditions ahead of time and modifying the association's @finder_sql. I think this is because it wasn't taking advantage of with_scope, and as I said before, all tests continue to pass, so I think the refactor went off without a hitch... But if someone can show a test case where this patch breaks things, I'd like to include that test and fix the bug.

  • Mike Champion

    Mike Champion August 20th, 2008 @ 07:31 PM

    Ernie, I applied your patch and it fixed my small sample app's tests that exercised the HABTM count problem. Thanks!

    I have not yet tried it in our real app to see if other issues are exhibited.

  • Ernie Miller

    Ernie Miller August 28th, 2008 @ 07:10 PM

    • Tag changed from “2.1, activerecord, bug, edge, patch” to “2.1, activerecord, bug, edge, patch, tests”

    Added a test based on Mike's example above, and merged Tarmo's :offset and :limit constraints from #348 as well.

  • Jeremy Kemper

    Jeremy Kemper August 28th, 2008 @ 08:04 PM

    • State changed from “new” to “open”
    • Assigned user set to “Jeremy Kemper”
    • Milestone cleared.
  • Jeremy Kemper

    Jeremy Kemper August 28th, 2008 @ 08:04 PM

    Doesn't apply cleanly to 2-1-stable so this'll be 2.2 only.

  • Repository

    Repository August 28th, 2008 @ 08:05 PM

    • State changed from “open” to “resolved”

    (from [44af2efa2c7391681968c827ca47201a0a02e974]) Refactored AssociationCollection#count for uniformity and Ruby 1.8.7 support.

    [#831 state:resolved]

    Signed-off-by: Jeremy Kemper jeremy@bitsweat.net http://github.com/rails/rails/co...

  • Mike Champion

    Mike Champion August 28th, 2008 @ 08:52 PM

    Great, thanks for the patch Ernie!

  • Tekin

    Tekin September 24th, 2008 @ 01:50 PM

    • Tag changed from “2.1, activerecord, bug, edge, patch, tests” to “2.1, activerecord, bug, edge, patch, tests”

    :counter_sql has not been added to the valid keys hash in create_has_and_belongs_to_many_reflections on line 1574 of associations.rb so :counter_sql is being rejected.

  • Ernie Miller

    Ernie Miller September 24th, 2008 @ 02:02 PM

    Good catch. This is a trivial fix -- Jeremy, do you want me to submit another patch, or is this fixed in edge already?

  • Ernie Miller

    Ernie Miller September 24th, 2008 @ 02:40 PM

    I checked, and it hasn't been fixed in edge yet. See #1102 for the one-liner patch. We should really get some fixtures that use counter_sql on a HABTM to catch this in the future, but I don't have time to knock that out today and since David mentioned 2.2 beta was coming soon, I didn't want this simple bug sneaking into the beta if it could be helped.

  • Michael Simons

    Michael Simons November 5th, 2008 @ 08:44 PM

    This problem applies to 2.1.2 as well.

    I confirm this for Debian/Lenny, 2.6.26-1-686 #1 SMP, with ruby 1.8.7 (2008-08-11 patchlevel 72) [i486-linux] and Rails 2.1.2

    HABTM is:

    
has_and_belongs_to_many :friends,
                          :class_name => 'User',
                          :join_table => 'friends',
                          :foreign_key => 'user_id',
                          :association_foreign_key => 'friend_id',
                          :order => 'NAME ASC, VORNAME ASC'

    Query is

    
friends.count(:conditions => ["friend_id = :user_id", {:user_id => user_id}])

    Any help (and fixing) is appreciated!

    I really like to go to 2.2 but i can't (and i guess a lot of people that are stuck to gettext for the time being can't either)

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Attachments

Tags

Referenced by

Pages