Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

Fix NaN value incorrectly passing validation as a number. #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

selaux
Copy link

@selaux selaux commented Apr 29, 2015

Currently an object with a NaN parameter that is required to be a number in the schema will validate without an error. Since NaN is not actually a value that can appear in JSON and is called "Not a Number" is should not validate as a number.

var amanda = require('amanda');
var schema = {
  type: 'object',
  properties: {
    name: {
      required: true,
      type: 'number'
    }
  }
};
var data = {
  name: NaN
};

var jsonSchemaValidator = amanda('json');
jsonSchemaValidator.validate(data, schema, function(error) {
  console.log(error); // Currently outputs null, but should throw an error
});

@kuba-kubula
Copy link
Member

Hi @selaux.
In regards to http://stackoverflow.com/a/26518924 and taken into account the fact that Amanda is meant for JSON validations, I'd never receive NaN as value anywhere, so it is ok.

@selaux
Copy link
Author

selaux commented Jun 2, 2015

Well imho it's still an invalid value passing as valid. Even the post you linked sais Therefore any schema for JSON structures which permitted NaN would be permitting things that are not JSON.. If Amanda should only validate JSON it should take a JSON string as an input, parse it internally and tell me if that string is valid, not take an object and tell me it's valid although it is clearly not.

@@ -65,7 +66,7 @@ suite('JSON/Attribute/type#number', function() {
});

test('should run 21 times', function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you please change the number here too, so the expect...eql is in sync with the test comment?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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

Successfully merging this pull request may close these issues.

2 participants