plantbreeding / BrAPI

Repository for version control of the BrAPI specifications
https://brapi.org
MIT License
57 stars 32 forks source link

IDs in calls call response #144

Closed GuilhemSempere closed 6 years ago

GuilhemSempere commented 6 years ago

I remember having raised this question in Seattle last summer. We need consistency in the way we format the calls call's response where IDs are involved. Because clients will probably want to be able to check in a simple way if a given server implements the calls it requires.

What I remember is that we ended up deciding that no matter which call / entity type an ID refers to, it must be formatted as the following static string: {id}

I still see implementations in brapi-resources.json that do not comply with this (should it be clearly stated somewhere in the docs?). Same situation with Peter's brapi-Java-TestServer (you can't be blamed, you were not there at that time).

Anyway, unless we have a good reason for changing our mind (Peter, you might have one?), can we please all take this into account?

BrapiCoordinatorSelby commented 6 years ago

To be clear you are talking about changing this: { "call": "germplasm/{germplasmDbId}/markerprofiles", "datatypes": ["json"], "methods": ["GET"] } to this: { "call": "germplasm/{id}/markerprofiles", "datatypes": ["json"], "methods": ["GET"] } correct?

I would ask why it is necessary? What problem is this solving for clients?

I would be cautious of this type of change, because it is making an internal inconsistency which developers will need to think about, which could cause errors. It is natural to assume that the name of the call returned matches the name of the call provided by the server and matches the name of that call in the documentation.

This is not a strong argument against making this change, just a caution. If the long form is really inconvenient for client developers, then I'm all for it. But I need some feedback on how the "calls" call is being used by clients which makes it inconvenient. I might suggest making it {param} since there is no reason it has to be limited to an ID, but that's just semantics.

sebastian-raubach commented 6 years ago

IIRC it is correct that we decided to use {id} for all the placeholders representing ids. That way, the client doesn't have to know that for some calls it might be germplasmId for others it might be observationVariableDbId. Having a single (well documented) placeholder makes it easier to implement servers and clients.

martinbaste commented 6 years ago

I don't want to add chaos if this was decided already, just pointing out a few things.

I also see the arguments for the {id}, just wanted to add my 0.02

sebastian-raubach commented 6 years ago

If you're referring to this one: https://github.com/plantbreeding/API/blob/master/Specification/GenomeMaps/GenomeMapDataByRangeOnLinkageGroup.md

Then that has for a map id and a linkage group name (not id).

But multiple ids are certainly something that needs to be considered.

BrapiCoordinatorSelby commented 6 years ago

I assume that the primary use of "/calls" is for a client to verify that a particular server has a particular call available. For this scenario I assume the client is just doing a string comparison between the call its trying to make and the return of "/calls" and I would argue a string comparison doesn't care if its {id} or {xxDbId} or {linkageGroupName}, as long as both server and client do the same thing.

I'm missing the history of this discussion. What was the scenario which made "/calls" difficult to deal with in its current form? Can someone please post an example of how they are using the "/calls" call in a client and how {id} would make that example easier?

sebastian-raubach commented 6 years ago

I agree that it's just a simple string comparison and, personally, I don't really mind what we do. I think the discussion was originally brought up because lots of times when systems were trying to talk to each other they failed to do so because of very minor typos like germplasmDbId vs germplasmDbID vs germplasmDbid etc. I think the idea was, the simpler the parameter name the better. As long as whatever we go with is properly documented and highlighted that should be fine.

GuilhemSempere commented 6 years ago

I agree, the important fact is to be able to rely on a precise string, whatever it is. However I think it is fairly urgent to make a definite decision on this and document it, because people are now using V1 and it truly lacks this clarification.

BrapiCoordinatorSelby commented 6 years ago

Ok I am making the official decision: the "/calls" call will return path parameter names exactly as they are listed in the path definition documentation for each call. This reverses the decision made in Seattle. Please address all hate mail to me directly lol

This means if GermplasmDetailsByGermplasmDbId.md has [/brapi/v1/germplasm/{germplasmDbId}] listed at the top as the call definition, then the "/calls" call will return { "call": "germplasm/{germplasmDbId}" }

I will add to the documentation of the "/calls" call explaining this and stress how important it is to copy the name exactly in BrAPI v1.1.

Reasoning