ga4gh-beacon / beacon-v2

Unified repository for the GA4GH Beacon v2 API standard
Creative Commons Zero v1.0 Universal
26 stars 22 forks source link

Invalid Syntax in `beaconErrorResponse.yaml` #135

Open datsirul opened 3 months ago

datsirul commented 3 months ago

The file beaconErrorResponse.yaml contains invalid OpenAPI syntax.

Existing Syntax:

$schema: https://json-schema.org/draft/2020-12/schema
type: object
description: >-
  An unsuccessful operation.
properties:
  meta:
    $ref: ./sections/beaconResponseMeta.yaml
  error:
    $ref: ../common/beaconCommonComponents.yaml#/definitions/BeaconError
required:
  - meta
  - error
additionalProperties: true

Corrected Syntax: As this file is a response definition, it should be defined as a response object in the OpenAPI specification. The correct syntax should be:

$schema: https://json-schema.org/draft/2020-12/schema
type: object
description: >-
  An unsuccessful operation.
content:
  application/json:
    schema:
      type: object
      properties:
        meta:
          $ref: ./sections/beaconResponseMeta.yaml
        error:
          $ref: ../common/beaconCommonComponents.yaml#/definitions/BeaconError
      required:
        - meta
        - error
      additionalProperties: true

Reference: OpenAPI Specification - Describing Responses

Note: After fixing the .yaml files, the .json should be fixed as well by running the schema conversion script located in the bin folder.

jrambla commented 3 months ago

Hi Daniel,

Here is where discussion starts to get interesting... The changes you are suggesting would make OpenAPI to properly integrate the referenced schema, right? But, how much will it interfere with using that schema outside of OpenAPI? (Which we do, a lot)

datsirul commented 3 months ago

Hi Jordi,

It seems that all endpoints have a default response of:

default:
  description: An unsuccessful operation
  $ref: ./responses/beaconErrorResponse.yaml

Which leads to the beaconErrorResponse.yaml file, but, the syntax I posted is the correct way of defining a response (as described in the OpanAPI docs). For example, without this fix, this error response object is not properly generated when using a code generator with the .yaml files.

I'm not sure in what ways this schema is used outside of OpenAPI, as you mentioned. Are there other tools/frameworks that use these files or are you referring to manually creating a Beacon implementation from the schema? Would be great to get some context so I could better address the concern.

jrambla commented 3 months ago

"All" the schemas are used in validating responses by Json validators. This means: by the client when getting a response from a beacon and wanting to validate, before processing it, Or by the Beacon Verifier as a compliance tool.

datsirul commented 3 months ago

@jrambla I see, so if it's important to keep the structure of the file beaconErrorResponse.yaml as-is for various validation tools, another proposal is to fix the syntax issue in the OpenAPI enpoints files. For example, instead of this:

      responses:
        '200':
          ...
        default:
          description: An unsuccessful operation.
          $ref: ./responses/beaconErrorResponse.yaml

It will be:

      responses:
        '200':
          ...
        default:
          description: An unsuccessful operation.
          content:
            application/json:
              schema:
                $ref: ./responses/beaconErrorResponse.yaml

One thing to note is that this fix will be needed in all the endpoints that reference the error response, in the framework and model parts, which amounts to about ~60 instances.

Would this be a better approach?

jrambla commented 3 months ago

As Michael and myself said: editing the endpoints.yaml files could be done w/o much more discussion than pointing (directly in PRs would be OK). Anything that is outside endpoints.yaml (referenced by it) should be reviewed carefully. The issue above falls into the first category, hence I don't see any issue with it.