-
Notifications
You must be signed in to change notification settings - Fork 22
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
Collections response fix endpoints #121
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything makes sense, as cohorts and datasets endpoints should be a collection response, not a resultSet response.
I'm also seeing updated the way of showing arrays in examples:
examples:
-
- OMIABIS:0001017
This will need to be updated everywhere to follow this pattern, I guess.
I am in doubts... Before changes, only the "root" endpoint was returning the collection an the rest "usual" ResultsOKResponse (like any other entryType e.g. "biosamples"). Are we changing the contract? D. |
@redmitry Oh well, you are right w/ this breaking the resultsetResponses if allowing those endpoints (which I guess we should). I guess the naming of |
Reviewer spotted other breaking fixes that need to be reviewed again.
Have a look at ea4324b Only for cohorts so far. My reasoning there is that But generally this all is also a big argument for ditching the concept of having separate Boolean and Count response types and just having a single one for each data response flavour (collections, resultsets, filtering terms ...) where we the data payload (and count) are optional... This is probably against some "how it should be done" rule and I remember that the different response schemas looked nice when drafting them; but now we see a confusion of alternative response types and alternative payload types. Alas, for now how about this? |
IMO "ResultsetsResponse" is more appropriate name. |
But resultSets response gives results splitted by dataset and if I'm not wrong, boolean and count responses don't do that, do they? |
Yes and I really would like to see this (since the previous generic Now, in principle we should then propagate this everywhere we have such a generic to make it clearer what is being executed. Pro: clarity, won't break anything since references inside of schemas. I'm clearly pro here, but for all entry types. |
@costero-e I left the datasets just since I didn't want to do changes I have to reverse ... ResultSets are actually split by any type of collection:
... so a datasets response for individuals could be for the dataset or split for its cohorts, theoretically. |
I think you tagged another Oriol @mbaudis, jeje. Ok, thank you, it's good you are just giving an example using cohorts.
For me, putting the "countResponse" and "booleanResponse" under a "resultSetsresponse" makes it a bit confusing. That's why I'm more in favor of keeping "resultsOkResponse". |
@costero-e It is confusing in any case. Therefore I would just prefer that we have only a ResultSetsResponse with a Boolean granularity instead of switching responses. But overall the new way would be slightly less confusing... You need to name the response anyway and Anyway, naming is schema internal. And IMO it is correct to have an endpoint with a But again, this is a reason for having a single response for and the granularity handled inside it. |
... and another inconsistency is the "cohorts have individuals for records retrieval while datasets have all entry types". I understand the argument ("cohorts as a group of individuals") but still you may want to get the samples etc. Not wanting to change right now but good to keep in mind. |
@redmitry @costero-e So WDYT - moving ahead in the current version w/o going for the other "ResultsOKResponse" instances, but doing it for the collections which have these 2 types of responses? I'd like to cloes #116 I guess we should have the "response schemas for entry types should be defined somewhere" in a general "remove dependencies on OpenAPI" discussion/issue. |
Again we are changing the contract: was: "/cohorts/{id}": {
"get": {
"responses": {
"200": {
"$ref": "#/components/responses/ResultsOKResponse"
}, now: "/cohorts/{id}": {
"get": {
"responses": {
"200": {
"$ref": "#/components/responses/CollectionsResponse"
}, Most probably that was the error in the spec coz I remember reporting this to Oriol #98, but since I implemented my java implementation from spec. Should we fix both the spec. and the implementation? Cheers, Dmitry |
@redmitry This just changes the name I've fixed now the messy partial changes in the last commit (all the separate ones should be squashed...).
|
I do not doubt that it probably (😏) should, but the current spec's "ResultsOKResponse" is So that my comment: if we want the Dmitry |
collections response new prototypes cleanup This fixes the wrong response for some of teh collections endpoints and changes the (definitely now) ambiguous ResultsOKResponse to the correct instances of * CollectionsResponse * ResultsetsResponse
This is just a temporary removal of files colliding w/ case insensitive file systems. We'll have to address the doc file re-generation at some point ...
This is just a temporary removal of files colliding w/ case insensitive file systems. We'll have to address the doc file re-generation at some point ...
3500d55
to
b854dc4
Compare
@redmitry That is actually in line with all entry types: biosamples etc. also only know the
... so yes, a One can argue that we should have a 3rd type of response which is an How do you respond to |
https://beacons.bsc.es/beacon/v2.0.0/cohorts/CINECA_synthetic_cohort_UK1 |
@redmitry Ah; that's what you mean; i.e. the wrong response type (BeaconResultsetsResponse instead of BeaconCollectionsResponse). Yes, that should just be a single collection info. Alt least according to the description in the
... and
... which is IMO the clear intention; in BeaconCollectionsResponse this would result in a My point above was already a bit further - a separate |
Agree @redmitry that this will change the implementations but anyway, I'm changing it almost every day for adding new features that are on demand so no problem. Plus, it makes more sense to have a collectionResponse rather than resultSetsResponse as @mbaudis says. For me, the PR is good to be merged and we can proceed. |
So that. Once merged, I update java implementation in accordance. |
Great! Note: I removed some schema .md files from the documentation since they always break merges on Mac OS (same name, different case). This doesn't affect anything since the documentation branch is kept separately, and the website is being built from it. IMO the schema -> .md -> web scripts should be redone (and rerun!) but that's for a separate discussion. |
Without having read the whole thread in detail yet, as I'm trying to understand which issue we are trying to solve here... cohorts/endpoints.json and datasets/endpoints.json have CollectionsResponse response object derfined as a Should be defined as beaconCollectionsResponse precising the type according the endpoint cohorts/defaultSchema.json or cohorts/defaultSchema.json. The assertion "is wrong" needs to be clarified. What is exactly wrong in that? |
@jrambla Yeah, well, this filtered down to the collections response in cohorts mapping to a resultsets response as the data option in the beaconOKresponse (in contrast to what was written everywhere else). But we need a resultset response, too, for the cohorts/{id}/individuals (and more in datasets) items. Unless we say they are also just a list since the cohort / dataset is already the wrapper ... But this just isn't clear; it was wrong in any case and the current change basically allows to have now resultsets for the records payloads (e.g. a |
@mbaudis I'll have a look and rename the clashing .md files. My idea was to create a Gihtub action to run the scripts...but that was before we had the re-branching... |
I fear this is incorrect. |
Well.. I may probably messed it up... |
Yes, @jrambla is right that this is more a change than a fix and can wait. Maybe it needs more discussion on whether the solution proposed here is better than the one we had so I would also stop the PR until it is properly discussed. |
I'm closing this - see fresh start in #123 |
This addresses the easy part of #116 (collections responses w/ strange endpoints etc.)