From f88e4d3c9fc33e02196df3b8347a002846aab6af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20Wojtasik?= Date: Tue, 7 Dec 2021 11:45:59 +0100 Subject: [PATCH] Apply review - Fix descriptions - Make tests paralell - Add graphql_SUITE to default.spec - Log schema loading error - Simplify mongoose_graphql_cowboy_handler init --- big_tests/default.spec | 1 + big_tests/tests/graphql_SUITE.erl | 14 +++++++------- priv/graphql/schemas/admin/admin_schema.gql | 6 +++--- src/mongoose_graphql.erl | 14 +++++++++++--- .../mongoose_graphql_cowboy_handler.erl | 13 ++++--------- .../mongoose_graphql_permissions.erl | 7 ++++--- test/mongoose_graphql_SUITE.erl | 8 ++++---- 7 files changed, 34 insertions(+), 29 deletions(-) diff --git a/big_tests/default.spec b/big_tests/default.spec index 9f75467ca4e..2b370393096 100644 --- a/big_tests/default.spec +++ b/big_tests/default.spec @@ -25,6 +25,7 @@ {suites, "tests", disco_and_caps_SUITE}. {suites, "tests", extdisco_SUITE}. {suites, "tests", gdpr_SUITE}. +{suites, "tests", graphql_SUITE}. {suites, "tests", inbox_SUITE}. {suites, "tests", inbox_extensions_SUITE}. {suites, "tests", jingle_SUITE}. diff --git a/big_tests/tests/graphql_SUITE.erl b/big_tests/tests/graphql_SUITE.erl index 41313d14078..a588599be80 100644 --- a/big_tests/tests/graphql_SUITE.erl +++ b/big_tests/tests/graphql_SUITE.erl @@ -15,16 +15,16 @@ all() -> {group, user_handler}]. groups() -> - [{cowboy_handler, [], [can_connect_to_admin, + [{cowboy_handler, [parallel], [can_connect_to_admin, can_connect_to_user ]}, - {user_handler, [], [wrong_creds_cannot_access_protected_types, + {user_handler, [parallel], [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 ]}, - {admin_handler, [], [wrong_creds_cannot_access_protected_types, + {admin_handler, [parallel], [wrong_creds_cannot_access_protected_types, unauth_cannot_access_protected_types, unauth_can_access_unprotected_types, can_execute_query_with_variables, @@ -48,7 +48,7 @@ init_per_group(admin_handler, Config) -> true -> [{schema_endpoint, Endpoint} | Config]; false -> - {skipped, <<"Admin credentials not defined in config">>} + {skipped, <<"Admin credentials are not defined in config">>} end; init_per_group(user_handler, Config) -> Config1 = escalus:create_users(Config, escalus:get_users([alice])), @@ -92,7 +92,7 @@ wrong_creds_cannot_access_protected_types(Config) -> assert_no_permissions(Status, Data). auth_user_can_access_protected_types(Config) -> - escalus:story( + escalus:fresh_story( Config, [{alice, 1}], fun(Alice) -> Password = user_password(alice), @@ -121,7 +121,7 @@ can_execute_query_with_variables(Config) -> {Status, Data} = execute(Ep, Body, undefined), ?assertEqual({<<"200">>,<<"OK">>}, Status), % operation M1 was executed, because id is in path - % access was granted, error was returned because valid resolver was not defined + % access was granted, an error was returned because valid resolver was not defined ?assertMatch(#{<<"data">> := #{<<"id">> := null}, <<"errors">> := [#{<<"extensions">> := #{<<"code">> := <<"resolver_crash">>}, @@ -136,7 +136,7 @@ assert_no_permissions(Status, Data) -> assert_access_granted(Status, Data) -> ?assertEqual({<<"200">>,<<"OK">>}, Status), - % access was granted, error was returned because valid resolver was not defined + % access was granted, an error was returned because valid resolver was not defined ?assertMatch(#{<<"errors">> := [#{<<"extensions">> := #{<<"code">> := <<"resolver_crash">>}}]}, Data). diff --git a/priv/graphql/schemas/admin/admin_schema.gql b/priv/graphql/schemas/admin/admin_schema.gql index 15da81d8e8f..717017445dd 100644 --- a/priv/graphql/schemas/admin/admin_schema.gql +++ b/priv/graphql/schemas/admin/admin_schema.gql @@ -7,7 +7,7 @@ directive @protected on FIELD_DEFINITION | OBJECT """ Contains all admin available queries. -Only authenticated admin can execute this queries. +Only an authenticated admin can execute these queries. """ type AdminQuery @protected{ "Get all enabled domains by hostType" @@ -18,7 +18,7 @@ type AdminQuery @protected{ """ Contains all admin available mutations -Only authenticated admin can execute this mutations. +Only an authenticated admin can execute these mutations. """ type AdminMutation @protected{ "Add new domain" @@ -33,7 +33,7 @@ type AdminMutation @protected{ """ A dynamic domain representation. -Some operation could return not complete object i.e. some fields can be null. +Some operation could return incomplete object i.e. some fields can be null. """ type Domain{ "Domain name" diff --git a/src/mongoose_graphql.erl b/src/mongoose_graphql.erl index 8e37f558e0c..e13023ddecf 100644 --- a/src/mongoose_graphql.erl +++ b/src/mongoose_graphql.erl @@ -3,6 +3,8 @@ %% @end -module(mongoose_graphql). +-include_lib("kernel/include/logger.hrl"). + %API -export([init/0, get_endpoint/1, @@ -122,10 +124,16 @@ load_multiple_file_schema(Pattern) -> SchemaData = [read_schema_file(P) || P <- Paths], {ok, lists:flatten(SchemaData)} catch - _:_ -> + throw:{error, Reason, Path} -> + ?LOG_ERROR(#{what => graphql_cannot_load_schema, + reason => Reason, path => Path}), {error, cannot_load} end. read_schema_file(Path) -> - {ok, Data} = file:read_file(Path), - binary_to_list(Data). + case file:read_file(Path) of + {ok, Data} -> + binary_to_list(Data); + {error, Reason} -> + throw({error, Reason, Path}) + end. diff --git a/src/mongoose_graphql/mongoose_graphql_cowboy_handler.erl b/src/mongoose_graphql/mongoose_graphql_cowboy_handler.erl index 49eb7fb3d9c..ae67041edb7 100644 --- a/src/mongoose_graphql/mongoose_graphql_cowboy_handler.erl +++ b/src/mongoose_graphql/mongoose_graphql_cowboy_handler.erl @@ -8,9 +8,6 @@ -behavior(cowboy_rest). -%% ejabberd_cowboy callbacks --export([cowboy_router_paths/2]). - %% Cowboy Handler Interface -export([init/2]). @@ -33,16 +30,14 @@ -ignore_xref([cowboy_router_paths/2, from_json/2, to_html/2, to_json/2]). -%% -- API --------------------------------------------------- - -cowboy_router_paths(BasePath, Opts) -> - [{[BasePath, "/"], ?MODULE, [{priv_file, mongooseim, "graphql/wsite/index.html"} | Opts]}]. +%% API -init(Req, [{priv_file, _, _} = PrivFile | Opts]) -> +init(Req, Opts) -> + IndexLocation = {priv_file, mongooseim, "graphql/wsite/index.html"}, OptsMap = maps:from_list(Opts), {cowboy_rest, Req, - OptsMap#{index_location => PrivFile} + OptsMap#{index_location => IndexLocation} }. allowed_methods(Req, State) -> diff --git a/src/mongoose_graphql/mongoose_graphql_permissions.erl b/src/mongoose_graphql/mongoose_graphql_permissions.erl index 7148416144d..c8744fadab4 100644 --- a/src/mongoose_graphql/mongoose_graphql_permissions.erl +++ b/src/mongoose_graphql/mongoose_graphql_permissions.erl @@ -2,9 +2,10 @@ %% permissions. %% %% GraphQL has directives that allow attaching additional information to schema, -%% objects, fields, and more. The custom directive `@protected' is created to mark -%% which objects or fields could be accessed only by authorized request. This module -%% analyzes the AST and tries to find if there is at least one protected resource. +%% 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. +%% 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, %% an error is thrown. diff --git a/test/mongoose_graphql_SUITE.erl b/test/mongoose_graphql_SUITE.erl index 472677d6c81..4564570a4aa 100644 --- a/test/mongoose_graphql_SUITE.erl +++ b/test/mongoose_graphql_SUITE.erl @@ -9,24 +9,24 @@ all() -> [can_create_endpoint, can_load_splitted_schema, - {group, unprotected_graphql}, + {group, unprotected_graphql}, {group, protected_graphql}, {group, errors_handling}]. groups() -> - [{protected_graphql, [], + [{protected_graphql, [parallel], [auth_can_execute_protected_query, auth_can_execute_protected_mutation, unauth_cannot_execute_protected_query, unauth_cannot_execute_protected_mutation, unauth_can_access_introspection]}, - {unprotected_graphql, [], + {unprotected_graphql, [parallel], [can_execute_query_with_vars, auth_can_execute_query, auth_can_execute_mutation, unauth_can_execute_query, unauth_can_execute_mutation]}, - {errors_handling, [], + {errors_handling, [parallel], [should_catch_parsing_errors, should_catch_type_check_params_errors, should_catch_type_check_errors