From 4b18e3f81c4eeebaea47bb2397e64124e2a95450 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Chrz=C4=85szcz?= Date: Fri, 8 Jul 2022 16:15:51 +0200 Subject: [PATCH] Divide command processing into steps This way it is easier to control the error messages, and to print out additional hints. There is a plan to add more usage information in a new PR. --- big_tests/tests/graphql_helper.erl | 3 +- big_tests/tests/mongooseimctl_SUITE.erl | 48 ++++++++++++- src/ejabberd_ctl.erl | 41 +++++++---- src/graphql/mongoose_graphql_commands.erl | 84 +++++++++++++++++++---- 4 files changed, 148 insertions(+), 28 deletions(-) diff --git a/big_tests/tests/graphql_helper.erl b/big_tests/tests/graphql_helper.erl index 413f5d6acc7..547b870de48 100644 --- a/big_tests/tests/graphql_helper.erl +++ b/big_tests/tests/graphql_helper.erl @@ -25,7 +25,8 @@ execute_command(Category, Command, Args, Config) -> execute_command(Category, Command, Args, Config, Protocol). execute_command(Category, Command, Args, Config, http) -> - {ok, Doc} = rpc(mim(), mongoose_graphql_commands, find_document, [Category, Command]), + #{Category := #{Command := #{doc := Doc}}} = + rpc(mim(), mongoose_graphql_commands, get_specs, []), execute_auth(#{query => Doc, variables => Args}, Config); execute_command(Category, Command, Args, Config, cli) -> VarsJSON = jiffy:encode(Args), diff --git a/big_tests/tests/mongooseimctl_SUITE.erl b/big_tests/tests/mongooseimctl_SUITE.erl index 1532a2d6c79..4816e4ac4ac 100644 --- a/big_tests/tests/mongooseimctl_SUITE.erl +++ b/big_tests/tests/mongooseimctl_SUITE.erl @@ -177,7 +177,15 @@ upload_enabled() -> graphql() -> [graphql_wrong_arguments_number, can_execute_admin_queries_with_permissions, - can_handle_execution_error]. + can_handle_execution_error, + graphql_error_unknown_command_with_args, + graphql_error_unknown_command_without_args, + graphql_error_no_command, + graphql_error_invalid_vars, + graphql_error_no_vars, + graphql_error_too_many_args, + graphql_error_missing_variable, + graphql_command]. suite() -> require_rpc_nodes([mim]) ++ escalus:suite(). @@ -1147,6 +1155,44 @@ graphql_wrong_arguments_number(Config) -> Data2 = element(1, ResTooManyArgs), ?assertNotEqual(nomatch, string:find(Data2, ExpectedFragment)). +%% Generic GraphQL command tests +%% Specific commands are tested in graphql_*_SUITE + +graphql_error_unknown_command_with_args(Config) -> + {Res, 1} = mongooseimctl("account", ["makeCoffee", "{}"], Config), + ?assertMatch({match, _}, re:run(Res, "Unknown command")). + +graphql_error_unknown_command_without_args(Config) -> + {Res, 1} = mongooseimctl("account", ["makeCoffee"], Config), + ?assertMatch({match, _}, re:run(Res, "Unknown command")). + +graphql_error_no_command(Config) -> + {Res, 1} = mongooseimctl("account", [], Config), + ?assertMatch({match, _}, re:run(Res, "No command")). + +graphql_error_invalid_vars(Config) -> + {Res, 1} = mongooseimctl("account", ["countUsers", "now"], Config), + ?assertMatch({match, _}, re:run(Res, "Could not parse")). + +graphql_error_no_vars(Config) -> + {Res, 1} = mongooseimctl("account", ["countUsers"], Config), + ?assertMatch({match, _}, re:run(Res, "This command requires variables")). + +graphql_error_too_many_args(Config) -> + {Res, 1} = mongooseimctl("account", ["countUsers", "{}", "{}"], Config), + ?assertMatch({match, _}, re:run(Res, "Too many arguments")). + +graphql_error_missing_variable(Config) -> + {ResJSON, 1} = mongooseimctl("account", ["countUsers", "{}"], Config), + #{<<"errors">> := Errors} = rest_helper:decode(ResJSON, #{return_maps => true}), + ?assertMatch([#{<<"extensions">> := #{<<"code">> := <<"missing_non_null_param">>}}], Errors). + +graphql_command(Config) -> + VarsJSON = jiffy:encode(#{domain => domain()}), + {ResJSON, 0} = mongooseimctl("account", ["countUsers", VarsJSON], Config), + #{<<"data">> := Data} = rest_helper:decode(ResJSON, #{return_maps => true}), + ?assertMatch(#{<<"account">> := #{<<"countUsers">> := _}}, Data). + %%----------------------------------------------------------------- %% Improve coverage %%----------------------------------------------------------------- diff --git a/src/ejabberd_ctl.erl b/src/ejabberd_ctl.erl index 617e1cbeb5b..70b0dd77615 100644 --- a/src/ejabberd_ctl.erl +++ b/src/ejabberd_ctl.erl @@ -179,7 +179,8 @@ process(["mnesia", Arg]) when is_list(Arg) -> ?STATUS_SUCCESS; process(["graphql", Arg]) when is_list(Arg) -> Doc = list_to_binary(Arg), - Result = mongoose_graphql_commands:execute(Doc, #{}), % TODO Support vars? + Ep = mongoose_graphql:get_endpoint(admin), + Result = mongoose_graphql:execute(Ep, undefined, Doc), handle_graphql_result(Result); process(["graphql" | _]) -> ?PRINT("This command requires one string type argument!\n", []), @@ -213,19 +214,33 @@ process(["help" | Mode]) -> print_usage_commands(CmdStringU, MaxC, ShCode), ?STATUS_SUCCESS end; -process([CategoryStr, CommandStr, VarsStr] = Args) -> - Category = list_to_binary(CategoryStr), - Command = list_to_binary(CommandStr), - case mongoose_graphql_commands:find_document(Category, Command) of - {ok, Doc} -> - Vars = jiffy:decode(VarsStr, [return_maps]), - Result = mongoose_graphql_commands:execute(Doc, Vars), - handle_graphql_result(Result); - {error, not_found} -> - run_command(Args) - end; process(Args) -> - run_command(Args). + case mongoose_graphql_commands:process(Args) of + {error, #{reason := Reason}} when Reason =:= no_args; + Reason =:= unknown_category -> + run_command(Args); % Fallback to the old commands + {error, #{reason := unknown_command, category := Category, cat_spec := CatSpec, + command := Command}} -> + ?PRINT("Unknown command '~s'. Available commands in category '~s':~n", + [Command, Category]), + [?PRINT(" ~s~n", [Cmd]) || Cmd <- lists:sort(maps:keys(CatSpec))], + ?STATUS_ERROR; + {error, #{reason := no_command, category := Category, cat_spec := CatSpec}} -> + ?PRINT("No command was specified. Available commands in category '~s':~n", [Category]), + [?PRINT(" ~s~n", [Cmd]) || Cmd <- lists:sort(maps:keys(CatSpec))], + ?STATUS_ERROR; + {error, #{reason := invalid_vars}} -> + ?PRINT("Could not parse command variables from JSON~n", []), + ?STATUS_ERROR; + {error, #{reason := no_vars}} -> + ?PRINT("This command requires variables in JSON~n", []), + ?STATUS_ERROR; + {error, #{reason := too_many_args}} -> + ?PRINT("Too many arguments~n", []), + ?STATUS_ERROR; + Result -> + handle_graphql_result(Result) + end. handle_graphql_result({ok, Result}) -> JSONResult = mongoose_graphql_response:term_to_pretty_json(Result), diff --git a/src/graphql/mongoose_graphql_commands.erl b/src/graphql/mongoose_graphql_commands.erl index a9640b0a02b..8d2453e3198 100644 --- a/src/graphql/mongoose_graphql_commands.erl +++ b/src/graphql/mongoose_graphql_commands.erl @@ -2,7 +2,13 @@ -module(mongoose_graphql_commands). --export([start/0, stop/0, find_document/2, execute/2]). +%% API +-export([start/0, stop/0, process/1]). + +%% Only for tests +-export([get_specs/0]). + +-ignore_xref([get_specs/0]). %% This level of nesting is needed for basic type introspection, e.g. see below for [String!]! %% NON_NULL LIST NON_NULL SCALAR @@ -22,6 +28,14 @@ -type category_spec() :: #{command() => command_spec()}. -type specs() :: #{category() => category_spec()}. -type json_map() :: #{binary() => graphql:json()}. +-type ctx() :: #{args := [string()], + category => category(), + cat_spec => category_spec(), + command => command(), + doc => doc(), + vars => json_map(), + reason => atom()}. +-type result() :: {ok, #{atom() => graphql:json()}} | {error, any()}. %% API @@ -39,18 +53,62 @@ stop() -> persistent_term:erase(?MODULE), ok. --spec find_document(category(), command()) -> {ok, doc()} | {error, not_found}. -find_document(Category, Command) -> - case persistent_term:get(?MODULE) of - #{Category := #{Command := CommandSpec}} -> - #{doc := Doc} = CommandSpec, - {ok, Doc}; - _ -> - {error, not_found} - end. +-spec process([string()]) -> result(). +process(Args) -> + lists:foldl(fun(_, {error, _} = Error) -> Error; + (StepF, Ctx) -> StepF(Ctx) + end, + #{args => Args}, + [fun find_category/1, fun find_command/1, fun parse_vars/1, fun execute/1]). + +-spec get_specs() -> specs(). +get_specs() -> + persistent_term:get(?MODULE). --spec execute(doc(), json_map()) -> {ok, map()} | {error, term()}. -execute(Doc, Vars) -> +%% Internals + +-spec find_category(ctx()) -> ctx() | {error, ctx()}. +find_category(CtxIn = #{args := [CategoryStr | Args]}) -> + Category = list_to_binary(CategoryStr), + Ctx = CtxIn#{category => Category, args => Args}, + case get_specs() of + #{Category := CatSpec} -> + Ctx#{cat_spec => CatSpec}; + #{} -> + {error, Ctx#{reason => unknown_category}} + end; +find_category(Ctx = #{args := []}) -> + {error, Ctx#{reason => no_args}}. + +-spec find_command(ctx()) -> ctx() | {error, ctx()}. +find_command(CtxIn = #{args := [CommandStr | Args]}) -> + Command = list_to_binary(CommandStr), + Ctx = #{cat_spec := CatSpec} = CtxIn#{command => Command, args => Args}, + case CatSpec of + #{Command := CommandSpec} -> + #{doc := Doc} = CommandSpec, + Ctx#{doc => Doc}; + #{} -> + {error, Ctx#{reason => unknown_command}} + end; +find_command(Ctx) -> + {error, Ctx#{reason => no_command}}. + +-spec parse_vars(ctx()) -> ctx() | {error, ctx()}. +parse_vars(Ctx = #{args := [VarsStr]}) -> + try jiffy:decode(VarsStr, [return_maps]) of + Vars -> + Ctx#{vars => Vars} + catch _:_ -> + {error, Ctx#{reason => invalid_vars}} + end; +parse_vars(Ctx = #{args := []}) -> + {error, Ctx#{reason => no_vars}}; +parse_vars(Ctx = #{}) -> + {error, Ctx#{reason => too_many_args}}. + +-spec execute(ctx()) -> result(). +execute(#{doc := Doc, vars := Vars}) -> execute(mongoose_graphql:get_endpoint(admin), Doc, Vars). %% Internals @@ -186,7 +244,7 @@ return_field(#{name := Name, fields := Fields}) -> return_field(#{name := Name}) -> Name. --spec execute(ep(), doc(), json_map()) -> {ok, #{atom() => graphql:json()}} | {error, term()}. +-spec execute(ep(), doc(), json_map()) -> result(). execute(Ep, Doc, Vars) -> mongoose_graphql:execute(Ep, #{document => Doc, operation_name => undefined,