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 CollectionsResponse in the OpenAPI #116

Closed
redmitry opened this issue Mar 6, 2024 · 10 comments
Closed

Invalid CollectionsResponse in the OpenAPI #116

redmitry opened this issue Mar 6, 2024 · 10 comments

Comments

@redmitry
Copy link
Collaborator

redmitry commented Mar 6, 2024

Hi all,

cohorts/endpoints.json and datasets/endpoints.json have CollectionsResponse response object derfined as a
choice of beaconBooleanResponse, beaconCountResponse and beaconCollectionsResponse which is wrong.

Should be defined as beaconCollectionsResponse precising the type according the endpoint cohorts/defaultSchema.json or cohorts/defaultSchema.json.

Best,

Dmitry

@mbaudis
Copy link
Member

mbaudis commented Mar 6, 2024

True. And the "ResultsOKResponse" should also only have this value, right?

components:
  responses:
    ResultsOKResponse:
      description: Successful operation.
      content:
        application/json:
          schema:
            $ref: https://raw.githubusercontent.com/ga4gh-beacon/beacon-v2/main/framework/json/responses/beaconCollectionsResponse.json
    CollectionsResponse:
      description: Successful collection list operation.
      content:
        application/json:
          schema:
            $ref: https://raw.githubusercontent.com/ga4gh-beacon/beacon-v2/main/framework/json/responses/beaconCollectionsResponse.json

@redmitry
Copy link
Collaborator Author

redmitry commented Mar 6, 2024

True. And the "ResultsOKResponse" should also only have this value, right?

This would be an improvement because neither "CollectionsResponse" nor "ResultsOKResponse" defines the entryType.
The question how to do this in json schema with no pain (not rewriting ALL components)
I should think a little bit...

---edit---
The ResultsOKResponse should not return the collection. But it could include concrete entryType for items.

ResultsOKResponse:
    description: Successful operation.
    content:
        application/json:
            schema:
                $ref: https://raw.githubusercontent.com/ga4gh-beacon/beacon-v2/main/framework/json/responses/beaconCollectionsResponse.json

@jrambla
Copy link
Contributor

jrambla commented Mar 6, 2024

Let me disagree, a query to a collection could return a boolean or count response, telling he number of collections (datasets or cohorts in the Bv2 Model) that fulfill the query.
The question could be: "how many cohorts do you have with men above 50 yo?"

@mbaudis
Copy link
Member

mbaudis commented Mar 6, 2024

@jordi That is all fine and I agree that this could be a use case; and also that there is IMO a need for having a general response granularity and overall the boolean/count response options.

The response still we have to be fixed from beaconResultsetsResponse to beaconCollectionsResponse etc.

But then in the long run IMO response granularity should be per resultSet to allow especially different granularities in aggregators, but also for Beacons w/ multiple datasets. (AND collections responses could also be resultSets since we're having this construct - e.g. datasets have cohorts. Same for filteringTerms which are per dataset etc - we have to deal with this already!). Just not sure if this would make things more simple (only one response type with different granularities) or more complex ...

@mbaudis
Copy link
Member

mbaudis commented Mar 6, 2024

... so in the short run beaconResultsetsResponse => beaconCollectionsResponse should do the trick, right?

@redmitry
Copy link
Collaborator Author

redmitry commented Mar 6, 2024

a query to a collection could return a boolean or count response,

... I see no way for boolean response for "collections" in the beaconCollectionsResponse. The items in the "collections" are entryType(s) beaconCollectionsResponse.json

I also wonder, how could I know if the response is the beaconCollectionsResponse or beaconResultsetsResponse from the configuration/map ?

@mbaudis
Copy link
Member

mbaudis commented Mar 21, 2024

@redmitry @jrambla

... I see no way for boolean response for "collections" in the beaconCollectionsResponse. The items in the "collections" are entryType(s) beaconCollectionsResponse.json

That is true but if you drop the requirement on response like that:

required:
  - meta
  - responseSummary

... you can use boolean or count granularity.

I also wonder, how could I know if the response is the beaconCollectionsResponse or beaconResultsetsResponse from the configuration/map ?

Yes, not possible righty now; only defined in the OpenAPI endpoints.

We actually save such info min our configs:

individual:
  request_entity_path_id: individuals
  response_entity_id: individual
  collection: individuals
  response_schema: beaconResultsetsResponse
  beacon_schema:
    entity_type: individual
    schema: https://progenetix.org/services/schemas/individual/

cohort:
  request_entity_path_id: cohorts
  response_entity_id: cohort
  collection: collations
  response_schema: beaconCollectionsResponse
  beacon_schema:
    entity_type: cohort
    schema: https://progenetix.org/services/schemas/cohort/
  pagination:
    skip: 0
    limit: 10

@mbaudis
Copy link
Member

mbaudis commented Mar 21, 2024

TODO: Rewrite this into 2 issues:

  1. remove requirement for results response element to allow summary responses
  2. determine where/how to define the response schema for entry types (collections... or resultsets)

@mbaudis
Copy link
Member

mbaudis commented Mar 21, 2024

@jordi @redmitry @costero-e Well, I guess issue 1 is different from what I mentioned very excitedly in the discussion & my claim was probably influenced by my re-thinking of the general "how are response types" are being defined (since we'll need responses with mixed boolean/count/record ... granularity).

Here,

remove requirement for results response element to allow summary responses

... would result in a summary response (boolean or count). And the same is true for beaconResultsetsResponse ... etc.

But when using OpenAPI to retrieve the allowed responses the missing option of Boolean ... responses for datasets and cohorts had been fixed at some point already; so issue 1. disappears.

This leaves 2; the only place where response schemas are associated with entry types are the OpenAPI endpoints; there is no definition of associated response types in the model itself.

So for 1. we're back to a simple bug fix for

- $ref: https://raw.githubusercontent.com/ga4gh-beacon/beacon-v2/main/framework/json/responses/beaconResultsetsResponse.json
etc.; otherwise good. But having to look at the horribly complex (and therefore potential erroneous - see the link) for figuring out the response schema is ... not nice.

@mbaudis
Copy link
Member

mbaudis commented Mar 27, 2024

I'm retiring this, to be continued in #123. I'll also clean out the PR etc. Was helpful; for the discussions, though ¯_(ツ)_/¯

@mbaudis mbaudis closed this as completed Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants