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