[PATCH] Controller: Improve Kimchi Specific Exception Reporting
From: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com> The resource and collection class catches exception in the index method and translates to HTTP error code with error message. While this is good and friendly to the user, the exception handling code is somewhat ad-hoc. The first problem is that it does not catch and translate all the Kimchi exception. When new code or feature is added to Kimchi, it's possible that the new code raises an exception that is not expected before. In this case it fails to translate the error message, so the cherrypy framework translates it to HTTP code 500 with a vague message saying "The server encountered an internal error". This is not friendly to the user. In this patch it "except KimchiException" at the last of the except sequence, and translates it into HTTP error code 500 with specific error message, so that the user knows what's going on. The second problem is that the index method handles GET, POST, PUT and DELETE requests, and the error translating code for each type of request is duplicated. This is not necessary. So this patch merges the error handling code. Signed-off-by: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com> --- src/kimchi/control/base.py | 76 +++++++++++++++++++--------------------------- 1 file changed, 32 insertions(+), 44 deletions(-) diff --git a/src/kimchi/control/base.py b/src/kimchi/control/base.py index d2d041e..320239b 100644 --- a/src/kimchi/control/base.py +++ b/src/kimchi/control/base.py @@ -26,6 +26,7 @@ from kimchi.control.utils import get_class_name, internal_redirect, model_fn from kimchi.control.utils import parse_request, validate_method from kimchi.control.utils import validate_params from kimchi.exception import InvalidOperation, InvalidParameter +from kimchi.exception import KimchiException from kimchi.exception import MissingParameter, NotFoundError, OperationFailed @@ -87,6 +88,8 @@ class Resource(object): raise cherrypy.HTTPError(500, e.message) except NotFoundError, e: raise cherrypy.HTTPError(404, e.message) + except KimchiException, e: + raise cherrypy.HTTPError(500, e.message) wrapper.__name__ = action_name wrapper.exposed = True @@ -116,31 +119,20 @@ class Resource(object): @cherrypy.expose def index(self): method = validate_method(('GET', 'DELETE', 'PUT')) - if method == 'GET': - try: - return self.get() - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) - except InvalidOperation, e: - raise cherrypy.HTTPError(400, e.message) - except OperationFailed, e: - raise cherrypy.HTTPError(406, e.message) - elif method == 'DELETE': - try: - return self.delete() - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) - except InvalidParameter, e: - raise cherrypy.HTTPError(400, e.message) - elif method == 'PUT': - try: - return self.update() - except InvalidParameter, e: - raise cherrypy.HTTPError(400, e.message) - except InvalidOperation, e: - raise cherrypy.HTTPError(400, e.message) - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) + try: + return {'GET': self.get, + 'DELETE': self.delete, + 'PUT': self.update}[method]() + except InvalidOperation, e: + raise cherrypy.HTTPError(400, e.message) + except InvalidParameter, e: + raise cherrypy.HTTPError(400, e.message) + except NotFoundError, e: + raise cherrypy.HTTPError(404, e.message) + except OperationFailed, e: + raise cherrypy.HTTPError(500, e.message) + except KimchiException, e: + raise cherrypy.HTTPError(500, e.message) def update(self): try: @@ -269,29 +261,25 @@ class Collection(object): @cherrypy.expose def index(self, *args, **kwargs): method = validate_method(('GET', 'POST')) - if method == 'GET': - try: + try: + if method == 'GET': filter_params = cherrypy.request.params validate_params(filter_params, self, 'get_list') return self.get(filter_params) - except InvalidOperation, e: - raise cherrypy.HTTPError(400, e.message) - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) - - elif method == 'POST': - try: + elif method == 'POST': return self.create(parse_request(), *args) - except MissingParameter, e: - raise cherrypy.HTTPError(400, e.message) - except InvalidParameter, e: - raise cherrypy.HTTPError(400, e.message) - except OperationFailed, e: - raise cherrypy.HTTPError(500, e.message) - except InvalidOperation, e: - raise cherrypy.HTTPError(400, e.message) - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) + except InvalidOperation, e: + raise cherrypy.HTTPError(400, e.message) + except InvalidParameter, e: + raise cherrypy.HTTPError(400, e.message) + except MissingParameter, e: + raise cherrypy.HTTPError(400, e.message) + except NotFoundError, e: + raise cherrypy.HTTPError(404, e.message) + except OperationFailed, e: + raise cherrypy.HTTPError(500, e.message) + except KimchiException, e: + raise cherrypy.HTTPError(500, e.message) class AsyncCollection(Collection): -- 1.8.5.3
Reviewed-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> On 03/18/2014 03:16 PM, zhshzhou@linux.vnet.ibm.com wrote:
From: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com>
The resource and collection class catches exception in the index method and translates to HTTP error code with error message. While this is good and friendly to the user, the exception handling code is somewhat ad-hoc.
The first problem is that it does not catch and translate all the Kimchi exception. When new code or feature is added to Kimchi, it's possible that the new code raises an exception that is not expected before. In this case it fails to translate the error message, so the cherrypy framework translates it to HTTP code 500 with a vague message saying "The server encountered an internal error". This is not friendly to the user. In this patch it "except KimchiException" at the last of the except sequence, and translates it into HTTP error code 500 with specific error message, so that the user knows what's going on.
The second problem is that the index method handles GET, POST, PUT and DELETE requests, and the error translating code for each type of request is duplicated. This is not necessary. So this patch merges the error handling code.
Signed-off-by: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com> --- src/kimchi/control/base.py | 76 +++++++++++++++++++--------------------------- 1 file changed, 32 insertions(+), 44 deletions(-)
diff --git a/src/kimchi/control/base.py b/src/kimchi/control/base.py index d2d041e..320239b 100644 --- a/src/kimchi/control/base.py +++ b/src/kimchi/control/base.py @@ -26,6 +26,7 @@ from kimchi.control.utils import get_class_name, internal_redirect, model_fn from kimchi.control.utils import parse_request, validate_method from kimchi.control.utils import validate_params from kimchi.exception import InvalidOperation, InvalidParameter +from kimchi.exception import KimchiException from kimchi.exception import MissingParameter, NotFoundError, OperationFailed
@@ -87,6 +88,8 @@ class Resource(object): raise cherrypy.HTTPError(500, e.message) except NotFoundError, e: raise cherrypy.HTTPError(404, e.message) + except KimchiException, e: + raise cherrypy.HTTPError(500, e.message)
wrapper.__name__ = action_name wrapper.exposed = True @@ -116,31 +119,20 @@ class Resource(object): @cherrypy.expose def index(self): method = validate_method(('GET', 'DELETE', 'PUT')) - if method == 'GET': - try: - return self.get() - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) - except InvalidOperation, e: - raise cherrypy.HTTPError(400, e.message) - except OperationFailed, e: - raise cherrypy.HTTPError(406, e.message) - elif method == 'DELETE': - try: - return self.delete() - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) - except InvalidParameter, e: - raise cherrypy.HTTPError(400, e.message) - elif method == 'PUT': - try: - return self.update() - except InvalidParameter, e: - raise cherrypy.HTTPError(400, e.message) - except InvalidOperation, e: - raise cherrypy.HTTPError(400, e.message) - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) + try: + return {'GET': self.get, + 'DELETE': self.delete, + 'PUT': self.update}[method]() + except InvalidOperation, e: + raise cherrypy.HTTPError(400, e.message) + except InvalidParameter, e: + raise cherrypy.HTTPError(400, e.message) + except NotFoundError, e: + raise cherrypy.HTTPError(404, e.message) + except OperationFailed, e: + raise cherrypy.HTTPError(500, e.message) + except KimchiException, e: + raise cherrypy.HTTPError(500, e.message)
def update(self): try: @@ -269,29 +261,25 @@ class Collection(object): @cherrypy.expose def index(self, *args, **kwargs): method = validate_method(('GET', 'POST')) - if method == 'GET': - try: + try: + if method == 'GET': filter_params = cherrypy.request.params validate_params(filter_params, self, 'get_list') return self.get(filter_params) - except InvalidOperation, e: - raise cherrypy.HTTPError(400, e.message) - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) - - elif method == 'POST': - try: + elif method == 'POST': return self.create(parse_request(), *args) - except MissingParameter, e: - raise cherrypy.HTTPError(400, e.message) - except InvalidParameter, e: - raise cherrypy.HTTPError(400, e.message) - except OperationFailed, e: - raise cherrypy.HTTPError(500, e.message) - except InvalidOperation, e: - raise cherrypy.HTTPError(400, e.message) - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) + except InvalidOperation, e: + raise cherrypy.HTTPError(400, e.message) + except InvalidParameter, e: + raise cherrypy.HTTPError(400, e.message) + except MissingParameter, e: + raise cherrypy.HTTPError(400, e.message) + except NotFoundError, e: + raise cherrypy.HTTPError(404, e.message) + except OperationFailed, e: + raise cherrypy.HTTPError(500, e.message) + except KimchiException, e: + raise cherrypy.HTTPError(500, e.message)
class AsyncCollection(Collection):
-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center
Applied. Thanks. Regards, Aline Manera
On 03/18/2014 03:16 PM, zhshzhou@linux.vnet.ibm.com wrote:
From: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com>
The resource and collection class catches exception in the index method and translates to HTTP error code with error message. While this is good and friendly to the user, the exception handling code is somewhat ad-hoc.
The first problem is that it does not catch and translate all the Kimchi exception. When new code or feature is added to Kimchi, it's possible that the new code raises an exception that is not expected before. In this case it fails to translate the error message, so the cherrypy framework translates it to HTTP code 500 with a vague message saying "The server encountered an internal error". This is not friendly to the user. In this patch it "except KimchiException" at the last of the except sequence, and translates it into HTTP error code 500 with specific error message, so that the user knows what's going on. Yes, it's a necessary improvement for user experience.
The second problem is that the index method handles GET, POST, PUT and DELETE requests, and the error translating code for each type of request is duplicated. This is not necessary. So this patch merges the error handling code. I agree with that we need do some cleanup on the code you mentioned. But I prefer to split it into another patch since I hope we have a further refactoring. I think we can add a http error code for kimchi exception, and then we can construct HTTPError from kimchi exception by one method defined in the exception class
Signed-off-by: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com> --- src/kimchi/control/base.py | 76 +++++++++++++++++++--------------------------- 1 file changed, 32 insertions(+), 44 deletions(-)
diff --git a/src/kimchi/control/base.py b/src/kimchi/control/base.py index d2d041e..320239b 100644 --- a/src/kimchi/control/base.py +++ b/src/kimchi/control/base.py @@ -26,6 +26,7 @@ from kimchi.control.utils import get_class_name, internal_redirect, model_fn from kimchi.control.utils import parse_request, validate_method from kimchi.control.utils import validate_params from kimchi.exception import InvalidOperation, InvalidParameter +from kimchi.exception import KimchiException from kimchi.exception import MissingParameter, NotFoundError, OperationFailed
@@ -87,6 +88,8 @@ class Resource(object): raise cherrypy.HTTPError(500, e.message) except NotFoundError, e: raise cherrypy.HTTPError(404, e.message) + except KimchiException, e: + raise cherrypy.HTTPError(500, e.message)
wrapper.__name__ = action_name wrapper.exposed = True @@ -116,31 +119,20 @@ class Resource(object): @cherrypy.expose def index(self): method = validate_method(('GET', 'DELETE', 'PUT')) - if method == 'GET': - try: - return self.get() - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) - except InvalidOperation, e: - raise cherrypy.HTTPError(400, e.message) - except OperationFailed, e: - raise cherrypy.HTTPError(406, e.message) - elif method == 'DELETE': - try: - return self.delete() - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) - except InvalidParameter, e: - raise cherrypy.HTTPError(400, e.message) - elif method == 'PUT': - try: - return self.update() - except InvalidParameter, e: - raise cherrypy.HTTPError(400, e.message) - except InvalidOperation, e: - raise cherrypy.HTTPError(400, e.message) - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) + try: + return {'GET': self.get, + 'DELETE': self.delete, + 'PUT': self.update}[method]() + except InvalidOperation, e: + raise cherrypy.HTTPError(400, e.message) + except InvalidParameter, e: + raise cherrypy.HTTPError(400, e.message) + except NotFoundError, e: + raise cherrypy.HTTPError(404, e.message) + except OperationFailed, e: + raise cherrypy.HTTPError(500, e.message) + except KimchiException, e: + raise cherrypy.HTTPError(500, e.message)
def update(self): try: @@ -269,29 +261,25 @@ class Collection(object): @cherrypy.expose def index(self, *args, **kwargs): method = validate_method(('GET', 'POST')) - if method == 'GET': - try: + try: + if method == 'GET': filter_params = cherrypy.request.params validate_params(filter_params, self, 'get_list') return self.get(filter_params) - except InvalidOperation, e: - raise cherrypy.HTTPError(400, e.message) - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) - - elif method == 'POST': - try: + elif method == 'POST': return self.create(parse_request(), *args) - except MissingParameter, e: - raise cherrypy.HTTPError(400, e.message) - except InvalidParameter, e: - raise cherrypy.HTTPError(400, e.message) - except OperationFailed, e: - raise cherrypy.HTTPError(500, e.message) - except InvalidOperation, e: - raise cherrypy.HTTPError(400, e.message) - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) + except InvalidOperation, e: + raise cherrypy.HTTPError(400, e.message) + except InvalidParameter, e: + raise cherrypy.HTTPError(400, e.message) + except MissingParameter, e: + raise cherrypy.HTTPError(400, e.message) + except NotFoundError, e: + raise cherrypy.HTTPError(404, e.message) + except OperationFailed, e: + raise cherrypy.HTTPError(500, e.message) + except KimchiException, e: + raise cherrypy.HTTPError(500, e.message)
class AsyncCollection(Collection):
On 03/19/2014 03:42 PM, Mark Wu wrote:
On 03/18/2014 03:16 PM, zhshzhou@linux.vnet.ibm.com wrote:
From: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com>
The resource and collection class catches exception in the index method and translates to HTTP error code with error message. While this is good and friendly to the user, the exception handling code is somewhat ad-hoc.
The first problem is that it does not catch and translate all the Kimchi exception. When new code or feature is added to Kimchi, it's possible that the new code raises an exception that is not expected before. In this case it fails to translate the error message, so the cherrypy framework translates it to HTTP code 500 with a vague message saying "The server encountered an internal error". This is not friendly to the user. In this patch it "except KimchiException" at the last of the except sequence, and translates it into HTTP error code 500 with specific error message, so that the user knows what's going on. Yes, it's a necessary improvement for user experience.
The second problem is that the index method handles GET, POST, PUT and DELETE requests, and the error translating code for each type of request is duplicated. This is not necessary. So this patch merges the error handling code. I agree with that we need do some cleanup on the code you mentioned. But I prefer to split it into another patch since I hope we have a further refactoring. I think we can add a http error code for kimchi exception, and then we can construct HTTPError from kimchi exception by one method defined in the exception class
we can overridden the HTTPError. We have send a patch to overridden the HTTPError before. http://lists.ovirt.org/pipermail/kimchi-devel/2014-January/002004.html seems later others think the kimchi exception can derived from HTTPError.
Signed-off-by: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com> --- src/kimchi/control/base.py | 76 +++++++++++++++++++--------------------------- 1 file changed, 32 insertions(+), 44 deletions(-)
diff --git a/src/kimchi/control/base.py b/src/kimchi/control/base.py index d2d041e..320239b 100644 --- a/src/kimchi/control/base.py +++ b/src/kimchi/control/base.py @@ -26,6 +26,7 @@ from kimchi.control.utils import get_class_name, internal_redirect, model_fn from kimchi.control.utils import parse_request, validate_method from kimchi.control.utils import validate_params from kimchi.exception import InvalidOperation, InvalidParameter +from kimchi.exception import KimchiException from kimchi.exception import MissingParameter, NotFoundError, OperationFailed
@@ -87,6 +88,8 @@ class Resource(object): raise cherrypy.HTTPError(500, e.message) except NotFoundError, e: raise cherrypy.HTTPError(404, e.message) + except KimchiException, e: + raise cherrypy.HTTPError(500, e.message)
wrapper.__name__ = action_name wrapper.exposed = True @@ -116,31 +119,20 @@ class Resource(object): @cherrypy.expose def index(self): method = validate_method(('GET', 'DELETE', 'PUT')) - if method == 'GET': - try: - return self.get() - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) - except InvalidOperation, e: - raise cherrypy.HTTPError(400, e.message) - except OperationFailed, e: - raise cherrypy.HTTPError(406, e.message) - elif method == 'DELETE': - try: - return self.delete() - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) - except InvalidParameter, e: - raise cherrypy.HTTPError(400, e.message) - elif method == 'PUT': - try: - return self.update() - except InvalidParameter, e: - raise cherrypy.HTTPError(400, e.message) - except InvalidOperation, e: - raise cherrypy.HTTPError(400, e.message) - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) + try: + return {'GET': self.get, + 'DELETE': self.delete, + 'PUT': self.update}[method]() + except InvalidOperation, e: + raise cherrypy.HTTPError(400, e.message) + except InvalidParameter, e: + raise cherrypy.HTTPError(400, e.message) + except NotFoundError, e: + raise cherrypy.HTTPError(404, e.message) + except OperationFailed, e: + raise cherrypy.HTTPError(500, e.message) + except KimchiException, e: + raise cherrypy.HTTPError(500, e.message)
def update(self): try: @@ -269,29 +261,25 @@ class Collection(object): @cherrypy.expose def index(self, *args, **kwargs): method = validate_method(('GET', 'POST')) - if method == 'GET': - try: + try: + if method == 'GET': filter_params = cherrypy.request.params validate_params(filter_params, self, 'get_list') return self.get(filter_params) - except InvalidOperation, e: - raise cherrypy.HTTPError(400, e.message) - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) - - elif method == 'POST': - try: + elif method == 'POST': return self.create(parse_request(), *args) - except MissingParameter, e: - raise cherrypy.HTTPError(400, e.message) - except InvalidParameter, e: - raise cherrypy.HTTPError(400, e.message) - except OperationFailed, e: - raise cherrypy.HTTPError(500, e.message) - except InvalidOperation, e: - raise cherrypy.HTTPError(400, e.message) - except NotFoundError, e: - raise cherrypy.HTTPError(404, e.message) + except InvalidOperation, e: + raise cherrypy.HTTPError(400, e.message) + except InvalidParameter, e: + raise cherrypy.HTTPError(400, e.message) + except MissingParameter, e: + raise cherrypy.HTTPError(400, e.message) + except NotFoundError, e: + raise cherrypy.HTTPError(404, e.message) + except OperationFailed, e: + raise cherrypy.HTTPError(500, e.message) + except KimchiException, e: + raise cherrypy.HTTPError(500, e.message)
class AsyncCollection(Collection):
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center
participants (4)
-
Aline Manera -
Mark Wu -
Sheldon -
zhshzhou＠linux.vnet.ibm.com