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 includeResultsetResponses #136

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

Invalid Syntax in includeResultsetResponses #136

datsirul opened this issue Jul 10, 2024 · 4 comments

Comments

@datsirul
Copy link

The endpoints that use the includeResultsetResponses parameter contain invalid OpenAPI syntax.

These include the following files:

Existing Syntax:

includeResultsetResponses:
  $ref: https://raw.githubusercontent.com/ga4gh-beacon/beacon-v2/main/framework/json/common/beaconCommonComponents.json#/definitions/IncludeResultsetResponses

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

includeResultsetResponses:
  name: includeResultsetResponses
  in: query
  schema:
    $ref: https://raw.githubusercontent.com/ga4gh-beacon/beacon-v2/main/framework/json/common/beaconCommonComponents.json#/definitions/IncludeResultsetResponses

Reference:
OpenAPI Specification - Describing Parameters

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

Please do the PR.
I see that you are working with the "transcribed to yaml" version that eventually became the "source" ... I actually haven't tested that, and I don't know if anyone else has done it, hence, it is likely that errors pop up.

@mbaudis
Copy link
Member

mbaudis commented Jul 11, 2024

@jordi @datsirul While in principle it is great to spot & correct this, the overarching question we had previously was about maintaining the OpenAPI documents at all. In the Ascona developer session the conclusion was basically that:

  • the Beacon schema (esp. model) cannot be defined in OpenAPI (at least not reasonably)
  • OpenAPI documents (endpoints ...) replicate definitions done elsewhere (map ...)
  • the "fire up a UI from the specs" isn't really a use case for Beacon, besides testing scenarios
  • none of the implementers/devs actually make use of them
  • for tests we need dedicated tools
  • OpenAPI specifications can be generated
  • OpenAPI specs are misleading since many aspects of Beacon (parameter use & combination, aggregation, terminologies ...) need to be understood from other parts of the spec. and/or documentation

... just paraphrasing & YMMV on some of the arguments. But the longstanding question about ditching OpenAPI in one of the next releases should be answered (which it was) and documented as the path forward. OTOH it could be nice to keep OpenAPI docs as generated artifacts, but not as edited ones...

@zykonda
Copy link

zykonda commented Jul 15, 2024

  • the Beacon schema (esp. model) cannot be defined in OpenAPI (at least not reasonably)
  • OpenAPI documents (endpoints ...) replicate definitions done elsewhere (map ...)
    @mbaudis Those two points are not clear to me. While OpenAPI helps to define the Beacon Framework (endpoints, request, response, parameters, entities and types used), the internal data representation is not dictated by the standard and should be mapped to entities defined by standards. Or what is that "Beacon schema (esp.model)" are you referring and what cannot be defined?

datsirul added a commit to datsirul/beacon-v2 that referenced this issue Jul 16, 2024
@jrambla
Copy link
Contributor

jrambla commented Jul 16, 2024

@mbaudis @zykonda:
Please continue the conversation in Discussion #40

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

4 participants