Fixed several bugs with notifications #16
Conversation
Fixed inbound http/websocket notifications to return correct transport-specific results
| @@ -253,12 +267,12 @@ def __handle(self, content, message): | |||
| else: | |||
| if isinstance(data, dict): | |||
|
|
|||
| if data.get('method') is not None and data.get('params') is not None and data.get('id') is None: | |||
There was a problem hiding this comment.
@soulhunter1987
Why did you remove the check for 'params' and 'method'?
That is what make the tests fail.
If sending {"value": "my_value"} which should be received as a wrongly formatted JSON-RPC frame, it handles it as a notification,....
There was a problem hiding this comment.
You are right here, I did not think about such case. I'll change this back now...
channels_jsonrpc/jsonrpcconsumer.py
Outdated
| # Send responce back | ||
| self.send(text=self._encode(result)) | ||
| # Send responce back only if it is a call, not notification | ||
| if result_type == 0: |
There was a problem hiding this comment.
@soulhunter1987
please use some GLOBAL_VAR for the result_type:
RESULT_NOTIFICATION_TYPE/RESULT_REQUEST_TYPE, for clarity purposes, or 'is_notification as variable name' ...
There was a problem hiding this comment.
Of course it should be done, but for now It is just a quick working solution. These lines would anywhere look different after moving to a JSON-RPC decoding/encoding library, and I plan to do it soon, because I need support of batch calls, and, may be, protocol versions 1.1/1.2 along with 2.0. So this is a temporary working solution.
There was a problem hiding this comment.
Pushed a quick fix
…ng it for certain use cases (allow passing flags to json.encode())
|
I pushed one more fix with contant usage of JsonRpcConsumer._encode() to simplify json encoder configuration. |
Fixed method JsonRpcConsumer.notify_channel()
Fixed inbound http/websocket notifications to return correct transport-specific results