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

Updated the common test suite #191

Merged
merged 1 commit into from
Nov 28, 2014

Conversation

iainbeeston
Copy link
Contributor

This PR just updates json-schema to use the latest version of the common test suite.

Disclaimer: I've just added some tests around how defaults are expected to work (i.e. they're for documentation purposes only) and this will probably fail with the json-schema gem.

@pd
Copy link
Contributor

pd commented Nov 28, 2014

The invalid defaults will only cause validation to fail if we use insert_defaults, it seems. I took the first example from your commit to the common test suite (json-schema-org/JSON-Schema-Test-Suite@f57888c):

schema = { 'properties' => { 'foo' => { 'type' => 'integer', 'default' => [] } } }
obj = {}

JSON::Validator.validate(schema, obj) #=> true
obj #=> {}

JSON::Validator.validate(schema, obj, insert_defaults: true) #=> false
obj #=> {"foo"=>[]}

I suppose we should add a test to demonstrate this case. Or actually change the behavior so that default values are filled in after validation? But the interaction of required + default prolly gets weird there.

@pd
Copy link
Contributor

pd commented Nov 28, 2014

Meanwhile, 👍, I see no reason not to merge this as is. It at least validates that the standard calls to validation will work as draft04 expects.

@iainbeeston
Copy link
Contributor Author

Oh really? I thought that we always insert defaults during validation? (But copy them across to the original object at the end, if insert defaults is enabled) Maybe my understanding is wrong

Anyway, I'm glad to see the tests pass!

@RST-J
Copy link
Contributor

RST-J commented Nov 28, 2014

I believe the current behavior is to insert defaults into a copy of the current object under validation and if this object does not pass validation the original object is restored and the validation error propagated.

But I like @pd's idea to first validate and insert defaults afterwards. I think this is the most intuitive approach (considering the fact that for validation they are irrelevant as per the spec) to handle defaults if we decide to do so.
However, we should only introduce/change this handling of defaults if it is not too messy to handle (anyOf, allOf pops to my mind again) and otherwise just get rid of insert defaults.

And for the original subject: 👍 ;)

RST-J added a commit that referenced this pull request Nov 28, 2014
@RST-J RST-J merged commit f9506b1 into voxpupuli:master Nov 28, 2014
@iainbeeston
Copy link
Contributor Author

Just for completeness - I was wrong and you guys were right - defaults are only ever inserted (even into the working copy) of insert_defaults is enabled.=

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.

3 participants