number.multiple() enhancement - Issue #916 #918

santanu-biswas · 2016-06-11T11:44:08Z

‘number.multiple()’ enhanced for:

Accept any Number (integer or float) as argument.
Accept references as argument.
For any non-number or zero multiple base, validate now returns error
instead of throwing it.

‘number.multiple()’ enhanced for: 1. Accept any Number (integer or float) as argument. 2. Accept references as argument. 3. For any non-number or zero multiple base, validate now returns error instead of throwing it.

‘number.multiple()’ enhanced for: 1. Accept any Number (integer or float) as argument. 2. Accept references as argument. 3. For any non-number or zero multiple base, validate now returns error instead of throwing it. 4. Required tests added & checked.

…nhancement-#916 # Conflicts: # lib/number.js

santanu-biswas · 2016-06-17T09:47:36Z

Rebase done

Marsup · 2016-06-17T09:51:31Z

You need to check error messages as well, false is not enough (eg. https://github.com/hapijs/joi/blob/master/test/number.js#L548). That's a new policy I intend to apply to all the tests but didn't have time to do.

Marsup · 2016-06-17T09:54:58Z

lib/number.js


-        return this._test('multiple', base, function (value, state, options) {
+            const mult = Math.pow(10, Math.max(internals.precision(value), internals.precision(divisor)));


Which case it this trying to prevent ?

@Marsup
It is to allow number.multiple() to allow float numbers to be checked. Currently number.multiple() cannot handle floats. However % operator cannot handle decimal numbers correctly. So the idea is to change the decimal numbers to integers before using % operator.

Example:

To check if 4.523 is a multiple of 1.3, it checks if 4523 (4.523 * 10^3) is a multiple of 1300 (1.3 * 10^3).

To check if 4.2 is a multiple of 1.05, it checks if 420 (4.2 * 10^2) is a multiple of 105 (1.05 * 10^2)

Added internals.precision(num) to find the number of decimal places in a any number.

Is it a real world problem for existing use cases though ? We don't want accurate modulo, we just want 0 when it's a multiple.

Yes @Marsup it is a real life problem for existing use cases in financial application involving exchange rates that can have 2 to 5 decimal accuracy.

number.multiple() fails to validate entered currency value as exact multiple of an amount in another currency because of modulo operator's inability to handle decimal/floats correctly.

Example:
6711 % 67.11 results in 5.684341886080802e-14
3230 % 32.3 results in 2.8421709430404007e-13

Although in both the above cases it should have been 0 but modulo operator is broken for decimal/float values. (Check JSFiddle)

Similar real world use cases also exist in stock trading application.

santanu-biswas · 2016-06-17T14:13:20Z

@Marsup
I have added the required error message checks for tests that return false.

Marsup · 2016-06-18T08:48:14Z

I'm now wondering whether it should be an extension or not, it's still unsafe and we'd be better off using something like big.js/decimal.js or whatever module works.

santanu-biswas · 2016-06-18T10:58:36Z

@Marsup
Initially I did think about adding a module float.js, however, then I realized that number.js will have to be modified to handle only integers. Otherwise, there would be an ambiguity between Joi.number() and Joi.float(). But then if this change is done then it would break current usage code wherever Joi.number() is being used to validate integers.

Thats why I felt that for now doing it in number.js will be better and then upgrade by adding float.js to clearly distinguish between integer and float validation.

Marsup · 2016-06-18T12:10:34Z

You misunderstood me, I was talking about modules, not files. I don't see any point in having another type since javascript doesn't make this difference either.

santanu-biswas · 2016-06-18T12:23:04Z

@Marsup
OK so what do you suggest now?
What makes you feel that it is unsafe?

Marsup · 2016-06-18T13:12:11Z

Sorry not unsafe, unreliable. However unlikely, given enough digits, you'll overflow the maximum capacity of javascript numbers with the multiplication. I'm still looking whether we can achieve the same thing without increasing original values but I can't find anything.

santanu-biswas · 2016-06-18T13:30:38Z

Marsup I understand your point now.
So, is it alright if I change it to simple modulus like before and keep the other enhancements:

Accept references as argument.
For any non-number or zero multiple base, validate now returns error
instead of throwing it.

santanu-biswas · 2016-06-18T15:46:03Z

@Marsup
Please have a look at decimal.js. It has a robust & reliable mod function.

(A lighter version of it is also available at decimal.js-light).

I tested the mod function with fairly large numbers (See JSFiddle) and it works perfectly alright.

If you agree, it can be used.

Marsup · 2016-06-18T16:50:08Z

I already mentioned that library a few comments ago, it's also why I said I might have it as an extension, since this is exactly why it was made.

# Conflicts: # test/number.js

santanu-biswas · 2016-06-19T12:12:54Z

@Marsup
Added big.js and made the required changes in modulo function and completed all tests.

Marsup · 2016-06-19T12:47:46Z

When I said an extension, I meant a separate module (see Joi.extend()), sorry if that was unclear. I'm not adding any dependencies to the core unless they serve a common interest.
What can be done in the core is accepting floats and references though, but documenting that it has the same flaws as javascript itself.

Removed big.js and reverted back to regular modulo operator. Also added notes to the documentation about the limitations of number.multiple() with decimals/floats.

santanu-biswas · 2016-06-19T14:57:10Z

@Marsup Okay... I understand. Removed big.js but kept float acceptance and refs.
Also added Notes to documentation as suggested by you. Hope I got it alright now.

Marsup · 2016-06-19T15:27:02Z

lib/number.js

@@ -53,12 +53,29 @@ internals.Number = class extends Any {

    multiple(base) {

-        Hoek.assert(Hoek.isInteger(base), 'multiple must be an integer');


Don't remove those, just allow floats.

If you don't remove this then it throws error instead of gracefully validating (thats how it happens even now). It starts throwing exception for non-integer number, so what is the point in allowing float numbers?

And that's intended, this is to prevent developers mistakes.

So that means:

The developer should first validate if the multiple is zero (the developer is using Joi for validation and there is no way to validate that in Joi).

Allowing floats has no meaning, because it will throw exception anyways.

Am I right?
So in effect we are just making the changes for allowing refs.?

The developer is using joi to validate external inputs, not to validate its own inputs, there are synchronous throws for that yes.
How would allowing floats crash anything ?

Marsup · 2016-06-19T15:32:34Z

lib/number.js


-        return this._test('multiple', base, function (value, state, options) {
+        return this._test('multiple', base, (value, state, options) => {


Do not use => here.

Restored errors to be thrown for non-integer and zero value of multiple.

Marsup · 2016-06-20T08:09:10Z

lib/number.js

+        const isRef = Ref.isRef(base);
+
+        if (!isRef) {
+            Hoek.assert(Hoek.isInteger(base), 'multiple must be an integer');


So why aren't you allowing floats here ?

@Marsup Allowed floats now.

santanu-biswas · 2016-06-20T13:44:32Z

@Marsup Have allowed all numbers (Integers & floats). Is there anything else that I have missed?

Marsup · 2016-06-20T13:55:17Z

I don't think so but I'll review more in depth tonight.

Marsup · 2016-06-22T21:02:27Z

Merged as 3a32b92. Cleaned it up a bit before merging, thanks for your contribution.

lock · 2020-01-09T10:04:34Z

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

santanu-biswas added 5 commits June 11, 2016 17:10

number.multiple() enhancement - Issue #916

814adbe

‘number.multiple()’ enhanced for: 1. Accept any Number (integer or float) as argument. 2. Accept references as argument. 3. For any non-number or zero multiple base, validate now returns error instead of throwing it.

number.multiple()-enhancement

16d7ea4

Merge remote-tracking branch 'hapijs/master' into number.multiple()-e…

4a307af

…nhancement-#916 # Conflicts: # lib/number.js

Rebase

bac830b

santanu-biswas mentioned this pull request Jun 17, 2016

number.multiple() throws exception for non-integer and ref values #916

Closed

Marsup reviewed Jun 17, 2016
View reviewed changes

Error messages added to tests that return false

4ba4fa2

Marsup added the feature New functionality or improvement label Jun 17, 2016

Marsup self-assigned this Jun 17, 2016

santanu-biswas added 2 commits June 19, 2016 17:35

Merge remote-tracking branch 'hapijs/master'

a566a01

# Conflicts: # test/number.js

big.js added for number.multiple()

12bd7e2

Removed big.js

aa735d6

Removed big.js and reverted back to regular modulo operator. Also added notes to the documentation about the limitations of number.multiple() with decimals/floats.

Marsup reviewed Jun 19, 2016
View reviewed changes

Restored errors

a5183b6

Restored errors to be thrown for non-integer and zero value of multiple.

Marsup reviewed Jun 20, 2016
View reviewed changes

Allow integers & floats instead of only integers

d3ace56

Marsup added this to the 9.0.0 milestone Jun 22, 2016

Marsup closed this Jun 22, 2016

lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

number.multiple() enhancement - Issue #916 #918

number.multiple() enhancement - Issue #916 #918

santanu-biswas commented Jun 11, 2016

santanu-biswas commented Jun 17, 2016

Marsup commented Jun 17, 2016

Marsup Jun 17, 2016

santanu-biswas Jun 17, 2016 •

edited

Loading

Marsup Jun 17, 2016

santanu-biswas Jun 17, 2016 •

edited

Loading

santanu-biswas commented Jun 17, 2016

Marsup commented Jun 18, 2016

santanu-biswas commented Jun 18, 2016 •

edited

Loading

Marsup commented Jun 18, 2016

santanu-biswas commented Jun 18, 2016

Marsup commented Jun 18, 2016

santanu-biswas commented Jun 18, 2016 •

edited

Loading

santanu-biswas commented Jun 18, 2016

Marsup commented Jun 18, 2016

santanu-biswas commented Jun 19, 2016

Marsup commented Jun 19, 2016

santanu-biswas commented Jun 19, 2016

Marsup Jun 19, 2016

santanu-biswas Jun 19, 2016

Marsup Jun 19, 2016

santanu-biswas Jun 19, 2016

Marsup Jun 19, 2016

Marsup Jun 19, 2016

Marsup Jun 20, 2016

santanu-biswas Jun 20, 2016

santanu-biswas commented Jun 20, 2016

Marsup commented Jun 20, 2016

Marsup commented Jun 22, 2016

lock bot commented Jan 9, 2020


		return this._test('multiple', base, function (value, state, options) {
		const mult = Math.pow(10, Math.max(internals.precision(value), internals.precision(divisor)));

		@@ -53,12 +53,29 @@ internals.Number = class extends Any {

		multiple(base) {

		Hoek.assert(Hoek.isInteger(base), 'multiple must be an integer');


		return this._test('multiple', base, function (value, state, options) {
		return this._test('multiple', base, (value, state, options) => {

number.multiple() enhancement - Issue #916 #918

number.multiple() enhancement - Issue #916 #918

Conversation

santanu-biswas commented Jun 11, 2016

santanu-biswas commented Jun 17, 2016

Marsup commented Jun 17, 2016

Choose a reason for hiding this comment

santanu-biswas Jun 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santanu-biswas Jun 17, 2016 • edited Loading

Choose a reason for hiding this comment

santanu-biswas commented Jun 17, 2016

Marsup commented Jun 18, 2016

santanu-biswas commented Jun 18, 2016 • edited Loading

Marsup commented Jun 18, 2016

santanu-biswas commented Jun 18, 2016

Marsup commented Jun 18, 2016

santanu-biswas commented Jun 18, 2016 • edited Loading

santanu-biswas commented Jun 18, 2016

Marsup commented Jun 18, 2016

santanu-biswas commented Jun 19, 2016

Marsup commented Jun 19, 2016

santanu-biswas commented Jun 19, 2016

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

Choose a reason for hiding this comment

santanu-biswas commented Jun 20, 2016

Marsup commented Jun 20, 2016

Marsup commented Jun 22, 2016

lock bot commented Jan 9, 2020

santanu-biswas Jun 17, 2016 •

edited

Loading

santanu-biswas Jun 17, 2016 •

edited

Loading

santanu-biswas commented Jun 18, 2016 •

edited

Loading

santanu-biswas commented Jun 18, 2016 •

edited

Loading