✨ ⚡️Change PUT to PATCH in all resource APIs and minor updates #173
Conversation
znatty22
added
feature
znatty22
changed the title
znatty22
force-pushed
the
put-to-patch
branch
2 times, most recently
from
6ac6f13
to
d5ccc4b
Compare
znatty22
force-pushed
the
put-to-patch
branch
4 times, most recently
from
4e78d32
to
c7b7950
Compare
znatty22
changed the title
| for k in unchanged_keys: | ||
| val = getattr(dm, k) | ||
| if isinstance(val, datetime): | ||
| d = val.replace(tzinfo=tz.tzutc()) |
There was a problem hiding this comment.
Does this add the timezone to the dt or does it change an existing one?
There was a problem hiding this comment.
It adds a timezone to val since val doesn't have one and then returns a new datetime obj.
https://stackoverflow.com/questions/7065164/how-to-make-an-unaware-datetime-timezone-aware-in-python
| @@ -369,3 +334,12 @@ def _create_save_to_db(self): | |||
| kwargs['kf_id'] = d.kf_id | |||
|
|
|||
| return kwargs | |||
|
|
|||
| def _test_response_content(self, resp, status_code): | |||
There was a problem hiding this comment.
If we really want this, it could be part of the base test class. However, it's been removed from all the other tests and replaced by the general api tests.
tests/test_api.py
Outdated
| @pytest.mark.parametrize('endpoint,field', [ | ||
| ('/participants', 'blah'), | ||
| ('/samples', 'blah') | ||
| @pytest.mark.parametrize('endpoint, method, field', [ |
There was a problem hiding this comment.
We can also double parametrize things to reduce duplication. We could probably just use 'blah' for all the tests too. I think something like this will work:
@pytest.mark.parametrize('method', ['POST', 'PATCH'])
@pytest.mark.parametrize('endopint,', ['/participants', '/demographics' ... ])
def test_unkown_field(self, client, entities, endpoint, method):
pass
dataservice/__init__.py
Outdated
| @@ -119,6 +119,7 @@ def register_error_handlers(app): | |||
| from dataservice.api import errors | |||
| app.register_error_handler(HTTPException, errors.http_error) | |||
| app.register_error_handler(IntegrityError, errors.integrity_error) | |||
| app.register_error_handler(405, errors.http_error) | |||
There was a problem hiding this comment.
If you try sending a PUT request (or any other HTTP method we don't support). It looks like
app.register_error_handler(HTTPException, errors.http_error) isn't catching all http exceptions. Maybe we need to investigate that further
There was a problem hiding this comment.
This seems to work though?
from werkzeug.exceptions import default_exceptions
for ex in default_exceptions:
app.register_error_handler(ex, errors.http_error)
We could probably remove the other explicit HTTP error registrations (i.e. 400, 404, etc) if we use this.
@dankolbman I vaguely remember you said you tried this or something similar and it didn't work or you had issues with it?
There was a problem hiding this comment.
Yeah, it has something to do with the scoping on the exception. I forget what exactly I changed to make it work, but it had to do with where the HTTPException was imported.
| @@ -83,26 +82,21 @@ def get(self, kf_id): | |||
| resource: | |||
| Demographic | |||
| """ | |||
|
|
|||
| # Get all | |||
| if kf_id is None: | |||
There was a problem hiding this comment.
This logic was supposed to be removed when we changed to the two-view method.
Update resource, unit tests, and swagger docs
Update unit tests, resource, and documentation
Update unit tests, resource, and documentation
Update unit tests, resource, and documentation
| app.register_error_handler(400, errors.http_error) | ||
| from werkzeug.exceptions import default_exceptions | ||
| for ex in default_exceptions: | ||
| app.register_error_handler(ex, errors.http_error) |
There was a problem hiding this comment.
No luck with HTTPException? It looks like default_exceptions is just all the subclasses of HTTPException, so you would think it would work.
There was a problem hiding this comment.
I think we have to go with this approach for now. Doesn't look like the fix for the issue with HTTPException is in Flask 0.12. Its slated for Flask milestone 1.0 which isn't complete yet
| @@ -12,6 +14,8 @@ | |||
| DIAGNOSES_URL = 'api.diagnoses' | |||
| DIAGNOSES_LIST_URL = 'api.diagnoses_list' | |||
|
|
|||
| from pprint import pprint | |||
For participant, demographic, diagnosis, and sample - change the PUT http method in the resource class to PATCH http method.
For participant, demographic, diagnosis, and sample - change method of retrieval of resource to use SQLAlchemy get() which is optimized for retrieval by primary key.