Discrepancies between descriptions and type definitions

[blankdots]

Short version:

More of a request rather than an issue, and maybe there is some CA4GH convention (and I could not find anything about it), but there seems to be some discrepancies between field descriptions and type definitions in the OpenAPI specification, can these be addressed ?

Despite the long version the points below are not so daunting or urgent, but improving them would definitely make the specification easier to follow and implement.

Long Version:

1. https://github.com/ga4gh-beacon/specification/blob/master/beacon.yaml#L462-L467
   is of type: array but in the description it says it can also take null, but that is not a valid value for type:array, at best in JavaScript null is type:object
2. https://github.com/ga4gh-beacon/specification/blob/master/beacon.yaml#L462-L467 it says that error can be null but it has a required key errorCode ... well a null value cannot have a required key
3. https://github.com/ga4gh-beacon/specification/blob/master/beacon.yaml#L454-L459 it is a bit confusing, and some can interpret that this key can be null if error key is non-null,
   but here is the thing the type: boolean can never be null, there can be a use case where the assemblyId is not supported and the exists can be null, and I assume it is the general rule when something is not supported to be null.
4. by no means this list is exhaustive :)

The issue:

Assuming one make use of a JSON Schema, (maybe a bias here as it is a way I would implement it for this specification) to validate + test that something adheres to a specification for either a request or response, then:

• If one would only take into consideration the type definitions then it would be breaking the details specified in the descriptions.
• If one would take both the types and the description from specification into account, then that means the specification is incomplete.

Possible reason why this happened ?

When moving from 0.3 Avro to 0.4 OpenAPI it was initially with both types defined (null and some other type), and I think 0.4 kinda broke backwards compatibility to 0.3.
There are many examples in the 0.3 version, e.g.:
https://github.com/ga4gh-beacon/specification/blob/v0.3.0/src/main/resources/avro/beacon.avdl#L62

How to fix (options):

1. remove null from descriptions for fields that are not required (in the implementation one would just have to check if the key exists), or if there is a an actual use case for the null (and I am aware of the point of null meaning there is no data) values make it specific and apply Fix option 2.

2. Make use of nullable: true https://swagger.io/docs/specification/data-models/data-types/
   In JSON Schema it would be "type": ["number", "null"] or make use of anyOf/oneOf

Is this backwards compatible ?

• Fix 1 would make it easier for backwards compatibility, as it will not add additional type definitions, even though some fields are required, because nullable is not set.
• Fix 2 would add new type values, but should not break that much - However for this fix please clarify what are the exact use cases for null, because these are not 100% clear also e.g. datasetAlleleResponses is not required why even add null?

[sdelatorrep]

Regarding the examples (comment in #254), you mean that to be compliant with the spec they can't be null, is this correct? What would be the correct syntax to specify they can be omitted (not mandatory) or null (present but empty)? Would add nullable: true to BeaconError work?

    BeaconError:
      description: >-
        Beacon-specific error. This should be non-null in exceptional situations
        only, in which case `exists` has to be null.
      type: object
      nullable: true
      required:
        - errorCode
      properties:
        errorCode:
          type: integer
          format: int32
          example: 'same as HTTP status code'
        errorMessage:
          type: string

Does this mean that error field can be omitted (it's not mandatory in, for instance, BeaconAlleleResponse) or error: null?

[blankdots]

From my point of view null means no data , and it is different than having an empty response, or that no data was found when querying.
The specification requires for BeaconAlleleResponse only beaconId meaning other fields such as error do not need to be present or used if there is no need to do so e.g. an actual error occurred.
The same goes for BeaconDatasetAlleleResponse which requires only datasetId, other fields can be omitted.
Also datasetAlleleResponses should only be null if the whole beacon contains no data, otherwise an empty array [] would do just fine, if one wants to show the response did not find any datasets.

Now from an implementation standpoint I would assume that most consumers of the API would make use of exists - meaning that nullable: true besides the boolean type, should be added given that an error is present
Other fields do not need to be included in the error response or if they do not contain any valuable data, hence making them nullable will not provide any value, that includes error.

Another issue to address is: https://github.com/ga4gh-beacon/specification/blob/master/beacon.yaml#L119 usage or alternateBases and variantType i think https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/#oneof should be used. It is my understanding both cannot be used in queries, it should be only one of them.

[mbaudis]

@blankdots Maybe open an issue about this, in general?

Another issue to address is: https://github.com/ga4gh-beacon/specification/blob/master/beacon.yaml#L119 usage or `alternateBases` and `variantType` i think https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/#oneof should be used. It is my understanding both cannot be used in queries, it should be only one of them.

There is also the related problem of the different "start" attributes, which in principle could be solved through having start, end just as arrays (and get rid of the ...Min, ...Max attributes.

We actually dump all values for the start and end attributes into 2 sorted arrays, and then

• start = (start_array)[0]
  • error if (start_array) != 1
• start_min = (start_array)[0]
• start_max = (start_array)[-1]
• end_min = (end_array)[0]
• end_max = (end_array)[-1]

... etc.

[blankdots]

@mbaudis i see your point and here it is: #273 although it is related to these discrepancies, that is why i added it as part of that comment.