Merge "Make sure not to swallow original exceptions on handling remote errors"

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(
+            msg = (
-                "Size limit %dKB exceed for class [%s], "
+                "Field size limit exceeded"
-                "field %s of size %dKB.",
+                " [class={}, field={}, size={}KB, limit={}KB]"
-                size_limit_kb, str(cls), field_name, size_kb
+            ).format(
-            )
+                cls.__name__,
-
-            raise exc.SizeLimitExceededException(
                 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.

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

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
+    oslo.messaging has different behavior on raising exceptions depending on
-    when fake or rabbit transports are used. In case of rabbit
+    whether we use 'fake' or 'rabbit' transports. In case of 'rabbit' transport
-    transport it raises wrapped RemoteError which forwards directly
+    it raises an instance of RemoteError which forwards directly to the API.
-    to API. Wrapped RemoteError contains one of MistralException raised
+    The RemoteError instance contains one of the MistralException instances
-    remotely on Engine and for correct exception interpretation we
+    raised remotely on the RPC server side and for correct exception handling
-    need to unwrap and raise given exception and manually send it to
+    we need to unwrap and raise the original wrapped exception.
-    API layer.
     """
     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)

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)
         )