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

v6 validation: required properties not otherwise defined are considered defined with a blank schema #65

Closed
handrews opened this issue Sep 19, 2016 · 9 comments

Comments

@handrews
Copy link
Contributor

Originally filed at json-schema/json-schema#240 by @redben

This proposes that if a property name is given in required, even if additionalProperties is false, the property should be allowed and considered to be defined with a blank schema.

Note that if additionalProperties is true or set to a schema, there is no change to the behavior- the required properties that are not directly specified (or specified via a pattern) still use the additionalProperties schema.

The only change is that a property being named in required is never forbidden by "additionalProperties": false

@handrews
Copy link
Contributor Author

Getting back to our old friend issue #55 , does this fall under the "JSON Schema generally allows nonsensical schemas as legal" ? The other question is whether this is more likely to cause a bug (because the thing in required was spelled wrong or should have been defined with a specific schema) or reduce bugs?

Since required involves duplicating property names, I am somewhat inclined to not allow it to define new properties- then it can silently get out of sync and cause exactly the kind of bugs "additionalProperties": false is often used to detect - misspelled property names.

@awwright
Copy link
Member

I'd say this is too confusing and breaks the principle that keywords are linear (if that makes any sense). Properties/keywords don't normally interact with each other, except for well-defined exceptions that don't make sense any other way (additionalProperties, additionalItems).

There's so many ways you can define a schema that's always invalid:

{type:"array", minItems:1, maxItems:0}
{not:{}}
{oneOf: [{},{}]}
{enum:[]}

etc...

@epoberezkin
Copy link
Member

I agree with @awwright. I also think that changing the JSON-schema semantics in a way that breaks backward compatibility without adding any capabilities to the standard is wrong.
The limitation @handrews proposes (don't allow required define new properties) would simply make many schemas more verbose. Consider this:

{
  "type": "object",
  "properties": {
    "foo": {},
    "bar": {}
  },
  "anyOf": [
    { "required": ["foo"] },
    { "required": ["bar"] }
  ],
  "not": { "required": ["foo", "bar"] }
}

The schema requires that exactly one property is present. With the proposed limitation you would either have to stick properties keywords in all places or to add an additional idea that required should be "aware" about properties declared anywhere in the schema.

I've seen similar proposals with regards to additionalProperties keyword - make it "aware" of all properties, not only local (used in the same schema object). I very much hope that the standard would never evolve to the point where we lose the current simplicity of all keywords being either independent or only aware of the siblings.

@handrews
Copy link
Contributor Author

handrews commented Oct 3, 2016

@epoberezkin I wasn't proposing any new/different change, I was just saying (poorly) that I disagree with the proposal. I think you, @awwright , and I are all on the same page here.

As I've played around with more complex schemas, I have come to appreciate the rules that allow what I called 'nonsensical' schemas. It turns out these can be useful in unexpected contexts, especially when not gets involved.

@handrews
Copy link
Contributor Author

@redben : This doesn't seem to be getting any support- do you want to put forward an argument in favor or can we close this out as rejected?

@awwright
Copy link
Member

I'll just close this for inactivity, feel free to reopen/refile.

Personally, I think the "additionalProperties" pattern works because it's obvious. If we start defining "properties" in terms of "required" it might get non-obvious. It's not that hard to add an additional line to the schema.

I also still favor the boolean form required, on the grounds it's not that hard to look into "properties" and make up a list of all the properties that have required==true, and use that.

@handrews
Copy link
Contributor Author

PLEASE don't go back to boolean required. It messes up parent/child independence and makes schema re-use much harder. If required is expressed as booleans, then $ref'ing a schema into an object is much harder- I have to use an allOf to add a required, and if someone put a"required": true and I want to use that schema for a non-required property, I can't.

@awwright
Copy link
Member

@handrews As I said, personal opinion

@handrews
Copy link
Contributor Author

@awwright as long as it's not in the spec I'll support your right to that opinion ;-)

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

No branches or pull requests

3 participants