ga4gh / workflow-execution-service-schemas

The WES API is a standard way to run and manage portable workflows.
Apache License 2.0
82 stars 38 forks source link

Use JSON schemas required notation everywhere (its meant to be) #82

Open david4096 opened 6 years ago

david4096 commented 6 years ago

Another artifact of converting from protobuf IDL was that we did not use the "required" notation in the schema description. In many places it is a comment, making the item optional to automated clients.

Some parameter's required properties use the correct notation: https://github.com/ga4gh/workflow-execution-service-schemas/blob/develop/openapi/workflow_execution_service.swagger.yaml#L89 , while some do not https://github.com/ga4gh/workflow-execution-service-schemas/blame/develop/openapi/workflow_execution_service.swagger.yaml#L455

https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md

jaeddy commented 6 years ago

So, this is a bit odd, but I'm pretty sure that the required: true/false convention only applies to parameters. In contrast, for properties (or other object definitions), required should be an array — i.e.:

parameter:

parameters:
- name: id
  in: path
  description: ID of pet to use
  required: true
  type: array
  items:
    type: string
  collectionFormat: csv

schema:

type: object
required:
- name
properties:
  name:
    type: string
  address:
    $ref: '#/definitions/Address'
  age:
    type: integer
    format: int32
    minimum: 0
david4096 commented 6 years ago

Yeah OK, it would be a very long title but something like "use required:true in params and array in JSON schemas definitions". ;)

https://github.com/ga4gh/data-object-service-schemas/blob/master/openapi/data_object_service.swagger.yaml#L551

https://github.com/ga4gh/data-object-service-schemas/blob/master/openapi/data_object_service.swagger.yaml#L530

ruchim commented 3 years ago

@david4096 I believe this is solved in the develop branch -- would you have a few mins to review and confirm if my understanding is correct?

patmagee commented 2 years ago

@jaeddy it looks like the openapi spec does not have any of the components, is that a gap from the liftover from swagger?

jaeddy commented 2 years ago

@patmagee they're there, but as references to individual model YAMLs under components/schemas/.

The main OpenaAPI YAML gets dereferenced when the docs are build, but the fix for components to show up in the rendered docs is still only in develop (see https://ga4gh.github.io/workflow-execution-service-schemas/preview/develop/docs/).