[PATCH 0/3 RFC] Refactor exception
From: Aline Manera <alinefm@br.ibm.com> Hi all, This is the RFC for the refactor exception task. Sheldon has already done something related to that some time ago. He also created a wiki for that: - https://github.com/kimchi-project/kimchi/wiki/refactor-exception The only change I made in him approach is translating the message in backend and send it to UI That way we don't need to duplicate the messages on backend and UI (as it is explained on wiki) The steps to get it done: 1) Change cherrypy error handler to send the exception data to UI 2) Create a common Exception class (KimchiError) to translate the error message and proper set the parameters 3) Update UI to show the message received from backend to the user I also would like to create a file on backend: __messages__.py which will contain all messages used on backend. For example: # __messages__.py VM_START_FAILED = "<code>: Failed to start virtual machine %(name)s" The contants names should be: <resource>_<action>_<error> The <code> need to be a unique value between all messages. It is needed in order to we identify the error independent of the message language. That way a user running Kimchi in German can easily post his error on mail list or github, and we can identify where the problem is by the message code. I also will add this code in the UI messages. For the code, we can build it like: <resource><BACK|UI><type-of-error><ident> And on backend: VMBACKINFO01 VMBACKERRO01 VMBACKWARN01 And on UI: VMUIINFO01 VMUIERRO01 VMUIWARN01 So we will also need: 4) Create a __messages__ file with all messages used on backend 5) Update build process to add the messages in this file to .po files 6) Update UI messages to add a code for them I made a small patch set with my proposal. This patch set does not include the items 4,5 and 6 In fact, I used an existent message for test (as you can notice on patches) And *just for example* it will *always* raise an exception while starting a VM. Any comments/suggestions are welcome. =) Aline Manera (2): refactor exception: Create a common Exception to translate error messages refator exception: Example of use ShaoHe Feng (1): refactor exception: Add a kimchi special error handler src/kimchi/control/base.py | 5 ++++- src/kimchi/exception.py | 29 ++++++++++++++++++++++------- src/kimchi/model/vms.py | 4 +++- src/kimchi/template.py | 14 ++++++++++++++ ui/js/src/kimchi.guest_main.js | 4 ++-- 5 files changed, 45 insertions(+), 11 deletions(-) -- 1.7.10.4
From: ShaoHe Feng <shaohef@linux.vnet.ibm.com> Cherrpy support a custom error handling. http://docs.cherrypy.org/stable/refman/_cperror.html#custom-error-handling But the arguments of custom error handling are fixed. They are (status, message, traceback, version) http://docs.cherrypy.org/stable/refman/_cperror.html#anticipated-http-respon... But now in code we would raise an exception as: raise KimchiError(<msg>, <params>) We need to pass the message and the args So we should not use the cherrpy custom error handling. Overridden the HTTPError for kimchi needs. Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> --- src/kimchi/template.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/kimchi/template.py b/src/kimchi/template.py index 173e7c6..f14ee90 100644 --- a/src/kimchi/template.py +++ b/src/kimchi/template.py @@ -107,3 +107,17 @@ def render(resource, data): raise else: raise cherrypy.HTTPError(406) + +class HTTPError(cherrypy.HTTPError): + def __init__(self, status=500, exception=None, message=None): + super(HTTPError, self).__init__(status, message) + self.exception = exception + + + def get_error_page(self, status, **kwargs): + data = {'message': self.exception.message} + if cherrypy.config.get("environment") != "production": + data['call_stack'] = cherrypy._cperror.format_exc() + res = render('error.html', data) + + return res.encode("utf-8") -- 1.7.10.4
From: Aline Manera <alinefm@br.ibm.com> KimchiError will be the base for all other Exception types in Kimchi It will translate the error message before raising it. Then UI can use the message directly. That way we avoid duplicating messages on UI and backend. Signed-off-by: Aline Manera <alinefm@br.ibm.com> --- src/kimchi/exception.py | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/src/kimchi/exception.py b/src/kimchi/exception.py index a37015b..f7dc200 100644 --- a/src/kimchi/exception.py +++ b/src/kimchi/exception.py @@ -20,30 +20,45 @@ # License along with this library; if not, write to the Free Software # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +import cherrypy +import gettext -class NotFoundError(Exception): +from kimchi.template import get_lang, validate_language + + +class KimchiError(Exception): + def __init__(self, msg, params): + lang = validate_language(get_lang()) + paths = cherrypy.request.app.root.paths + translation = gettext.translation('kimchi', paths.mo_dir, [lang]) + text = msg.encode('utf-8') + self.msg = unicode(translation.gettext(text), 'utf-8') % params + Exception.__init__(self, self.msg) + + +class NotFoundError(KimchiError): pass -class OperationFailed(Exception): +class OperationFailed(KimchiError): pass -class MissingParameter(Exception): +class MissingParameter(KimchiError): pass -class InvalidParameter(Exception): +class InvalidParameter(KimchiError): pass -class InvalidOperation(Exception): +class InvalidOperation(KimchiError): pass -class IsoFormatError(Exception): +class IsoFormatError(KimchiError): pass -class TimeoutExpired(Exception): +class TimeoutExpired(KimchiError): pass -- 1.7.10.4
From: Aline Manera <alinefm@br.ibm.com> This is just an example with which changes are needed in backend and UI to properly show the error messages translated to the user Signed-off-by: Aline Manera <alinefm@br.ibm.com> --- src/kimchi/control/base.py | 5 ++++- src/kimchi/model/vms.py | 4 +++- ui/js/src/kimchi.guest_main.js | 4 ++-- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/kimchi/control/base.py b/src/kimchi/control/base.py index f50ff6e..e50720a 100644 --- a/src/kimchi/control/base.py +++ b/src/kimchi/control/base.py @@ -30,8 +30,9 @@ import kimchi.template 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 InvalidOperation, InvalidParameter, KimchiError from kimchi.exception import MissingParameter, NotFoundError, OperationFailed +from kimchi.template import HTTPError class Resource(object): @@ -82,6 +83,8 @@ class Resource(object): raise cherrypy.HTTPError(500, "Operation Failed: '%s'" % msg) except NotFoundError, msg: raise cherrypy.HTTPError(404, "Not found: '%s'" % msg) + except KimchiError, e: + raise HTTPError(400, e) wrapper.__name__ = action_name wrapper.exposed = True diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index e9f7753..374bbe0 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -30,8 +30,9 @@ from cherrypy.process.plugins import BackgroundTask from kimchi import vnc from kimchi import xmlutils -from kimchi.exception import InvalidOperation, InvalidParameter +from kimchi.exception import KimchiError, InvalidOperation, InvalidParameter from kimchi.exception import NotFoundError, OperationFailed +from kimchi.model.__messages__ import ERR_VM_START_FAILED from kimchi.model.config import CapabilitiesModel from kimchi.model.templates import TemplateModel from kimchi.model.utils import get_vm_name @@ -346,6 +347,7 @@ class VMModel(object): vnc.remove_proxy_token(name) def start(self, name): + raise KimchiError("Go to Homepage", {}) dom = self.get_vm(name, self.conn) dom.create() diff --git a/ui/js/src/kimchi.guest_main.js b/ui/js/src/kimchi.guest_main.js index 8467f3f..2fadbe5 100644 --- a/ui/js/src/kimchi.guest_main.js +++ b/ui/js/src/kimchi.guest_main.js @@ -25,8 +25,8 @@ kimchi.initVmButtonsAction = function() { $(this).addClass('loading'); kimchi.startVM($(this).data('vm'), function(result) { kimchi.listVmsAuto(); - }, function() { - kimchi.message.error(i18n['msg.fail.start']); + }, function(err) { + kimchi.message.error(err.responseJSON.message); }); } else { event.preventDefault(); -- 1.7.10.4
Or even better, we map all code and messages on UI (i18n.html) and backend returns the code and params. For example: KimchiError('VMERRO01', 'my-vm') And on UI: i18n[code], params That way the translation will be done just in one place - UI But while using the REST API we will return code and params - not a full message. (I am not sure I like it) What do you think about those 2 proposals? On Jan 31, 2014 4:21 PM, "Aline Manera" <alinefm@linux.vnet.ibm.com> wrote:
From: Aline Manera <alinefm@br.ibm.com>
Hi all,
This is the RFC for the refactor exception task. Sheldon has already done something related to that some time ago.
He also created a wiki for that: - https://github.com/kimchi-project/kimchi/wiki/refactor-exception
The only change I made in him approach is translating the message in backend and send it to UI That way we don't need to duplicate the messages on backend and UI (as it is explained on wiki)
The steps to get it done: 1) Change cherrypy error handler to send the exception data to UI 2) Create a common Exception class (KimchiError) to translate the error message and proper set the parameters 3) Update UI to show the message received from backend to the user
I also would like to create a file on backend: __messages__.py which will contain all messages used on backend. For example:
# __messages__.py VM_START_FAILED = "<code>: Failed to start virtual machine %(name)s"
The contants names should be: <resource>_<action>_<error>
The <code> need to be a unique value between all messages. It is needed in order to we identify the error independent of the message language. That way a user running Kimchi in German can easily post his error on mail list or github, and we can identify where the problem is by the message code.
I also will add this code in the UI messages.
For the code, we can build it like: <resource><BACK|UI><type-of-error><ident> And on backend:
VMBACKINFO01 VMBACKERRO01 VMBACKWARN01
And on UI:
VMUIINFO01 VMUIERRO01 VMUIWARN01
So we will also need: 4) Create a __messages__ file with all messages used on backend 5) Update build process to add the messages in this file to .po files 6) Update UI messages to add a code for them
I made a small patch set with my proposal. This patch set does not include the items 4,5 and 6 In fact, I used an existent message for test (as you can notice on patches) And *just for example* it will *always* raise an exception while starting a VM.
Any comments/suggestions are welcome. =)
Aline Manera (2): refactor exception: Create a common Exception to translate error messages refator exception: Example of use
ShaoHe Feng (1): refactor exception: Add a kimchi special error handler
src/kimchi/control/base.py | 5 ++++- src/kimchi/exception.py | 29 ++++++++++++++++++++++------- src/kimchi/model/vms.py | 4 +++- src/kimchi/template.py | 14 ++++++++++++++ ui/js/src/kimchi.guest_main.js | 4 ++-- 5 files changed, 45 insertions(+), 11 deletions(-)
-- 1.7.10.4
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
Based in all comments I will follow the second approach I proposed. That way we just translate messages in a single place. On 01/31/2014 07:17 PM, aline.manera@gmail.com wrote:
Or even better, we map all code and messages on UI (i18n.html) and backend returns the code and params.
For example: KimchiError('VMERRO01', 'my-vm')
And on UI: i18n[code], params
That way the translation will be done just in one place - UI
But while using the REST API we will return code and params - not a full message. (I am not sure I like it)
What do you think about those 2 proposals?
On Jan 31, 2014 4:21 PM, "Aline Manera" <alinefm@linux.vnet.ibm.com <mailto:alinefm@linux.vnet.ibm.com>> wrote:
From: Aline Manera <alinefm@br.ibm.com <mailto:alinefm@br.ibm.com>>
Hi all,
This is the RFC for the refactor exception task. Sheldon has already done something related to that some time ago.
He also created a wiki for that: - https://github.com/kimchi-project/kimchi/wiki/refactor-exception
The only change I made in him approach is translating the message in backend and send it to UI That way we don't need to duplicate the messages on backend and UI (as it is explained on wiki)
The steps to get it done: 1) Change cherrypy error handler to send the exception data to UI 2) Create a common Exception class (KimchiError) to translate the error message and proper set the parameters 3) Update UI to show the message received from backend to the user
I also would like to create a file on backend: __messages__.py which will contain all messages used on backend. For example:
# __messages__.py VM_START_FAILED = "<code>: Failed to start virtual machine %(name)s"
The contants names should be: <resource>_<action>_<error>
The <code> need to be a unique value between all messages. It is needed in order to we identify the error independent of the message language. That way a user running Kimchi in German can easily post his error on mail list or github, and we can identify where the problem is by the message code.
I also will add this code in the UI messages.
For the code, we can build it like: <resource><BACK|UI><type-of-error><ident> And on backend:
VMBACKINFO01 VMBACKERRO01 VMBACKWARN01
And on UI:
VMUIINFO01 VMUIERRO01 VMUIWARN01
So we will also need: 4) Create a __messages__ file with all messages used on backend 5) Update build process to add the messages in this file to .po files 6) Update UI messages to add a code for them
I made a small patch set with my proposal. This patch set does not include the items 4,5 and 6 In fact, I used an existent message for test (as you can notice on patches) And *just for example* it will *always* raise an exception while starting a VM.
Any comments/suggestions are welcome. =)
Aline Manera (2): refactor exception: Create a common Exception to translate error messages refator exception: Example of use
ShaoHe Feng (1): refactor exception: Add a kimchi special error handler
src/kimchi/control/base.py | 5 ++++- src/kimchi/exception.py | 29 ++++++++++++++++++++++------- src/kimchi/model/vms.py | 4 +++- src/kimchi/template.py | 14 ++++++++++++++ ui/js/src/kimchi.guest_main.js | 4 ++-- 5 files changed, 45 insertions(+), 11 deletions(-)
-- 1.7.10.4
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org <mailto:Kimchi-devel@ovirt.org> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
I would pick the first approach because that way we can parameterize the backend error messages. Even though I would still prefer having one single file with the messages. Am 04-02-2014 10:24, schrieb Aline Manera:
Based in all comments I will follow the second approach I proposed. That way we just translate messages in a single place.
On 02/04/2014 12:19 PM, Crístian Viana wrote:
I would pick the first approach because that way we can parameterize the backend error messages. Even though I would still prefer having one single file with the messages.
Thanks very much for the comments, Cristian. I am OK to follow the first approach as well.
Am 04-02-2014 10:24, schrieb Aline Manera:
Based in all comments I will follow the second approach I proposed. That way we just translate messages in a single place.
participants (3)
-
Aline Manera -
aline.manera＠gmail.com -
Crístian Viana