From a719db03c600a57a11b48b89976dc8a4448fb993 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20Wojtasik?= Date: Thu, 9 Dec 2021 10:56:53 +0100 Subject: [PATCH] Apply review, Fix formatting - Use jiffy instead of jsx - Fix descriptions - Other small fixes --- big_tests/dynamic_domains.spec | 2 + big_tests/tests/graphql_SUITE.erl | 16 ++--- big_tests/tests/mongooseimctl_SUITE.erl | 4 +- priv/graphql/wsite/index.html | 6 +- src/ejabberd_ctl.erl | 6 +- src/mongoose_graphql.erl | 4 +- .../mongoose_graphql_cowboy_handler.erl | 27 ++++---- .../mongoose_graphql_cowboy_response.erl | 2 +- .../mongoose_graphql_default.erl | 2 +- .../mongoose_graphql_errors.erl | 4 +- .../mongoose_graphql_permissions.erl | 11 ++- test/mongoose_graphql_SUITE.erl | 68 +++++++++---------- .../mutation.gql | 0 .../query.gql | 0 .../root.gql | 0 test/mongoose_graphql_default_resolver.erl | 2 - 16 files changed, 69 insertions(+), 85 deletions(-) rename test/mongoose_graphql_SUITE_data/{splitted_schema => split_schema}/mutation.gql (100%) rename test/mongoose_graphql_SUITE_data/{splitted_schema => split_schema}/query.gql (100%) rename test/mongoose_graphql_SUITE_data/{splitted_schema => split_schema}/root.gql (100%) diff --git a/big_tests/dynamic_domains.spec b/big_tests/dynamic_domains.spec index e5e8630cdd..d652d4b66a 100644 --- a/big_tests/dynamic_domains.spec +++ b/big_tests/dynamic_domains.spec @@ -39,6 +39,8 @@ remove_personal_data_pubsub], "at the moment mod_pubsub doesn't support dynamic domains"}. +{suites, "tests", graphql_SUITE}. + {suites, "tests", inbox_SUITE}. {suites, "tests", inbox_extensions_SUITE}. diff --git a/big_tests/tests/graphql_SUITE.erl b/big_tests/tests/graphql_SUITE.erl index d411591b42..707946a7fa 100644 --- a/big_tests/tests/graphql_SUITE.erl +++ b/big_tests/tests/graphql_SUITE.erl @@ -25,23 +25,15 @@ cowboy_handler() -> can_connect_to_user]. user_handler() -> - [wrong_creds_cannot_access_protected_types, - unauth_cannot_access_protected_types, - unauth_can_access_unprotected_types, - can_execute_query_with_variables, - auth_user_can_access_protected_types, - invalid_json_body_error, - no_query_supplied_error, - variables_invalid_json_error, - can_load_graphiql]. - - + [auth_user_can_access_protected_types | common_tests()]. admin_handler() -> + [auth_admin_can_access_protected_types | common_tests()]. + +common_tests() -> [wrong_creds_cannot_access_protected_types, unauth_cannot_access_protected_types, unauth_can_access_unprotected_types, can_execute_query_with_variables, - auth_admin_can_access_protected_types, invalid_json_body_error, no_query_supplied_error, variables_invalid_json_error, diff --git a/big_tests/tests/mongooseimctl_SUITE.erl b/big_tests/tests/mongooseimctl_SUITE.erl index 749bcb805b..1ac86e94c9 100644 --- a/big_tests/tests/mongooseimctl_SUITE.erl +++ b/big_tests/tests/mongooseimctl_SUITE.erl @@ -1138,12 +1138,12 @@ can_handle_execution_error(Config) -> graphql_wrong_arguments_number(Config) -> ExpectedFragment = "This command requires", ResNoArgs = mongooseimctl("graphql", [], Config), - ?assertMatch({_, 0}, ResNoArgs), + ?assertMatch({_, 1}, ResNoArgs), Data1 = element(1, ResNoArgs), ?assertNotEqual(nomatch, string:find(Data1, ExpectedFragment)), ResTooManyArgs = mongooseimctl("graphql", ["{}", "{}"], Config), - ?assertMatch({_, 0}, ResTooManyArgs), + ?assertMatch({_, 1}, ResTooManyArgs), Data2 = element(1, ResTooManyArgs), ?assertNotEqual(nomatch, string:find(Data2, ExpectedFragment)). diff --git a/priv/graphql/wsite/index.html b/priv/graphql/wsite/index.html index fc31c6f58b..81c1477a0c 100644 --- a/priv/graphql/wsite/index.html +++ b/priv/graphql/wsite/index.html @@ -1,9 +1,9 @@ diff --git a/src/ejabberd_ctl.erl b/src/ejabberd_ctl.erl index 6411f59411..07d9116901 100644 --- a/src/ejabberd_ctl.erl +++ b/src/ejabberd_ctl.erl @@ -182,7 +182,7 @@ process(["graphql", Arg]) when is_list(Arg) -> Ep = mongoose_graphql:get_endpoint(admin), case mongoose_graphql:execute(Ep, undefined, Doc) of {ok, Result} -> - PrettyResult = jsx:prettify(jsx:encode(Result)), + PrettyResult = jiffy:encode(Result, [pretty]), ?PRINT("~s\n", [PrettyResult]); {error, _} = Err -> ?PRINT("~p\n", [Err]) @@ -190,7 +190,7 @@ process(["graphql", Arg]) when is_list(Arg) -> ?STATUS_SUCCESS; process(["graphql" | _]) -> ?PRINT("This command requires one string type argument!\n", []), - ?STATUS_SUCCESS; + ?STATUS_ERROR; %% @doc The arguments --long and --dual are not documented because they are %% automatically selected depending in the number of columns of the shell @@ -571,7 +571,7 @@ print_usage(HelpMode, MaxC, ShCode) -> {"restart", [], "Restart MongooseIM"}, {"help", ["[--tags [tag] | com?*]"], "Show help (try: mongooseimctl help help)"}, {"mnesia", ["[info]"], "show information of Mnesia system"}, - {"graphql", ["query"], "Executes graphql query or mutation"}] ++ + {"graphql", ["query"], "Execute graphql query or mutation"}] ++ get_list_commands() ++ get_list_ctls(), diff --git a/src/mongoose_graphql.erl b/src/mongoose_graphql.erl index e13023ddec..4054812897 100644 --- a/src/mongoose_graphql.erl +++ b/src/mongoose_graphql.erl @@ -89,9 +89,7 @@ execute(Ep, OpName, Doc) -> % Internal schema_pattern(DirName) -> - filename:join([code:priv_dir(mongooseim), - "graphql/schemas", - DirName, "*.gql"]). + filename:join([code:priv_dir(mongooseim), "graphql/schemas", DirName, "*.gql"]). graphql_parse(Doc) -> case graphql:parse(Doc) of diff --git a/src/mongoose_graphql/mongoose_graphql_cowboy_handler.erl b/src/mongoose_graphql/mongoose_graphql_cowboy_handler.erl index 43182c14ab..3a3aca78fa 100644 --- a/src/mongoose_graphql/mongoose_graphql_cowboy_handler.erl +++ b/src/mongoose_graphql/mongoose_graphql_cowboy_handler.erl @@ -62,13 +62,12 @@ is_authorized(Req, State) -> State2 = check_auth(Auth, State), {true, Req, State2}. -resource_exists(#{ method := <<"GET">> } = Req, State) -> +resource_exists(#{method := <<"GET">>} = Req, State) -> {true, Req, State}; -resource_exists(#{ method := <<"POST">> } = Req, State) -> +resource_exists(#{method := <<"POST">>} = Req, State) -> {false, Req, State}. -to_html(Req, #{ index_location := - {priv_file, App, FileLocation}} = State) -> +to_html(Req, #{index_location := {priv_file, App, FileLocation}} = State) -> Filename = filename:join(code:priv_dir(App), FileLocation), {ok, Data} = file:read_file(Filename), {Data, Req, State}. @@ -107,7 +106,7 @@ auth_admin(_, State) -> % auth credentials not provided in config State#{authorized => true}. -run_request(#{ document := undefined }, Req, State) -> +run_request(#{document := undefined}, Req, State) -> reply_error(400, no_query_supplied, Req, State); run_request(#{} = ReqCtx, Req, #{schema_endpoint := EpName, authorized := AuthStatus} = State) -> @@ -127,11 +126,11 @@ run_request(#{} = ReqCtx, Req, #{schema_endpoint := EpName, gather(Req) -> {ok, Body, Req2} = cowboy_req:read_body(Req), Bindings = cowboy_req:bindings(Req2), - try jsx:decode(Body, [return_maps]) of + try jiffy:decode(Body, [return_maps]) of JSON -> gather(Req2, JSON, Bindings) catch - error:badarg -> + _:_ -> {error, invalid_json_body} end. @@ -140,26 +139,26 @@ gather(Req, Body, Params) -> case variables([Params, Body]) of {ok, Vars} -> Operation = operation_name([Params, Body]), - {ok, Req, #{ document => QueryDocument, + {ok, Req, #{document => QueryDocument, vars => Vars, operation_name => Operation}}; {error, Reason} -> {error, Reason} end. -document([#{ <<"query">> := Q }|_]) -> Q; +document([#{<<"query">> := Q}|_]) -> Q; document([_|Next]) -> document(Next); document([]) -> undefined. -variables([#{ <<"variables">> := Vars} | _]) -> +variables([#{<<"variables">> := Vars} | _]) -> if is_binary(Vars) -> - try jsx:decode(Vars, [return_maps]) of + try jiffy:decode(Vars, [return_maps]) of null -> {ok, #{}}; JSON when is_map(JSON) -> {ok, JSON}; _ -> {error, variables_invalid_json} catch - error:badarg -> + _:_ -> {error, variables_invalid_json} end; is_map(Vars) -> @@ -172,7 +171,7 @@ variables([_ | Next]) -> variables([]) -> {ok, #{}}. -operation_name([#{ <<"operationName">> := OpName } | _]) -> +operation_name([#{<<"operationName">> := OpName} | _]) -> OpName; operation_name([_ | Next]) -> operation_name(Next); @@ -189,7 +188,7 @@ reply_error(Code, Msg, Req, State) -> [#{type => error, message => Formatted}] end, - Body = jsx:encode(#{ errors => Errors}), + Body = jiffy:encode(#{errors => Errors}), Req2 = cowboy_req:set_resp_body(Body, Req), Reply = cowboy_req:reply(Code, Req2), {stop, Reply, State}. diff --git a/src/mongoose_graphql/mongoose_graphql_cowboy_response.erl b/src/mongoose_graphql/mongoose_graphql_cowboy_response.erl index 04df0ba487..80dc6665c7 100644 --- a/src/mongoose_graphql/mongoose_graphql_cowboy_response.erl +++ b/src/mongoose_graphql/mongoose_graphql_cowboy_response.erl @@ -3,7 +3,7 @@ -export([term_to_json/1]). term_to_json(Term) -> - jsx:encode(fixup(Term)). + jiffy:encode(fixup(Term)). %% Ground types fixup(Term) when is_number(Term) -> Term; diff --git a/src/mongoose_graphql/mongoose_graphql_default.erl b/src/mongoose_graphql/mongoose_graphql_default.erl index b5a37df82a..ff2fedb091 100644 --- a/src/mongoose_graphql/mongoose_graphql_default.erl +++ b/src/mongoose_graphql/mongoose_graphql_default.erl @@ -5,6 +5,6 @@ -ignore_xref([execute/4]). %% Assume we are given a map(). Look up the field in the map. If not -%% %% present, return the value null. +%% present, return the value null. execute(_Ctx, Obj, Field, _Args) -> {ok, maps:get(Field, Obj, null)}. diff --git a/src/mongoose_graphql/mongoose_graphql_errors.erl b/src/mongoose_graphql/mongoose_graphql_errors.erl index 71b4c37dfe..f47ed3b2ee 100644 --- a/src/mongoose_graphql/mongoose_graphql_errors.erl +++ b/src/mongoose_graphql/mongoose_graphql_errors.erl @@ -5,14 +5,14 @@ -ignore_xref([err/2, crash/2]). -% callback invoked when resorer returns error tuple +% callback invoked when resolver returns error tuple err(_Ctx, domain_not_found) -> #{message => <<"Given domain does not exist">>, extensions => #{code => resolver_error}}; err(_Ctx, ErrorTerm) -> #{message => iolist_to_binary(io_lib:format("~p", [ErrorTerm])), extensions => #{code => resolver_error}}. -% callback invoked when resoler crashes +% callback invoked when resolver crashes crash(_Ctx, #{type := Type}) -> #{message => <<"Unexpected ", Type/binary, " resolver crash">>, extensions => #{code => resolver_crash}}. diff --git a/src/mongoose_graphql/mongoose_graphql_permissions.erl b/src/mongoose_graphql/mongoose_graphql_permissions.erl index c8744fadab..937764849d 100644 --- a/src/mongoose_graphql/mongoose_graphql_permissions.erl +++ b/src/mongoose_graphql/mongoose_graphql_permissions.erl @@ -1,13 +1,12 @@ -%% @doc Module that checks if a requested query can be executed with provided -%% permissions. +%% @doc Checks if a requested query can be executed with provided permissions. %% %% GraphQL has directives that allow attaching additional information to schema, %% to objects, to fields, and more. The custom directive `@protected' is created -%% to mark which objects or fields could be accessed only by authorized request. +%% to mark which objects or fields could be accessed only by an authorized request. %% This module analyzes the AST and tries to find if there is at least one protected %% resource. %% -%% If unauthorized request want to execute a query that contains protected resources, +%% If an unauthorized request wants to execute a query that contains protected resources, %% an error is thrown. %% %% Directives can have arguments, so if needed this functionality can be easily @@ -25,7 +24,7 @@ -type auth_status() :: boolean(). %% @doc Checks if query can be executed by unauthorized request. If not, throws -%% an error. When request is authorized just skip. +%% an error. When request is authorized, just skip. %% @end -spec check_permissions(binary(), auth_status(), #document{}) -> ok. check_permissions(OpName, false, #document{definitions = Definitions}) -> @@ -41,7 +40,7 @@ check_permissions(OpName, false, #document{definitions = Definitions}) -> % When an object is protected we need to ensure that the request % query contains only introspection fields to execute it without % authorization. Otherwise, a user couldn't access documentation - % without login in. + % without logging in. case is_introspection_op(Op1) of true -> ok; diff --git a/test/mongoose_graphql_SUITE.erl b/test/mongoose_graphql_SUITE.erl index 4564570a4a..7338d24209 100644 --- a/test/mongoose_graphql_SUITE.erl +++ b/test/mongoose_graphql_SUITE.erl @@ -7,15 +7,15 @@ -include_lib("graphql/src/graphql_schema.hrl"). all() -> - [can_create_endpoint, - can_load_splitted_schema, + [can_create_endpoint, + can_load_split_schema, {group, unprotected_graphql}, {group, protected_graphql}, {group, errors_handling}]. groups() -> [{protected_graphql, [parallel], - [auth_can_execute_protected_query, + [auth_can_execute_protected_query, auth_can_execute_protected_mutation, unauth_cannot_execute_protected_query, unauth_cannot_execute_protected_mutation, @@ -66,38 +66,34 @@ can_create_endpoint(Config) -> Ep = mongoose_graphql:get_endpoint(Name), ?assertMatch({endpoint_context, Name, Pid, _, _}, Ep), - ?assertMatch(#root_schema{id = 'ROOT', query = <<"UserQuery">>, + ?assertMatch(#root_schema{id = 'ROOT', query = <<"UserQuery">>, mutation = <<"UserMutation">>}, graphql_schema:get(Ep, 'ROOT')). -can_load_splitted_schema(Config) -> +can_load_split_schema(Config) -> Name = ?config(endpoint_name, Config), - {Mapping, Pattern} = example_splitted_schema_data(Config), - Pattern2 = filelib:wildcard(Pattern), + {Mapping, Pattern} = example_split_schema_data(Config), {ok, Pid} = mongoose_graphql:create_endpoint(Name, Mapping, Pattern), Ep = mongoose_graphql:get_endpoint(Name), ?assertMatch({endpoint_context, Name, Pid, _, _}, Ep), - ?assertMatch(#root_schema{id = 'ROOT', query = <<"Query">>, + ?assertMatch(#root_schema{id = 'ROOT', query = <<"Query">>, mutation = <<"Mutation">>}, graphql_schema:get(Ep, 'ROOT')), - ?assertMatch(#object_type{id = <<"Query">>}, - graphql_schema:get(Ep, <<"Query">>)), - ?assertMatch(#object_type{id = <<"Mutation">>}, - graphql_schema:get(Ep, <<"Mutation">>)). - + ?assertMatch(#object_type{id = <<"Query">>}, graphql_schema:get(Ep, <<"Query">>)), + ?assertMatch(#object_type{id = <<"Mutation">>}, graphql_schema:get(Ep, <<"Mutation">>)). auth_can_execute_protected_query(Config) -> Ep = ?config(endpoint, Config), Doc = <<"{ field }">>, Res = mongoose_graphql:execute(Ep, undefined, Doc), - ?assertEqual({ok,#{data => #{<<"field">> => <<"Test field">>}}}, Res). + ?assertEqual({ok, #{data => #{<<"field">> => <<"Test field">>}}}, Res). auth_can_execute_protected_mutation(Config) -> Ep = ?config(endpoint, Config), Doc = <<"mutation { field }">>, Res = mongoose_graphql:execute(Ep, undefined, Doc), - ?assertEqual({ok,#{data => #{<<"field">> => <<"Test field">>}}}, Res). + ?assertEqual({ok, #{data => #{<<"field">> => <<"Test field">>}}}, Res). unauth_cannot_execute_protected_query(Config) -> Ep = ?config(endpoint, Config), @@ -115,15 +111,15 @@ unauth_can_access_introspection(Config) -> Ep = ?config(endpoint, Config), Doc = <<"{ __schema { queryType { name } } __type(name: \"UserQuery\") { name } }">>, Res = mongoose_graphql:execute(Ep, request(Doc, false)), - Expected = + Expected = {ok, #{data => #{<<"__schema">> => - #{<<"queryType">> => + #{<<"queryType">> => #{<<"name">> => <<"UserQuery">>} }, - <<"__type">> => - #{<<"name">> => + <<"__type">> => + #{<<"name">> => <<"UserQuery">> } } @@ -134,38 +130,38 @@ unauth_can_access_introspection(Config) -> can_execute_query_with_vars(Config) -> Ep = ?config(endpoint, Config), Doc = <<"query Q1($value: String!) { id(value: $value)}">>, - Req = - #{document => Doc, - operation_name => <<"Q1">>, - vars => #{<<"value">> => <<"Hello">>}, - authorized => false, + Req = + #{document => Doc, + operation_name => <<"Q1">>, + vars => #{<<"value">> => <<"Hello">>}, + authorized => false, ctx => #{}}, Res = mongoose_graphql:execute(Ep, Req), - ?assertEqual({ok,#{data => #{<<"id">> => <<"Hello">>}}}, Res). + ?assertEqual({ok, #{data => #{<<"id">> => <<"Hello">>}}}, Res). unauth_can_execute_query(Config) -> Ep = ?config(endpoint, Config), Doc = <<"query { field }">>, Res = mongoose_graphql:execute(Ep, request(Doc, false)), - ?assertEqual({ok,#{data => #{<<"field">> => <<"Test field">>}}}, Res). + ?assertEqual({ok, #{data => #{<<"field">> => <<"Test field">>}}}, Res). unauth_can_execute_mutation(Config) -> Ep = ?config(endpoint, Config), Doc = <<"mutation { field }">>, Res = mongoose_graphql:execute(Ep, request(Doc, false)), - ?assertEqual({ok,#{data => #{<<"field">> => <<"Test field">>}}}, Res). + ?assertEqual({ok, #{data => #{<<"field">> => <<"Test field">>}}}, Res). auth_can_execute_query(Config) -> Ep = ?config(endpoint, Config), Doc = <<"query { field }">>, Res = mongoose_graphql:execute(Ep, request(Doc, true)), - ?assertEqual({ok,#{data => #{<<"field">> => <<"Test field">>}}}, Res). + ?assertEqual({ok, #{data => #{<<"field">> => <<"Test field">>}}}, Res). auth_can_execute_mutation(Config) -> Ep = ?config(endpoint, Config), Doc = <<"mutation { field }">>, Res = mongoose_graphql:execute(Ep, request(Doc, true)), - ?assertEqual({ok,#{data => #{<<"field">> => <<"Test field">>}}}, Res). + ?assertEqual({ok, #{data => #{<<"field">> => <<"Test field">>}}}, Res). should_catch_parsing_errors(Config) -> Ep = ?config(endpoint, Config), @@ -188,15 +184,15 @@ should_catch_type_check_params_errors(Config) -> %% Helpers request(Doc, Authorized) -> - #{document => Doc, - operation_name => undefined, - vars => #{}, - authorized => Authorized, + #{document => Doc, + operation_name => undefined, + vars => #{}, + authorized => Authorized, ctx => #{}}. -example_splitted_schema_data(Config) -> - Pattern = filename:join([proplists:get_value(data_dir, Config), - "splitted_schema", "*.gql"]), +example_split_schema_data(Config) -> + Pattern = filename:join([proplists:get_value(data_dir, Config), + "split_schema", "*.gql"]), Mapping = #{objects => #{'Query' => mongoose_graphql_default_resolver, diff --git a/test/mongoose_graphql_SUITE_data/splitted_schema/mutation.gql b/test/mongoose_graphql_SUITE_data/split_schema/mutation.gql similarity index 100% rename from test/mongoose_graphql_SUITE_data/splitted_schema/mutation.gql rename to test/mongoose_graphql_SUITE_data/split_schema/mutation.gql diff --git a/test/mongoose_graphql_SUITE_data/splitted_schema/query.gql b/test/mongoose_graphql_SUITE_data/split_schema/query.gql similarity index 100% rename from test/mongoose_graphql_SUITE_data/splitted_schema/query.gql rename to test/mongoose_graphql_SUITE_data/split_schema/query.gql diff --git a/test/mongoose_graphql_SUITE_data/splitted_schema/root.gql b/test/mongoose_graphql_SUITE_data/split_schema/root.gql similarity index 100% rename from test/mongoose_graphql_SUITE_data/splitted_schema/root.gql rename to test/mongoose_graphql_SUITE_data/split_schema/root.gql diff --git a/test/mongoose_graphql_default_resolver.erl b/test/mongoose_graphql_default_resolver.erl index f3664a4fff..53ae509f58 100644 --- a/test/mongoose_graphql_default_resolver.erl +++ b/test/mongoose_graphql_default_resolver.erl @@ -10,5 +10,3 @@ execute(_Ctx, _Obj, <<"id">>, #{<<"value">> := Value}) -> {ok, Value}; execute(_Ctx, _Obj, Field, _Attrs) -> {error, {not_implemented, Field}}. - -