Analysis PUT method for labels #868
Conversation
| permchecker(noop)('PUT') | ||
|
|
||
|
|
||
| payload = self.request.json_body |
There was a problem hiding this comment.
This would technically 500 if the PUT contained no request body, not just {}.
| permchecker(noop)('PUT') | ||
|
|
||
|
|
||
| payload = self.request.json_body |
There was a problem hiding this comment.
We check that the payload exists everywhere, it would be nice to have a decorator such as @verify_payload_exists that just does this for us. Feel free to add one if you'd like :)
api/handlers/refererhandler.py
Outdated
|
|
||
| try: | ||
| result = self.storage.update_el(_id, payload) | ||
| except APIStorageException as e: |
There was a problem hiding this comment.
This would only ever get thrown when we can't transform the _id to an ObjectId. We have route filters that require an ObjectId structure. I think allowing the APIStorageException to bubble up on it's own and cause a 500 is a better way to handle it, it would show us where there are holes in the filter, if any.
|
Overall looks good, I did make some comments on code that is following existing examples in our codebase, but those existing examples have issues I'd like to correct with time. Mind correcting them here and we can go back and cleanup problems as we have time? |
hkethi002
force-pushed
the
analysis-put
branch
2 times, most recently
from
5a6c7c2 to
6acb738
Compare
|
I simplified the decorator, with that change LGTM! |
Fixes #331
Review Checklist