return proper HTTP status codes, when deleting postgraas instances #31
Conversation
ghost
requested review from
| app.config['SQLALCHEMY_DATABASE_URI'] = get_meta_db_config_path(config) | ||
| app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False | ||
| app.config['BUNDLE_ERRORS'] = True |
There was a problem hiding this comment.
Out of curiosity, what does this do?
There was a problem hiding this comment.
http://flask-restful.readthedocs.io/en/latest/reqparse.html#error-handling
It reports all errors found in the requests and reports all and not only the first one.
| args = parser.parse_args() | ||
|
|
||
| entity = DBInstance.query.get(id) | ||
| if not entity: | ||
| return {'status': 'failed', 'msg': 'Postgraas instance {} does not exist'.format(id)} | ||
| return {'status': 'failed', 'msg': 'Postgraas instance {} does not exist'.format(id)}, 404 |
There was a problem hiding this comment.
in other cases we do abort(404, ...) not clear to me why or what the difference is, but at least we could harmonize it.
There was a problem hiding this comment.
ACK, strange, that they mix it up here, too:
http://flask-restful.readthedocs.io/en/latest/quickstart.html#full-example
abort(404, message="Todo {} doesn't exist".format(todo_id))
vs
return '', 204
There was a problem hiding this comment.
The difference is that abort raises an exception rather than simply returning something. It can therefore happen deep inside the call stack.
In the given context it does not really matter what we use. However, given that we already use abort for other error messages, I agree that abort will be more consistent.
There was a problem hiding this comment.
I guess it is ok in case of success (=2XX) to have return result, 201, and in all kinds of failure abort(500, ...). Just having some failures with return result, 400` and others with abort is strange.
| @@ -91,7 +91,7 @@ def delete(self, id): | |||
| return { | |||
| 'status': 'failed', | |||
| 'msg': 'Could not connect to postgres instance: {}'.format(str(ex)) | |||
| } | |||
| }, 401 | |||
There was a problem hiding this comment.
I guess we have to differentiate here, 401 is only the correct status code, in case the authorization fail because of a wrong password. Most other exceptions should still give 500 imho
There was a problem hiding this comment.
I believe there is no easy way to differentiate between the different error classes without parsing exception texts. Maybe just using a generic 400 error here?
…ilures when connecting to postgres database, when deleting a database - return status code in other cases of failures, too
Fra-nk
deleted the
bugfix/delete-pg-instances-with-proper-http-status-codes
branch
No description provided.