Skip to content

Commit

Permalink
KAZOO-5784: Improve attachment handler error handling (2600hz#4575)
Browse files Browse the repository at this point in the history
* KAZOO-5784: Add verbosity for error_response

* KAZOO-5784: wip

* KAZOO-5784: Improve onedrive and s3 attachment handlers

* KAZOO-5784: Get a more meaningful error reason in dropbox, gdrive, onedrive, and s3 attachment handlers

* KAZOO-5784: Finish attachment handlers error handling improvements

* KAZOO-5784: Use an modb document instead of storage plan document to check storage settings

* KAZOO-5784: Take care of `error_verbosity` option when creating (PUT) an attachment

* KAZOO-5784: Take care of `error_verbosity` option

* KAZOO-5784: Improve cb_storage:maybe_check_storage_settings tests using FixtureDB (WIP)

* KAZOO-5784: Fix issue within s3 attachments handler and add default_routines to kz_att_error module
harenson authored and k-anderson committed Feb 23, 2018
1 parent 1fb1e7b commit 3c88b40
Showing 21 changed files with 995 additions and 375 deletions.
32 changes: 22 additions & 10 deletions applications/crossbar/src/modules/cb_storage.erl
Original file line number Diff line number Diff line change
@@ -452,15 +452,24 @@ validate_attachment_settings_fold(AttId, Att, ContextAcc) ->
Random = kz_binary:rand_hex(16),
Content = <<"some random content: ", Random/binary>>,
AName = <<Random/binary, "_test_credentials_file.txt">>,
DbName = cb_context:account_db(ContextAcc),
DocId = doc_id(ContextAcc),
AccountId = cb_context:account_id(ContextAcc),
%% Create dummy document where the attachment(s) will be attached to.
%% TODO: move this tmpdoc creation to maybe_check_storage_settings function.
TmpDoc = kz_json:from_map(#{<<"att_uuid">> => AttId
,<<"pvt_type">> => <<"storage_settings_probe">>
}),
{ok, Doc} = kazoo_modb:save_doc(AccountId, TmpDoc),
DbName = kazoo_modb:get_modb(AccountId),
DocId = kz_json:get_value(<<"_id">>, Doc),
Handler = kz_json:get_ne_binary_value(<<"handler">>, Att),
Settings = kz_json:get_json_value(<<"settings">>, Att),
AttHandler = kz_term:to_atom(<<"kz_att_", Handler/binary>>, 'true'),
AttSettings = kz_maps:keys_to_atoms(kz_json:to_map(Settings)),
Opts = [{'plan_override', #{'att_handler' => {AttHandler, AttSettings}
,'att_post_handler' => 'external'
}}],
}}
,{'error_verbosity', 'verbose'}
],
%% Check the storage settings have permissions to create files
case kz_datamgr:put_attachment(DbName, DocId, AName, Content, Opts) of
{'ok', _CreatedDoc, _CreatedProps} ->
@@ -485,12 +494,12 @@ add_datamgr_error(AttId, Error, Context) ->
crossbar_doc:handle_datamgr_errors(Error, AttId, Context).

-spec add_att_settings_validation_error(kz_term:ne_binary()
,gen_attachment:error_response()
,kz_att_error:error()
,cb_context:context()
) -> cb_context:context().
add_att_settings_validation_error(AttId, ErrorResponse, Context) ->
ErrorCode = gen_attachment:error_code(ErrorResponse),
ErrorBody = gen_attachment:error_body(ErrorResponse),
add_att_settings_validation_error(AttId, {'error', Reason, ExtendedError}, Context) ->
ErrorCode = kz_att_error:resp_code(ExtendedError),
ErrorBody = kz_att_error:resp_body(ExtendedError),
EmptyJObj = kz_json:new(),
%% Some attachment handlers return a bitstring as the value for `ErrorBody` variable,
%% some other return an encoded JSON object which is also a binary value but
@@ -499,10 +508,13 @@ add_att_settings_validation_error(AttId, ErrorResponse, Context) ->
EmptyJObj -> ErrorBody;
DecodedErrorBody -> DecodedErrorBody
end,
Error = [{<<"error_code">>, ErrorCode}, {<<"error_body">>, NewErrorBody}],
Reason = kz_json:insert_values(Error, kz_json:new()),
Error = [{<<"error_code">>, ErrorCode}
,{<<"error_body">>, NewErrorBody}
,{<<"message">>, Reason}
],
ErrorObj = kz_json:insert_values(Error, kz_json:new()),
cb_context:add_validation_error([<<"attachments">>, AttId]
,<<"invalid">>
,Reason
,ErrorObj
,Context
).
5 changes: 3 additions & 2 deletions applications/crossbar/test/cb_storage_test.erl
Original file line number Diff line number Diff line change
@@ -7,7 +7,8 @@
%% Generators
%% =======================================================================================
maybe_check_storage_settings_test_() ->
Context = cb_context:new(), %% By default context.resp_status = error.
%% By default context.resp_status = error.
Context = cb_context:new(),
SuccessContext = cb_context:set_resp_status(Context, 'success'),

[{"Skip check if context's `resp_status /= success`"
@@ -22,7 +23,7 @@ maybe_check_storage_settings_test_() ->
].

%% =======================================================================================
%% Private functions
%% Helper functions
%% =======================================================================================
maybe_check(Context, HTTPVerb) ->
cb_storage:maybe_check_storage_settings(Context, HTTPVerb).
67 changes: 5 additions & 62 deletions core/kazoo_attachments/src/gen_attachment.erl
Original file line number Diff line number Diff line change
@@ -1,12 +1,5 @@
-module(gen_attachment).

%% Setter
-export([error_response/2]).
%% Getters
-export([error_code/1, error_body/1]).
%% Helper(s)
-export([is_error_response/1]).

-include("kz_att.hrl").

-type settings() :: kz_data:connection().
@@ -16,16 +9,12 @@
-type contents() :: kz_term:ne_binary().
-type options() :: kz_data:options().
-type handler_props() :: kz_data:connection().
-type error_code() :: pos_integer() | atom(). % 400, 404, 409, etc.
-type error_body() :: binary() | % encoded map()
bitstring() | % <<"example">>
atom(). % 'not_found' | 'return_id_missing' | etc.
-opaque error_response() :: {'gen_attachment_error', [{'error_code', error_code()} |
{'error_body', error_body()}
]}.

-type put_response() :: {'ok', [{atom(), [{binary(), binary() | kz_json:object()}]}]} |
error_response().
-type fetch_response() :: {'ok', iodata()} | error_response().
kz_att_error:error().

-type fetch_response() :: {'ok', iodata()} |
kz_att_error:error().

-export_type([settings/0
,db_name/0
@@ -34,18 +23,10 @@
,contents/0
,options/0
,handler_props/0
,error_response/0
,put_response/0
,fetch_response/0
]).

%% Note: Some functions have the `kz_datamgr:data_error()' type in its spec but it is not
%% being used, it is just to avoid dialyzer complaints because since `cb_storage'
%% mod is using `kazoo_attachments' app through `kz_datamgr' mod it (cb_storage) can
%% not get only opaque `error_response()' errors but also `data_error()' from
%% kz_datamgr and when you call ?MODULE:[error_code/1, error_body/1, is_error_response/1]
%% you might actually be sending a `data_error()' instead of a `error_response()' value.

%% =======================================================================================
%% Callbacks
%% =======================================================================================
@@ -62,41 +43,3 @@
,doc_id()
,att_name()
) -> fetch_response().

%% =======================================================================================
%% API
%% =======================================================================================
%% Setter
-spec error_response(error_code(), error_body()) -> error_response().
error_response(ErrorCode, ErrorBody) ->
{'gen_attachment_error', [{'error_code', ErrorCode}, {'error_body', ErrorBody}]}.

%% Getters
%% Read the note about `kz_datamgr:data_error()' after the types declaration
-spec error_code(error_response()) -> error_code() | 'undefined'.
error_code({'gen_attachment_error', Reason}) ->
props:get_value('error_code', Reason).

%% Read the note about `kz_datamgr:data_error()' after the types declaration
-spec error_body(error_response()) -> error_body() | 'undefined'.
error_body({'gen_attachment_error', Reason}) ->
props:get_value('error_body', Reason).

%% Helper(s)
%% Read the note about `kz_datamgr:data_error()' after the types declaration
-spec is_error_response(error_response() | kz_datamgr:data_error()) -> boolean().
is_error_response({'gen_attachment_error', ErrorProps}) when is_list(ErrorProps) ->
lists:all(fun check_error_response_element/1, ErrorProps);
is_error_response(_) ->
'false'.

-spec check_error_response_element(error_code() | error_body()) -> boolean().
check_error_response_element({'error_code', Code}) ->
is_atom(Code)
orelse is_integer(Code);
check_error_response_element({'error_body', Body}) ->
is_binary(Body)
orelse is_bitstring(Body)
orelse is_atom(Body);
check_error_response_element(_) ->
'false'.
47 changes: 31 additions & 16 deletions core/kazoo_attachments/src/kz_att_azure.erl
Original file line number Diff line number Diff line change
@@ -26,12 +26,16 @@
,gen_attachment:contents()
,gen_attachment:options()
) -> gen_attachment:put_response().
put_attachment(Settings, DbName, DocId, AName, Contents, _Options) ->
put_attachment(Settings, DbName, DocId, AName, Contents, Options) ->
CT = kz_mime:from_filename(AName),

{Container, Name} = resolve_path(Settings, {DbName, DocId, AName}),
Pid = azure_pid(Settings),
AzureOptions = [{content_type, kz_term:to_list(CT)}, return_headers],
Url = lists:concat([Container, "/", Name]),
Routines = [{fun kz_att_error:set_req_url/2, Url}
| kz_att_error:put_routines(Settings, DbName, DocId, AName, Contents, Options)
],
try
case erlazure:put_block_blob(Pid, Container, Name, Contents, AzureOptions) of
{ok, Headers, _Body} ->
@@ -42,21 +46,16 @@ put_attachment(Settings, DbName, DocId, AName, Contents, _Options) ->
,{<<"metadata">>, Metadata}
]}
]};
{'error', ErrorCode} = _E when is_atom(ErrorCode) -> % from erlazure:execute_request/2
lager:error("error storing attachment in azure : ~p", [_E]),
gen_attachment:error_response(ErrorCode, <<"Error storing attachment in Azure">>);
{'error', ErrorBody} = _E -> % from erlazure:execute_request/2
lager:error("error storing attachment in azure : ~p", [_E]),
gen_attachment:error_response(400, ErrorBody);
SomethingElse ->
lager:error("Failed to put the attachment to Azure: ~p", [SomethingElse]),
gen_attachment:error_response(400, <<"Something failed(?)">>)
Resp ->
handle_erlazure_error_response(Resp, Routines)
end
catch
_:{{{'case_clause', {'error', {'failed_connect', _}}}, _StackTrace}, _FnFailing} ->
%% Next line left just for reference (cause it's not easy to spot it in the code)
%%_:{{{'case_clause', {'error', {'failed_connect', _}}}, _StackTrace}, _FnFailing} ->
_:{{{'case_clause', ErrorResp}, _StackTrace}, _FnFailing} ->
lager:debug("Failed to put the attachment.~nStackTrace: ~p~nFnFailing: ~p",
[_StackTrace, _FnFailing]),
gen_attachment:error_response(400, <<"Failed to connect. Check your settings">>)
handle_erlazure_error_response(ErrorResp, Routines)
end.


@@ -66,16 +65,20 @@ put_attachment(Settings, DbName, DocId, AName, Contents, _Options) ->
,gen_attachment:att_name()
) -> gen_attachment:fetch_response().
fetch_attachment(HandlerProps, DbName, DocId, AName) ->
Routines = kz_att_error:fetch_routines(HandlerProps, DbName, DocId, AName),
case kz_json:get_value(<<"azure">>, HandlerProps) of
'undefined' ->
gen_attachment:error_response(400, 'invalid_data');
kz_att_error:new('invalid_data', Routines);
AzureData ->
{Settings, _ContentId} = binary_to_term(base64:decode(AzureData)),
Pid = azure_pid(Settings),
{Container, Name} = resolve_path(Settings, {DbName, DocId, AName}),
Pid = azure_pid(Settings),
case erlazure:get_blob(Pid, Container, Name) of
{'error', Reason} -> gen_attachment:error_response(400, Reason);
{'ok', _RespBody} = Resp -> Resp
{'ok', _RespBody} = Resp -> Resp;
Resp ->
Url = lists:concat([Container, '/', Name]),
NewRoutines = [{fun kz_att_error:set_req_url/2, Url} | Routines],
handle_erlazure_error_response(Resp, NewRoutines)
end
end.

@@ -111,3 +114,15 @@ azure_pid(Account, Key) ->
end;
Pid -> Pid
end.

-spec handle_erlazure_error_response({'error', string() | binary() | atom()},
kz_att_error:update_routines()) -> kz_att_error:error().
handle_erlazure_error_response({'error', {'failed_connect', _}} = _E, Routines) ->
lager:error("Azure request failed: ~p", [_E]),
kz_att_error:new('failed_to_connect', Routines);
handle_erlazure_error_response({'error', Reason} = _E, Routines) -> % from erlazure:execute_request/2
lager:error("Azure request failed : ~p", [_E]),
kz_att_error:new(Reason, Routines);
handle_erlazure_error_response(_E, Routines) ->
lager:error("Azure request failed: ~p", [_E]),
kz_att_error:new('request_error', Routines).
Loading

0 comments on commit 3c88b40

Please sign in to comment.