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

NaN validates as integer, ignoring minimum #182

Closed
benbuckman opened this issue May 4, 2016 · 17 comments
Closed

NaN validates as integer, ignoring minimum #182

benbuckman opened this issue May 4, 2016 · 17 comments

Comments

@benbuckman
Copy link

const schema = {
  type: 'object',
  properties: {
    n: {
      type: 'integer',
      minimum: 1
    }
  }
};

validator = ajv()
validator.validate(schema, {n: NaN});  // true!

NaN should not be accepted as an integer, and does not meet the minimum: 1 condition.

Is this per the spec, or a bug?
Thank you!

@epoberezkin
Copy link
Member

Outside of spec I'd say... JSON-schema is for JSON validation. NaN is not JSON.
What is the use-case for supporting NaNs?

@benbuckman
Copy link
Author

It shouldn't be supported – it's not a valid number, so it should fail.
(FWIW the use case is using this module in a JS codebase where NaN can be produced, for example, by adding undefineds.)

@epoberezkin
Copy link
Member

By "supporting" I meant "failing" :)

@benbuckman
Copy link
Author

benbuckman commented May 4, 2016

I would expect that schema to fail with the input object, because the value, whatever it is, is not an integer. Wouldn't you expect the same?

@benbuckman
Copy link
Author

JSON.stringify { foo: NaN }
'{"foo":null}'

validator.validate(schema, {n: null}) is false (correctly), so validator.validate(schema, {n: NaN}) should be as well.

@epoberezkin
Copy link
Member

I will fix this particular issue. I agree that NaN should not be considered "integer" and maximum/minimum constraints should not pass, it doesn't make much sense, particularly given the use case of validating the computation results (even though it's not the main use case for validation).

I was just trying to say that supporting NaNs (= returning "expected" results) is part of the bigger issue of supporting all JavaScript types/primitives in "expected" way. In some cases expectations will vary. Why typeof NaN === 'number' if it's "not a number"? Is it expected behaviour for NaN to pass {"type": "number"} or to fail? I am not sure of either, it's a special case and there are arguments for NaN both passing, failing, throwing an exception and logging warning if it's encountered in the data. Whatever decision is made about it, it should be documented.

JSON-schema as a standard is defined for validating JSON; in JavaScript terms - the result of JSON.parse(). So how a validator should behave with NaN or any other language-specific artefact is outside of spec - ajv is definitely not the only one that will pass NaN as an integer. I am sure there are other edge cases that would return unexpected results - there are no tests for them at the moment.

@epoberezkin
Copy link
Member

NaN now fails minimum and maximum, fails type: integer but passes type: number.

If it were to fail type: number then it would have passed the schema {minimum: 1} because minimum keyword only applies to numbers.

@epoberezkin
Copy link
Member

in 4.1.0

@benbuckman
Copy link
Author

Thank you @epoberezkin !

@achrist
Copy link

achrist commented May 4, 2018

@epoberezkin Could you elaborate a little more on why you chose to allow type: number to allow NaN? I understand that in Javascript NaN is considered a number but in JSON NaN is not an accepted value so my assumption would be that if I pass NaN to ajv and its expected type: number it would fail.

If I understand correctly I can use a min or max in order to get ajv to not accept NaN but that would mean I have to fill my schema with a bunch of cruft (meaningless mins or maxes) that might be confusing to anyone that looks at. For example I have properties that are of type: number but the allowed range is not an easily defined min or max. It can accept essentially -Infinity to Infinity so there is no clear min or max I could set that wouldn't look bazaar to anyone that crosses it.

One option I could see if you don't want to just flat out say NaN is invalid for type: number is maybe add an option to ajv where I could explicitly tell it to not allow NaN. This would allow consumers of ajv to toggle it on/off as they see fit.

@scottrippey
Copy link

I fully agree with @achrist on this! If it's not a valid JSON value, like NaN or Infinity, then it should not be valid for type: number.

If there's any "backwards compatibility" concern, then this feature could certainly be introduced as an option, like allowNaN: false.

@epoberezkin
Copy link
Member

in JSON NaN is not an accepted value so my assumption would be that if I pass NaN to ajv and its expected type: number it would fail.

If your data is JSON then there would be no NaNs. Given that Ajv validates arbitrary objects, it has to make decisions about how to validate everything that's non JSON. Following your logic, everything that cannot be JSON should be validated as invalid, that would have been a very limiting design choice - users use Ajv for a lot of validation scenarios beyond JSON validation.

If I understand correctly I can use a min or max in order to get ajv to not accept NaN but that would mean I have to fill my schema with a bunch of cruft (meaningless mins or maxes) that might be confusing to anyone that looks at.

A better approach is to prevent NaNs passed to Ajv in the first place. NaN can only appear as the result of some expression, and the use case for validation is to do it before any other data manipulation, so it's not quite clear why would you get NaNs into your data in the first place.

If there's any "backwards compatibility" concern, then this feature could certainly be introduced as an option, like allowNaN: false.

I will think about it. I am not convinced that validating computation results/NaNs is a common enough use case that justifies one more option - there are way too many already.

@vladyslav2
Copy link

@epoberezkin
Evgeny first you are telling:

users use Ajv for a lot of validation scenarios beyond JSON validation.

Then:

A better approach is to prevent NaNs passed to Ajv in the first place.

Which mean that Ajv works only with valid JSON and your first statement then makes no sense.

A problem that currently have is that I need to run NEW validation library on top of the Ajv just to convert non-JSON data to JSON data in order to be sure Ajv will catch all errors.

I'm okay if that's a case but I would like to have some buildin function even if I need to run it separately or some additional parameter for a constructor.
Mainly my problem that values like NaN, undefined and etc get passed when you are not expecting them to pass and the last place you are looking for a problem is ajv validation

@diachedelic
Copy link

A problem that currently have is that I need to run NEW validation library on top of the Ajv just to convert non-JSON data to JSON data in order to be sure Ajv will catch all errors.

Exactly - I am using ajv to ensure any javascript objects i write to disk conform to a schema. I just had the baffling experience of finding nulls in deserialized, validated (!) data which was supposed to contain only { type: 'number' }. I realised after some time that a few of the values must have been NaN or Infinity before serialization.

In my case it would have eased my debugging experience if ajv had rejected the NaNs. Just my 2 cents.

@m4recek
Copy link

m4recek commented Nov 28, 2019

We have "found" this problem as well. We are using ajv to validate objects sent to and loaded from blob storage.

Our usecase is

  1. run first part of calculations
    1.1 store result to blob storage (validate by ajv first)
    1.2. trigger second part of recalculations

  2. run second part of calculations
    2.1 load from blob storage (validate by ajv)
    2.2 run recalcs (one route if field is number second one when null)
    2.3 save to blob storage (validate by ajv)

To make matters worse our field has a type type: ['number', 'null'] which means when we have calculated invalid result NaN in first recalc process it has passed ajv validation and was converted to null by JSON serialization. This validated in second part of recalc as valid again (null this time) and took second part of recalculation on invalid route making whole process useless.

Needles to say it took us quite some time to find what is happening..

Its completely unexpected for NaN, -Infinity, +Infinity to validate as valid numbers, my quick test in the office found that 8 from 8 programmers would fail to write correct schema with this behaviour.

As a workaround we have started using ajv-bsontype which will correctly fail validation with message: 'should be number got NaN'

@epoberezkin
Copy link
Member

Looks like there are a few cases to validate NaN differently. I am ok with adding an option strictNumbers (false by default) which, if set, would lead to NaN and Infinity failing type: 'number'

In the next major version the option can be flipped (become true by default).

@issacgerges
Copy link
Contributor

I've opened a PR to add this option #1219

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

No branches or pull requests

8 participants