From a96e632bf5dbc95dc361c9a66d7485c8b19ba963 Mon Sep 17 00:00:00 2001 From: Akihiro Motoki Date: Sat, 11 Nov 2017 18:43:46 +0000 Subject: [PATCH] Avoid duplicated message in Batch/DeleteAction Previously BatchAction.handle() automatically adds an error message when handle() method raises an exception. If some custom error message is shown in handle() method, two error messages will be shown. This commit allows a special handling for HandledException to BatchAction.handle(). In individual BatchAction instances, handle() method can now show a custom error message and raise an HandledException, and then BatchAction.handle() skips to show an error message for that. The reason of using HandledException is because the exception can be raised from exceptions.handle() function which is commonly used to handle exceptions. The common pattern in handle() would be: try: except Exception as e: # HandledException which wrapps the original exception will be raised. # HandledException will be caught by BatchAction.handle(). exceptions.handle(request, msg, escalate=True) To make it easier to use the pattern, a new decorator horizon.tables.actions.handle_exception_with_detail_message is introduced. Partial-Bug: #1733207 Change-Id: I4bb0f61c7b63ea6ecd2e30c03eddecee62ff9b13 --- horizon/tables/actions.py | 95 +++++++++++++++++++++++-- horizon/test/unit/tables/test_tables.py | 84 ++++++++++++++++++++++ 2 files changed, 175 insertions(+), 4 deletions(-) diff --git a/horizon/tables/actions.py b/horizon/tables/actions.py index cf6d3eca33..906560acca 100644 --- a/horizon/tables/actions.py +++ b/horizon/tables/actions.py @@ -15,6 +15,7 @@ from collections import defaultdict from collections import OrderedDict import copy +import functools import logging import types @@ -28,6 +29,7 @@ from django.utils.translation import ugettext_lazy as _ from django.utils.translation import ungettext_lazy import six +from horizon import exceptions from horizon import messages from horizon.utils import functions from horizon.utils import html @@ -775,10 +777,22 @@ class BatchAction(Action): {'action': self._get_action_name(past=True), 'datum_display': datum_display}) except Exception as ex: - # Handle the exception but silence it since we'll display - # an aggregate error message later. Otherwise we'd get - # multiple error messages displayed to the user. - action_failure.append(datum_display) + handled_exc = isinstance(ex, exceptions.HandledException) + if handled_exc: + # In case of HandledException, an error message should be + # handled in exceptions.handle() or other logic, + # so we don't need to handle the error message here. + # NOTE(amotoki): To raise HandledException from the logic, + # pass escalate=True and do not pass redirect argument + # to exceptions.handle(). + # If an exception is handled, the original exception object + # is stored in ex.wrapped[1]. + ex = ex.wrapped[1] + else: + # Handle the exception but silence it since we'll display + # an aggregate error message later. Otherwise we'd get + # multiple error messages displayed to the user. + action_failure.append(datum_display) action_description = ( self._get_action_name(past=True).lower(), datum_display) LOG.warning( @@ -879,6 +893,79 @@ class DeleteAction(BatchAction): """ +class handle_exception_with_detail_message(object): + """Decorator to allow special exception handling in BatchAction.action(). + + An exception from BatchAction.action() or DeleteAction.delete() is + normally caught by BatchAction.handle() and BatchAction.handle() displays + an aggregated error message. However, there are cases where we would like + to provide an error message which explains a failure reason if some + exception occurs so that users can understand situation better. + + This decorator allows us to do this kind of special handling easily. + This can be applied to BatchAction.action() and DeleteAction.delete() + methods. + + :param normal_log_message: Log message template when an exception other + than ``target_exception`` is detected. Keyword substituion "%(id)s" + and "%(exc)s" can be used. + + :param target_exception: Exception class should be handled specially. + If this exception is caught, a log message will be logged using + ``target_log_message`` and a user visible will be shown using + ``target_user_message``. In this case, an aggregated error message + generated by BatchAction.handle() does not include an object which + causes this exception. + + :param target_log_message: Log message template when an exception specified + in ``target_exception`` is detected. Keyword substituion "%(id)s" + and "%(exc)s" can be used. + + :param target_user_message: User visible message template when an exception + specified in ``target_exception`` is detected. It is recommended to + use an internationalized string. Keyword substituion "%(name)s" + and "%(exc)s" can be used. + + :param logger_name: (optional) Logger name to be used. + The usual pattern is to pass __name__ of a caller. + This allows us to show a module name of a caller in a logged message. + """ + + def __init__(self, normal_log_message, target_exception, + target_log_message, target_user_message, logger_name=None): + self.logger = logging.getLogger(logger_name or __name__) + self.normal_log_message = normal_log_message + self.target_exception = target_exception + self.target_log_message = target_log_message + self.target_user_message = target_user_message + + def __call__(self, fn): + @functools.wraps(fn) + def decorated(instance, request, obj_id): + try: + fn(instance, request, obj_id) + except self.target_exception as e: + self.logger.info(self.target_log_message, + {'id': obj_id, 'exc': e}) + obj = instance.table.get_object_by_id(obj_id) + name = instance.table.get_object_display(obj) + msg = self.target_user_message % {'name': name, 'exc': e} + # 'escalate=True' is required to notify the caller + # (DeleteAction) of the failure. exceptions.handle() will + # raise a wrapped exception of HandledException and BatchAction + # will handle it. 'redirect' should not be passed here as + # 'redirect' has a priority over 'escalate' argument. + exceptions.handle(request, msg, escalate=True) + except Exception as e: + self.logger.info(self.normal_log_message, + {'id': obj_id, 'exc': e}) + # NOTE: No exception handling is required here because + # BatchAction.handle() does it. What we need to do is + # just to re-raise the exception. + raise + return decorated + + class Deprecated(type): # TODO(lcastell) Replace class with similar functionality from # oslo_log.versionutils when it's finally added in 11.0 diff --git a/horizon/test/unit/tables/test_tables.py b/horizon/test/unit/tables/test_tables.py index eaac020698..77ed4a4549 100644 --- a/horizon/test/unit/tables/test_tables.py +++ b/horizon/test/unit/tables/test_tables.py @@ -15,6 +15,9 @@ # License for the specific language governing permissions and limitations # under the License. +import unittest +import uuid + from django.core.urlresolvers import reverse from django import forms from django import http @@ -23,10 +26,13 @@ from django.template import defaultfilters from django.test.utils import override_settings from django.utils.translation import ungettext_lazy +import mock from mox3.mox import IsA import six +from horizon import exceptions from horizon import tables +from horizon.tables import actions from horizon.tables import formset as table_formset from horizon.tables import views as table_views from horizon.test import helpers as test @@ -1603,3 +1609,81 @@ class FormsetTableTests(test.TestCase): form_data = form.initial self.assertEqual('object_1', form_data['name']) self.assertEqual(2, form_data['value']) + + +class MyException(Exception): + pass + + +class OtherException(Exception): + pass + + +class BatchActionDecoratorTests(unittest.TestCase): + + def setUp(self): + obj = uuid.uuid4() + self.obj_id = 'id-%s' % str(obj) + self.obj_name = 'name-%s' % str(obj) + table = mock.Mock() + table.get_object_by_id.return_value = obj + table.get_object_display.return_value = self.obj_name + + # getLogger is called insdie handle_exception_with_detail_message + # decorator, so this needs to be mocked before using the decorator. + self.logger = mock.Mock() + with mock.patch('logging.getLogger', + return_value=self.logger) as mock_getlogger: + class MyAction(object): + def __init__(self, table): + self.table = table + + @actions.handle_exception_with_detail_message( + 'normal log message %(id)s %(exc)s', + MyException, + 'target log message %(id)s %(exc)s', + 'target user message %(name)s %(exc)s', + 'mylogger') + def action(self, request, obj_id): + self._to_be_mocked() + + # This is required because if mock.patch replaces + # a decorated method. We are testing a decorated method + # so we need a separate method to mock, + def _to_be_mocked(self): + pass + + self.action = MyAction(table) + self.mock_getlogger = mock_getlogger + + def test_normal_exception(self): + myexc = OtherException() + with mock.patch.object(self.action, '_to_be_mocked', + side_effect=myexc): + self.assertRaises(OtherException, + self.action.action, + mock.sentinel.request, self.obj_id) + self.mock_getlogger.assert_called_once_with('mylogger') + self.logger.info.assert_called_once_with( + 'normal log message %(id)s %(exc)s', + {'id': self.obj_id, 'exc': myexc}) + + def test_target_exception(self): + myexc = MyException() + handled = exceptions.HandledException(myexc) + with mock.patch.object(self.action, '_to_be_mocked', + side_effect=myexc), \ + mock.patch.object(exceptions, 'handle', + side_effect=handled) as mocked_handle: + self.assertRaises(exceptions.HandledException, + self.action.action, + mock.sentinel.request, self.obj_id) + self.mock_getlogger.assert_called_once_with('mylogger') + self.logger.info.assert_called_once_with( + 'target log message %(id)s %(exc)s', + {'id': self.obj_id, 'exc': myexc}) + mocked_handle.assert_called_once_with( + mock.sentinel.request, + 'target user message %(name)s %(exc)s' % + {'name': self.obj_name, 'exc': myexc}, + escalate=True)