Merge pull request #190 from onefinestay/exception_serialization

try harder to serialize exceptions

nameko/exceptions.py

@@ -1,3 +1,4 @@
+import collections
 import inspect
 
 
@@ -55,19 +56,38 @@ def __init__(self, exc_type=None, value=""):
         super(RemoteError, self).__init__(message)
 
 
-def serialize(exc):
-    """ Serialize `self.exc` into a data dictionary representing it.
+def safe_for_json(value):
+    """ Transform a value in preparation for serializing as json
+
+    no-op for strings, mappings and iterables have their entries made safe,
+    and all other values are stringified, with a fallback value if that fails
     """
+
+    if isinstance(value, basestring):
+        return value
+    if isinstance(value, dict):
+        return {
+            safe_for_json(key): safe_for_json(val)
+            for key, val in value.iteritems()
+        }
+    if isinstance(value, collections.Iterable):
+        return map(safe_for_json, value)
+
     try:
-        value = unicode(exc)
+        return unicode(value)
     except Exception:
-        value = '[__unicode__ failed]'
+        return '[__unicode__ failed]'
+
+
+def serialize(exc):
+    """ Serialize `self.exc` into a data dictionary representing it.
+    """
 
     return {
         'exc_type': type(exc).__name__,
         'exc_path': get_module_path(type(exc)),
-        'exc_args': exc.args,
-        'value': value,
+        'exc_args': map(safe_for_json, exc.args),
+        'value': safe_for_json(exc),
     }
 
 

nameko/rpc.py

@@ -209,18 +209,17 @@ def send_response(self, container, result, exc_info, **kwargs):
         if exc_info is not None:
             error = serialize(exc_info[1])
 
-        # disaster avoidance serialization check
-        # `result` and `error` must both be json serializable, otherwise
-        # the container will commit suicide assuming unrecoverable errors
-        # (and the message will be requeued for another victim)
-        for item in (result, error):
-            try:
-                json.dumps(item)
-            except Exception:
-                result = None
-                exc_info = sys.exc_info()
-                # `error` below is guaranteed to serialize to json
-                error = serialize(UnserializableValueError(item))
+        # disaster avoidance serialization check: `result` must be json
+        # serializable, otherwise the container will commit suicide assuming
+        # unrecoverable errors (and the message will be requeued for another
+        # victim)
+        try:
+            json.dumps(result)
+        except Exception:
+            exc_info = sys.exc_info()
+            # `error` below is guaranteed to serialize to json
+            error = serialize(UnserializableValueError(result))
+            result = None
 
         conn = Connection(container.config[AMQP_URI_CONFIG_KEY])
 

test/test_exceptions.py

@@ -1,9 +1,14 @@
+import json
+
 from mock import patch
 import pytest
 
 from nameko.exceptions import (
-    serialize, deserialize, deserialize_to_instance, RemoteError,
-    UnserializableValueError)
+    serialize, safe_for_json, deserialize, deserialize_to_instance,
+    RemoteError, UnserializableValueError)
+
+
+OBJECT_REPR = "<type 'object'>"
 
 
 class CustomError(Exception):
@@ -23,7 +28,7 @@ def test_serialize():
     assert serialize(exc) == {
         'exc_type': 'CustomError',
         'exc_path': 'test.test_exceptions.CustomError',
-        'exc_args': ('something went wrong',),
+        'exc_args': ['something went wrong'],
         'value': 'something went wrong',
     }
 
@@ -40,11 +45,18 @@ def __str__(self):
     assert serialize(exc) == {
         'exc_type': 'CustomError',
         'exc_path': 'test.test_exceptions.CustomError',
-        'exc_args': (bad_string,),
+        'exc_args': ['[__unicode__ failed]'],
         'value': '[__unicode__ failed]',
     }
 
 
+def test_serialize_args():
+    cause = Exception('oops')
+    exc = CustomError('something went wrong', cause)
+
+    assert json.dumps(serialize(exc))
+
+
 def test_deserialize_to_remote_error():
 
     exc = CustomError('something went wrong')
@@ -140,3 +152,29 @@ def __repr__(self):
 
     assert exc.repr_value == "[__repr__ failed]"
     assert str(exc) == "Unserializable value: `[__repr__ failed]`"
+
+
+@pytest.mark.parametrize("value, safe_value", [
+    ('str', 'str'),
+    (object, OBJECT_REPR),
+    ([object], [OBJECT_REPR]),
+    ({object}, [OBJECT_REPR]),
+    ({'foo': 'bar'}, {'foo': 'bar'}),
+    ({object: None}, {OBJECT_REPR: 'None'}),
+    ({None: object}, {'None': OBJECT_REPR}),
+    ((1, 2), ['1', '2']),
+    ([1, [2]], ['1', ['2']]),
+])
+def test_safe_for_json(value, safe_value):
+    assert safe_for_json(value) == safe_value
+
+
+def test_safe_for_json_bad_str():
+    class BadStr(object):
+        def __str__(self):
+            raise Exception('boom')
+
+    obj = BadStr()
+    safe = safe_for_json(obj)
+    assert isinstance(safe, basestring)
+    assert 'failed' in safe

test/test_rpc_responder.py

@@ -2,7 +2,6 @@
 import pytest
 
 from nameko.constants import AMQP_URI_CONFIG_KEY
-from nameko.exceptions import serialize
 from nameko.rpc import Responder
 
 
@@ -60,7 +59,7 @@ def test_responder_worker_exc(mock_publish):
             'exc_path': 'exceptions.Exception',
             'value': 'error',
             'exc_type': 'Exception',
-            'exc_args': ('error',)
+            'exc_args': ['error']
         }
     }
     (msg,), _ = mock_publish.call_args
@@ -95,44 +94,7 @@ def test_responder_unserializable_result(mock_publish):
             'exc_path': 'nameko.exceptions.UnserializableValueError',
             'value': 'Unserializable value: `{}`'.format(worker_result),
             'exc_type': 'UnserializableValueError',
-            'exc_args': ()
-        }
-    }
-    (msg,), _ = mock_publish.call_args
-    assert msg == expected_msg
-
-
-def test_responder_unserializable_exc(mock_publish):
-
-    message = Mock()
-    message.properties = {'reply_to': ''}
-
-    container = Mock()
-    container.config = {AMQP_URI_CONFIG_KEY: ''}
-
-    responder = Responder(message)
-
-    # unserialisable exception
-    worker_exc = Exception(object())
-    result, exc_info = responder.send_response(
-        container, True, (Exception, worker_exc, "tb"))
-
-    # responder will return the TypeError from json.dumps
-    assert result is None
-    assert exc_info == (TypeError, ANY, ANY)
-    assert exc_info[1].message == ("{} is not JSON "
-                                   "serializable".format(worker_exc.args[0]))
-
-    # and publish a dictionary-serialized UnserializableValueError
-    # (where the unserialisable value is a dictionary-serialized worker_exc)
-    serialized_exc = serialize(worker_exc)
-    expected_msg = {
-        'result': None,
-        'error': {
-            'exc_path': 'nameko.exceptions.UnserializableValueError',
-            'value': 'Unserializable value: `{}`'.format(serialized_exc),
-            'exc_type': 'UnserializableValueError',
-            'exc_args': ()
+            'exc_args': [],
         }
     }
     (msg,), _ = mock_publish.call_args

test/test_utils.py

@@ -3,6 +3,7 @@
 from eventlet import GreenPool, sleep
 from eventlet.event import Event
 import pytest
+
 from nameko.utils import fail_fast_imap, repr_safe_str