-
Notifications
You must be signed in to change notification settings - Fork 80
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
Add prep test API creation #1872
Add prep test API creation #1872
Conversation
class PrepTemplateAPItestHandler(OauthBaseHandler): | ||
@authenticate_oauth | ||
def post(self): | ||
prep_info_dict = loads(self.get_argument('prep_info')) |
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.
Is the size of the metadata something we should be concern about? It seems like if we have a large study we can hit the maximum parameter size really quickly.
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.
That's a good question - I will open an issue to keep track of this. In this case, this endpoint is tailored only for being able to execute the plugin tests and it will not be exposed in a live environment. Do you agree with this solution?
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.
Issue: #1873
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.
I think it's worth testing with the largest study in our current version of Qiita before deploying because if it breaks we need to rethink this section. Now, this can happen before merging this or after as this is going to its own branch. If you decide we should merge, could you add a point to the list so we make sure it's done and tested before this branch is merged to master?
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.
Fair point - I've investigated it with @ElDeveloper
The name of the function (get_argument
) seems a bit misleading here. The information is sent in the body of the request, so we are not limited at the argument of the URL. Here is the code snippet that we use to test this, sending a fake template of ~74Mb (which I don't know how often we see templates this big):
import requests
from json import dumps
from qiita_client import QiitaClient
metadata_dict = {
'SKB8.640193': {
'primer': 'GTGCCAGCMGCCGCGGTAA',
'barcode': 'GTCCGCAAGTTA',
'platform': 'ILLUMINA',
'instrument_model': 'Illumina MiSeq'},
'SKD8.640184': {
'primer': 'GTGCCAGCMGCCGCGGTAA',
'barcode': 'GTCCGCAAGTTA',
'platform': 'ILLUMINA',
'instrument_model': 'Illumina MiSeq'}}
# Create a mapping file of ~76MB
metadata_dict['SKB8.640193']['primer'] = metadata_dict['SKB8.640193']['primer'] * 200000
metadata_dict['SKB8.640193']['primer'] = metadata_dict['SKB8.640193']['primer'] * 20
data = {
'prep_info': dumps(metadata_dict),
'study': 1,
'data_type': '16S'}
CLIENT_ID = '19ndkO3oMKsoChjVVWluF7QkxHRfYhTKSFbAVt8IhK7gZgDaO4'
CLIENT_SECRET = ('J7FfQ7CQdOxuKhQAf1eoGgBAE81Ns8Gu3EKaWFm3IO2JKh'
'AmmCWZuabe0O5Mp28s1')
qclient = QiitaClient("https://localhost:21174", CLIENT_ID, CLIENT_SECRET,
server_cert='/Users/jose/qiime_software/QiiTa/qiita_core/support_files/server.crt')
r = qclient._request_oauth2(requests.post, 'https://localhost:21174/apitest/prep_template/', data=data, verify=False)
r.request.headers
Out[24]: {'Content-Length': '76000409', 'Accept-Encoding': 'gzip, deflate', 'Accept': '*/*', 'User-Agent': 'python-requests/2.8.1', 'Connection': 'keep-alive', 'Content-Type': 'application/x-www-form-urlencoded', 'Authorization': u'Bearer ZVvdQbf0Xyp5WWzqH0XRuYikfktsVF5jJ2hJBfJNjoEwup4GVQJKfbG'}
r.request.url
Out[28]: 'https://localhost:21174/apitest/prep_template/'
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.
👍 thanks for making sure. In this kind of things is better to be sure about them working fine.
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.
Sure! Can we merge if there are no more comments?
1 comment. |
👍 |
This depends on #1871 so review/merge that one first.
This adds an endpoint so plugins can create a prep template.