json-schema-org / json-schema-spec

The JSON Schema specification
http://json-schema.org/
Other
3.71k stars 262 forks source link

Suggestion: optional keyword to complement required #1112

Open Altreus opened 3 years ago

Altreus commented 3 years ago

I don't write objects with optional properties most of the time. I tend to prefer to use polymorphism to allow one of several data structures. When I do have an optional field, it's often something like "comments" or "extra info", and even then I don't have any philosophical objection to simply making it required but allowing it to contain the empty string.

An optional keyword to complement required would greatly benefit me (and hopefully others), without treading on the toes of people who are writing schemata with many optional values.

My proposition is thus:

Hope that's both clear and a useful idea.

gregsdennis commented 3 years ago

This seems like a duplicate/variant of #1096.

jdesrosiers commented 3 years ago

I can see how this could work. It would work similarly to additionalProperties in that it would collect the property names declared by properties and use that to make its assertion. The only change I would make to the proposal is to not forbid required when using optional. That would make a good linting rule, but it's unnecessary from JSON Schema architecture point of view.

Something like this has been requested for a long time, but this is the first proposal of this type that I think fits well into the JSON Schema architecture and I think we should consider it. Requiring properties by default (#1096) and requiredProperties (#846) are problematic because they assert more than one thing: (1) a property schema and (2) that the property is required. This proposal is better because it deals only with which properties are required.

Although I hate to add more keywords that break keyword independence, I think this is a good solution. In particular, this wouldn't have the same kind of problems with combining schemas that additionalProperties has.

karenetheridge commented 3 years ago

The only change I would make to the proposal is to not forbid required when using optional. That would make a good linting rule, but it's unnecessary from JSON Schema architecture point of view.

Agreed. But this also means we need to work out what happens when they are used together: for example if one schema pulls in another with $ref, and the reffed schema contains one of these keywords and the first schema contains the other. Would 'required' always overrule 'options'? Or the other way around? or should the innermost keyword always overrule the outermost (akin to a subclass overriding a superclass)?

edit: a subschema keyword overriding a superschema keyword isn't even doable, since both are evaluated, each in their own contexts. keywords generally need to stand on their own when not siblings, unless we explicitly define an annotation behaviour that can carry state.

jdesrosiers commented 3 years ago

we need to work out what happens when they are used together

When they are used together, they each assert independently just like any other keyword. That does mean it's possible for them to contradict each other, but not in any way that isn't possible already in JSON Schema. They don't need to interact or override each other. optional would be no more affected by a reference than additionalProperties is. That's what I like about this proposal.

Altreus commented 3 years ago

Grateful to hear my idea has legs! Happy you agree that it fits in without obviously breaking anything.

The only edge case I can think of is accidentally adding a property to a schema, forgetting that optional: [] was provided, thus breaking backward-compatibility. This is probably another linter case, however, rather than a strong reason to disallow that. Personally I would prefer to release a new version if I weren't going to put that new property in the optional array.

jdesrosiers commented 3 years ago

I implemented optional in my draft-future dialect. If you want to try it out, you can go to https://json-schema.hyperjump.io and use $schema": "https://json-schema.org/draft/future/schema".

Note: Things in my draft-future dialect are not necessarily going to appear in the next draft and this dialect is not available in npm. It should only be used for the purposes of evaluating potential changes.

awwright commented 3 years ago

Requiring properties by default (#1096) and requiredProperties (#846) are problematic because they assert more than one thing: (1) a property schema and (2) that the property is required

I don't believe doing multiple things is a problem as such. If there is a keyword does two things (an "authoring convenience keyword"), then as long as we also have keywords that do each of those things separately, there is no issue.

The problem would be if keywords do multiple things and there's no way around it. For example, suppose "minLength" (which only applies when the input is a string), also required that the value be a string, or if it were a number, applied to numbers as well. We would have to craft a more complicated if/then statement (if type="string" then minLength: 1), which would be more difficult to reason about.

jdesrosiers commented 3 years ago

I don't believe doing multiple things is a problem as such. If there is a keyword does two things (an "authoring convenience keyword"), then as long as we also have keywords that do each of those things separately, there is no issue.

Fair enough. I think your example is a bit of a stretch, but I agree with your point. However, I would still prefer a solution with a keyword that only does one thing. Requiring properties by default is off the table. A requiredProperties keyword is reasonable, but not ideal. An optional keyword is not perfect either, but I think it has the fewest issues.

awwright commented 3 years ago

However, I would still prefer a solution with a keyword that only does one thing. Requiring properties by default is off the table.

I would say that an "optional" keyword does at least two things.

It's presence means changes the behavior of "properties" so that it requires the listed properties; except the ones listed by "optional". This seems more complicated to me from both an authoring perspective as well as an implementation perspective.

If I understand correctly, these schemas have the same behavior:

{
   properties: {
      "name": { type: "string" },
      "phone": { type: "string" }
   },
   optional: [ "phone" ]
}

Versus:

{
   requiredProperties: {
      "name": { type: "string" }
   },
   properties: {
      "phone": { type: "string" }
   }
}

The only advantage I see to an "optional" keyword is it allows you to group/order properties the way you want, instead of forcing you to group them by if it's required/optional. But this has no effect on the validation, it's purely an authoring choice.

gregsdennis commented 3 years ago

It's presence means changes the behavior of "properties"

Not necessarily. optional can read the contents of properties to get a list of keywords, then enforce that the ones not declared within itself are present. properties can remain as is, oblivious to the presence of required or optional.

gregsdennis commented 3 years ago

I do like the idea of enforcing that both cannot be present. This can be enforced in the meta-schema.

karenetheridge commented 3 years ago

properties can remain as is, oblivious to the presence of required or optional.

Yes, this is the correct way forward.

This can be enforced in the meta-schema.

I agree with this too (but since the specification text itself will mention this restriction, it is not out of the question for an implementation to enforce this with code as well).

jdesrosiers commented 3 years ago

This seems more complicated to me from both an authoring perspective as well as an implementation perspective.

The whole reason I implemented this keyword was to explore things like this. I can assure you that optional was trivial to implement and requires nothing remotely unusual or unique to JSON Schema. As others have pointed out, it doesn't effect other keywords and makes it's own assertions which makes it a good fit.

As for making authoring complicated, I don't see how that would be, but having a web based implementation available makes it easy for us all to try it out and evaluate the developer experience implications.

I do like the idea of enforcing that both cannot be present.

It could be done, but I consider this kind of thing out of scope. I see it as a linting responsibility. I'm sure there's no good reason to use both at once, but there's no reason they couldn't both be used.

Altreus commented 3 years ago

$schema": "https://json-schema.org/draft/future/schema"

Brilliant. If I ever get back onto that project in a reasonable time frame I'll update the code to pull this schema and try it out. Hopefully, others will have an impetus to try it as well.

I'm sure there's no good reason to use both at once, but there's no reason they couldn't both be used.

Happy to stay on the sidelines on this point, but I would observe that if one allows a situation, one should handle it; therefore it should be defined what happens if you do use both at once, or defined that you cannot. Or it should at least be defined that it is undefined, which hedges against someone in future coming up with a genius idea that we can put in here.

jdesrosiers commented 3 years ago

Just to be clear, the draft-future dialect is not really a public implementation. It shouldn't be used for anything other than to evaluate if it's a good idea to add to the next draft. I'd have to create a vocabulary and release an implementation publicly if we expect people outside of this discussion to try it out on a real project.

it should be defined what happens if you do use both at once

Each keyword would behave independently, just like any other keyword. If you had "required": ["foo"], "optional": ["foo"], then an object with a "foo" property would pass the required assertion because it has a "foo" property and it would pass the optional assertion because "foo" this keyword considers it optional. It reads like a contradiction, but it's unambiguous how it would evaluate in JSON Schema.


I thought of an example where it would sense to use both required and optional in the same schema. Let's say we have this schema.

{
  "$id": "https://example.com/schemas/base-object",
  "type": "object",
  "properties": {
    "aaa": { "type": "string" },
    "bbb": { "type": "string" }
  },
  "optional": ["bbb"]
}

We want to extend this schema add a required property "ccc" and make the "bbb" property required.

{
  "$id": "https://example.com/extended-object",
  "allOf": [{ "$ref": "/schemas/base-object" }],
  "properties": {
    "ccc": { "type": "string" }
  },
  "required": ["bbb"],
  "optional": []
}

The "bbb" property isn't declared in this schema's properties, so we have to use required, but that doesn't mean it doesn't make sense to use optional as well.

gregsdennis commented 3 years ago

I thought of these two cases as well.

Suppose foo, bar, and baz are available properties, and required lists foo.

I do like the usage of having bbb in optional but not in properties. This means that it's not required, but it would be covered so that keywords like unevaluatedProperties would skip them. It seems like a more intention-clear alternative to

{
  "properties": {
    "bbb": true,
  }
}
jdesrosiers commented 3 years ago

If optional lists foo, then we have an invalid schema because foo can't both be optional and required.

This isn't correct. This is what I meant when I said it reads like a contradiction, but it's not. "optional": ["foo"] is equivalent to "required": ["bar", "baz"] which does not conflict with "required": ["foo"]. optional doesn't assert what is optional, it asserts what is required. It just does so backwards. It computes what is required based on it's value and the properties declared in properties.

I do like the usage of having bbb in optional but not in properties. This means that it's not required, but it would be covered so that keywords like unevaluatedProperties would skip them.

I don't think optional would effect unevaluatedProperties any more than required does. If we think it should, we would want to change required as well so they behave similarly.

awwright commented 3 years ago

optional doesn't assert what is optional, it asserts what is required.

This isn't intuitive to me, but it makes a lot more sense than what I was thinking at first.

jdesrosiers commented 3 years ago

This isn't intuitive to me

If this is confusing to us, it's going to be much more so for users. Maybe we can come up with a better name. Something like requireAllPropertiesExcept is descriptive, but no one want to have to write all that out. Anyone have any ideas?

gregsdennis commented 3 years ago

That behavior is also unintuitive to me, as illustrated by my examples.

karenetheridge commented 3 years ago

It's too bad that required isn't called requiredProperties, or that would naturally imply optionalProperties. This points a little more clearly to the sibling properties keyword being used as a reference.

Altreus commented 3 years ago

This isn't intuitive to me

If this is confusing to us, it's going to be much more so for users.

It's confusing framed like this, but it seems to me the outcome of applying rules like this basically means "required overrides optional", which is easy to understand.

"optional": ["foo"] is equivalent to "required": ["bar", "baz"] which does not conflict with "required": ["foo"].

If we expand this further:

{
  "properties": {
    "foo": { "type": "string" },
    "bar": { "type": "string" },
    "baz": { "type": "string" }
  },
  "required": ["foo"],
  "optional": ["foo"]
}

We consider this to be:

{
  "properties": {
    "foo": { "type": "string" },
    "bar": { "type": "string" },
    "baz": { "type": "string" }
  },
  "required": ["foo"],
  "required": ["bar", "baz"]
}

Obviously, repeating that is invalid, but I see no sensible objection not to consider optional to be equivalent to a computed required and merged with any existing required:

{
  "properties": {
    "foo": { "type": "string" },
    "bar": { "type": "string" },
    "baz": { "type": "string" }
  },
  "required": ["foo", "bar", "baz"]
}

As a result, it seems to be emergent behaviour that "required overrides optional", and thus it's quite easy to encode this behaviour in an easily-understood rule.

jdesrosiers commented 3 years ago

The problem isn't that it's difficult to understand or to explain. The problem is that people will see the word "optional", assume they know what that means, and skip over the explanation of what it really means. The unintuitive part is that it doesn't mean what it sounds like it should mean.

ericsampson commented 3 years ago

Yeah it’s a good idea, but calling it optional seems like it’s going to be a trainwreck for the reasons mentioned above.

There’s got to be some other option like requiredExcept or requiredExceptFor or …

gregsdennis commented 3 years ago

It's ironic that {"not":{"required”:[...]}} doesn't do this...

jdesrosiers commented 3 years ago

The best suggestion I have is requireAllExcept.

Relequestual commented 3 years ago

It's ironic that {"not":{"required”:[...]}} doesn't do this...

I've seen people trying to do it this way though. not is often confused.

Relequestual commented 3 years ago

The best suggestion I have is requireAllExcept.

I agree this is the best we have.

I suggest we leave it open for a week, and barign further comments, call consensus and move to PR.

racquetmaster commented 2 years ago

My 2 cents is I greatly prefer requiredProperties vs requireAllExcept. I think it is a simpler concept to understand and makes reading and writing schemes much easier and more straight forward.

For example, to say that all properties are required (a very common case) would require either:

  1. Using required and duplicating all of the property names (which is prone to sync issues). This is the current way.
  2. Uses requireAllExcept: [] - which is harder to read. It switches the implicit meaning when reading the properties keyword from these are optional to these are required unless specified in this other list.
  3. Use requiredProperties - which is straightforward to read. These are the required properties, these are the optional properties. In the event you want to group all of the properties together, you are still free to use the original required keyword.

In addition, one of the common mistakes that I see developer's make when writing schemas is forgetting to specify if something is required or not. This wouldn't solve that issue, it would just have the same problem with a different default behavior.

Considering this addition alone, I think that requireAllExcept is the clearer of the names proposed. Although if implemented, would still like to see requiredProperties added as well.

gregsdennis commented 1 year ago

A proposal doc for this should be created. See https://github.com/json-schema-org/json-schema-spec/pull/1450 for an example.

spacether commented 1 year ago

In my opinion adding this option adds complexity without adding much value. These new optional keywords may not have schema value definitions in properties. That makes it harder for codegen use cases to know what value types those properties should have.

If one really wants separate buckets for required and optional properties, my suggestion is to:

  1. remove required
  2. remove properties
  3. replace them with optionalProperties + requiredProperties maps of string property name to schema

Which could be done in openapi v4.0.0 (codenamed moonwalk) referring to a new later version of the json schema spec which contains the above suggested changes

Right now optional properties can be describe with properties defined only in properties and not in required. One can fully describe optional use cases right now, so in my opinion this new optional keyword is unneeded.

gregsdennis commented 1 year ago

Which could be done in moonwalk (4.0.0)

Please be aware that OpenAPI is not the only consumer of JSON Schema. We have to consider how changes like this impact all consumers. Changes that benefit OpenAPI could make JSON Schema very painful for another consumer.

spacether commented 1 year ago

Sorry, my mistake. The next major (breaking) release of json schema then is when I would propose adding optionalProperties + requiredProperties to meet the needs of the author of this issue

ahunigel commented 9 months ago

any update?

jdesrosiers commented 9 months ago

any update?

Progress on this issue ended up stalling out due to lack of consensus on a solution. I expect discussion to to resume once we get some other things worked out, but I wouldn't expect anything to be available in the near future.

gregsdennis commented 3 months ago

Now that we have the feature life cycle and proposal mechanism in place, development of this feature will need to continue per that process.

Removing from the stable release development milestone and project.