Re: [Kimchi-devel] [PATCH] Controller: Improve Kimchi Specific Exception Reporting

On 03/18/2014 03:16 PM, zhshzhou(a)linux.vnet.ibm.com wrote: > From: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt; > > 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(a)linux.vnet.ibm.com&gt; > --- > 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(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel