fix truncation in ActiveRecord::Calculations #13308

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
@acapilleri
Contributor

acapilleri commented Dec 13, 2013

This PR fixes a wrong conversion to column_type in
ActiveRecord::Calculations#calculation
when the calculation has complex fields and the result
is a different type than the column.

This prevent truncation of result and allows the user
to have the original data type of result

Before:

  Account.sum("2.1 * accounts.credit_limit") => 667

After:

  Account.sum("2.1 * accounts.credit_limit") => 667.8

Fixes #12937

@acapilleri

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri Dec 13, 2013

Contributor

cc @senny

Contributor

acapilleri commented Dec 13, 2013

cc @senny

@acapilleri

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri Dec 13, 2013

Contributor

for other informations please read #13195

Contributor

acapilleri commented Dec 13, 2013

for other informations please read #13195

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Dec 13, 2013

Member

@acapilleri ❤️ will take a look soon.

Member

senny commented Dec 13, 2013

@acapilleri ❤️ will take a look soon.

@senny

View changes

activerecord/CHANGELOG.md
+
+ Account.sum("2.1 * accounts.credit_limit") # => is not type casted
+
+ Fixes #12937

This comment has been minimized.

Show comment Hide comment
@senny

senny Dec 13, 2013

Member

Can you move this above the Example and below the paragraph? Also please add a dot at the end:

Fixes #12937.
@senny

senny Dec 13, 2013

Member

Can you move this above the Example and below the paragraph? Also please add a dot at the end:

Fixes #12937.
@acapilleri

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri Dec 13, 2013

Contributor

@senny done.

Contributor

acapilleri commented Dec 13, 2013

@senny done.

@@ -338,8 +338,8 @@ def column_alias_for(keys)
end
def column_for(field)
- field_name = field.respond_to?(:name) ? field.name.to_s : field.to_s.split('.').last
- @klass.columns_hash[field_name]
+ field = field.name if field.respond_to?(:name)

This comment has been minimized.

Show comment Hide comment
@pftg

pftg Dec 13, 2013

Contributor

👍

@pftg

pftg Dec 13, 2013

Contributor

👍

@@ -396,6 +396,25 @@ def test_sum_expression_returns_zero_when_no_records_to_sum
assert_equal 0, Account.where('1 = 2').sum("2 * credit_limit")
end
+ def test_complex_expressions_return_type_different_than_the_calculated_column_type

This comment has been minimized.

Show comment Hide comment
@senny

senny Dec 16, 2013

Member

the test name is a bit missleading. We are not making sure that the column type does not match, we just don't cast it to loose data. Maybe you can reword to express the reason more clearly.

@senny

senny Dec 16, 2013

Member

the test name is a bit missleading. We are not making sure that the column type does not match, we just don't cast it to loose data. Maybe you can reword to express the reason more clearly.

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri Dec 16, 2013

Contributor

thinking back, you're right

@acapilleri

acapilleri Dec 16, 2013

Contributor

thinking back, you're right

@acapilleri

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri Dec 16, 2013

Contributor

updated

Contributor

acapilleri commented Dec 16, 2013

updated

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Dec 16, 2013

Member

This PR will change the current behavior because we no longer parse the fields from the expression. Perhaps we need to deprecate the current behavior first before moving to this solution. Problem is that we can't suggest a "new" way of doing things. This means we would also need a flag to disable the deprecated behavior and the warning. Basically a feature flag for this PR.

@rafaelfranca what do you think?

Member

senny commented Dec 16, 2013

This PR will change the current behavior because we no longer parse the fields from the expression. Perhaps we need to deprecate the current behavior first before moving to this solution. Problem is that we can't suggest a "new" way of doing things. This means we would also need a flag to disable the deprecated behavior and the warning. Basically a feature flag for this PR.

@rafaelfranca what do you think?

@pftg

This comment has been minimized.

Show comment Hide comment
@pftg

pftg Dec 16, 2013

Contributor

Elsewhere, @acapilleri please also update api documentation.

Contributor

pftg commented Dec 16, 2013

Elsewhere, @acapilleri please also update api documentation.

@pftg

This comment has been minimized.

Show comment Hide comment
@pftg

pftg Dec 16, 2013

Contributor

And one more, should we update type_cast_calculated_value to be consistent, because I supose when 'average' then value.respond_to?(:to_d) ? value.to_d : value is redundant, what do you think @acapilleri?

Contributor

pftg commented Dec 16, 2013

And one more, should we update type_cast_calculated_value to be consistent, because I supose when 'average' then value.respond_to?(:to_d) ? value.to_d : value is redundant, what do you think @acapilleri?

@acapilleri

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri Dec 16, 2013

Contributor

@pftg Thanks for your review, but we are playing with mysql and mysql2.
Your update do not works with mysql, but works with mysql2.
There are other code to update when mysql will be deprecated, but currently it seems the best compromise

Contributor

acapilleri commented Dec 16, 2013

@pftg Thanks for your review, but we are playing with mysql and mysql2.
Your update do not works with mysql, but works with mysql2.
There are other code to update when mysql will be deprecated, but currently it seems the best compromise

@acapilleri

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri Dec 18, 2013

Contributor

@yahonda could test this PR with oracle adapter?
I have no possibility to do this.
thanks in advance

Contributor

acapilleri commented Dec 18, 2013

@yahonda could test this PR with oracle adapter?
I have no possibility to do this.
thanks in advance

@yahonda

This comment has been minimized.

Show comment Hide comment
@yahonda

yahonda Dec 18, 2013

Contributor

dbeee30 looks good with Oracle adapter.

$ ARCONN=oracle ruby -Itest test/cases/calculations_test.rb -n test_complex_expression_not_lose_data_when_returns_different_type_than_column_type
Using oracle
Run options: -n test_complex_expression_not_lose_data_when_returns_different_type_than_column_type --seed 24805

# Running:

.

Finished in 0.852083s, 1.1736 runs/s, 10.5623 assertions/s.

1 runs, 9 assertions, 0 failures, 0 errors, 0 skips
2.0.0-p353@railsmaster [ prevent_conversion_to_int ~/git/rails/activerecord]$

You can use https://github.com/yahonda/rails-dev-box-runs-oracle for ActiveRecord testing with Oracle database. Please give it a try.

Contributor

yahonda commented Dec 18, 2013

dbeee30 looks good with Oracle adapter.

$ ARCONN=oracle ruby -Itest test/cases/calculations_test.rb -n test_complex_expression_not_lose_data_when_returns_different_type_than_column_type
Using oracle
Run options: -n test_complex_expression_not_lose_data_when_returns_different_type_than_column_type --seed 24805

# Running:

.

Finished in 0.852083s, 1.1736 runs/s, 10.5623 assertions/s.

1 runs, 9 assertions, 0 failures, 0 errors, 0 skips
2.0.0-p353@railsmaster [ prevent_conversion_to_int ~/git/rails/activerecord]$

You can use https://github.com/yahonda/rails-dev-box-runs-oracle for ActiveRecord testing with Oracle database. Please give it a try.

@acapilleri

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri Dec 18, 2013

Contributor

Good! Thanks for all

Angelo Capilleri

Il giorno 18/dic/2013, alle ore 19:08, Yasuo Honda notifications@github.com ha scritto:

dbeee30 looks good with Oracle adapter.

$ ARCONN=oracle ruby -Itest test/cases/calculations_test.rb -n test_complex_expression_not_lose_data_when_returns_different_type_than_column_type
Using oracle
Run options: -n test_complex_expression_not_lose_data_when_returns_different_type_than_column_type --seed 24805

Running:

.

Finished in 0.852083s, 1.1736 runs/s, 10.5623 assertions/s.

1 runs, 9 assertions, 0 failures, 0 errors, 0 skips
2.0.0-p353@railsmaster [ prevent_conversion_to_int ~/git/rails/activerecord]$
You can use https://github.com/yahonda/rails-dev-box-runs-oracle for ActiveRecord testing with Oracle database. Please give it a try.


Reply to this email directly or view it on GitHub.

Contributor

acapilleri commented Dec 18, 2013

Good! Thanks for all

Angelo Capilleri

Il giorno 18/dic/2013, alle ore 19:08, Yasuo Honda notifications@github.com ha scritto:

dbeee30 looks good with Oracle adapter.

$ ARCONN=oracle ruby -Itest test/cases/calculations_test.rb -n test_complex_expression_not_lose_data_when_returns_different_type_than_column_type
Using oracle
Run options: -n test_complex_expression_not_lose_data_when_returns_different_type_than_column_type --seed 24805

Running:

.

Finished in 0.852083s, 1.1736 runs/s, 10.5623 assertions/s.

1 runs, 9 assertions, 0 failures, 0 errors, 0 skips
2.0.0-p353@railsmaster [ prevent_conversion_to_int ~/git/rails/activerecord]$
You can use https://github.com/yahonda/rails-dev-box-runs-oracle for ActiveRecord testing with Oracle database. Please give it a try.


Reply to this email directly or view it on GitHub.

+
+ credit_limit_averange = 53 * 2.112
+ assert_equal credit_limit_averange, Account.average("accounts.credit_limit * 2.112")
+ assert_equal credit_limit_averange, Account.average("credit_limit * 2.112")

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 18, 2013

Owner

Typo on var name average.

@carlosantoniodasilva

carlosantoniodasilva Dec 18, 2013

Owner

Typo on var name average.

@acapilleri

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri Dec 18, 2013

Contributor

updated , many thanks @carlosantoniodasilva

Contributor

acapilleri commented Dec 18, 2013

updated , many thanks @carlosantoniodasilva

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Dec 21, 2013

Member

@acapilleri going to take a look at this after 4.1 has been released. I assigned the 4.2 milestone,

Member

senny commented Dec 21, 2013

@acapilleri going to take a look at this after 4.1 has been released. I assigned the 4.2 milestone,

@acapilleri

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri Dec 21, 2013

Contributor

Thanks for your work

Angelo Capilleri

Il giorno 21/dic/2013, alle ore 11:18, Yves Senn notifications@github.com ha scritto:

@acapilleri going to take a look at this after 4.1 has been released. I assigned the 4.2 milestone,


Reply to this email directly or view it on GitHub.

Contributor

acapilleri commented Dec 21, 2013

Thanks for your work

Angelo Capilleri

Il giorno 21/dic/2013, alle ore 11:18, Yves Senn notifications@github.com ha scritto:

@acapilleri going to take a look at this after 4.1 has been released. I assigned the 4.2 milestone,


Reply to this email directly or view it on GitHub.

@acapilleri

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri Apr 25, 2014

Contributor

....any news ?
tks

Contributor

acapilleri commented Apr 25, 2014

....any news ?
tks

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Apr 26, 2014

Member

haven't had the time to revisit this. We have enough time till 4.2 though.

Member

senny commented Apr 26, 2014

haven't had the time to revisit this. We have enough time till 4.2 though.

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Aug 15, 2014

Member

/cc @matthewd

Member

senny commented Aug 15, 2014

/cc @matthewd

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny May 13, 2015

Member

@acapilleri sorry for the delay, could you push a rebased version?

Member

senny commented May 13, 2015

@acapilleri sorry for the delay, could you push a rebased version?

@acapilleri

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri May 13, 2015

Contributor

Yes!

Angelo Capilleri

Il giorno 13/mag/2015, alle ore 10:44, Yves Senn notifications@github.com ha scritto:

@acapilleri sorry for the delay, could you push a rebased version?


Reply to this email directly or view it on GitHub.

Contributor

acapilleri commented May 13, 2015

Yes!

Angelo Capilleri

Il giorno 13/mag/2015, alle ore 10:44, Yves Senn notifications@github.com ha scritto:

@acapilleri sorry for the delay, could you push a rebased version?


Reply to this email directly or view it on GitHub.

@acapilleri

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri May 13, 2015

Contributor

@senny rebased!

Contributor

acapilleri commented May 13, 2015

@senny rebased!

- field_name = field.respond_to?(:name) ? field.name.to_s : field.to_s.split('.').last
- @klass.type_for_attribute(field_name)
+ field = field.name if field.respond_to?(:name)
+ @klass.columns_hash[field.to_s]

This comment has been minimized.

Show comment Hide comment
@sgrif

sgrif May 13, 2015

Member

The columns_hash no longer contains appropriate type information. type_for_attribute is the correct method to use.

@sgrif

sgrif May 13, 2015

Member

The columns_hash no longer contains appropriate type information. type_for_attribute is the correct method to use.

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri May 13, 2015

Contributor

@sgrif thanks for comment, It's an hold Pr

@acapilleri

acapilleri May 13, 2015

Contributor

@sgrif thanks for comment, It's an hold Pr

@sgrif

This comment has been minimized.

Show comment Hide comment
@sgrif

sgrif May 13, 2015

Member

Wouldn't this fail on any adapter which doesn't cast primitives?

Member

sgrif commented May 13, 2015

Wouldn't this fail on any adapter which doesn't cast primitives?

@acapilleri

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri May 13, 2015

Contributor

@sgrif in what sense? The only difference with the code of before is that now we don't search the column type by splitting the words.

Contributor

acapilleri commented May 13, 2015

@sgrif in what sense? The only difference with the code of before is that now we don't search the column type by splitting the words.

@sgrif

This comment has been minimized.

Show comment Hide comment
@sgrif

sgrif May 13, 2015

Member

assert_equal credit_limit_average, Account.average("accounts.credit_limit * 2.112")

The result of that should be a string on MySQL and PG pre-5.0. Looks like travis isn't running against this PR?

Member

sgrif commented May 13, 2015

assert_equal credit_limit_average, Account.average("accounts.credit_limit * 2.112")

The result of that should be a string on MySQL and PG pre-5.0. Looks like travis isn't running against this PR?

@acapilleri

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri May 14, 2015

Contributor

I don't know why travis doesn't confirm here, but it looks ok

Contributor

acapilleri commented May 14, 2015

I don't know why travis doesn't confirm here, but it looks ok

+
+ Account.sum(:credit_limit) # => is type casted
+
+ Account.sum("2.1 * accounts.credit_limit") # => is not type casted

This comment has been minimized.

Show comment Hide comment
@senny

senny May 26, 2015

Member

Let's also include an example where you'd previously expected the type casting. Like Account.sum("accounts.credit_limit")

@senny

senny May 26, 2015

Member

Let's also include an example where you'd previously expected the type casting. Like Account.sum("accounts.credit_limit")

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny May 26, 2015

Member

Can you start a section in the upgrading guide that highlights this change? I don't think many people are affected but it's a breaking change without any generated message. Better to state the new behavior explicitly.

Member

senny commented May 26, 2015

Can you start a section in the upgrading guide that highlights this change? I don't think many people are affected but it's a breaking change without any generated message. Better to state the new behavior explicitly.

@senny senny added the activerecord label May 26, 2015

@senny senny self-assigned this May 26, 2015

@acapilleri

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri May 27, 2015

Contributor

done

Contributor

acapilleri commented May 27, 2015

done

activerecord/CHANGELOG.md
+
+ Account.sum("2.1 * accounts.credit_limit") # => is not type casted
+
+ Account.sum("accounts.credit_limit") # => is type casted

This comment has been minimized.

Show comment Hide comment
@senny

senny May 27, 2015

Member

Are you sure this will be type casted? I think after this patch it is no longer being casted.

@senny

senny May 27, 2015

Member

Are you sure this will be type casted? I think after this patch it is no longer being casted.

guides/source/upgrading_ruby_on_rails.md
@@ -52,6 +52,15 @@ Don't forget to review the difference, to see if there were any unexpected chang
Upgrading from Rails 4.2 to Rails 5.0
-------------------------------------
+### Complex expressions in `ActiveRecord::Calculations#calculation`
+
+In Rails 4.2 the aggregate methods in `ActiveRecord::Calculations#calculation` type cast the result

This comment has been minimized.

Show comment Hide comment
@senny

senny May 27, 2015

Member

can we link ActiveRecord::Calculations to the API docs? Also it would be good to include a user facing example as well, like sum or average.

@senny

senny May 27, 2015

Member

can we link ActiveRecord::Calculations to the API docs? Also it would be good to include a user facing example as well, like sum or average.

This PR fixes a wrong conversion to column_type in
`ActiveRecord::Calculations#calculation`
when the calculation has complex fields and the result
is a different type than the column.

This prevent truncation of result and allows the user
to have the original data type of result

Fixes #12937.

  Before:

      Account.sum("2.1 * accounts.credit_limit") => 667

  After:

      Account.sum("2.1 * accounts.credit_limit") => 667.8
@acapilleri

This comment has been minimized.

Show comment Hide comment
@acapilleri

acapilleri May 27, 2015

Contributor

updated

Contributor

acapilleri commented May 27, 2015

updated

@rafaelfranca rafaelfranca removed this from the 5.0.0 [temp] milestone Apr 5, 2016

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