Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

count on HABTMs broken by Ruby 1.8.7 Array.count

#831

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.

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

State: resolved
Milestone: none
Assigned to: Jeremy Kemper Jeremy Kemper
Importance: none

Activity

  1. Ernie Miller
    Ernie Miller
    • 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.

    August 14th, 2008 @ 09:14 PM

  2. Mike Champion
    Mike Champion

    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.

    August 14th, 2008 @ 09:27 PM

  3. Ernie Miller
    Ernie Miller

    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! ;)

    August 14th, 2008 @ 09:38 PM

  4. Ernie Miller
    Ernie Miller

    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.

    August 15th, 2008 @ 03:30 AM

  5. Mike Champion
    Mike Champion

    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.

    August 20th, 2008 @ 07:31 PM

  6. Ernie Miller
    Ernie Miller
    • 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.

    August 28th, 2008 @ 07:10 PM

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

    August 28th, 2008 @ 08:04 PM

  8. Jeremy Kemper
    Jeremy Kemper

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

    August 28th, 2008 @ 08:04 PM

  9. Repository
    Repository
    • 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...

    August 28th, 2008 @ 08:05 PM

  10. Mike Champion
    Mike Champion

    Great, thanks for the patch Ernie!

    August 28th, 2008 @ 08:52 PM

  11. Tekin
    Tekin
    • 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.

    September 24th, 2008 @ 01:50 PM

  12. Ernie Miller
    Ernie Miller

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

    September 24th, 2008 @ 02:02 PM

  13. Ernie Miller
    Ernie Miller

    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.

    September 24th, 2008 @ 02:40 PM

  14. Michael Simons
    Michael Simons

    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)

    November 5th, 2008 @ 08:44 PM