-
-
Notifications
You must be signed in to change notification settings - Fork 242
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
oneOf
and anyOf
errors where default values are present
#181
oneOf
and anyOf
errors where default values are present
#181
Conversation
…g subschemas Prior to this change, a JSON object validating against `anyOf` would be mutated by each subschema with default values, even if that subschema then failed validation. These new properties would then be present in the JSON for validation against subsequent subschemas. Now, on subschema validation failure we set the JSON data back to the original data. So only the defaults of the first matching subschema persist after returning from the call to `validate`.
…g subschemas Prior to this change, a JSON object validating against `oneOf` would be mutated by each subschema with default values, even if that subschema then failed validation. These new properties would then be present in the JSON for validation against subsequent subschemas. Now, on success, only the single passing subschema will mutate the JSON data.
I'm afraid json-schema needs to support ruby 1.8 for now... (sorry, no ruby 1.9 style hashes!) |
Looks good to merge. For all of I'd almost expect all of the sub schema defaults to be applied (in order of definition), but I think that's the existing behaviour. |
"bar" => "baz" | ||
} | ||
|
||
assert(JSON::Validator.validate(schema, data, :insert_defaults => true, :strict => true)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this test insert "view"
for the validation? I think yes as the schema validates and I think this should also be asserted here then.
And there should be a test that checks that no defaults are entered for a violated schema with defaults in anyOf.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think it should be "view"
, because the validation passed for the second of the anyOf
schemas.
The test below this is the second situation you described, I think, and it's actually asserting the opposite: the first of the anyOf
values is going to be used as the default, if set. I'm curious what the behavior would be if, say, the second anyOf
had the default. Or both. etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, lets conclude that the tests are not complete with respect to how insert defaults should behave for anyOf
. The same possibly holds for allOf
(what if more than one schema has a different default value for the same absent property).
Unfortunately the spec says nothing about defaults in allOf
and anyOf
(or I overlooked it). So we need to define the behaviour for json-schema
and therefore should make it explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In test_default_with_strict_and_anyof
, "view" will not be added, because that subschema does not match the data. In test_default_with_anyof
, because we pass empty data, the default ("view") gets added, which then means the first subschema validates, and the returned data will contain "view".
The data will only ever contain the defaults for the subschema that first passes validation. The looping search stops at this point and returns early, so it seems like a sane approach to me. But as far as I can tell the spec is silent on all these issues.
If you have specific test cases you think I should cover just let me know.
I can't even think of a sensible approach for allOf
. I suspect it hasn't been fully considered. I think I'm personally going to stop using defaults at all in schemas I write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see my mistake. I overlooked strict, which makes the first schema in the first test to not validate. In this respect I agree that this is fine.
My hypothetical question is, whether the following test should hold:
def test_default_with_anyof_no_strict
schema = {
"anyOf" => [
{
"type" => "object",
"properties" => {
"foo" => {
"enum" => ["view", "search"],
"default" => "view"
}
}
},
{
"type" => "object",
"properties" => {
"bar" => {
"type" => "string",
"default" => "baz"
}
}
}
]
}
data = {}
assert(JSON::Validator.validate(schema, data, :insert_defaults => true))
assert(data['foo'] == 'view')
assert(data['bar'] == 'baz')
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my view: no. I think assert(data['bar'] == 'baz')
should fail. But I agree that it is a judgement call. The spec says nothing either way.
Imagine the complication you would start to get when considering required
or additionalProperties
. You could build a data structure such that a call to validate
would pass, but that would return a modified data structure which would fail if validated again against exact same schema.
But again, I don't think there is a perfect solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I want to say is, that we should sort that stuff out to have a clear line for json-schema
where the spec is not explicit. But none of this relates to your PR, so I think we should have an issue that deals with this for the v3 milestone, where we make the behavior explicit, even if it stays the same as it is now.
The thoughts/concerns I have about |
…-values `oneOf` and `anyOf` errors where default values are present
Using
anyOf
oroneOf
shouldn’t fill JSON object with defaults of non-matching subschemas.Prior to this change, a JSON object validating against
anyOf
oroneOf
would be mutated by each subschema with default values, even if that subschema then failed validation. These new properties would then be present in the JSON for validation against subsequent subschemas.This pull request makes changes so that:
On Success
For
anyOf
, the defaults of the first matching subschema persist after returning from the call tovalidate
.For
oneOf
, the defaults of the only matching subschema persist after returning from the call tovalidate
.(It's not obvious what should happen for
allOf
. So we have not attempted to change behaviour here.)On Failure
We set the JSON data back to the original data.