Skip to content
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

isMultipleOf() false negatives? #172

Open
pateketrueke opened this issue Sep 8, 2018 · 6 comments
Open

isMultipleOf() false negatives? #172

pateketrueke opened this issue Sep 8, 2018 · 6 comments

Comments

@pateketrueke
Copy link

pateketrueke commented Sep 8, 2018

I'm validating this value:

{
  "num": 0.47000000000000003
}

...against the schema:

{
  "type": "object",
  "properties": {
    "num": {
      "type": "number",
      "minimum": 0,
      "maximum": 1,
      "multipleOf": 0.01
    }
  },
  "required": [
    "num"
  ]
}

...but is failing with 'has a remainder' which I think is bit wrong?

Also I tested the same sample/schema with: tv4, z-schema and jayschema and it there's no problem.

I noticed that 0.47 passes isMultipleOf as expected but then the other validators fails...

@LinusU
Copy link
Collaborator

LinusU commented Sep 8, 2018

...but is failing with 'has a remainder' which I think is bit wrong?

Isn't that how it should work? 🤔

0.47000000000000003 isn't a multiple of 0.01? e.g. 0.47 and 0.48 is though?

@pateketrueke
Copy link
Author

pateketrueke commented Sep 8, 2018

Yeah, I think it's correct, but then all other validators are wrong... I mean, I don't know if the others are coercing somehow such long values. That's the question.

I'm want to use is-my-json-valid here too.

But then it happens: such values are generated and only the validation made by this module didn't passed.

TBH probably it's wrong behavior from my side, just I'm investigating. 💣

@LinusU
Copy link
Collaborator

LinusU commented Sep 8, 2018

Hmm 🤔

A possible solution could be to change value % multipleOf === 0 into Math.abs(value % multipleOf) < Number.EPSILON.

That could potentially let something thru that someone isn't expecting though 🤔

Appreciate the investigations! 🙌

@pateketrueke
Copy link
Author

pateketrueke commented Sep 9, 2018

I made a change:

var isMultipleOf = function(name, multipleOf) {
  var res;
  var factor = ((multipleOf | 0) !== multipleOf) ? Math.pow(10, multipleOf.toString().split('.').pop().length) : 1
  if (factor > 1) {
    var factorName = ((name | 0) !== name) ? Math.pow(10, name.toString().split('.').pop().length) : 1
-    if (factorName > factor) res = true
+    if (factorName > factor) res = (factorName / factor) % factor
    else res = Math.round(factor * name) % (factor * multipleOf)
  }
  else res = name % multipleOf;
  return !res;
}

The code I changed was related to if (factorName > factor) res = true which I did not understood properly.

With that change my tests are passing now, without issues.

Also tests from is-my-json-valid seems OK:

1..408
# tests 408
# pass  408

# ok

I wonder if that's fine and could be a PR?

@LinusU
Copy link
Collaborator

LinusU commented Sep 9, 2018

Hmm, I was trying to figure out why that function was so complicated. Turns out that there is a good reason 😆

screen shot 2018-09-09 at 13 35 15

@pateketrueke
Copy link
Author

So I found a bug? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants