From 56de5ee634a2414a6e3151e6fcc52e6bd4bde7a3 Mon Sep 17 00:00:00 2001 From: Sebastien Merle Date: Wed, 13 Nov 2024 20:12:35 +0100 Subject: [PATCH 1/4] Cleanup JsonRPC codec * 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. --- src/grisp_connect_api.erl | 30 ++-- src/grisp_connect_jsonrpc.erl | 205 ++++++++++++++++++--------- test/grisp_connect_jsonrpc_SUITE.erl | 37 +++-- 3 files changed, 178 insertions(+), 94 deletions(-) diff --git a/src/grisp_connect_api.erl b/src/grisp_connect_api.erl index f67d0c9..41c6862 100644 --- a/src/grisp_connect_api.erl +++ b/src/grisp_connect_api.erl @@ -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) @@ -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) -> @@ -80,7 +89,7 @@ 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, @@ -88,8 +97,7 @@ handle_request(?method_post, #{type := <<"start_update">>} = Params, ID) -> 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 @@ -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, @@ -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) -> diff --git a/src/grisp_connect_jsonrpc.erl b/src/grisp_connect_jsonrpc.erl index 3e83b09..3ac1c53 100644 --- a/src/grisp_connect_jsonrpc.erl +++ b/src/grisp_connect_jsonrpc.erl @@ -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()}. + + + +%--- Macros -------------------------------------------------------------------- + -define(V, jsonrpc => <<"2.0">>). -define(is_valid(Message), (map_get(jsonrpc, Message) == <<"2.0">>) @@ -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 always be a binary, and `id` will always be either +%% a binary or an integer. +%% +%%

The possible decoded messages are: +%%

+-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). 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; +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)]). + +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)]). diff --git a/test/grisp_connect_jsonrpc_SUITE.erl b/test/grisp_connect_jsonrpc_SUITE.erl index 1f7efab..2e43fc9 100644 --- a/test/grisp_connect_jsonrpc_SUITE.erl +++ b/test/grisp_connect_jsonrpc_SUITE.erl @@ -6,6 +6,7 @@ -export([positional_parameters/1, named_parameters/1, + using_existing_atoms/1, notification/1, invalid_json/1, invalid_request/1, @@ -15,6 +16,7 @@ all() -> [ positional_parameters, named_parameters, + using_existing_atoms, notification, invalid_json, invalid_request, @@ -25,7 +27,7 @@ all() -> [ positional_parameters(_) -> Term = {request, <<"subtract">>, [42,23], 1}, Json = <<"{\"id\":1,\"jsonrpc\":\"2.0\",\"method\":\"subtract\",\"params\":[42,23]}">>, - ?assertMatch({single, Term}, grisp_connect_jsonrpc:decode(Json)), + ?assertMatch([Term], grisp_connect_jsonrpc:decode(Json)), Json2 = grisp_connect_jsonrpc:encode(Term), ?assert(jsonrpc_check([<<"\"id\":1">>, <<"\"method\":\"subtract\"">>, @@ -34,36 +36,44 @@ positional_parameters(_) -> named_parameters(_) -> Term = {request, <<"divide">>, #{<<"dividend">> => 42, <<"divisor">> => 2}, 2}, Json = <<"{\"id\":2,\"jsonrpc\":\"2.0\",\"method\":\"divide\",\"params\":{\"dividend\":42,\"divisor\":2}}">>, - ?assertMatch({single, Term}, grisp_connect_jsonrpc:decode(Json)), + ?assertMatch([Term], grisp_connect_jsonrpc:decode(Json)), Json2 = grisp_connect_jsonrpc:encode(Term), ?assert(jsonrpc_check([<<"\"id\":2">>, <<"\"method\":\"divide\"">>, <<"\"dividend\":42">>, <<"\"divisor\":2">>], Json2)). +using_existing_atoms(_) -> + % The ID and method are matching existing atoms, checks they are not atoms + Term = {request, <<"notification">>, #{}, <<"request">>}, + Json = <<"{\"id\":\"request\",\"jsonrpc\":\"2.0\",\"method\":\"notification\",\"params\":{}}">>, + ?assertMatch([Term], grisp_connect_jsonrpc:decode(Json)), + Json2 = grisp_connect_jsonrpc:encode(Term), + ?assert(jsonrpc_check([<<"\"id\":\"request\"">>, <<"\"method\":\"notification\"">>], Json2)). + notification(_) -> Term = {notification, <<"update">>, [1,2,3,4,5]}, Json = <<"{\"jsonrpc\":\"2.0\",\"method\":\"update\",\"params\":[1,2,3,4,5]}">>, - ?assertMatch({single, Term}, grisp_connect_jsonrpc:decode(Json)), + ?assertMatch([Term], grisp_connect_jsonrpc:decode(Json)), Json2 = grisp_connect_jsonrpc:encode(Term), ?assert(jsonrpc_check([<<"\"method\":\"update\"">>, <<"\"params\":[1,2,3,4,5]">>], Json2)). invalid_json(_) -> - Term = {internal_error, parse_error, null}, + Term = {decoding_error, -32700, <<"Parse error">>, undefined, undefined}, Json = <<"{\"jsonrpc\":\"2.0\",\"method\":\"foobar,\"params\":\"bar\",\"baz]">>, - ?assertMatch({single, Term}, grisp_connect_jsonrpc:decode(Json)), - JsonError = grisp_connect_jsonrpc:encode(grisp_connect_jsonrpc:format_error(Term)), + ?assertMatch([Term], grisp_connect_jsonrpc:decode(Json)), + JsonError = grisp_connect_jsonrpc:encode(Term), ?assert(jsonrpc_check([<<"\"error\":{">>, <<"\"code\":-32700">>, <<"\"message\":\"Parse error\"">>, <<"\"id\":null">>], JsonError)). invalid_request(_) -> - Term = {internal_error, invalid_request, null}, + Term = {decoding_error, -32600, <<"Invalid request">>, undefined, undefined}, Json = <<"{\"jsonrpc\":\"2.0\",\"method\":1,\"params\":\"bar\"}">>, - ?assertMatch({single, Term}, grisp_connect_jsonrpc:decode(Json)), - JsonError = grisp_connect_jsonrpc:encode(grisp_connect_jsonrpc:format_error(Term)), + ?assertMatch([Term], grisp_connect_jsonrpc:decode(Json)), + JsonError = grisp_connect_jsonrpc:encode(Term), ?assert(jsonrpc_check([<<"\"error\":{">>, <<"\"code\":-32600">>, <<"\"message\":\"Invalid request\"">>, @@ -71,10 +81,10 @@ invalid_request(_) -> batch(_) -> Term1 = {request, <<"sum">>, [1,2,4], <<"1">>}, - Term2 = {internal_error, invalid_request, null}, + Term2 = {decoding_error, -32600, <<"Invalid request">>, undefined, undefined}, Json = <<"[{\"jsonrpc\":\"2.0\",\"method\":\"sum\",\"params\":[1,2,4],\"id\":\"1\"},{\"foo\":\"boo\"}]">>, - ?assertMatch({batch, [Term1,Term2]}, grisp_connect_jsonrpc:decode(Json)), - JsonError = grisp_connect_jsonrpc:encode([Term1, grisp_connect_jsonrpc:format_error(Term2)]), + ?assertMatch([Term1, Term2], grisp_connect_jsonrpc:decode(Json)), + JsonError = grisp_connect_jsonrpc:encode([Term1, Term2]), ?assert(jsonrpc_check([<<"\"id\":\"1\"">>, <<"\"method\":\"sum\"">>, <<"params\":[1,2,4]">>, @@ -86,12 +96,11 @@ batch(_) -> result(_) -> Term = {result, 7, 45}, Json = <<"{\"id\":45,\"jsonrpc\":\"2.0\",\"result\":7}">>, - ?assertMatch({single, Term}, grisp_connect_jsonrpc:decode(Json)), + ?assertMatch([Term], grisp_connect_jsonrpc:decode(Json)), Json2 = grisp_connect_jsonrpc:encode(Term), ?assert(jsonrpc_check([<<"\"id\":45">>, <<"\"result\":7">>], Json2)). - jsonrpc_check(Elements, JsonString) -> Elements2 = [<<"\"jsonrpc\":\"2.0\"">>| Elements], lists:all(fun(E) -> binary:match(JsonString, E) =/= nomatch end, Elements2). From ac8207004d3d522e0f3653fafaf2d6f92cd8ed34 Mon Sep 17 00:00:00 2001 From: Sebastien Merle Date: Thu, 14 Nov 2024 09:33:29 +0100 Subject: [PATCH 2/4] Fix doc generation --- rebar.config | 2 ++ src/grisp_connect_jsonrpc.erl | 22 +++++++++++----------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/rebar.config b/rebar.config index ecee336..03c5966 100644 --- a/rebar.config +++ b/rebar.config @@ -22,6 +22,8 @@ {hex, [{doc, #{provider => ex_doc}}]}. +{edoc_opts, [{preprocess, true}]}. + {ex_doc, [ {extras, [ {"CHANGELOG.md", #{title => "Changelog"}}, diff --git a/src/grisp_connect_jsonrpc.erl b/src/grisp_connect_jsonrpc.erl index 3ac1c53..ae6d899 100644 --- a/src/grisp_connect_jsonrpc.erl +++ b/src/grisp_connect_jsonrpc.erl @@ -40,23 +40,23 @@ %% @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 +%% 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`. +%% During JSON decoding, the `null' values are changed to `undefined' and when +%% encoding, `undefined' values are changed back to `null'. %% -%% The `method` will always be a binary, and `id` will always be either +%% The `method' will always be a binary, and `id' will always be either %% a binary or an integer. %% %%

The possible decoded messages are: %%

+%%
  • `{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() | integer()}'
    • +%%

    -spec decode(Data :: iodata()) -> [json_rpc_message()]. decode(Data) -> case json_to_term(iolist_to_binary(Data)) of @@ -71,7 +71,7 @@ decode(Data) -> end. %% @doc Encode a JSONRpc message or a list of JSONRpc messages to JSON text. -%% For backward compatibility, the `method` can be an atom. +%% 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]); From 1743f40b09ae44f7ce523e3b51d59d184fda01d7 Mon Sep 17 00:00:00 2001 From: Sebastien Merle Date: Thu, 14 Nov 2024 13:20:44 +0100 Subject: [PATCH 3/4] Fixes from review --- src/grisp_connect_jsonrpc.erl | 17 +++++++---------- test/grisp_connect_jsonrpc_SUITE.erl | 18 +++++++++++++++--- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/src/grisp_connect_jsonrpc.erl b/src/grisp_connect_jsonrpc.erl index ae6d899..be0f5ce 100644 --- a/src/grisp_connect_jsonrpc.erl +++ b/src/grisp_connect_jsonrpc.erl @@ -15,7 +15,7 @@ | {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()}. + Data :: undefined | term(), ReqRef :: undefined | binary() | integer()}. @@ -83,14 +83,12 @@ encode(Message) -> 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_bin(List) when is_list(List) -> list_to_binary(List). 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). +as_id(List) when is_list(List) -> list_to_binary(List). unpack(#{method := Method, params := Params, id := ID} = M) when ?is_valid(M), ?is_method(Method), ?is_params(Params), ID =/= undefined -> @@ -152,8 +150,8 @@ 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}). +pack(Message) -> + erlang:error({badarg, Message}). json_to_term(Bin) -> try jsx:decode(Bin, [{labels, attempt_atom}, return_maps]) of @@ -166,14 +164,13 @@ term_to_json(Term) -> jsx:encode(preprocess(Term)). postprocess(null) -> undefined; -postprocess(Atom) when is_atom(Atom) -> Atom; 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)]). + maps:map(fun(_K, V) -> postprocess(V) end, Map). preprocess(undefined) -> null; preprocess(Atom) when is_atom(Atom) -> Atom; @@ -183,4 +180,4 @@ 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)]). + maps:map(fun(_K, V) -> preprocess(V) end, Map). diff --git a/test/grisp_connect_jsonrpc_SUITE.erl b/test/grisp_connect_jsonrpc_SUITE.erl index 2e43fc9..9273333 100644 --- a/test/grisp_connect_jsonrpc_SUITE.erl +++ b/test/grisp_connect_jsonrpc_SUITE.erl @@ -11,7 +11,8 @@ invalid_json/1, invalid_request/1, batch/1, - result/1]). + result/1, + null_values/1]). all() -> [ positional_parameters, @@ -21,7 +22,8 @@ all() -> [ invalid_json, invalid_request, batch, - result + result, + null_values ]. positional_parameters(_) -> @@ -87,7 +89,7 @@ batch(_) -> JsonError = grisp_connect_jsonrpc:encode([Term1, Term2]), ?assert(jsonrpc_check([<<"\"id\":\"1\"">>, <<"\"method\":\"sum\"">>, - <<"params\":[1,2,4]">>, + <<"\"params\":[1,2,4]">>, <<"\"error\":{">>, <<"\"code\":-32600">>, <<"\"message\":\"Invalid request\"">>, @@ -101,6 +103,16 @@ result(_) -> ?assert(jsonrpc_check([<<"\"id\":45">>, <<"\"result\":7">>], Json2)). +null_values(_) -> + Term = {notification, <<"test_null">>, #{array => [undefined], object => #{foo => undefined}, value => undefined}}, + Json = <<"{\"jsonrpc\":\"2.0\",\"method\":\"test_null\",\"params\":{\"array\":[null],\"object\":{\"foo\":null},\"value\":null}}">>, + ?assertMatch([Term], grisp_connect_jsonrpc:decode(Json)), + Json2 = grisp_connect_jsonrpc:encode(Term), + ?assert(jsonrpc_check([<<"\"array\":[null]">>, + <<"\"foo\":null">>, + <<"\"value\":null">>], + Json2)). + jsonrpc_check(Elements, JsonString) -> Elements2 = [<<"\"jsonrpc\":\"2.0\"">>| Elements], lists:all(fun(E) -> binary:match(JsonString, E) =/= nomatch end, Elements2). From cac413f5a60fbed5119b9d1e12c1015d24ce53e7 Mon Sep 17 00:00:00 2001 From: Sebastien Merle Date: Thu, 14 Nov 2024 14:34:35 +0100 Subject: [PATCH 4/4] Fix batch handling --- src/grisp_connect_api.erl | 9 +++++++-- src/grisp_connect_jsonrpc.erl | 23 +++++++++++++++-------- test/grisp_connect_jsonrpc_SUITE.erl | 16 ++++++++-------- 3 files changed, 30 insertions(+), 18 deletions(-) diff --git a/src/grisp_connect_api.erl b/src/grisp_connect_api.erl index 41c6862..f9bb96c 100644 --- a/src/grisp_connect_api.erl +++ b/src/grisp_connect_api.erl @@ -57,8 +57,13 @@ format_error({internal_error, invalid_params, ID}) -> format_error({internal_error, Reason, ID}) -> {error, -32603, <<"Internal error">>, Reason, ID}. -handle_jsonrpc(Messages) -> - handle_rpc_messages(Messages, []). +%FIXME: Batch are not supported yet. When receiving a batch of messages, as per +% the JSON-RPC standard, all the responses should goes in a single batch +% of responses. +handle_jsonrpc(Messages) when is_list(Messages) -> + handle_rpc_messages(Messages, []); +handle_jsonrpc(Message) -> + handle_rpc_messages([Message], []). handle_rpc_messages([], Replies) -> lists:reverse(Replies); handle_rpc_messages([{request, M, Params, ID} | Batch], Replies) diff --git a/src/grisp_connect_jsonrpc.erl b/src/grisp_connect_jsonrpc.erl index be0f5ce..f37d1f8 100644 --- a/src/grisp_connect_jsonrpc.erl +++ b/src/grisp_connect_jsonrpc.erl @@ -38,10 +38,15 @@ %--- API ----------------------------------------------------------------------- -%% @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. +%% @doc Decode a JSONRpc text packet and decoded message or a list of decoded +%% messages in the case of a batch. +%% +%% If it returns a list, the all the responses are supposed to be sent back in +%% a batch too, as per the JSONRpc 2.0 specifications. +%% +%% If some decoding errors occure, a special error message with the tag +%% `decoding_error' will be returned, this message can be encoded and sent back +%% directly to the JSON-RPC peer. %% %% During JSON decoding, the `null' values are changed to `undefined' and when %% encoding, `undefined' values are changed back to `null'. @@ -57,7 +62,7 @@ %%
  • `{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() | integer()}'
    • %%

    --spec decode(Data :: iodata()) -> [json_rpc_message()]. +-spec decode(Data :: iodata()) -> json_rpc_message() | [json_rpc_message()]. decode(Data) -> case json_to_term(iolist_to_binary(Data)) of [] -> @@ -65,9 +70,11 @@ decode(Data) -> Messages when is_list(Messages) -> [unpack(M) || M <- Messages]; Message when is_map(Message) -> - [unpack(Message)]; + unpack(Message); {error, _Reason} -> - [{decoding_error, -32700, <<"Parse error">>, undefined, undefined}] + % Even though the data could have been a batch, we return a single + % error, as per JSON-RPC specifications + {decoding_error, -32700, <<"Parse error">>, undefined, undefined} end. %% @doc Encode a JSONRpc message or a list of JSONRpc messages to JSON text. @@ -76,7 +83,7 @@ decode(Data) -> encode(Messages) when is_list(Messages) -> term_to_json([pack(M) || M <- Messages]); encode(Message) -> - encode([Message]). + term_to_json(pack(Message)). %--- Internal ------------------------------------------------------------------ diff --git a/test/grisp_connect_jsonrpc_SUITE.erl b/test/grisp_connect_jsonrpc_SUITE.erl index 9273333..16853a1 100644 --- a/test/grisp_connect_jsonrpc_SUITE.erl +++ b/test/grisp_connect_jsonrpc_SUITE.erl @@ -29,7 +29,7 @@ all() -> [ positional_parameters(_) -> Term = {request, <<"subtract">>, [42,23], 1}, Json = <<"{\"id\":1,\"jsonrpc\":\"2.0\",\"method\":\"subtract\",\"params\":[42,23]}">>, - ?assertMatch([Term], grisp_connect_jsonrpc:decode(Json)), + ?assertMatch(Term, grisp_connect_jsonrpc:decode(Json)), Json2 = grisp_connect_jsonrpc:encode(Term), ?assert(jsonrpc_check([<<"\"id\":1">>, <<"\"method\":\"subtract\"">>, @@ -38,7 +38,7 @@ positional_parameters(_) -> named_parameters(_) -> Term = {request, <<"divide">>, #{<<"dividend">> => 42, <<"divisor">> => 2}, 2}, Json = <<"{\"id\":2,\"jsonrpc\":\"2.0\",\"method\":\"divide\",\"params\":{\"dividend\":42,\"divisor\":2}}">>, - ?assertMatch([Term], grisp_connect_jsonrpc:decode(Json)), + ?assertMatch(Term, grisp_connect_jsonrpc:decode(Json)), Json2 = grisp_connect_jsonrpc:encode(Term), ?assert(jsonrpc_check([<<"\"id\":2">>, <<"\"method\":\"divide\"">>, @@ -49,14 +49,14 @@ using_existing_atoms(_) -> % The ID and method are matching existing atoms, checks they are not atoms Term = {request, <<"notification">>, #{}, <<"request">>}, Json = <<"{\"id\":\"request\",\"jsonrpc\":\"2.0\",\"method\":\"notification\",\"params\":{}}">>, - ?assertMatch([Term], grisp_connect_jsonrpc:decode(Json)), + ?assertMatch(Term, grisp_connect_jsonrpc:decode(Json)), Json2 = grisp_connect_jsonrpc:encode(Term), ?assert(jsonrpc_check([<<"\"id\":\"request\"">>, <<"\"method\":\"notification\"">>], Json2)). notification(_) -> Term = {notification, <<"update">>, [1,2,3,4,5]}, Json = <<"{\"jsonrpc\":\"2.0\",\"method\":\"update\",\"params\":[1,2,3,4,5]}">>, - ?assertMatch([Term], grisp_connect_jsonrpc:decode(Json)), + ?assertMatch(Term, grisp_connect_jsonrpc:decode(Json)), Json2 = grisp_connect_jsonrpc:encode(Term), ?assert(jsonrpc_check([<<"\"method\":\"update\"">>, <<"\"params\":[1,2,3,4,5]">>], Json2)). @@ -64,7 +64,7 @@ notification(_) -> invalid_json(_) -> Term = {decoding_error, -32700, <<"Parse error">>, undefined, undefined}, Json = <<"{\"jsonrpc\":\"2.0\",\"method\":\"foobar,\"params\":\"bar\",\"baz]">>, - ?assertMatch([Term], grisp_connect_jsonrpc:decode(Json)), + ?assertMatch(Term, grisp_connect_jsonrpc:decode(Json)), JsonError = grisp_connect_jsonrpc:encode(Term), ?assert(jsonrpc_check([<<"\"error\":{">>, <<"\"code\":-32700">>, @@ -74,7 +74,7 @@ invalid_json(_) -> invalid_request(_) -> Term = {decoding_error, -32600, <<"Invalid request">>, undefined, undefined}, Json = <<"{\"jsonrpc\":\"2.0\",\"method\":1,\"params\":\"bar\"}">>, - ?assertMatch([Term], grisp_connect_jsonrpc:decode(Json)), + ?assertMatch(Term, grisp_connect_jsonrpc:decode(Json)), JsonError = grisp_connect_jsonrpc:encode(Term), ?assert(jsonrpc_check([<<"\"error\":{">>, <<"\"code\":-32600">>, @@ -98,7 +98,7 @@ batch(_) -> result(_) -> Term = {result, 7, 45}, Json = <<"{\"id\":45,\"jsonrpc\":\"2.0\",\"result\":7}">>, - ?assertMatch([Term], grisp_connect_jsonrpc:decode(Json)), + ?assertMatch(Term, grisp_connect_jsonrpc:decode(Json)), Json2 = grisp_connect_jsonrpc:encode(Term), ?assert(jsonrpc_check([<<"\"id\":45">>, <<"\"result\":7">>], Json2)). @@ -106,7 +106,7 @@ result(_) -> null_values(_) -> Term = {notification, <<"test_null">>, #{array => [undefined], object => #{foo => undefined}, value => undefined}}, Json = <<"{\"jsonrpc\":\"2.0\",\"method\":\"test_null\",\"params\":{\"array\":[null],\"object\":{\"foo\":null},\"value\":null}}">>, - ?assertMatch([Term], grisp_connect_jsonrpc:decode(Json)), + ?assertMatch(Term, grisp_connect_jsonrpc:decode(Json)), Json2 = grisp_connect_jsonrpc:encode(Term), ?assert(jsonrpc_check([<<"\"array\":[null]">>, <<"\"foo\":null">>,