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

oneOf and anyOf errors where default values are present #181

Merged

Conversation

tonymarklove
Copy link
Contributor

Using anyOf or oneOf shouldn’t fill JSON object with defaults of non-matching subschemas.

Prior to this change, a JSON object validating against anyOf or 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.

This pull request makes changes so that:

On Success
For anyOf, the defaults of the first matching subschema persist after returning from the call to validate.
For oneOf, the defaults of the only matching subschema persist after returning from the call to validate.

(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.

Tony Marklove added 2 commits November 21, 2014 15:12
…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.
@iainbeeston
Copy link
Contributor

I'm afraid json-schema needs to support ruby 1.8 for now... (sorry, no ruby 1.9 style hashes!)

@iainbeeston
Copy link
Contributor

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))
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@RST-J
Copy link
Contributor

RST-J commented Nov 23, 2014

The thoughts/concerns I have about anyOf and allOf in the comments above are things that should be considered separately.
So for the original intent I think its good to go 👍

RST-J added a commit that referenced this pull request Nov 23, 2014
…-values

`oneOf` and `anyOf` errors where default values are present
@RST-J RST-J merged commit c0147e9 into voxpupuli:master Nov 23, 2014
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.

4 participants