Improve error handling in httpUpload

esl · Nov 22, 2022 · f90811b · f90811b
1 parent 89d1b2b
commit f90811b
Show file tree

Hide file tree

Showing 5 changed files with 92 additions and 49 deletions.
diff --git a/big_tests/tests/graphql_http_upload_SUITE.erl b/big_tests/tests/graphql_http_upload_SUITE.erl
@@ -6,7 +6,7 @@
 -import(distributed_helper, [mim/0, require_rpc_nodes/1]).
 -import(domain_helper, [host_type/0, domain/0, secondary_domain/0]).
 -import(graphql_helper, [execute_user_command/5, execute_command/4, get_ok_value/2,
-                         get_err_msg/1, get_err_code/1, get_unauthorized/1]).
+                         get_err_msg/1, get_err_code/1, get_coercion_err_msg/1, get_unauthorized/1]).
 
 -include_lib("eunit/include/eunit.hrl").
 
@@ -47,6 +47,8 @@ domain_admin_groups() ->
 
 user_http_upload_tests() ->
     [user_get_url_test,
+     user_get_url_test_no_content_type,
+     user_get_url_test_empty_filename,
      user_get_url_zero_size,
      user_get_url_too_large_size,
      user_get_url_zero_timeout].
@@ -56,6 +58,8 @@ user_http_upload_not_configured_tests() ->
 
 admin_http_upload_tests() ->
     [admin_get_url_test,
+     admin_get_url_test_no_content_type,
+     admin_get_url_test_empty_filename,
      admin_get_url_zero_size,
      admin_get_url_too_large_size,
      admin_get_url_zero_timeout,
@@ -66,6 +70,8 @@ admin_http_upload_not_configured_tests() ->
 
 domain_admin_http_upload_tests() ->
     [admin_get_url_test,
+     admin_get_url_test_no_content_type,
+     admin_get_url_test_empty_filename,
      admin_get_url_zero_size,
      admin_get_url_too_large_size,
      admin_get_url_zero_timeout,
@@ -148,18 +154,40 @@ user_get_url_test(Config) ->
 user_get_url_test(Config, Alice) ->
     Result = user_get_url(<<"test">>, 123, <<"Test">>, 123, Alice, Config),
     ParsedResult = get_ok_value([data, httpUpload, getUrl], Result),
-    #{<<"PutUrl">> := PutURL, <<"GetUrl">> := GetURL, <<"Header">> := _Headers} = ParsedResult,
+    #{<<"putUrl">> := PutURL, <<"getUrl">> := GetURL, <<"headers">> := _Headers} = ParsedResult,
     ?assertMatch({_, _}, binary:match(PutURL, [?S3_HOSTNAME])),
     ?assertMatch({_, _}, binary:match(GetURL, [?S3_HOSTNAME])).
 
+user_get_url_test_no_content_type(Config) ->
+    escalus:fresh_story_with_config(Config, [{alice, 1}],
+                                    fun user_get_url_test_no_content_type/2).
+
+user_get_url_test_no_content_type(Config, Alice) ->
+    user_get_url_test_no_content_type(Config, Alice, null),
+    user_get_url_test_no_content_type(Config, Alice, <<"">>).
+
+user_get_url_test_no_content_type(Config, Alice, ContentType) ->
+    Result = user_get_url(<<"test">>, 123, ContentType, 123, Alice, Config),
+    ParsedResult = get_ok_value([data, httpUpload, getUrl], Result),
+    #{<<"putUrl">> := PutURL, <<"getUrl">> := GetURL, <<"headers">> := _Headers} = ParsedResult,
+    ?assertMatch({_, _}, binary:match(PutURL, [?S3_HOSTNAME])),
+    ?assertMatch({_, _}, binary:match(GetURL, [?S3_HOSTNAME])).
+
+user_get_url_test_empty_filename(Config) ->
+    escalus:fresh_story_with_config(Config, [{alice, 1}],
+                                    fun user_get_url_test_empty_filename/2).
+
+user_get_url_test_empty_filename(Config, Alice) ->
+    Result = user_get_url(<<"">>, 123, <<"Test">>, 123, Alice, Config),
+    ?assertMatch({_, _}, binary:match(get_coercion_err_msg(Result), <<"Given string is empty">>)).
+
 user_get_url_zero_size(Config) ->
     escalus:fresh_story_with_config(Config, [{alice, 1}],
                                     fun user_get_url_zero_size/2).
 
 user_get_url_zero_size(Config, Alice) ->
     Result = user_get_url(<<"test">>, 0, <<"Test">>, 123, Alice, Config),
-    ?assertEqual(<<"size_error">>, get_err_code(Result)),
-    ?assertEqual(<<"size must be positive integer">>, get_err_msg(Result)).
+    ?assertMatch({_, _}, binary:match(get_coercion_err_msg(Result), <<"Value is not a positive integer">>)).
 
 user_get_url_too_large_size(Config) ->
     escalus:fresh_story_with_config(Config, [{alice, 1}],
@@ -176,8 +204,7 @@ user_get_url_zero_timeout(Config) ->
 
 user_get_url_zero_timeout(Config, Alice) ->
     Result = user_get_url(<<"test">>, 123, <<"Test">>, 0, Alice, Config),
-    ?assertEqual(<<"timeout_error">>, get_err_code(Result)),
-    ?assertEqual(<<"timeout must be positive integer">>, get_err_msg(Result)).
+    ?assertMatch({_, _}, binary:match(get_coercion_err_msg(Result), <<"Value is not a positive integer">>)).
 
 user_http_upload_not_configured(Config) ->
     escalus:fresh_story_with_config(Config, [{alice, 1}],
@@ -197,14 +224,28 @@ admin_get_url_test(Config) ->
 admin_get_url_test(Config, Domain) ->
     Result = admin_get_url(Domain, <<"test">>, 123, <<"Test">>, 123, Config),
     ParsedResult = get_ok_value([data, httpUpload, getUrl], Result),
-    #{<<"PutUrl">> := PutURL, <<"GetUrl">> := GetURL, <<"Header">> := _Headers} = ParsedResult,
+    #{<<"putUrl">> := PutURL, <<"getUrl">> := GetURL, <<"headers">> := _Headers} = ParsedResult,
+    ?assertMatch({_, _}, binary:match(PutURL, [?S3_HOSTNAME])),
+    ?assertMatch({_, _}, binary:match(GetURL, [?S3_HOSTNAME])).
+
+admin_get_url_test_no_content_type(Config) ->
+    admin_get_url_test_no_content_type(Config, null),
+    admin_get_url_test_no_content_type(Config, <<"">>).
+
+admin_get_url_test_no_content_type(Config, ContentType) ->
+    Result = admin_get_url(domain(), <<"test">>, 123, ContentType, 123, Config),
+    ParsedResult = get_ok_value([data, httpUpload, getUrl], Result),
+    #{<<"putUrl">> := PutURL, <<"getUrl">> := GetURL, <<"headers">> := _Headers} = ParsedResult,
     ?assertMatch({_, _}, binary:match(PutURL, [?S3_HOSTNAME])),
     ?assertMatch({_, _}, binary:match(GetURL, [?S3_HOSTNAME])).
 
+admin_get_url_test_empty_filename(Config) ->
+    Result = admin_get_url(domain(), <<"">>, 123, <<"Test">>, 123, Config),
+    ?assertMatch({_, _}, binary:match(get_coercion_err_msg(Result), <<"Given string is empty">>)).
+
 admin_get_url_zero_size(Config) ->
     Result = admin_get_url(domain(), <<"test">>, 0, <<"Test">>, 123, Config),
-    ?assertEqual(<<"size_error">>, get_err_code(Result)),
-    ?assertEqual(<<"size must be positive integer">>, get_err_msg(Result)).
+    ?assertMatch({_, _}, binary:match(get_coercion_err_msg(Result), <<"Value is not a positive integer">>)).
 
 admin_get_url_too_large_size(Config) ->
     Result = admin_get_url(domain(), <<"test">>, 100000, <<"Test">>, 123, Config),
@@ -213,8 +254,7 @@ admin_get_url_too_large_size(Config) ->
 
 admin_get_url_zero_timeout(Config) ->
     Result = admin_get_url(domain(), <<"test">>, 123, <<"Test">>, 0, Config),
-    ?assertEqual(<<"timeout_error">>, get_err_code(Result)),
-    ?assertEqual(<<"timeout must be positive integer">>, get_err_msg(Result)).
+    ?assertMatch({_, _}, binary:match(get_coercion_err_msg(Result), <<"Value is not a positive integer">>)).
 
 admin_get_url_no_domain(Config) ->
     Result = admin_get_url(<<"AAAAA">>, <<"test">>, 123, <<"Test">>, 123, Config),

diff --git a/priv/graphql/schemas/admin/http_upload.gql b/priv/graphql/schemas/admin/http_upload.gql
@@ -3,6 +3,6 @@ Allow admin to generate upload/download URL for a file on user's behalf".
 """
 type HttpUploadAdminMutation @use(modules: ["mod_http_upload"]) @protected{
     "Allow admin to generate upload/download URLs for a file on user's behalf"
-    getUrl(domain: DomainName!, filename: String!, size: Int!, contentType: String!, timeout: Int!): FileUrls
+    getUrl(domain: DomainName!, filename: NonEmptyString!, size: PosInt!, contentType: String, timeout: PosInt!): FileUrls
       @use(arg: "domain") @protected(type: DOMAIN, args: ["domain"])
 }
diff --git a/priv/graphql/schemas/global/http_upload.gql b/priv/graphql/schemas/global/http_upload.gql
@@ -3,9 +3,19 @@ Type containing put url and get url for the file
 """
 type FileUrls {
     "Url to put the file"
-    PutUrl: String
+    putUrl: String
     "Url to get the file"
-    GetUrl: String
+    getUrl: String
     "Http headers"
-    Header: String
+    headers: [Header]
+}
+
+"""
+Type containing a HTTP header
+"""
+type Header {
+    "Name"
+    name: String
+    "Value"
+    value: String
 }
diff --git a/priv/graphql/schemas/user/http_upload.gql b/priv/graphql/schemas/user/http_upload.gql
@@ -3,5 +3,5 @@ Allow user to generate upload/download URL for a file".
 """
 type HttpUploadUserMutation @use(modules: ["mod_http_upload"]) @protected{
     "Allow user to generate upload/download URLs for a file"
-    getUrl(filename: String!, size: Int!, contentType: String!, timeout: Int!): FileUrls @use
+    getUrl(filename: NonEmptyString!, size: PosInt!, contentType: String, timeout: PosInt!): FileUrls @use
 }
diff --git a/src/http_upload/mod_http_upload_api.erl b/src/http_upload/mod_http_upload_api.erl
@@ -9,56 +9,49 @@
                 {ok | error, string()}.
 get_urls_mongooseimctl(Domain, Filename, Size, ContentType, Timeout) ->
     case get_urls(Domain, Filename, Size, ContentType, Timeout) of
-        {ok, #{<<"PutUrl">> := PutURL, <<"GetUrl">> := GetURL, <<"Header">> := Header}} ->
-            {ok, generate_output_message(PutURL, GetURL, Header)};
+        {ok, #{<<"putUrl">> := PutURL, <<"getUrl">> := GetURL, <<"Header">> := _Headers}} ->
+            {ok, generate_output_message(PutURL, GetURL)};
         {_, Message} ->
             {error, Message}
     end.
 
--spec get_urls(Domain :: jid:lserver(), Filename :: binary(), Size :: pos_integer(),
-               ContentType :: binary() | undefined, Timeout :: pos_integer()) ->
-        {ok | size_error | timeout_error | module_not_loaded_error | domain_not_found |
-         file_too_large_error, string()} | {ok, #{binary() => binary() | string()}}.
-get_urls(_Domain, _Filename, Size, _ContentType, _Timeout) when Size =< 0 ->
-    {size_error, "size must be positive integer"};
-get_urls(_Domain, _Filename, _Size, _ContentType, Timeout) when Timeout =< 0 ->
-    {timeout_error, "timeout must be positive integer"};
-get_urls(Domain, Filename, Size, <<>>, Timeout) ->
-    get_urls(Domain, Filename, Size, undefined, Timeout);
+-spec get_urls(Domain :: jid:lserver(), Filename :: nonempty_binary(), Size :: pos_integer(),
+               ContentType :: binary() | null | undefined, Timeout :: pos_integer()) ->
+        {ok, #{binary() => term()}}
+        | {size_error | timeout_error | module_not_loaded_error | domain_not_found |
+           file_too_large_error, string()}.
 get_urls(Domain, Filename, Size, ContentType, Timeout) ->
+    ContentType1 = content_type(ContentType),
     case mongoose_domain_api:get_domain_host_type(Domain) of
         {ok, HostType} ->
-            check_module_and_get_urls(HostType, Filename, Size, ContentType, Timeout);
+            check_module_and_get_urls(HostType, Filename, Size, ContentType1, Timeout);
         _ ->
             {domain_not_found, "domain does not exist"}
     end.
 
+content_type(null) -> <<>>;
+content_type(undefined) -> <<>>;
+content_type(Binary) -> Binary.
+
 check_module_and_get_urls(HostType, Filename, Size, ContentType, Timeout) ->
-    %The check if the module is loaded is needed by one test in mongooseimctl_SUITE
-    case gen_mod:is_loaded(HostType, mod_http_upload) of
-        true ->
-            case mod_http_upload:get_urls(HostType, Filename, Size, ContentType, Timeout) of
-                {PutURL, GetURL, Header} ->
-                    {ok, #{<<"PutUrl">> => PutURL, <<"GetUrl">> => GetURL,
-                           <<"Header">> => header_output(Header)}};
-                file_too_large_error ->
-                    {file_too_large_error,
-                     "Declared file size exceeds the host's maximum file size."}
-            end;
-        false ->
-            {module_not_loaded_error, "mod_http_upload is not loaded for this host"}
+    case mod_http_upload:get_urls(HostType, Filename, Size, ContentType, Timeout) of
+        {PutURL, GetURL, Headers} ->
+            {ok, #{<<"putUrl">> => PutURL, <<"getUrl">> => GetURL,
+                   <<"headers">> => headers_output(Headers)}};
+        file_too_large_error ->
+            {file_too_large_error,
+                "Declared file size exceeds the host's maximum file size."}
     end.
 
--spec generate_output_message(PutURL :: binary(), GetURL :: binary(),
-                              Header :: string()) -> string().
-generate_output_message(PutURL, GetURL, Header) ->
+-spec generate_output_message(PutURL :: binary(), GetURL :: binary()) -> string().
+generate_output_message(PutURL, GetURL) ->
     PutURLOutput = url_output("PutURL:", PutURL),
     GetURLOutput = url_output("GetURL:", GetURL),
-    lists:flatten([PutURLOutput, GetURLOutput, Header]).
+    lists:flatten([PutURLOutput, GetURLOutput]).
 
 url_output(Name, Url) ->
     io_lib:format("~s ~s~n", [Name, Url]).
 
-header_output(Header) when Header =:= #{} -> [];
-header_output(Header) ->
-    io_lib:format("Header: ~p~n", [maps:to_list(Header)]).
+headers_output(Headers) when Headers =:= #{} -> [];
+headers_output(Headers) ->
+    lists:map(fun({Key, Value}) -> {ok, #{<<"name">> => Key, <<"value">> => Value}} end, maps:to_list(Headers)).