From 3e042238886e9a3640469423bbdf241832233b7e Mon Sep 17 00:00:00 2001 From: Renat Akhmerov Date: Wed, 7 Mar 2018 15:48:41 +0700 Subject: [PATCH] Make sure not to swallow original exceptions on handling remote errors * Also removed a custom initializer of SizeLimitExceededException with 3 parameters in favor of creating a static method because we have to assume that all MistralException classes have an initializer with only one parameter "message" to be able to restore an exception on the RPC client which was raised on the RPC server. Change-Id: Idd52bb7bb18168826644d8aaca3d96be46f8ae9e Closes-Bug: #1750752 --- mistral/db/v2/sqlalchemy/models.py | 16 +++++++------ mistral/exceptions.py | 5 ---- mistral/rpc/base.py | 24 ++++++++++++------- .../test_execution_fields_size_limitation.py | 20 ++++++++++------ 4 files changed, 37 insertions(+), 28 deletions(-) diff --git a/mistral/db/v2/sqlalchemy/models.py b/mistral/db/v2/sqlalchemy/models.py index 9e3367a91..54e84d6b2 100644 --- a/mistral/db/v2/sqlalchemy/models.py +++ b/mistral/db/v2/sqlalchemy/models.py @@ -65,18 +65,20 @@ def validate_long_type_length(cls, field_name, value): size_kb = int(sys.getsizeof(str(value)) / 1024) if size_kb > size_limit_kb: - LOG.error( - "Size limit %dKB exceed for class [%s], " - "field %s of size %dKB.", - size_limit_kb, str(cls), field_name, size_kb - ) - - raise exc.SizeLimitExceededException( + msg = ( + "Field size limit exceeded" + " [class={}, field={}, size={}KB, limit={}KB]" + ).format( + cls.__name__, field_name, size_kb, size_limit_kb ) + LOG.error(msg) + + raise exc.SizeLimitExceededException(msg) + def register_length_validator(attr_name): """Register an event listener on the attribute. diff --git a/mistral/exceptions.py b/mistral/exceptions.py index ce8c7d714..318bc752a 100644 --- a/mistral/exceptions.py +++ b/mistral/exceptions.py @@ -185,11 +185,6 @@ class InvalidResultException(MistralException): class SizeLimitExceededException(MistralException): http_code = 400 - def __init__(self, field_name, size_kb, size_limit_kb): - super(SizeLimitExceededException, self).__init__( - "Size of '%s' is %dKB which exceeds the limit of %dKB" - % (field_name, size_kb, size_limit_kb)) - class CoordinationException(MistralException): http_code = 500 diff --git a/mistral/rpc/base.py b/mistral/rpc/base.py index 03700dacd..69e5d64b7 100644 --- a/mistral/rpc/base.py +++ b/mistral/rpc/base.py @@ -85,25 +85,31 @@ def _wrap_exception_and_reraise(exception): def wrap_messaging_exception(method): - """This decorator unwrap remote error in one of MistralException. + """The decorator unwraps a remote error into one of the mistral exceptions. - oslo.messaging has different behavior on raising exceptions - when fake or rabbit transports are used. In case of rabbit - transport it raises wrapped RemoteError which forwards directly - to API. Wrapped RemoteError contains one of MistralException raised - remotely on Engine and for correct exception interpretation we - need to unwrap and raise given exception and manually send it to - API layer. + oslo.messaging has different behavior on raising exceptions depending on + whether we use 'fake' or 'rabbit' transports. In case of 'rabbit' transport + it raises an instance of RemoteError which forwards directly to the API. + The RemoteError instance contains one of the MistralException instances + raised remotely on the RPC server side and for correct exception handling + we need to unwrap and raise the original wrapped exception. """ def decorator(*args, **kwargs): try: return method(*args, **kwargs) - except exc.MistralException: raise except (client.RemoteError, exc.KombuException, Exception) as e: + # Since we're going to transform the original exception + # we need to log it as is. + LOG.exception( + "Caught a messaging remote error." + " See details of the original exception." + ) + if hasattr(e, 'exc_type') and hasattr(exc, e.exc_type): exc_cls = getattr(exc, e.exc_type) + raise exc_cls(e.value) _wrap_exception_and_reraise(e) diff --git a/mistral/tests/unit/engine/test_execution_fields_size_limitation.py b/mistral/tests/unit/engine/test_execution_fields_size_limitation.py index 9ef47229e..7ab75c577 100644 --- a/mistral/tests/unit/engine/test_execution_fields_size_limitation.py +++ b/mistral/tests/unit/engine/test_execution_fields_size_limitation.py @@ -140,7 +140,8 @@ class ExecutionFieldsSizeLimitTest(base.EngineTestCase): ) self.assertEqual( - "Size of 'input' is 1KB which exceeds the limit of 0KB", + 'Field size limit exceeded' + ' [class=TaskExecution, field=input, size=1KB, limit=0KB]', str(e) ) @@ -156,7 +157,8 @@ class ExecutionFieldsSizeLimitTest(base.EngineTestCase): ) self.assertEqual( - "Size of 'input' is 1KB which exceeds the limit of 0KB", + 'Field size limit exceeded' + ' [class=TaskExecution, field=input, size=1KB, limit=0KB]', str(e) ) @@ -170,7 +172,8 @@ class ExecutionFieldsSizeLimitTest(base.EngineTestCase): self.assertEqual(states.ERROR, wf_ex.state) self.assertIn( - "Size of 'input' is 1KB which exceeds the limit of 0KB", + "Field size limit exceeded" + " [class=TaskExecution, field=input, size=1KB, limit=0KB]", wf_ex.state_info ) @@ -189,7 +192,8 @@ class ExecutionFieldsSizeLimitTest(base.EngineTestCase): wf_ex = db_api.get_workflow_execution(wf_ex.id) self.assertIn( - "Size of 'output' is 1KB which exceeds the limit of 0KB", + 'Field size limit exceeded' + ' [class=TaskExecution, field=output, size=1KB, limit=0KB]', wf_ex.state_info ) self.assertEqual(states.ERROR, wf_ex.state) @@ -211,7 +215,7 @@ class ExecutionFieldsSizeLimitTest(base.EngineTestCase): task_execs = wf_ex.task_executions self.assertIn( - 'Failed to handle action completion [error=Size of', + 'Failed to handle action completion [error=Field size', wf_ex.state_info ) self.assertIn('wf=wf, task=task1', wf_ex.state_info) @@ -219,7 +223,8 @@ class ExecutionFieldsSizeLimitTest(base.EngineTestCase): task_ex = self._assert_single_item(task_execs, name='task1') self.assertIn( - "Size of 'published' is 1KB which exceeds the limit of 0KB", + 'Field size limit exceeded' + ' [class=TaskExecution, field=published, size=1KB, limit=0KB]', task_ex.state_info ) @@ -237,7 +242,8 @@ class ExecutionFieldsSizeLimitTest(base.EngineTestCase): ) self.assertIn( - "Size of 'params' is 1KB which exceeds the limit of 0KB", + 'Field size limit exceeded' + ' [class=TaskExecution, field=params, size=1KB, limit=0KB]', str(e) )