✨ ⚡️Change PUT to PATCH in all resource APIs and minor updates #173

znatty22 · 2018-03-07T19:11:57Z

For participant, demographic, diagnosis, and sample - change the PUT http method in the resource class to PATCH http method.

Previous PUT implementation was actually a PATCH implementation. PATCH allows for partial update of resources. PUT replaces the entire resource and will be implemented in the near future.

For participant, demographic, diagnosis, and sample - change method of retrieval of resource to use SQLAlchemy get() which is optimized for retrieval by primary key.

dankolbman · 2018-03-15T13:18:44Z

tests/demographic/test_demographic_resources.py

+        for k in unchanged_keys:
+            val = getattr(dm, k)
+            if isinstance(val, datetime):
+                d = val.replace(tzinfo=tz.tzutc())


Does this add the timezone to the dt or does it change an existing one?

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

dankolbman · 2018-03-15T13:21:29Z

tests/demographic/test_demographic_resources.py

@@ -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):


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.

dankolbman · 2018-03-15T13:26:35Z

tests/test_api.py

-    @pytest.mark.parametrize('endpoint,field', [
-        ('/participants', 'blah'),
-        ('/samples', 'blah')
+    @pytest.mark.parametrize('endpoint, method, field', [


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

dankolbman · 2018-03-15T13:34:48Z

dataservice/__init__.py

@@ -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)


when does this happen?

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

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?

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.

dankolbman · 2018-03-15T13:40:13Z

dataservice/api/demographic/resources.py

@@ -83,26 +82,21 @@ def get(self, kf_id):
            resource:
              Demographic
        """
-
        # Get all
        if kf_id is None:


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

parimalak

👍

dankolbman · 2018-03-15T20:57:35Z

dataservice/__init__.py

-    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)


No luck with HTTPException? It looks like default_exceptions is just all the subclasses of HTTPException, so you would think it would work.

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

pallets/flask#2314

dankolbman · 2018-03-15T20:59:00Z

tests/diagnosis/test_diagnosis_resources.py

@@ -12,6 +14,8 @@
 DIAGNOSES_URL = 'api.diagnoses'
 DIAGNOSES_LIST_URL = 'api.diagnoses_list'

+from pprint import pprint


dankolbman

💯

✨ ⚡️Change PUT to PATCH in all resource APIs and minor updates

znatty22 added feature New functionality refactor Something needs to be done better labels Mar 7, 2018

znatty22 added this to the CHOP Sprint 4 milestone Mar 7, 2018

znatty22 changed the title ~~🚧WIP: Change PUT to PATCH in all resource APIs~~ 🚧 WIP: Change PUT to PATCH in all resource APIs Mar 7, 2018

znatty22 force-pushed the put-to-patch branch 2 times, most recently from 6ac6f13 to d5ccc4b Compare March 9, 2018 17:50

znatty22 mentioned this pull request Mar 9, 2018

✨ Study resource #175

Merged

znatty22 force-pushed the put-to-patch branch 4 times, most recently from 4e78d32 to c7b7950 Compare March 12, 2018 19:53

dankolbman assigned znatty22 Mar 12, 2018

znatty22 force-pushed the put-to-patch branch from 04999c0 to 94c35b0 Compare March 12, 2018 20:36

znatty22 requested review from dankolbman and parimalak March 12, 2018 20:43

znatty22 changed the title ~~🚧 WIP: Change PUT to PATCH in all resource APIs~~ ✨ ⚡️Change PUT to PATCH in all resource APIs and minor updates Mar 12, 2018

znatty22 mentioned this pull request Mar 12, 2018

✨ Add phenotype endpoint #180

Merged

dankolbman reviewed Mar 15, 2018

View reviewed changes

znatty22 added 10 commits March 15, 2018 10:26

✨ Add http 405 handler

3142028

✨ Participant - change put to patch

9b461e5

Update resource, unit tests, and swagger docs

✨ Demographic - change put to patch

bcd6fd2

Update unit tests, resource, and documentation

✨ Diagnosis - change put to patch

0a7db22

Update unit tests, resource, and documentation

✨ Sample - change put to patch

f6cee15

Update unit tests, resource, and documentation

♻️ Participant - update error msgs to standard

88f60e3

⚡ Use get() to retrieve by kf_id, primary key

cbf3d0b

🐛 Fix handling of default http exceptions

07f8306

🔥 Remove redundant GET code from demographic res

4c93511

✅ Clean up tests

22ac7c1

znatty22 force-pushed the put-to-patch branch from 94c35b0 to 22ac7c1 Compare March 15, 2018 20:04

parimalak approved these changes Mar 15, 2018

View reviewed changes

dankolbman reviewed Mar 15, 2018

View reviewed changes

znatty22 force-pushed the put-to-patch branch from a7430bf to 52c69f7 Compare March 15, 2018 21:35

🔥 Remove unused imports + comments

f76133d

znatty22 force-pushed the put-to-patch branch from 52c69f7 to f76133d Compare March 15, 2018 21:39

dankolbman approved these changes Mar 15, 2018

View reviewed changes

znatty22 merged commit 4e7876d into master Mar 15, 2018

znatty22 deleted the put-to-patch branch March 16, 2018 14:15

dankolbman mentioned this pull request Apr 2, 2018

🏷 Release 0.2.0 #217

Merged

dankolbman removed the ready-for-review This PR needs a review label May 7, 2018

alubneuski pushed a commit that referenced this pull request Oct 11, 2024

Merge pull request #173 from kids-first/put-to-patch

d6eed80

✨ ⚡️Change PUT to PATCH in all resource APIs and minor updates

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ ⚡️Change PUT to PATCH in all resource APIs and minor updates #173

✨ ⚡️Change PUT to PATCH in all resource APIs and minor updates #173

znatty22 commented Mar 7, 2018 •

edited

Loading

dankolbman Mar 15, 2018

znatty22 Mar 15, 2018

dankolbman Mar 15, 2018

znatty22 Mar 15, 2018

dankolbman Mar 15, 2018

znatty22 Mar 15, 2018

dankolbman Mar 15, 2018

znatty22 Mar 15, 2018

znatty22 Mar 15, 2018 •

edited

Loading

dankolbman Mar 15, 2018

dankolbman Mar 15, 2018

parimalak left a comment

dankolbman Mar 15, 2018

znatty22 Mar 15, 2018

dankolbman Mar 15, 2018

dankolbman left a comment

✨ ⚡️Change PUT to PATCH in all resource APIs and minor updates #173

✨ ⚡️Change PUT to PATCH in all resource APIs and minor updates #173

Conversation

znatty22 commented Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

znatty22 Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parimalak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dankolbman left a comment

Choose a reason for hiding this comment

znatty22 commented Mar 7, 2018 •

edited

Loading

znatty22 Mar 15, 2018 •

edited

Loading