New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix numericality equality validation on floats #32852

Merged
merged 1 commit into from Dec 12, 2018

Conversation

Projects
None yet
6 participants
@gmcgibbon
Copy link
Member

gmcgibbon commented May 9, 2018

Summary

Currently, the numericality validator of active model has trouble with equality of decimal values. Notably, some single decimal floats (eg. 65.6). This PR fixes #26085 by casting Floats to BigDecimals on both ends of the validation.

If there's interest in merging this, please let me know how I can improve the decimal regex.

@rails-bot

This comment has been minimized.

Copy link

rails-bot commented May 9, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (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.

@gmcgibbon gmcgibbon force-pushed the gmcgibbon:fix_numericality_float_equality branch to 797dad9 Jun 2, 2018

@@ -9,6 +9,9 @@ class NumericalityValidator < EachValidator # :nodoc:

RESERVED_OPTIONS = CHECKS.keys + [:only_integer]

INTEGER_REGEX = /\A[+-]?\d+\z/

This comment has been minimized.

@oniofchaos

oniofchaos Sep 29, 2018

Contributor

Add .freeze at the end of constants.

This comment has been minimized.

@gmcgibbon

gmcgibbon Oct 5, 2018

Member

Regexes in ruby are immutable so I actually don't need to do that. TIL

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 12, 2018

Member

And no, we don't freeze constants just because they are constants. This is defensive programming and have no benefits at all in terms of runtime. If people wants to mutate a constant it is not a freeze that will stop them.

end

def is_integer?(raw_value)
/\A[+-]?\d+\z/ === raw_value.to_s
INTEGER_REGEX === raw_value.to_s

This comment has been minimized.

@oniofchaos

oniofchaos Sep 29, 2018

Contributor

I know it's what was here before, but I'm a bit suspicious of the performance of this vs using something like =~ or match. Would be worth a benchmark to test IMO.

This comment has been minimized.

@gmcgibbon

gmcgibbon Oct 1, 2018

Member
Warming up --------------------------------------
                 ===   142.302k i/100ms
                  =~   139.718k i/100ms
              #match   139.561k i/100ms
Calculating -------------------------------------
                 ===      2.230M (± 2.3%) i/s -     11.242M in   5.044579s
                  =~      2.238M (± 2.5%) i/s -     11.317M in   5.060422s
              #match      2.167M (± 3.0%) i/s -     10.886M in   5.027772s

I think they're all relatively similar. Let's keep using ===.

This comment has been minimized.

@oniofchaos

oniofchaos Oct 1, 2018

Contributor

TIL. Thanks for the benchmark.

@@ -68,18 +69,24 @@ def validate_each(record, attr_name, value)
private

def is_number?(raw_value)
!parse_raw_value_as_a_number(raw_value).nil?
!parse_as_number(raw_value).nil?

This comment has been minimized.

@oniofchaos

oniofchaos Sep 29, 2018

Contributor

I prefer the readability of positives parse_as_number(raw_value).present? vs what's here before, but with a quick benchmark to make sure it doesn't impact performance.

This comment has been minimized.

@gmcgibbon

gmcgibbon Oct 1, 2018

Member

That does read better, but I think because the raw_value can be an anything (including another ActiveRecord object by mistake), its best not to use #present? for performance reasons.

@@ -279,6 +279,13 @@ def test_validates_numericality_with_invalid_args
assert_raise(ArgumentError) { Topic.validates_numericality_of :approved, equal_to: "foo" }
end

def test_validates_numericality_equality_for_float_and_big_decimal

This comment has been minimized.

@oniofchaos

oniofchaos Sep 29, 2018

Contributor

Might be worth separating this out into two or more tests. Feels kinda smushed as-is right now.

@gmcgibbon gmcgibbon force-pushed the gmcgibbon:fix_numericality_float_equality branch 5 times, most recently from c66d14a to 710066b Oct 1, 2018

@gmcgibbon gmcgibbon force-pushed the gmcgibbon:fix_numericality_float_equality branch 2 times, most recently from 81349ed to e54df31 Oct 9, 2018

@guilleiguaran

This comment has been minimized.

Copy link
Member

guilleiguaran commented Dec 8, 2018

@gmcgibbon please rebase this!!!

@gmcgibbon gmcgibbon force-pushed the gmcgibbon:fix_numericality_float_equality branch from e54df31 to ea07a65 Dec 8, 2018

@gmcgibbon

This comment has been minimized.

Copy link
Member

gmcgibbon commented Dec 8, 2018

Done!

rescue ArgumentError, TypeError
false
end

def parse_raw_value_as_a_number(raw_value)
def parse_as_number(raw_value)
return raw_value.to_d if raw_value.is_a?(Float)

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 12, 2018

Member

Too many returns here. I'd go with a if/else so it is explicit all the branches.

return raw_value.to_i if is_integer?(raw_value)
Kernel.Float(raw_value) unless is_hexadecimal_literal?(raw_value)
BigDecimal(raw_value) if is_decimal?(raw_value) && ! is_hexadecimal_literal?(raw_value)

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 12, 2018

Member

There is an extra space after the !

@gmcgibbon gmcgibbon force-pushed the gmcgibbon:fix_numericality_float_equality branch from ea07a65 to d126c0d Dec 12, 2018

@rafaelfranca rafaelfranca merged commit 3a3a3d6 into rails:master Dec 12, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

rafaelfranca added a commit that referenced this pull request Dec 12, 2018

Merge pull request #32852 from gmcgibbon/fix_numericality_float_equality
Fix numericality equality validation on floats

@gmcgibbon gmcgibbon deleted the gmcgibbon:fix_numericality_float_equality branch Dec 12, 2018

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