ActiveRecord: sum expression returns string '0' for no records, fixed #7439

Merged
merged 1 commit into from Oct 15, 2012

Conversation

Projects
None yet
6 participants
@refractalize
Contributor

refractalize commented Aug 24, 2012

The bug:

Account.where('1 = 2').sum("2 * credit_limit") => '0'

That is, sum with an expression (not a column name) on a condition or collection that returns no records returns the string "0", whereas it should return an integer, 0.

@pixeltrix

This comment has been minimized.

Show comment Hide comment
@pixeltrix

pixeltrix Aug 24, 2012

Member

There was a reason it returned a string - calling to_d on a Fixnum in Ruby 1.8 would give a NoMethodError. This is no longer the case in Ruby 1.9 so it's probably okay to change.

The relevant Lighthouse tickets are 1066 and 4633 - to quote myself from 1066:

The problem with type casting to float for decimal columns and then back to decimal is that you may introduce floating point precision errors. My view would be that if we are doing a straightforward sum on a column then typecast to that column's type otherwise return a string and leave it to the developer to cast to whatever they want. This would also apply to average as well.

Member

pixeltrix commented Aug 24, 2012

There was a reason it returned a string - calling to_d on a Fixnum in Ruby 1.8 would give a NoMethodError. This is no longer the case in Ruby 1.9 so it's probably okay to change.

The relevant Lighthouse tickets are 1066 and 4633 - to quote myself from 1066:

The problem with type casting to float for decimal columns and then back to decimal is that you may introduce floating point precision errors. My view would be that if we are doing a straightforward sum on a column then typecast to that column's type otherwise return a string and leave it to the developer to cast to whatever they want. This would also apply to average as well.

@jfeaver

This comment has been minimized.

Show comment Hide comment
@jfeaver

jfeaver Aug 26, 2012

I ran the tests, everything passes, and the code looks legit.

The test matches the bug and matches the style of the tests around it.

jfeaver commented Aug 26, 2012

I ran the tests, everything passes, and the code looks legit.

The test matches the bug and matches the style of the tests around it.

@refractalize

This comment has been minimized.

Show comment Hide comment
@refractalize

refractalize Aug 31, 2012

Contributor

I haven't had a chance to test this under 1.8.7 yet, but having read through the code it appears as though it handles a missing to_d by casting to string first, see here in connection_adapters/column.rb:

def value_to_decimal(value)
  # Using .class is faster than .is_a? and
  # subclasses of BigDecimal will be handled
  # in the else clause
  if value.class == BigDecimal
    value
  elsif value.respond_to?(:to_d)
    value.to_d
  else
    value.to_s.to_d
  end
end

BTW: is there an easy way to run the tests under 1.8.7? I had trouble running bundler (due to use of new hash syntax in rails/Gemfile)...

Contributor

refractalize commented Aug 31, 2012

I haven't had a chance to test this under 1.8.7 yet, but having read through the code it appears as though it handles a missing to_d by casting to string first, see here in connection_adapters/column.rb:

def value_to_decimal(value)
  # Using .class is faster than .is_a? and
  # subclasses of BigDecimal will be handled
  # in the else clause
  if value.class == BigDecimal
    value
  elsif value.respond_to?(:to_d)
    value.to_d
  else
    value.to_s.to_d
  end
end

BTW: is there an easy way to run the tests under 1.8.7? I had trouble running bundler (due to use of new hash syntax in rails/Gemfile)...

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Aug 31, 2012

Member

I haven't had a chance to test this under 1.8.7 yet,

You don't need to. We don't support 1.8.7 on master.

Member

steveklabnik commented Aug 31, 2012

I haven't had a chance to test this under 1.8.7 yet,

You don't need to. We don't support 1.8.7 on master.

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Sep 8, 2012

Member

@refractalize the change looks fine, but it needs a changelog entry, can you please add that and squash everything into one commit if necessary? Thanks.

@refractalize the change looks fine, but it needs a changelog entry, can you please add that and squash everything into one commit if necessary? Thanks.

@refractalize

This comment has been minimized.

Show comment Hide comment
@refractalize

refractalize Sep 22, 2012

Contributor

@carlosantoniodasilva - took a while to get around to the changelog entry, all done now.

Contributor

refractalize commented Sep 22, 2012

@carlosantoniodasilva - took a while to get around to the changelog entry, all done now.

@refractalize

This comment has been minimized.

Show comment Hide comment
@refractalize

refractalize Oct 15, 2012

Contributor

Bump?

Contributor

refractalize commented Oct 15, 2012

Bump?

tenderlove added a commit that referenced this pull request Oct 15, 2012

Merge pull request #7439 from featurist/master
ActiveRecord: sum expression returns string '0' for no records, fixed

@tenderlove tenderlove merged commit 0a78417 into rails:master Oct 15, 2012

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Oct 15, 2012

Member

@refractalize awesome, thanks, and sorry for the delay :)

@refractalize awesome, thanks, and sorry for the delay :)

@refractalize

This comment has been minimized.

Show comment Hide comment
@refractalize

refractalize Oct 16, 2012

Contributor

Nice one, thanks all!

Contributor

refractalize commented Oct 16, 2012

Nice one, thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment