Avoid big numbers for money #3189

mgrassotti · 2017-08-17T10:30:00Z

Trello

https://trello.com/c/y3bOwPKj/293-2-floatdomainerror-infinity-in-multiple-smart-answers

Problem

We noticed some Float::Infinity errors were happening in production, leading to a 500 Error. That’s probably due to some users trying to input very big numbers.

Staging

https://smart-answers-preview-pr-3189.herokuapp.com/am-i-getting-minimum-wage/y/current_payment/not_an_apprentice/111/23/2.0/12.0/2.4132314123431646e+273/111,1111111111111122222222222222222222222222222222222222222222222222222222222222222222222222222222225555555555555555555555555555555555555555555555555555555555222222222222222222222211111111111111111133333333333333333333555555555555555555555555555555555555555555555555555555555555555555555555555555555555555555555444444444444444444444444444448888888888888888888888888888888888888888888888888888888888887777777777777778888888888888889999999999.0

Production

https://www.gov.uk/am-i-getting-minimum-wage/y/current_payment/not_an_apprentice/111/23/2.0/12.0/2.4132314123431646e+273/1111111111111111122222222222222222222222222222222222222222222222222222222222222222222222222222222225555555555555555555555555555555555555555555555555555555555222222222222222222222211111111111111111133333333333333333333555555555555555555555555555555555555555555555555555555555555555555555555555555555555555555555444444444444444444444444444448888888888888888888888888888888888888888888888888888888888887777777777777778888888888888889999999999.0

Solution

Raise an InvalidResponse error if the value for money is too
big to be represented in a Float variable.

alecgibson · 2017-08-17T11:19:15Z

Shouldn't the infinite? check go last? Will this pass if I feed in eg "999, 999, 999, ...,999" ?

alecgibson · 2017-08-17T13:30:03Z

Okay, so apparently in Ruby:
"999,999,999".to_f # => 999.0

Which means that "999,999,<loads of numbers>,999".to_f.infinte? # => nil

alecgibson · 2017-08-17T14:48:37Z

An example of the failure I'm trying to point out: https://smart-answers-preview-pr-3189.herokuapp.com/am-i-getting-minimum-wage/y/current_payment/not_an_apprentice/111/23/2.0/12.0/2.4132314123431646e+273/111,1111111111111122222222222222222222222222222222222222222222222222222222222222222222222222222222225555555555555555555555555555555555555555555555555555555555222222222222222222222211111111111111111133333333333333333333555555555555555555555555555555555555555555555555555555555555555555555555555555555555555555555444444444444444444444444444448888888888888888888888888888888888888888888888888888888888887777777777777778888888888888889999999999.0

mgrassotti · 2017-08-18T08:57:10Z

@alecgibson I put it at the end because otherwise it would fall in the condition raw_input.is_a?(Numeric). Why do you think it should go at the end? Thanks

alecgibson · 2017-08-18T09:12:38Z

@mgrassotti have you clicked the example link above? It still breaks the site.

At the moment we have two paths to generating a BigDecimal:

Check if the input is Numeric, and if so, cast it
Do some logic to a string to try and turn it into a Numeric, and then cast it

The above link breaks the code, because it follows the second path, where we don't check infinite?.

I think logically we should do all casting first, and then check infinite?, kind of like:

money_as_float = some_function_that_definitely_returns_a_number_or_raises(some_input_string)
raise if money_as_float.infinite?

mgrassotti · 2017-08-18T10:02:59Z

@alecgibson sorry I understand now. I pushed an update

alecgibson · 2017-08-18T10:06:58Z

lib/smart_answer/money.rb

@@ -9,14 +9,11 @@ class Money
    attr_reader :value

    def initialize(raw_input)
-      if raw_input.is_a?(Numeric)
-        @value = BigDecimal.new(raw_input.to_s)
+      number, error_message = self.class.raw_input_to_number(raw_input)


I'm not sure I like this pattern of returning a tuple with an error message. Why not just raise directly from raw_input_to_number? It should hopefully tidy up your control logic a lot more.

alecgibson · 2017-08-18T10:07:42Z

lib/smart_answer/money.rb

+          [raw_input, nil]
+      else
+        raw_input = raw_input.to_s.delete(',').gsub(/\s/, '')
+        if raw_input.to_f.infinite?


As per my previous comment, you could avoid having to check infinite? multiple times if you just call it once when you know you have a Numeric (probably outside this method).

mgrassotti · 2017-08-18T11:17:44Z

@alecgibson ok please check it now

alecgibson · 2017-08-18T12:21:07Z

lib/smart_answer/money.rb

+      end
+    end
+
+    def self.valid_raw_input(raw_input)


Would be nice to keep the validation predicate separate from the validation method. So, I'd expect this to be two methods:

valid_raw_input?, which returns a boolean

validate_raw_input, which calls valid_raw_input? and will raise if it's invalid. You could also move the infinite? check into the validation, because it's part of what makes the input valid.

@alecgibson ok. I think if could be tidy also if done in 1 method. Have a look to my latest changes. Thanks

alecgibson · 2017-08-18T12:22:37Z

lib/smart_answer/money.rb

+      if raw_input.is_a?(Numeric) || raw_input.is_a?(Money) || raw_input =~ /\A *[0-9]+(\.[0-9]{1,2})? *\z/
+        raw_input
+      else
+        raise InvalidResponse, "Sorry, I couldn't understand that number. Please try again.", caller


(I know this isn't part of your change, but while we're here, do you think "I couldn't understand that number" is a normal sort of error message to return? I don't think I'd expect a validation message to use the first person. Maybe more like "The entered value is not a valid number. Please try again."?)

alecgibson · 2017-08-18T12:27:42Z

lib/smart_answer/money.rb

+    def self.raw_input_to_number(raw_input)
+      raw_input = raw_input.is_a?(Numeric) ? raw_input : raw_input.to_s.delete(',').gsub(/\s/, '')
+      if raw_input.to_f.infinite?
+        raise InvalidResponse, "Sorry, that number is too big. Please try again.", caller


I still find it a bit untidy how our validation logic is tangled up in the casting logic.

I basically expect these three separate steps to happen:

Attempt to coerce the raw input into a number

Validate the number (and raise if appropriate)

Cast the number to a BigDecimal

I'd expect these three steps to be represented at a top-level function (probably initialize), and to each have a single line of code, calling to methods if required. This way you get proper separation of concerns.

@alecgibson ok.

alecgibson · 2017-08-18T12:29:00Z

lib/smart_answer/money.rb

-        @value = BigDecimal.new(raw_input.to_s)
-      end
+      number = self.class.raw_input_to_number(raw_input)
+      @value = BigDecimal.new(number.to_s)


Why do we call .to_s?

@alecgibson that should be useless indeed

@alecgibson indeed without the .to_s call it is returning ArgumentError: can't omit precision for a Float.. It probably guesses the precision needed if the argument is a String. That's a little bit weird. Anyway I would leave it as it is for moment, if it is ok for you.

Huh, weird. Yeah fine to leave then.

alecgibson · 2017-08-21T10:00:01Z

lib/smart_answer/money.rb

-        @value = BigDecimal.new(raw_input.to_s)
-      end
+      input = self.class.parse(raw_input)
+      number = self.class.valid?(input) && input


Why bother assigning number? Given that valid? raises, couldn't you just do something like:

input = self.class.parse(raw_input) self.class.validate(input) @value = BigDecimal.new(input.to_s)

alecgibson

Looks great! 🚀

Raise an InvalidResponse error if the user tries to insert numbers too big to be represented in a Float variable.

mgrassotti added the Ready for code review label Aug 17, 2017

chao-xian temporarily deployed to smart-answers-preview-pr-3189 August 17, 2017 10:38 Inactive

mgrassotti force-pushed the avoid-big-numbers branch from 6b67b5f to e855e7d Compare August 18, 2017 10:01

chao-xian temporarily deployed to smart-answers-preview-pr-3189 August 18, 2017 10:01 Inactive

mgrassotti force-pushed the avoid-big-numbers branch from e855e7d to 9a923c5 Compare August 18, 2017 10:02

chao-xian temporarily deployed to smart-answers-preview-pr-3189 August 18, 2017 10:02 Inactive

alecgibson reviewed Aug 18, 2017

View reviewed changes

mgrassotti force-pushed the avoid-big-numbers branch from 9a923c5 to 6d1f6f2 Compare August 18, 2017 11:17

chao-xian temporarily deployed to smart-answers-preview-pr-3189 August 18, 2017 11:17 Inactive

mgrassotti force-pushed the avoid-big-numbers branch from 6d1f6f2 to 30b431d Compare August 18, 2017 11:19

chao-xian temporarily deployed to smart-answers-preview-pr-3189 August 18, 2017 11:19 Inactive

alecgibson reviewed Aug 18, 2017

View reviewed changes

mgrassotti force-pushed the avoid-big-numbers branch from 30b431d to f13c906 Compare August 21, 2017 09:47

chao-xian temporarily deployed to smart-answers-preview-pr-3189 August 21, 2017 09:47 Inactive

mgrassotti force-pushed the avoid-big-numbers branch from f13c906 to 88bb0e1 Compare August 21, 2017 09:58

chao-xian temporarily deployed to smart-answers-preview-pr-3189 August 21, 2017 09:58 Inactive

alecgibson reviewed Aug 21, 2017

View reviewed changes

mgrassotti force-pushed the avoid-big-numbers branch from 88bb0e1 to 7472fe5 Compare August 21, 2017 10:01

chao-xian temporarily deployed to smart-answers-preview-pr-3189 August 21, 2017 10:02 Inactive

mgrassotti force-pushed the avoid-big-numbers branch from 7472fe5 to 5ee0737 Compare August 21, 2017 10:02

alecgibson approved these changes Aug 21, 2017

View reviewed changes

chao-xian temporarily deployed to smart-answers-preview-pr-3189 August 21, 2017 10:03 Inactive

Avoid big numbers for money

1913b57

Raise an InvalidResponse error if the user tries to insert numbers too big to be represented in a Float variable.

mgrassotti force-pushed the avoid-big-numbers branch from 5ee0737 to 1913b57 Compare August 21, 2017 10:03

chao-xian deployed to smart-answers-preview-pr-3189 August 21, 2017 10:03 View deployment

mgrassotti merged commit bab72ec into master Aug 21, 2017

mgrassotti deleted the avoid-big-numbers branch August 21, 2017 10:06

mgrassotti added Passed code review and removed Ready for code review labels Aug 21, 2017

mgrassotti mentioned this pull request Aug 22, 2017

Add an exclamation mark on validate method #3196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid big numbers for money #3189

Avoid big numbers for money #3189

mgrassotti commented Aug 17, 2017 •

edited

Loading

alecgibson commented Aug 17, 2017 •

edited

Loading

alecgibson commented Aug 17, 2017

alecgibson commented Aug 17, 2017

mgrassotti commented Aug 18, 2017

alecgibson commented Aug 18, 2017

mgrassotti commented Aug 18, 2017

alecgibson Aug 18, 2017

alecgibson Aug 18, 2017

mgrassotti commented Aug 18, 2017 •

edited

Loading

alecgibson Aug 18, 2017

mgrassotti Aug 21, 2017 •

edited

Loading

alecgibson Aug 18, 2017

mgrassotti Aug 21, 2017

alecgibson Aug 18, 2017

mgrassotti Aug 21, 2017 •

edited

Loading

alecgibson Aug 18, 2017

mgrassotti Aug 21, 2017

mgrassotti Aug 21, 2017

alecgibson Aug 21, 2017

alecgibson Aug 21, 2017

mgrassotti Aug 21, 2017

alecgibson left a comment

Avoid big numbers for money #3189

Avoid big numbers for money #3189

Conversation

mgrassotti commented Aug 17, 2017 • edited Loading

Trello

Problem

Staging

Production

Solution

alecgibson commented Aug 17, 2017 • edited Loading

alecgibson commented Aug 17, 2017

alecgibson commented Aug 17, 2017

mgrassotti commented Aug 18, 2017

alecgibson commented Aug 18, 2017

mgrassotti commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgrassotti commented Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

mgrassotti Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgrassotti Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecgibson left a comment

Choose a reason for hiding this comment

mgrassotti commented Aug 17, 2017 •

edited

Loading

alecgibson commented Aug 17, 2017 •

edited

Loading

mgrassotti commented Aug 18, 2017 •

edited

Loading

mgrassotti Aug 21, 2017 •

edited

Loading

mgrassotti Aug 21, 2017 •

edited

Loading