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

Handle undefined properties #175

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

Conversation

noirbizarre
Copy link
Contributor

This pull-request allow to validate objects with properties set to undefined if the properties are not required.

It seems coherent with this:

JSON.stringify({nullProp: null, undefinedProp: undefined})
>> {"nullProp":null}"

So with this schema:

var schema = {
    properties: {
        key: {type: 'string'}
    }
};

we should have:

tv4.validate({key: undefined}, schema);
>> true
tv4.validate({key: null}, schema);
>> false

@noirbizarre
Copy link
Contributor Author

Hi!
Any comment on this one ?
Is it possible to merge it ?

@geraintluff
Copy link
Owner

Sorry for the delay.

I like this, but it may represent a subtle change in behaviour. I'm trying to figure out whether it will screw up anybody's existing validation.

The spec's understandably silent on undefined values, so I think it might be safe, though. This is probably good to go in. (@Bartvds, any objection?)

@noirbizarre
Copy link
Contributor Author

No worries, it was only to make sure you've seen it ;)

I agree, it introduce a very subtle change, but as you said, it was not specified.
I think this is because undefined is a JavaScript only concept and does not exists in JSON (which explain the JSON.stringify behavior), so it's not defined in JSON Schema (which is consistent).

@klyngbaek
Copy link

+1
Any chance we merge this in?

@klyngbaek
Copy link

This is actually the only thing keeping me from using this module :)

@noirbizarre
Copy link
Contributor Author

Hi!
Any news on this one ?

@polpo
Copy link

polpo commented Oct 23, 2015

I just made a PR for this same issue without noticing this one. Seems like this is a fix that a lot of people need!

@noirbizarre
Copy link
Contributor Author

Yes, this is the third PR on the topic.

I think we are just waiting for @Bartvds to confirm that it can be merged.

@drom
Copy link

drom commented Apr 28, 2017

Any progress on the merge of PR?

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

Successfully merging this pull request may close these issues.

5 participants