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

[Mark Wu]

On 03/18/2014 03:16 PM, zhshzhou at linux.vnet.ibm.com wrote:
> From: Zhou Zheng Sheng <zhshzhou at 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 at 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):