Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup JsonRPC codec #51

Merged
merged 4 commits into from
Nov 14, 2024
Merged

Cleanup JsonRPC codec #51

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
56de5ee
Cleanup JsonRPC codec
sylane Nov 13, 2024
ac82070
Fix doc generation
sylane Nov 14, 2024
1743f40
Fixes from review
sylane Nov 14, 2024
cac413f
Fix batch handling
sylane Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 19 additions & 11 deletions src/grisp_connect_api.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,19 @@ handle_msg(JSON) ->

%--- Internal Funcitons --------------------------------------------------------

handle_jsonrpc({batch, Batch}) ->
handle_rpc_messages(Batch, []);
handle_jsonrpc({single, Rpc}) ->
handle_rpc_messages([Rpc], []).
format_error({internal_error, parse_error, ID}) ->
{error, -32700, <<"Parse error">>, undefined, ID};
format_error({internal_error, invalid_request, ID}) ->
{error, -32600, <<"Invalid request">>, undefined, ID};
format_error({internal_error, method_not_found, ID}) ->
{error, -32601, <<"Method not found">>, undefined, ID};
format_error({internal_error, invalid_params, ID}) ->
{error, -32602, <<"Invalid params">>, undefined, ID};
format_error({internal_error, Reason, ID}) ->
{error, -32603, <<"Internal error">>, Reason, ID}.

handle_jsonrpc(Messages) ->
handle_rpc_messages(Messages, []).

handle_rpc_messages([], Replies) -> lists:reverse(Replies);
handle_rpc_messages([{request, M, Params, ID} | Batch], Replies)
Expand All @@ -61,8 +70,8 @@ handle_rpc_messages([{result, _, _} = Res| Batch], Replies) ->
handle_rpc_messages([{error, _Code, _Msg, _Data, _ID} = E | Batch], Replies) ->
?LOG_INFO("Received JsonRPC error: ~p",[E]),
handle_rpc_messages(Batch, [handle_response(E)| Replies]);
handle_rpc_messages([{internal_error, _, _} = E | Batch], Replies) ->
?LOG_ERROR("JsonRPC: ~p",[E]),
handle_rpc_messages([{decoding_error, _, _, _, _} = E | Batch], Replies) ->
?LOG_ERROR("JsonRPC decoding error: ~p",[E]),
handle_rpc_messages(Batch, Replies).

handle_request(?method_get, #{type := <<"system_info">>} = _Params, ID) ->
Expand All @@ -80,16 +89,15 @@ handle_request(?method_post, #{type := <<"start_update">>} = Params, ID) ->
{error, -12, boot_system_not_validated, undefined, ID};
{error, Reason} ->
ReasonBinary = iolist_to_binary(io_lib:format("~p", [Reason])),
grisp_connect_jsonrpc:format_error({internal_error, ReasonBinary, ID});
format_error({internal_error, ReasonBinary, ID});
ok ->
{result, ok, ID}
end,
{send_response, grisp_connect_jsonrpc:encode(Reply)}
catch
throw:bad_key ->
{send_response,
grisp_connect_jsonrpc:format_error(
{internal_error, invalid_params, ID})}
format_error({internal_error, invalid_params, ID})}
end;
handle_request(?method_post, #{type := <<"validate">>}, ID) ->
Reply = case grisp_connect_updater:validate() of
Expand All @@ -99,7 +107,7 @@ handle_request(?method_post, #{type := <<"validate">>}, ID) ->
{error, -13, validate_from_unbooted, PartitionIndex, ID};
{error, Reason} ->
ReasonBinary = iolist_to_binary(io_lib:format("~p", [Reason])),
grisp_connect_jsonrpc:format_error({internal_error, ReasonBinary, ID});
format_error({internal_error, ReasonBinary, ID});
ok ->
{result, ok, ID}
end,
Expand All @@ -117,7 +125,7 @@ handle_request(?method_post, #{type := <<"cancel">>}, ID) ->
{send_response, grisp_connect_jsonrpc:encode(Reply)};
handle_request(_T, _P, ID) ->
Error = {internal_error, method_not_found, ID},
FormattedError = grisp_connect_jsonrpc:format_error(Error),
FormattedError = format_error(Error),
{send_response, grisp_connect_jsonrpc:encode(FormattedError)}.

handle_response(Response) ->
Expand Down
205 changes: 136 additions & 69 deletions src/grisp_connect_jsonrpc.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,24 @@
% API
-export([decode/1]).
-export([encode/1]).
-export([format_error/1]).


%--- Types ---------------------------------------------------------------------

-type json_rpc_message() ::
{request, Method :: binary(), Params :: map() | list(),
ReqRef :: binary() | integer()}
| {result, Result :: term(), ReqRef :: binary()}
| {notification, Method :: binary(), Params :: map() | list()}
| {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()}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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




%--- Macros --------------------------------------------------------------------

-define(V, jsonrpc => <<"2.0">>).
-define(is_valid(Message),
(map_get(jsonrpc, Message) == <<"2.0">>)
Expand All @@ -17,103 +31,156 @@
-define(is_params(Params),
(is_map(Params) orelse is_list(Params))
).
-define(is_id(ID),
(is_binary(ID) orelse is_integer(ID))
).


%--- API ----------------------------------------------------------------------
%--- API -----------------------------------------------------------------------

decode(Term) ->
case json_to_term(Term) of
%% @doc Decode a JSONRpc text packet and returns a list of decoded messages.
%% If some decoding errors occure while doing do, a special error message with
%% the tag `decoding_error` that can be encoded and sent back directly to the
%% JSONRpc peer.
%%
%% During JSON decoding, the `null` values are changed to `undefined` and when
%% encoding, `undefined` values are changed back to `null`.
%%
%% The `method` will <b>always</b> be a binary, and `id` will always be either
%% a binary or an integer.
%%
%% <p>The possible decoded messages are:
%% <ul>
%% <li><b><c>{request, Method :: binary(), Params :: map() | list(), ReqRef :: binary() | integer()}</c></b></li>
%% <li><b><c>{result, Result :: term(), ReqRef :: binary()}</c></b></li>
%% <li><b><c>{notification, Method :: binary(), Params :: map() | list()}</c></b></li>
%% <li><b><c>{error, Code :: integer(), Message :: undefined | binary(), Data :: undefined | term(), ReqRef :: undefined | binary() | integer()}</c></b></li>
%% <li><b><c>{decoding_error, Code :: integer(), Message :: undefined | binary(), Data :: undefined | term(), ReqRef :: undefined | binary() | integer()}</c></b></li>
%% </ul>
-spec decode(Data :: iodata()) -> [json_rpc_message()].
decode(Data) ->
case json_to_term(iolist_to_binary(Data)) of
[] ->
{single, {internal_error, invalid_request, null}};
[{decoding_error, -32600, <<"Invalid Request">>, undefined, undefined}];
Messages when is_list(Messages) ->
{batch, [unpack(M) || M <- Messages]};
[unpack(M) || M <- Messages];
Message when is_map(Message) ->
{single, unpack(Message)};
{error, _E} ->
{single, {internal_error, parse_error, null}}
[unpack(Message)];
{error, _Reason} ->
[{decoding_error, -32700, <<"Parse error">>, undefined, undefined}]
end.

encode([Message]) ->
encode(Message);
%% @doc Encode a JSONRpc message or a list of JSONRpc messages to JSON text.
%% For backward compatibility, the `method` can be an atom.
-spec encode(Messages :: json_rpc_message() | [json_rpc_message()]) -> iodata().
encode(Messages) when is_list(Messages) ->
term_to_json([pack(M) || M <- Messages]);
encode(Message) ->
term_to_json(pack(Message)).
encode([Message]).

format_error({internal_error, parse_error, ID}) ->
{error, -32700, <<"Parse error">>, undefined, ID};
format_error({internal_error, invalid_request, ID}) ->
{error, -32600, <<"Invalid request">>, undefined, ID};
format_error({internal_error, method_not_found, ID}) ->
{error, -32601, <<"Method not found">>, undefined, ID};
format_error({internal_error, invalid_params, ID}) ->
{error, -32602, <<"Invalid params">>, undefined, ID};
format_error({internal_error, Reason, ID}) ->
{error, -32603, <<"Internal error">>, Reason, ID}.

%--- Internal -----------------------------------------------------------------
%--- Internal ------------------------------------------------------------------

as_bin(undefined) -> undefined;
as_bin(Binary) when is_binary(Binary) -> Binary;
as_bin(List) when is_list(List) -> list_to_binary(List);
as_bin(Atom) when is_atom(Atom) -> atom_to_binary(Atom).

as_id(undefined) -> undefined;
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).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@maehjam maehjam Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it will not. It only converts labels to existing atoms not values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the ID somehow becomes an atom, we should crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I mixed things up


unpack(#{method := Method, params := Params, id := ID} = M)
when ?is_valid(M), ?is_method(Method), ?is_params(Params) ->
{request, Method, Params, ID};
when ?is_valid(M), ?is_method(Method), ?is_params(Params), ID =/= undefined ->
{request, as_bin(Method), Params, as_id(ID)};
unpack(#{method := Method, id := ID} = M)
when ?is_valid(M), ?is_method(Method) ->
{request, Method, undefined, ID};
when ?is_valid(M), ?is_method(Method), ID =/= undefined ->
{request, as_bin(Method), undefined, as_id(ID)};
unpack(#{method := Method, params := Params} = M)
when ?is_valid(M), ?is_method(Method), ?is_params(Params) ->
{notification, Method, Params};
when ?is_valid(M), ?is_method(Method), ?is_params(Params) ->
{notification, as_bin(Method), Params};
unpack(#{method := Method} = M)
when ?is_valid(M), ?is_method(Method) ->
{notification, Method, undefined};
unpack(#{method := Method, params := _Params, id := ID} = M)
when ?is_valid(M), ?is_method(Method) ->
{internal_error, invalid_params, ID};
when ?is_valid(M), ?is_method(Method) ->
{notification, as_bin(Method), undefined};
unpack(#{result := Result, id := ID} = M)
when ?is_valid(M) ->
{result, Result, ID};
unpack(#{error := #{code := Code,
message := Message,
data := Data},
id := ID} = M)
when ?is_valid(M) ->
{error, Code, Message, Data, ID};
unpack(#{error := #{code := Code,
message := Message},
id := ID} = M)
when ?is_valid(M) ->
{error, Code, Message, undefined, ID};
unpack(M) ->
{internal_error, invalid_request, id(M)}.

pack({request, Method, undefined, ID}) ->
when ?is_valid(M) ->
{result, Result, as_id(ID)};
unpack(#{error := #{code := Code, message := Message, data := Data},
id := ID} = M)
when ?is_valid(M), is_integer(Code) ->
{error, Code, as_bin(Message), Data, as_id(ID)};
unpack(#{error := #{code := Code, message := Message}, id := ID} = M)
when ?is_valid(M), is_integer(Code) ->
{error, Code, as_bin(Message), undefined, as_id(ID)};
unpack(#{id := ID}) ->
{decoding_error, -32600, <<"Invalid request">>, undefined, as_id(ID)};
unpack(_M) ->
{decoding_error, -32600, <<"Invalid request">>, undefined, undefined}.

pack({request, Method, undefined, ID})
when is_binary(Method) orelse is_atom(Method), ?is_id(ID) ->
#{?V, method => Method, id => ID};
pack({request, Method, Params, ID}) ->
pack({request, Method, Params, ID})
when is_binary(Method) orelse is_atom(Method),
Params =:= undefined orelse ?is_params(Params),
?is_id(ID) ->
#{?V, method => Method, params => Params, id => ID};
pack({notification, Method, undefined}) ->
pack({notification, Method, undefined})
when is_binary(Method) orelse is_atom(Method) ->
#{?V, method => Method};
pack({notification, Method, Params}) ->
pack({notification, Method, Params})
when is_binary(Method), Params =:= undefined orelse ?is_params(Params) ->
#{?V, method => Method, params => Params};
pack({result, Result, ID}) ->
pack({result, Result, ID})
when ?is_id(ID) ->
#{?V, result => Result, id => ID};
pack({error, Type, ID}) ->
pack(format_error({internal_error, Type, ID}));
pack({error, Code, Message, undefined, undefined}) ->
pack({ErrorTag, Code, Message, undefined, undefined})
when ErrorTag =:= error orelse ErrorTag =:= decoding_error, is_integer(Code),
Message =:= undefined orelse is_binary(Message) ->
#{?V, error => #{code => Code, message => Message}, id => null};
pack({error, Code, Message, undefined, ID}) ->
pack({ErrorTag, Code, Message, undefined, ID})
when ErrorTag =:= error orelse ErrorTag =:= decoding_error, is_integer(Code),
Message =:= undefined orelse is_binary(Message), ?is_id(ID) ->
#{?V, error => #{code => Code, message => Message}, id => ID};
pack({error, Code, Message, Data, undefined}) ->
pack({ErrorTag, Code, Message, Data, undefined})
when ErrorTag =:= error orelse ErrorTag =:= decoding_error, is_integer(Code),
Message =:= undefined orelse is_binary(Message) ->
#{?V, error => #{code => Code, message => Message, data => Data, id => null}};
pack({error, Code, Message, Data, ID}) ->
#{?V, error => #{code => Code, message => Message, data => Data}, id => ID}.

id(Object) when is_map(Object) -> maps:get(id, Object, null);
id(_Object) -> null.

pack({ErrorTag, Code, Message, Data, ID})
when ErrorTag =:= error orelse ErrorTag =:= decoding_error, is_integer(Code),
Message =:= undefined orelse is_binary(Message), ?is_id(ID) ->
#{?V, error => #{code => Code, message => Message, data => Data}, id => ID};
pack(_Message) ->
erlang:error({badarg, _Message}).

json_to_term(Bin) ->
try jsx:decode(Bin, [{labels, attempt_atom}, return_maps])
try jsx:decode(Bin, [{labels, attempt_atom}, return_maps]) of
Json -> postprocess(Json)
catch
error:E -> {error, E}
end.

term_to_json(Map) ->
jsx:encode(Map).
term_to_json(Term) ->
jsx:encode(preprocess(Term)).

postprocess(null) -> undefined;
postprocess(Atom) when is_atom(Atom) -> Atom;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will never be called. jsx does not convert keys to atoms.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but post process is called recursively with the content of arrays and the values of the map, so there could be atoms.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is called on decoded JSON, that does not have atoms as values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

postprocess(Integer) when is_integer(Integer) -> Integer;
postprocess(Float) when is_float(Float) -> Float;
postprocess(Binary) when is_binary(Binary) -> Binary;
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)]).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use maps:map/2 instead of doing this back and forth conversion to lists.


preprocess(undefined) -> null;
preprocess(Atom) when is_atom(Atom) -> Atom;
preprocess(Integer) when is_integer(Integer) -> Integer;
preprocess(Float) when is_float(Float) -> Float;
preprocess(Binary) when is_binary(Binary) -> Binary;
preprocess(List) when is_list(List) ->
[preprocess(E) || E <- List];
preprocess(Map) when is_map(Map) ->
maps:from_list([{K, preprocess(V)} || {K, V} <- maps:to_list(Map)]).
Loading
Loading