Cleanup JsonRPC codec #51
Conversation
* Move the error formating out. * pre-process and post-process the message to handle null/undefined conversion. * Always return a list of message, no special case for single/batch. * Return decoding errors as proper jsonrpc errors that could be sent right away to the peer.
There was a problem hiding this comment.
How do we distinguish between a single JSON-RPC request and a batch with only one JSON-RPC request. As I understand the JSON-RPC 2.0 specification we should always response an array when receiving a batch, so I would do that for a batch request with one element as well. I guess that was the reason the differentiation was implemented the way it was.
src/grisp_connect_jsonrpc.erl
Outdated
| | {error, Code :: integer(), Message :: undefined | binary(), | ||
| Data :: undefined | term(), ReqRef :: undefined | binary() | integer()} | ||
| | {decoding_error, Code :: integer(), Message :: undefined | binary(), | ||
| Data :: undefined | term(), ReqRef :: undefined | binary()}. |
There was a problem hiding this comment.
Can we ever have a ReqRef for a decoding error? It is supposed to be the ID of the JSON-PRC request, right? When we can't decode the JSON we don't get the ID.
There was a problem hiding this comment.
The fact that the decoding error is due to invalid JSON or anything else is an implementation detail of the module. If something when wrong, it respond with an error that can be encoded and sent to the peer right away without the need to know what happened. In a way this is the same as an error, but for the sender, not the receiver.
There was a problem hiding this comment.
According to JSON-RPC in the response object the ID is REQUIRED and MUST be the same as in the request. We can't send a response, when we don't know the ID, we can't know the ID when we can't decode the JSON.
There was a problem hiding this comment.
Indeed, but you can always send back a generic error without an ID. if you don't have it you don't put one, this is why if can be undefined in the case, but we can still inform the client that there was some errors....
src/grisp_connect_jsonrpc.erl
Outdated
| jsx:encode(preprocess(Term)). | ||
|
|
||
| postprocess(null) -> undefined; | ||
| postprocess(Atom) when is_atom(Atom) -> Atom; |
There was a problem hiding this comment.
This will never be called. jsx does not convert keys to atoms.
There was a problem hiding this comment.
but post process is called recursively with the content of arrays and the values of the map, so there could be atoms.
There was a problem hiding this comment.
It is called on decoded JSON, that does not have atoms as values.
There was a problem hiding this comment.
My bad, I mixed up things, and somehow though the values could be atom too. I got bit before by this but it was a key, you are right on this.
src/grisp_connect_jsonrpc.erl
Outdated
| postprocess(List) when is_list(List) -> | ||
| [postprocess(E) || E <- List]; | ||
| postprocess(Map) when is_map(Map) -> | ||
| maps:from_list([{K, postprocess(V)} || {K, V} <- maps:to_list(Map)]). |
There was a problem hiding this comment.
I would use maps:map/2 instead of doing this back and forth conversion to lists.
src/grisp_connect_jsonrpc.erl
Outdated
| as_id(Integer) when is_integer(Integer) -> Integer; | ||
| as_id(Binary) when is_binary(Binary) -> Binary; | ||
| as_id(List) when is_list(List) -> list_to_binary(List); | ||
| as_id(Atom) when is_atom(Atom) -> atom_to_binary(Atom). |
There was a problem hiding this comment.
Why the atom_to_binary conversion? This is done by the JSON encoder (jsx) and I don't want to mess around with it, if not necessary. Who knows what encoding issues this can produce.
There was a problem hiding this comment.
Only case I see where it can be called is in a response object, but why would we put in another ID there than what we got (we check that we always get binary or integer with the ?is_id macro).
There was a problem hiding this comment.
The atom to binary is to enforce the id to be a binary or an integer without having to know if there is an atom that exists with this name. JSX will convert the id to atom if there is an existing atom and that could lead to strange bugs.
There was a problem hiding this comment.
No it will not. It only converts labels to existing atoms not values.
There was a problem hiding this comment.
If the ID somehow becomes an atom, we should crash.
There was a problem hiding this comment.
You are right, I mixed things up
First bit of mi refactor, the simple one, the rest will get harder....