This project is archived and is in readonly mode.

Enumerable#sum not called on some association collections

#2269

In Rails 2.3.2, Enumerable#sum is not always called on arrays that are association collections. If you have a simple parent-child relationship, the code parent.children.sum calls the association collection sum, but if you generate the collection with find, invoking its sum method calls Enumerable#sum.

There's a bit of code - patience is appreciated.


# With Rails 2.3.2
# Commands to re-create the issue:
$ rails sum
$ cd sum
sum $ script/generate model parent
sum $ script/generate model child parent_id:integer

# app/models/child.rb:
class Child < ActiveRecord::Base
  belongs_to :parent
end

# app/models/parent.rb:
class Parent < ActiveRecord::Base
  has_many :children
end

sum $ rake db:migrate
sum $ script/console

>> p=Parent.new
=> #<Parent id: nil, created_at: nil, updated_at: nil>
>> p.save
=> true
>> c=Child.new
=> #<Child id: nil, parent_id: nil, created_at: nil, updated_at: nil>
>> c.parent=p
=> #<Parent id: 1, created_at: "2009-03-17 01:57:30", updated_at: "2009-03-17 01:57:30">
>> c
=> #<Child id: nil, parent_id: 1, created_at: nil, updated_at: nil>
>> c.save
=> true

# Now the fun part. Find all children with find.
>> cs=Child.find(:all, :conditions => 'parent_id = 1')
=> [#<Child id: 1, parent_id: 1, created_at: "2009-03-17 01:57:54", updated_at: "2009-03-17 01:57:54">]
# Compute sum of id's
>> cs.sum(&:id)
=> 1
# Now collect them through the association
>> ds=Parent.find(1).children
=> [#<Child id: 1, parent_id: 1, created_at: "2009-03-17 01:57:54", updated_at: "2009-03-17 01:57:54">]
# Try to sum the id's
>> ds.sum(&:id)
ArgumentError: wrong number of arguments (1 for 2)
# Complete error copied below

# Try to make it call the desired method (Enumerable#sum):
>> ds.sum(0,&:id)
=> "0"
# This does it
>> ds.to_a.sum(&:id)
=> 1

# The two arrays appear to be equivalent
>> cs.class
=> Array
>> ds.class
=> Array
>> cs.size
=> 1
>> ds.size
=> 1
>> cs[0].class
=> Child(id: integer, parent_id: integer, created_at: datetime, updated_at: datetime)
>> ds[0].class
=> Child(id: integer, parent_id: integer, created_at: datetime, updated_at: datetime)
>> cs==ds
=> true
>> cs===ds
=> true

# Complete error output
>> ds.sum(&:id)
ArgumentError: wrong number of arguments (1 for 2)
  from /usr/lib/ruby/gems/1.8/gems/activerecord-2.3.2/lib/active_record/associations/association_collection.rb:373:in `calculate'
  from /usr/lib/ruby/gems/1.8/gems/activerecord-2.3.2/lib/active_record/associations/association_collection.rb:373:in `send'
  from /usr/lib/ruby/gems/1.8/gems/activerecord-2.3.2/lib/active_record/associations/association_collection.rb:373:in `method_missing'
  from /usr/lib/ruby/gems/1.8/gems/activerecord-2.3.2/lib/active_record/base.rb:2148:in `with_scope'
  from /usr/lib/ruby/gems/1.8/gems/activerecord-2.3.2/lib/active_record/associations/association_proxy.rb:206:in `send'
  from /usr/lib/ruby/gems/1.8/gems/activerecord-2.3.2/lib/active_record/associations/association_proxy.rb:206:in `with_scope'
  from /usr/lib/ruby/gems/1.8/gems/activerecord-2.3.2/lib/active_record/associations/association_collection.rb:371:in `method_missing'
  from /usr/lib/ruby/gems/1.8/gems/activerecord-2.3.2/lib/active_record/associations/association_collection.rb:155:in `sum'
  from (irb):4
  from :0

Reported by Chris Schumann · March 17th, 2009 @ 01:40 PM

Activity

Chris Schumann

A failing unit test:


# test/unit/child_test.rb
require 'test_helper'

class ChildTest < ActiveSupport::TestCase
  test "should sum ids" do
    p = Parent.new
    p.children.build
    p.save
    # This one passes
    assert p.children.find(:all).sum(&:id) > 0
    # This one fails
    assert p.children.sum(&:id) > 0
  end
end

March 17th, 2009 @ 03:24 PM

Will Bryant
This is the expected current behaviour. Calling some_has_many.sum calls the Calculations module sum, which passes the string argument you give it to the database - so you should use "p.children.sum('id')".

A patch to support loading the target array and delegating the calculation when passing a block instead of a string (which is effectively what you're doing above) to the calculation method (sum etc.) would be welcome.

May 6th, 2009 @ 04:15 AM
CancelProfileIsBroken
- Tag changed from 2.3.2, enumerable, sum to 2.3.2, bugmash, enumerable, sum
August 6th, 2009 @ 02:25 PM
Adam Keys
Will, I'm wondering if changing some_has_many.sum to load all records is too sugary. Case in point, I can look at this and think "hey, this is obviously loading a bunch of records into memory":

person.children.all.sum(&:id)

And that this is going to do some aggregate cleverness on the database and avoid huge loads:

Person.all(&:id)

But the changing this to load all the records on the has_many muddies the waters:

person.children.sum(&:id)

Does this do something clever with aggregates or does it load a bunch of objects? Or am I missing something?

August 8th, 2009 @ 05:01 PM
Dan Pickett
-1 - I disagree that this is a bug -

this is by design to me - the finder should always return an array. conditions should be supplied in the sum call.

August 8th, 2009 @ 08:01 PM
dira
I agree - this is design, not a bug. -1 for BugMash.

The requested syntactic sugar for has_many has two facets:
- it would cause 'huge loads' from the database if the 'many' are not loaded yet - it would be more efficient if the data is loaded already :)

I favor implementation of the suggestion because the some_has_many should support the Enumerable interface.

August 9th, 2009 @ 03:10 PM
Elad Meidar
-1 here too, seem that it will require too much db attention to avoid huge loads when the association is not memo'd, chaining to #all seem to be the way i would go for.

August 9th, 2009 @ 03:37 PM
José Valim
-1 For me this is by design too.

I also disagree supporting the Enumerable interface, since they are different things. In Chris case, for example, the proper way to calculate the sum, would be:
```
@parent.children.sum('id')
```
If we allow people to do this:
```
@parent.children.sum(&:id)
```
We are giving a shortcut to write non performant code. If you really want to do that, invoking to array looks like the way to go (and it also make obvious what is happening):
```
@parent.children.to_a.sum(&:id)
```
Could be handy a documentation patch, to explain the differences between AssoicationProxy#sum and Array#sum.
August 9th, 2009 @ 03:42 PM
Pratik
- State changed from new to invalid
August 9th, 2009 @ 11:58 PM
José Valim
- Tag changed from 2.3.2, bugmash, enumerable, sum to 2.3.2, enumerable, sum
August 9th, 2009 @ 11:59 PM