A minimal REST API for Qiita

[ElDeveloper]

A minimal API to support the functionality for the plate mapper. Whenever possible we've used JSON as the way to represent the transferred data. The endpoints are as follows:

action	URI	description
GET	/api/v1/study/<int>	get study details (title, contacts, abstract, description, alias and efo)
GET	/api/v1/study/<int>/samples	get samples associated with a study and the available metadata headers for these samples
GET	/api/v1/study/<int>/samples?categories=foo,bar	get metadata categories foo and bar for all samples in the study
GET	/api/v1/study/<int>/status	the status of a study (whether or not the study: is public, has sample information, sample information has warnings and a list of preparations
GET	/api/v1/person?name=foo&affiliation=bar	see if a person exists
POST	/api/v1/study	create a study (mirrors study creation on qiita UI with minimal requirements)
POST	/api/v1/person?name=foo&affiliation=bar&email=address	create a study person (ie lab person or PI)
POST	/api/v1/study/<int>/preparation	associate a prep with a study
POST	/api/v1/study/<int>/preparation/<int>/artifact	associate filepaths to a preparation, assuming this filepaths are present in the uploads folder

---

Open questions ( ¯\_(ツ)_/¯ )

• How to properly document this for consumers and developers.
• How should these handlers be name-spaced so they can be imported in webserver.py.
• Per-user authentication.
• What other information do we need to include in the study status.

[coveralls]

Changes Unknown when pulling 53b3803 on ElDeveloper:death-march into ** on biocore:master**.

[ElDeveloper]

One very important thing I forgot to mention: @wasade and I pair-programmed everything in this PR. 🎉

[antgonza]

Looks good, thanks!

Some extra things:

• Should the TODOs be issues?
• Why v1? Do we really expect different versions? Should we just settle in one and say that is in development and it possibly change in the future? I'm worry we are deciding to support old versions of something we haven't even tested internally AKA I'm sure is gonna change.
• I think adding the end points and inputs/outputs to the documentation will be the best. Now, this could be something we add manually, a pain in the long run, or something that is programmatically added, a pain right now. Either way, the documentation should go inside the doc folder in qiita_pet.
• name-spaced: not sure what you mean?
• authentication: I thought one option is to generate a unique "signature" (string) that you can obtain via the GUI (part of your user info page) and you use to create a valid connection. At this point, what ever is easy but secure at the same time.
• extra study info: hard to predict until we actually start using and goes back to my v1 question.

[antgonza]

Should all the errors/warnings be passed at once?

[wasade]

This is the only means we could find to determine if a study already existed. Not sure what else can be passed back.

[antgonza]

I was thinking in getting the warnings like this, unknown principal investigator, lab person etc all at once vs. each test by itself.

[wasade]

That would complicate the logic quite a bit as some checks can't be done if others fail

[josenavas]

I agree with @antgonza - it would be better to check the 3 things (study, PI and lab person) and return all errors at once - otherwise it becomes an iterative process to find all the errors.

[ElDeveloper]

The awkward thing about this is that we would have to mix the error codes, and this is also the way that Study.create does things i.e. by raising one exception at a time.

[josenavas]

Ok - sounds good - I don't think is that critical here as it is during metadata addition.

[antgonza]

I'm not sure what this refers to? data_type, information_type, other?

[wasade]

The call to validate requires knowledge of what the preparation actually is, and we didn't want to replicate a bunch of functionality for this.

[antgonza]

Got it! Depending on the data_type is how the validation happen. One simply option is to return all the validations avaialble? Not pretty but this will allow us to test and figure out a better solution. Other option will be to force the user to pass that, I guess a similar issue than artifact_type/id. BTW I think this is important cause the first use case will be either target gene or shotgun sequencing.

[wasade]

We'd need to hardcode the variable names of all the prep template constants which can be passed to validate, unless there is a mapping somewhere already? It would also make the response from this API variable as different requirements are added in which could easily cause issue with consumers.

[josenavas]

I think this can be englobed here: #2096

[antgonza]

I guess: "# to the regex associating the handler to the webserver" is missing ...

[wasade]

:)

[antgonza]

I guess this info needs to be added to the documentation too, right? Or will it need to be part of the API, something that returns the artifact_type, artifact_id?

[wasade]

Not clear what the best direction here is. Accepting internal IDs like this sucks, so having an informative endpoint to get these names probably makes sense and then just pass in strings. But, we opted to consider this low priority right now since this API is scoped for internal use only at the moment.

[antgonza]

Agree, perhaps next step if this is done? @josenavas, what do you think?

[josenavas]

I don't think passing the ids is required. IIRC you can pass the actual string and the code internally will change to the ID. So instead of 3 you can pass 'raw_barcodes'

[ElDeveloper]

You are right @josenavas, this works with either the identifier or the string value, we've modified a test to show that this works either with identifiers or with ids, should we do anything else?

[josenavas]

no that I can think of here

[antgonza]

Could you add a comment to separate the plugin handlers from the API?

[wasade]

Done

[wasade]

On the additional comments:

• TODOs, no strong feelings
• v1, it's fairly common to see use this convention but also no strong feelings
• Docs, yup, no really great solution within the current qiita ecosystem as numpydoc doesn't lend itself properly and the endpoints are detached from the handler logic anyway. Would you be okay with docstrings off the handlers? @ElDeveloper and I came up with a possible solution which places the endpoint definition in proximity to the handler logic, but we opted to not include it in this PR as its scope really ought to be much larger than just these endpoints.
• name-spaced: the names of the handlers on import in webserver.py, although we can keep them as is...
• authentication: an API key mechanism would be fantastic. At this point, I do not believe qiita has privilege granularity over the keys though, so an API key I believe grants full access.

[coveralls]

Changes Unknown when pulling e460390 on ElDeveloper:death-march into ** on biocore:master**.

[antgonza]

Thanks for the changes! I think it will be good for @josenavas to take a look.

• I think issues for the TODOs will be good to add.
• It makes me really nervous to see v1 cause in my head it means that we expect v2.
• keys: right, it will give full access to that user, with their given privileges. In other words, I think this is fine as that specific key will give access to the user to those things that it has access to. We can always, regenerate keys based on user request or based on abuse(?)

[antgonza]

I was thinking in getting the warnings like this, unknown principal investigator, lab person etc all at once vs. each test by itself.

[antgonza]

Got it! Depending on the data_type is how the validation happen. One simply option is to return all the validations avaialble? Not pretty but this will allow us to test and figure out a better solution. Other option will be to force the user to pass that, I guess a similar issue than artifact_type/id. BTW I think this is important cause the first use case will be either target gene or shotgun sequencing.

[antgonza]

Agree, perhaps next step if this is done? @josenavas, what do you think?

[wasade]

Our impression was that keys are full access and not user specific at this time?

…

On Mar 29, 2017 08:24, "Antonio Gonzalez" ***@***.***> wrote: ***@***.**** commented on this pull request. Thanks for the changes! I think it will be good for @josenavas <https://github.com/josenavas> to take a look. - I think issues for the TODOs will be good to add. - It makes me really nervous to see v1 cause in my head it means that we expect v2. - keys: right, it will give full access to that user, with their given privileges. In other words, I think this is fine as that specific key will give access to the user to those things that it has access to. We can always, regenerate keys based on user request or based on abuse(?) ------------------------------ In qiita_pet/handlers/rest/study.py <#2094 (comment)>: > + title = payload['title'] + study_abstract = payload['study_abstract'] + study_desc = payload['study_description'] + study_alias = payload['study_alias'] + + owner = payload['owner'] + if not User.exists(owner): + self.fail('Unknown user', 403) + return + else: + owner = User(owner) + + efo = payload['efo'] + contacts = payload['contacts'] + + if Study.exists(title): I was thinking in getting the warnings like this, unknown principal investigator, lab person etc all at once vs. each test by itself. ------------------------------ In qiita_pet/handlers/rest/study.py <#2094 (comment)>: > + with warnings.catch_warnings(): + try: + st.validate(SAMPLE_TEMPLATE_COLUMNS) + except Warning: + sample_information_warnings = True + else: + sample_information_warnings = False + else: + sample_information_warnings = False + + preparations = [] + for prep in study.prep_templates(): + pid = prep.id + art = prep.artifact is not None + # TODO: unclear how to test for warnings on the preparations as + # it requires knowledge of the preparation type. It is possible Got it! Depending on the data_type is how the validation happen. One simply option is to return all the validations avaialble? Not pretty but this will allow us to test and figure out a better solution. Other option will be to force the user to pass that, I guess a similar issue than artifact_type/id. BTW I think this is important cause the first use case will be either target gene or shotgun sequencing. ------------------------------ In qiita_pet/test/rest/test_study_preparation.py <#2094 (comment)>: > + bar_fp = os.path.join(uploads_dir, '1', 'bar.txt') + with open(foo_fp, 'w') as fp: + ***@***.***\nATGC\n+\nHHHH\n") + with open(bar_fp, 'w') as fp: + ***@***.***\nATGC\n+\nHHHH\n") + + prep = StringIO(EXP_PREP_TEMPLATE.format(1)) + prep_table = load_template_to_dataframe(prep) + + response = self.post('/api/v1/study/1/preparation?data_type=16S', + data=prep_table.T.to_dict(), + headers=self.headers, asjson=True) + prepid = json_decode(response.body)['id'] + + uri = '/api/v1/study/1/preparation/%d/artifact' % prepid + # 1 -> fwd or rev sequences in fastq Agree, perhaps next step if this is done? @josenavas <https://github.com/josenavas>, what do you think? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2094 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAc8sibpOznSSJBbb1Y-F78Z6AjC13-Yks5rqnfDgaJpZM4MsVci> .

[antgonza]

Yes, that's how it currently is. Sorry for not being clear. When I said: "(part of your user info page)" I meant extend that functionality so each user has its own.

[josenavas]

I thought I submitted my review earlier but apparently I never did! I have some comments in the code directly and some here in general:

• I agree with @antgonza to add the TODO's as issues, and link to the issue in the code - in the past helped a lot to keep track on what to focus effort.
• I agree on using v1 but it is not set in stone until we do the actual release of Qiita. We can use it as @antgonza is suggesting (i.e. development version) but it naturally help us to set a correct structure for the future in case that at some point we really need to change the interface. It doesn't harm anything right now and it has a lot of benefits.
• Documentation: I think having what you currently put in the issue description as documentation is a great start. It is the simplest thing to do right now (you already have it written) and we can create an issue to improve/automatically generate it - but it shouldn't be a blocker right now.
• Per-user authentication: it needs DB modifications to link a user with a specific client id/secret pair. Not hard to do, but time consuming and outside of the scope of this work. I would add an issue and fix later.
• Re study status. Not sure either - but it can expanded as we use it until we decide to do a release.

[josenavas]

Is the specific cast to int needed? The object returned should be an int if the column study_person_id is a bigserial.

[ElDeveloper]

Great point, not sure where this came from, fixed!

[josenavas]

@wasade @ElDeveloper What about using this? It is still not in master but I think it would be easy to port here.

Basically, instead of calling fail, you raise an error and let the normal tornado error handling to return the value. The error message, instead of going in the body as it is going here, it will go in a header.

It also has the advantage that it will stop the execution of the handler, so you can remove these type of if statements.

[ElDeveloper]

Sounds like a great idea! The reason why we had been putting the error message in the body is because we were modeling the API's etiquette after GitHub's API, where they always return either the data or a dictionary with just a message, and they even return a link to their API's documentation. So what about modifying QiitaHTTPError to write the message to the body?

Just for a reference, this is how it looks when you use curl to pull some data and the request fails:

(qiita)yoshikivazquezbaeza:qiita@death-march$ curl -ik 'https://127.0.0.1:21174/api/v1/person?name=LabDude&affiliation=knight%20lab2'
HTTP/1.1 404 Not Found
Date: Thu, 30 Mar 2017 17:59:45 GMT
Content-Length: 31
Content-Type: application/json; charset=UTF-8
Server: TornadoServer/3.1.1

{"message": "Person not found"}

Thinking more, should we be logging these errors into the database, or is the Tornado logging sufficient?

[josenavas]

I'm unsure if you are going to be able to modify the QiitaHTTPError to write the message in the body (is not a handler class so I'm unsure on how tornado is handling that internally). It may be worth testing how the current status of the QiitaHTTPError shows when using curl.

I don't think registering these errors in the DB is needed as these are misuses of the API and generally user generated. That would be an easy way of blowing up the size of our database.

[josenavas]

Can we have a better name for this function? Something like safe_get_study or something like this? I can also foresee these type of functions appearing for other objects so it would be great to have a standard way of naming these functions.

[ElDeveloper]

Sounds good, I've renamed the method to safe_get_study.

[josenavas]

i will not expose the efo. We are not really using it and it should disappear. (It is not exposed anywhere else in the GUI for example)

[ElDeveloper]

Sounds good, I've just removed this.

[josenavas]

I agree with @antgonza - it would be better to check the 3 things (study, PI and lab person) and return all errors at once - otherwise it becomes an iterative process to find all the errors.

[josenavas]

Should this error be something like Study %s doesn't have sample information?

[ElDeveloper]

The ID is already implied in the URI itself, so it seems a bit redundant. Also, after modeling from GitHub's API, none of their messages seemed to imply any specific information about the requested resource, they were succinct and generic.

[josenavas]

I was just thinking on differentiating this error from the one below

[ElDeveloper]

Great point, fixed.

[josenavas]

What about sending back the categories that were not found?

[ElDeveloper]

Maybe as an additional attribute? The response would be something like:

{'message': 'category not found', 'categories_not_found': ['foo', 'bar', 'baz']}

[josenavas]

that sounds good!

[ElDeveloper]

Great, fixed!

[josenavas]

I don't think passing the ids is required. IIRC you can pass the actual string and the code internally will change to the ID. So instead of 3 you can pass 'raw_barcodes'

[josenavas]

What about prefixing all the handlers with API or REST? It will avoid collisions with anything else and it will be clear that is part of the rest api

[ElDeveloper]

We've put all these under a constant, this avoids the namespace collisions :) we think this is better.

[josenavas]

🍻 I think that is an awesome solution! Never though about that and it is superclean!

[josenavas]

Are we expecting any args/kwargs in here? If not, why having them here?

[ElDeveloper]

Removed 👍

[coveralls]

Changes Unknown when pulling 9f528ea on ElDeveloper:death-march into ** on biocore:master**.

[ElDeveloper]

I believe all comments should be addressed, we've added a minimal documentation block in the Qiita dev docs.

[coveralls]

Changes Unknown when pulling e970392 on ElDeveloper:death-march into ** on biocore:master**.

[coveralls]

Changes Unknown when pulling 133b9f8 on ElDeveloper:death-march into ** on biocore:master**.

[ElDeveloper]

Thanks all!!

…

On (Mar-30-17|15:54), Antonio Gonzalez wrote: Merged #2094. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #2094 (comment)