Fix non_numeric_string? #33804
Fix non_numeric_string? #33804
Conversation
|
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review. Please see the contribution instructions for more information. |
| @@ -29,7 +29,7 @@ def non_numeric_string?(value) | |||
| # 'wibble'.to_i will give zero, we want to make sure | |||
| # that we aren't marking int zero to string zero as | |||
| # changed. | |||
| value.to_s !~ /\A-?\d+\.?\d*\z/ | |||
| value.to_s !~ /\A[-+]?\d+\.?\d*([eE][-+]\d+)?\z/ | |||
There was a problem hiding this comment.
Can you use match? to avoid creating unused match object and unused captures?
diff --git a/activemodel/lib/active_model/type/helpers/numeric.rb b/activemodel/lib/active_model/type/helpers/numeric.rb
index 16e14f9e5f..c9519dc908 100644
--- a/activemodel/lib/active_model/type/helpers/numeric.rb
+++ b/activemodel/lib/active_model/type/helpers/numeric.rb
@@ -29,7 +29,7 @@ def non_numeric_string?(value)
# 'wibble'.to_i will give zero, we want to make sure
# that we aren't marking int zero to string zero as
# changed.
- value.to_s !~ /\A-?\d+\.?\d*\z/
+ !/\A[-+]?\d+\.?\d*(?:[eE][-+]\d+)?\z/.match?(value.to_s)
end
end
endThere was a problem hiding this comment.
Thank you @kamipo.
I found 5e2 is also valid expression, so I'll change regex to /\A[-+]?\d+\.?\d*(?:[eE][-+]?\d+)?\z/
I also found "E-notataion" is valid for to_d and to_f but not to_i:
irb(main):001:0> require 'bigdecimal'
=> true
irb(main):002:0> require 'bigdecimal/util'
=> true
irb(main):003:0> '5e+2'.to_d
=> 0.5e3
irb(main):004:0> '5e+2'.to_f
=> 500.0
irb(main):005:0> '5e+2'.to_i
=> 5
Should I overwrite this method for AM::Type::Integer?
| @@ -60,6 +60,7 @@ def test_changed? | |||
| assert type.changed?(5.0, 5.0, "5.0wibble") | |||
There was a problem hiding this comment.
Now I come to think this test is not appropriate.
model.int_column = "5.0wibble" and "5.0wibble".to_i both result in 5.
We may not need \z in regex.
|
To avoid invalid numeric expression like I updated PR based on this. |
|
@rafaelfranca @kamipo Thank you for review! |
| @@ -21,9 +21,11 @@ def test_type_cast_float_from_invalid_string | |||
| def test_changing_float | |||
| type = Type::Float.new | |||
|
|
|||
| assert type.changed?(5.0, 5.0, "5wibble") | |||
| assert type.changed?(5.0, 0, "wibble") | |||
There was a problem hiding this comment.
Can you add assert type.changed?(0, 0, "wibble") too?
Since type.changed? always returns true if differ old_value and new_value regardless of the result of non_numeric_string?.
For example, dirty checking was not right for the following case: ``` model.int_column = "+5" model.float_column = "0.5E+1" model.decimal_column = "0.5e-3" ``` It is enough to see whether leading character is a digit for avoiding invalid numeric expression like 'wibble' to be type-casted to 0, as this method's comment says. Fixes rails#33801
|
@kamipo done. Do you have any other concerns? |
|
Thanks! |
|
I think this should be backported. |
|
Backported in b433433. |
For example, dirty checking was not right for the following case:
Fixes #33801