Improved error handling in Websocket server #229

blondejamtart · 2018-04-25T10:32:24Z

Added return error messages to client following errors in websocket comms
Also fixed test requirements.

Added return error messages to client following errors in websocket comms Also fixed test requirements.

Tweaked error messages for various components called in WebsocketHandler to improve readability and added to unit tests

thomascobb · 2018-04-26T12:58:51Z

malcolm/core/controller.py

@@ -187,7 +187,9 @@ def _handle_get(self, request):
        # type: (Get) -> CallbackResponses
        """Called with the lock taken"""
        data = self._block
-        for endpoint in request.path[1:]:
+
+        for point in range(1, len(request.path)):


Can I suggest:

for i, endpoint in enumerate(request.path[1:]):

thomascobb · 2018-04-26T12:59:08Z

malcolm/core/request.py

@@ -76,9 +76,12 @@ class PathRequest(Request):
    def __init__(self, id=0, path=None):
        # type: (AId, UPath) -> None
        super(PathRequest, self).__init__(id)
+        if path:
+            assert isinstance(path, list), "Path must be given as a list"


(list, tuple)

thomascobb · 2018-04-26T13:01:26Z

malcolm/core/serializable.py

@@ -108,7 +112,7 @@ def deserialize_object(ob, type_check=None):
        ob = subclass.from_dict(ob)
    if type_check is not None:
        assert isinstance(ob, type_check), \
-            "Expected %s, got %r" % (type_check, ob)
+            "Expected %s, got %r" % (type_check, subclass)


thomascobb · 2018-04-26T13:03:29Z

malcolm/core/serializable.py

+        try:
+            inst = cls(**filtered)
+        except TypeError as e:
+            raise TypeError("%s method %s" % (cls.typeid, str(e)))


"%s(**%s) raised error: %s" %(type(cls), filtered, e)

thomascobb · 2018-04-26T13:03:59Z

malcolm/core/serializable.py

+        try:
+            typeid = d["typeid"]
+        except KeyError:
+            raise KeyError("typeid field not present in JSON message")


include the dict?

thomascobb · 2018-04-26T13:05:36Z

malcolm/core/serializable.py

+            subclass = cls._subcls_lookup[typeid]
+            return subclass
+        except KeyError as e:
+            raise KeyError('%s not a valid malcolm message subclass' % str(e))


maybe do cls._subcls_lookup.get(typeid, None)

if subclass is None:
raise KeyError('%s not a valid typeid" % typeid)

is not a valid typeid

thomascobb · 2018-04-26T13:07:11Z

tests/test_modules/test_web/test_system_websocket.py

+    @gen.coroutine
+    def send_bad_json_message(self, error_type):
+        # 0 = no ID, 1 = bad typeID, 2 = no typeID, 3 = bad path, 4 = no path
+        req = [dict(


Can you move these to where they are called from please

blondejamtart · 2018-04-26T16:35:13Z

One test is failing on Travis but passes on my local machine; appears to be mostly to do with how I've included a stringified dict object in one of the error messages (raised in serializable.py line 213), since it causes inconsistent quotation marks and capitalisation. Need a more robust/reliable way to send the dict info (or just drop it from the message?)

thomascobb · 2018-04-27T08:09:30Z

tests/test_modules/test_web/test_system_websocket.py

        conn.write_message(json.dumps(req))
        resp = yield conn.read_message()
        resp = json.loads(resp)
        self.result.put(resp)
        conn.close()

+    @gen.coroutine
+    def send_non_JSON_message(self):


Can you merge this with the previous function please. I suggest

def send_message(self, req, json_convert=True):

thomascobb · 2018-04-27T08:11:39Z

malcolm/core/serializable.py

+        assert isinstance(o, OrderedDict), "didn't return OrderedDict"
+        return o
+    except Exception as e:
+            raise ValueError("Error decoding JSON object (%s)" % str(e))


4 space indent please

thomascobb · 2018-04-27T08:28:19Z

tests/test_modules/test_web/test_system_websocket.py

+        assert resp == dict(
+            typeid="malcolm:core/Error:1.0",
+            id=-1,
+            message="KeyError: 'id field not present in JSON message'"


To get rid of these extra quotes we need to change serialize_hook in serializable.py to return:

"%s: %s" % (type(o).__name__, o.message)

instead of

"%s: %s" % (type(o).__name__, o)

thomascobb · 2018-04-27T08:29:50Z

tests/test_modules/test_web/test_system_websocket.py

+        assert resp == dict(
+            typeid="malcolm:core/Error:1.0",
+            id=0,
+            message='''KeyError: "typeid field not present in dictionary ( d = OrderedDict([(u\'path\',''' +


Maybe change it to just print the keys rather than the dict

Temporarily disabled some tests which were being troublesome on Travis

Gone back to str(error) instead of error.message for compatibility reasons, also implemented "FieldError" class to replace KeyError for better behaviour with the above

Added different expected results for python3 (with version check) for some WebsocketHandler error cases

Attempt to correct inconsistent behaviour of OrderedDict.keys() between python versions

…malcolm into WebSocket-ErrorReporting

thomascobb · 2018-04-30T08:43:08Z

malcolm/core/serializable.py

@@ -6,7 +6,7 @@
 from enum import Enum

 from malcolm.compat import OrderedDict
-
+from malcolm.core.errors import FieldError


from .errors import FieldError

thomascobb · 2018-04-30T08:43:53Z

malcolm/core/serializable.py

+        try:
+            typeid = d["typeid"]
+        except KeyError:
+            raise FieldError("typeid field not present in dictionary ( d.keys() = %s )" % list(dict(d).keys()))


Also added extra test for superfluous params in message

Improved error handling in Websocket server

efb77a4

Added return error messages to client following errors in websocket comms Also fixed test requirements.

blondejamtart requested a review from thomascobb April 25, 2018 10:32

Improved WebsocketHandler errors

29251fe

Tweaked error messages for various components called in WebsocketHandler to improve readability and added to unit tests

thomascobb suggested changes Apr 26, 2018

View reviewed changes

Revised error messages and unit tests

4ad65a5

thomascobb suggested changes Apr 27, 2018

View reviewed changes

Bryan Robert Tester and others added 17 commits April 27, 2018 10:57

Further improved error messages

33e3937

Increased pytest logging verbosity

7c50713

Messed with pytest logging some more

efb4864

Disabled some tests

6718c8c

Temporarily disabled some tests which were being troublesome on Travis

Added check/workaround for exception message attribute

560f8da

Bugfix in serialize_hook exception message

cd294ff

Tweaked error messages for python3

15788db

Fixed error messages

f1591fa

Gone back to str(error) instead of error.message for compatibility reasons, also implemented "FieldError" class to replace KeyError for better behaviour with the above

Updated Unit tests for python3

6d1e7c1

Added different expected results for python3 (with version check) for some WebsocketHandler error cases

Fixed typo in error test case

21c3e4b

Tweaked dict key reporting in error message

92b00ea

Attempt to correct inconsistent behaviour of OrderedDict.keys() between python versions

Merge branch 'WebSocket-ErrorReporting' of github.com:dls-controls/py…

852ddeb

…malcolm into WebSocket-ErrorReporting

Fixed inconsistend dict key behaviour

8e4604c

Adjusted py3 error tests to account for unicode strings

24063b3

Adjusted error message because python3 is wierd

bdb33c7

Adjusted error to account for python3 being wierd

8be2950

Changed display of dict keys in error message

46fada0

thomascobb suggested changes Apr 30, 2018

View reviewed changes

Bryan Robert Tester added 3 commits April 30, 2018 09:48

Changed dict error reporting (take 2)

075ff46

Changed test messages to be OrderedDicts

1be74e7

Changes test messages to be OrderedDict (take 2)

1ea649a

Also added extra test for superfluous params in message

thomascobb merged commit 79db61c into annotypes Apr 30, 2018

thomascobb deleted the WebSocket-ErrorReporting branch April 30, 2018 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved error handling in Websocket server #229

Improved error handling in Websocket server #229

blondejamtart commented Apr 25, 2018

thomascobb Apr 26, 2018

thomascobb Apr 26, 2018

thomascobb Apr 26, 2018

thomascobb Apr 26, 2018

thomascobb Apr 26, 2018

thomascobb Apr 26, 2018

thomascobb Apr 26, 2018

blondejamtart commented Apr 26, 2018

thomascobb Apr 27, 2018

thomascobb Apr 27, 2018

thomascobb Apr 27, 2018

thomascobb Apr 27, 2018

thomascobb Apr 30, 2018

thomascobb Apr 30, 2018

Improved error handling in Websocket server #229

Improved error handling in Websocket server #229

Conversation

blondejamtart commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blondejamtart commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment