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

Object properties should be required by default #1096

Closed
erfanium opened this issue May 2, 2021 · 6 comments
Closed

Object properties should be required by default #1096

erfanium opened this issue May 2, 2021 · 6 comments

Comments

@erfanium
Copy link

erfanium commented May 2, 2021

Everything should be safe by default. For comparison, consider these two cases:

A. All properties are optional by default and I forget to make a property required. Result: I most probably introduced a major security bug
B. All properties are required by default and I forget to make a property optional. Result: I introduced a bug, but not a major security bug

Case A has happened to me several times.

I'm not entirely sure if this has been investigated before, but I could not find anything relevant to my search.

@gregsdennis
Copy link
Member

Some history may shed light on the current state.

In draft 3, required was actually part of the subschema under the properties keyword, e.g.

{
  "properties": {
    "foo": {
        "type": "integer",
        "required": true
    }
  }
}

This made sense in line with your option B, where you declare that a property is required along with defining the property. It makes it harder to forget to specify that it's required.

However, in terms of the schema itself, it makes things awkward because keywords should only know about the immediate schema that contains it and any subschemas it may contain. So required here can't know that it's part of a property declaration. Since it can't know that it's a property, it becomes the responsibility of the properties keyword to enforce required, which is a shift in responsibility. It also brings into question what this might mean:

{
  "type": "integer",
  "required": true
}

required here has no meaning and is weird, but it's also a completely valid draft 3 schema.

It also introduces a lot of duplication, repeating required for each property.

So as of draft 4, required was relocated to its currently location and changed to a list specifying which properties it applied to.


This change predates my involvement with JSON Schema, as I came in just as draft 6 was starting to be discussed. There was probably some discussion around implementing it as you suggest in option B, but I'm not sure.

@erfanium
Copy link
Author

erfanium commented May 2, 2021

@gregsdennis

required was relocated to its currently location and changed to a list specifying which properties it applied to.

What you explained is more about location. I agree with you and I think the draft 4 change was right.
This is my suggestion change, From:

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

To

{
  "type": "object",
  "properties": {
    "foo": {"type": "integer"},
    "bar" : {"type": "string"}
  },
  "optional": ["bar"]
}

I mean the implementation of this concept: Keep every object properties required, mark some of them as optional property if needed

A real-world example is Typescript interfaces, interface properties are required by default unless you put a ? character after property name that indicates it's an optional property:

interface X {
   foo: number // required
   bar?: string // optional
}

One other example is SQL nullable columns which expresses the same concept

@gregsdennis
Copy link
Member

I mean the implementation of this concept: Keep every object properties required, mark some of them as optional property if needed

Yes, I also explained that this was possibly discussed during this transition and that the discussion predates me.

I understand your request. I was just giving some historical context to the proposal.

Lastly, please know that it's a significantly breaking change and such things require a lot of discussion.

@awwright
Copy link
Member

awwright commented May 2, 2021

In short, I think this would be solved by #846, the "requiredProperties" keyword.


Everything should be safe by default

I agree in principle, although I'm not sure how JSON Schema can be "safe" or "unsafe" per se because it's declarative, it doesn't let you perform any actions by itself.

All properties are required by default

If a property is described and it's required by default, then how would you make it optional instead?

JSON Schema doesn't have a way of doing this; instead, keywords do not overlap. For example, to define a value as "must be a non-negative integer", you specify both the "type" and "minimum" keywords; "minimum" keyword does not do anything that the "type" keyword would be used for. With this in mind, it makes sense you create a "required property" by specifying both "required" and "properties".

I think the practical solution is we don't have a 'default' as such, but we have a "syntactic sugar keyword" where "requiredProperties" implies the behavior of both "properties" and "required.


Also, I think this is one of those things that's going to make more or less sense depending on your background. For example, in GraphQL schemas, all properties are nullable unless specifically marked as non-nullable.

@erfanium
Copy link
Author

erfanium commented May 3, 2021

In short, I think this would be solved by #846, the "requiredProperties" keyword.

I had not seen this, very good! I think it's good for a non-breaking solution.

I'm not sure how JSON Schema can be "safe" or "unsafe" per se because it's declarative.

Think about how a human defines a JSON schema. For example, first I define an object type, then I define object properties, and finally required properties. When I say safe by default, one of its meanings is that if this process (defining a JSON schema) leaves half done, is it still safe?
Yes, JSON schema doesn't perform any action by itself, but we can estimate where it will be used most often. for me, I mostly use it as an HTTP API request body validator.

it makes sense you create a "required property" by specifying both "required" and "properties".

I think comparing primitive types (strings, integers, ...) with objects is not always correct. because we can't set defaults for primitive types schema, we can't set a default minimum value for a integer type! but we can set object properties required by default.

Also, I think this is one of those things that's going to make more or less sense depending on your background. For example, in GraphQL schemas, all properties are nullable unless specifically marked as non-nullable.

There has been talk about this as well:
graphql/graphql-spec#63

But this issue is different from ours in two ways.
First, the reason that they chose nullable fields by default here is not related to JSON schema and its usage
Second, This issue is much more important to us in some ways, beacuse JSON schema's are mainly used for validating write requests, but GraphQL mainly used for read stuff

@handrews
Copy link
Contributor

handrews commented May 6, 2021

@erfanium since you agree that #846 is a good solution, I'm going to go ahead and close this issue in favor of that one. Thanks for the further information- feel free to comment over there. Keeping one issue per topic ensures that the level of interest is visible in one place.

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

4 participants