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

Invalid Syntax in beaconErrorResponse.yaml #135

Open
datsirul opened this issue Jul 10, 2024 · 5 comments
Open

Invalid Syntax in beaconErrorResponse.yaml #135

datsirul opened this issue Jul 10, 2024 · 5 comments

Comments

@datsirul
Copy link

datsirul commented Jul 10, 2024

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
Copy link
Contributor

jrambla commented Jul 10, 2024

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
Copy link
Author

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
Copy link
Contributor

jrambla commented Jul 10, 2024

"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
Copy link
Author

@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
Copy link
Contributor

jrambla commented Jul 16, 2024

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.

datsirul added a commit to datsirul/beacon-v2 that referenced this issue Jul 22, 2024
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

2 participants