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

Required field in Schema Object is redefined #794

Closed
txomon opened this issue Sep 26, 2016 · 3 comments
Closed

Required field in Schema Object is redefined #794

txomon opened this issue Sep 26, 2016 · 3 comments

Comments

@txomon
Copy link

txomon commented Sep 26, 2016

Reading the Swagger Spec, when reading about Schema Object, the document says which list of properties are taken from the document.

In this specific case, the problem is that required doesn't even have a description in the spec (http://json-schema.org/latest/json-schema-validation.html).

However, for some reason in the swagger docs it has two colliding definitions.

The first one is the one that anyone would think on, required is a boolean that says if the object it belongs to is required or not.

The second one, required is a list of children node names that are required.

Just in case, this dual behaviour is not specified anywhere in the docs, and is just through examples that I have understood this.

The main problem is that if you put required that the Swagger editor like:

definitions:
  Profile:
    type: object
    properties:
      req:
        type: integer
        required: true

It doesn't work, you need to put it as

definitions:
  Profile:
    type: object
    required:
      - req
    properties:
      req:
        type: integer
        required: true

I understand that the reason behind that undocumented behaviour is to be able to support something like the following:

definitions:
  User:
    type: object
    required: 
      - address
    properties:
      address:
        $ref: #/definitions/Address

However, I don't see the need, as we could perfectly do:

definitions:
  User:
    type: object
    properties:
      address:
        $ref: #/definitions/Address
        required: True

Which IMO makes way more sense, it is more intuitive, and helps cohesion. Indeed, it is more compliant with the json schema spec.

Therefore, I propose to deprecate required's use as a list in that way, to start using it as a boolean.

@ePaul
Copy link
Contributor

ePaul commented Sep 26, 2016

I didn't find any use of the boolean required for schemas in the spec – it is just for the Parameter object (and in 3.0, for the RequestBody and LinkParameters objects). (All the examples in the spec with booleans are those.)

For the schema object, the required value is inherited from JSON schema, where it is an array of string values:

5.4.3. required

5.4.3.1. Valid values

The value of this keyword MUST be an array. This array MUST have at least one element. Elements of this array MUST be strings, and MUST be unique.

All examples in the spec for a schema are string-arrays.

I don't see how you can argue that a boolean required is more compliant with the JSON schema spec.

For what is more intuitive, this can be argued ... one argument for having it in the parent object is that the same schema can be used in in several parent objects, and in one of them it could be mandatory, in the other one not.

(Though already the description is a bit conflicting, it can be a description of the schema or a description of a property in a schema.)

@txomon
Copy link
Author

txomon commented Sep 27, 2016

Uops, seems like I read wrong, those big horizontal lines confused me.

The counterargument to that is that you can override objects from the parent.

Because the reference method is by defining a dictionary who has a $ref key, adding more keys to this dictionary to override the referenced object looks more natural to me. As in the last example:

definitions:
  User:
    type: object
    properties:
      address:
        $ref: #/definitions/Address
        required: True

Finally, in the swagger spec, you are parameters are defined like I suggest:

skipParam:
  name: skip
  in: query
  description: number of items to skip
  required: true
  type: integer
  format: int32

@fehguy
Copy link
Contributor

fehguy commented Sep 29, 2016

This is not a bug. For JSON schema draft 4, the required field in models is a sibling array to the properties. In addition, per spec, any siblings to the $ref keyword will be ignored.

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