Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mam/report wrong stanza id more friendly #3591

Merged
merged 2 commits into from
Mar 15, 2022
Merged

Mam/report wrong stanza id more friendly #3591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0202be7
Improve the error checking in tests
NelsonVides Mar 15, 2022
2f6b47e
Report more friendly errors on invalid stanza id
NelsonVides Mar 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions big_tests/tests/mam_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
pagination_offset5_opt_count_all/1,
server_returns_item_not_found_for_before_filter_with_nonexistent_id/1,
server_returns_item_not_found_for_after_filter_with_nonexistent_id/1,
server_returns_item_not_found_for_after_filter_with_invalid_id/1,
%% complete_flag_cases tests
before_complete_false_last5/1,
before_complete_false_before10/1,
Expand Down Expand Up @@ -500,7 +501,8 @@ rsm_cases() ->
pagination_offset5_opt_count_all,
%% item_not_found response for nonexistent message ID in before/after filters
server_returns_item_not_found_for_before_filter_with_nonexistent_id,
server_returns_item_not_found_for_after_filter_with_nonexistent_id].
server_returns_item_not_found_for_after_filter_with_nonexistent_id,
server_returns_item_not_found_for_after_filter_with_invalid_id].

complete_flag_cases() ->
[before_complete_false_last5,
Expand Down Expand Up @@ -2577,22 +2579,31 @@ server_returns_item_not_found_for_before_filter_with_nonexistent_id(Config) ->
NonexistentID = <<"AV25E9SCO50K">>,
RSM = #rsm_in{max = 5, direction = 'before', id = NonexistentID},
StanzaID = <<"before-nonexistent-id">>,
server_returns_item_not_found_for_nonexistent_id(Config, RSM, StanzaID).
Condition = [<<"cancel">>, <<"item-not-found">>],
server_returns_item_not_found_for_nonexistent_id(Config, RSM, StanzaID, Condition).

server_returns_item_not_found_for_after_filter_with_nonexistent_id(Config) ->
NonexistentID = <<"AV25E9SCO50K">>,
RSM = #rsm_in{max = 5, direction = 'after', id = NonexistentID},
StanzaID = <<"after-nonexistent-id">>,
server_returns_item_not_found_for_nonexistent_id(Config, RSM, StanzaID).
Condition = [<<"cancel">>, <<"item-not-found">>],
server_returns_item_not_found_for_nonexistent_id(Config, RSM, StanzaID, Condition).

server_returns_item_not_found_for_nonexistent_id(Config, RSM, StanzaID) ->
server_returns_item_not_found_for_after_filter_with_invalid_id(Config) ->
NonexistentID = <<"bef3a242-99ce-402a-9ffc-2f3c20da92d4">>,
RSM = #rsm_in{max = 5, direction = 'after', from_id = NonexistentID},
StanzaID = <<"AV25E9SCO50K">>,
Condition = [<<"modify">>, <<"not-acceptable">>],
server_returns_item_not_found_for_nonexistent_id(Config, RSM, StanzaID, Condition).

server_returns_item_not_found_for_nonexistent_id(Config, RSM, StanzaID, Condition) ->
P = ?config(props, Config),
F = fun(Alice) ->
rsm_send(Config, Alice,
stanza_page_archive_request(P, StanzaID, RSM)),
IQ = stanza_page_archive_request(P, StanzaID, RSM),
rsm_send(Config, Alice, IQ),
Res = escalus:wait_for_stanza(Alice),
escalus:assert(is_iq_error, Res),
escalus:assert(is_error, [<<"cancel">>, <<"item-not-found">>], Res),
escalus:assert(is_iq_error, [IQ], Res),
escalus:assert(is_error, Condition, Res),
ok
end,
parallel_story(Config, [{alice, 1}], F).
Expand Down
30 changes: 25 additions & 5 deletions src/mam/mod_mam.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@
-include("mongoose.hrl").
-include("jlib.hrl").
-include("amp.hrl").
-include_lib("exml/include/exml.hrl").
-include("mod_mam.hrl").

%% ----------------------------------------------------------------------
Expand Down Expand Up @@ -432,13 +431,29 @@ handle_get_prefs_result({error, Reason}, IQ) ->
-spec handle_set_message_form(From :: jid:jid(), ArcJID :: jid:jid(),
IQ :: jlib:iq(), Acc :: mongoose_acc:t()) ->
jlib:iq() | ignore | {error, term(), jlib:iq()}.
handle_set_message_form(#jid{} = From, #jid{} = ArcJID,
#iq{xmlns=MamNs, sub_el = QueryEl} = IQ,
Acc) ->
handle_set_message_form(#jid{} = From, #jid{} = ArcJID, #iq{} = IQ, Acc) ->
HostType = acc_to_host_type(Acc),
ArcID = archive_id_int(HostType, ArcJID),
try iq_to_lookup_params(HostType, IQ) of
Params0 ->
do_handle_set_message_form(Params0, From, ArcID, ArcJID, IQ, HostType)
catch _C:R:S ->
report_issue({R, S}, mam_lookup_failed, ArcJID, IQ),
return_error_iq(IQ, R)
end.


-spec do_handle_set_message_form(Params :: mam_iq:lookup_params(),
From :: jid:jid(),
ArcId :: archive_id(),
ArcJID :: jid:jid(),
IQ :: jlib:iq(),
HostType :: mongooseim:host_type()) ->
jlib:iq() | ignore | {error, term(), jlib:iq()}.
do_handle_set_message_form(Params0, From, ArcID, ArcJID,
#iq{xmlns=MamNs, sub_el = QueryEl} = IQ,
HostType) ->
QueryID = exml_query:attr(QueryEl, <<"queryid">>, <<>>),
Params0 = iq_to_lookup_params(HostType, IQ),
Params = mam_iq:lookup_params_with_archive_details(Params0, ArcID, ArcJID, From),
case lookup_messages(HostType, Params) of
{error, Reason} ->
Expand Down Expand Up @@ -683,6 +698,9 @@ return_error_iq(IQ, {Reason, {stacktrace, _Stacktrace}}) ->
return_error_iq(IQ, timeout) ->
E = mongoose_xmpp_errors:service_unavailable(<<"en">>, <<"Timeout">>),
{error, timeout, IQ#iq{type = error, sub_el = [E]}};
return_error_iq(IQ, invalid_stanza_id) ->
Text = mongoose_xmpp_errors:not_acceptable(<<"en">>, <<"Invalid stanza id provided">>),
{error, invalid_stanza_id, IQ#iq{type = error, sub_el = [Text]}};
return_error_iq(IQ, item_not_found) ->
{error, item_not_found, IQ#iq{type = error, sub_el = [mongoose_xmpp_errors:item_not_found()]}};
return_error_iq(IQ, not_implemented) ->
Expand All @@ -698,6 +716,8 @@ report_issue({Reason, {stacktrace, Stacktrace}}, Issue, ArcJID, IQ) ->
report_issue(Reason, Issue, ArcJID, IQ) ->
report_issue(Reason, [], Issue, ArcJID, IQ).

report_issue(invalid_stanza_id, _Stacktrace, _Issue, _ArcJID, _IQ) ->
expected;
report_issue(item_not_found, _Stacktrace, _Issue, _ArcJID, _IQ) ->
expected;
report_issue(not_implemented, _Stacktrace, _Issue, _ArcJID, _IQ) ->
Expand Down
24 changes: 22 additions & 2 deletions src/mam/mod_mam_muc.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@
result_set/4,
result_query/2,
result_prefs/4,
make_fin_message/5,
make_fin_element/7,
parse_prefs/1,
borders_decode/1,
Expand Down Expand Up @@ -388,7 +387,23 @@ handle_set_message_form(HostType, #jid{} = From, #jid{} = ArcJID, IQ) ->
ResLimit = mod_mam_params:max_result_limit(?MODULE, HostType),
DefLimit = mod_mam_params:default_result_limit(?MODULE, HostType),
ExtMod = mod_mam_params:extra_params_module(?MODULE, HostType),
Params0 = mam_iq:form_to_lookup_params(IQ, ResLimit, DefLimit, ExtMod),
try mam_iq:form_to_lookup_params(IQ, ResLimit, DefLimit, ExtMod) of
Params0 ->
do_handle_set_message_form(HostType, From, ArcID, ArcJID, IQ, Params0)
catch _C:R:S ->
report_issue({R, S}, mam_lookup_failed, ArcJID, IQ),
return_error_iq(IQ, R)
end.


-spec do_handle_set_message_form(HostType :: mongooseim:host_type(),
From :: jid:jid(),
ArcId :: mod_mam:archive_id(),
ArcJID :: jid:jid(),
IQ :: jlib:iq(),
Params :: mam_iq:lookup_params()) ->
jlib:iq() | ignore | {error, term(), jlib:iq()}.
do_handle_set_message_form(HostType, From, ArcID, ArcJID, IQ, Params0) ->
Params = mam_iq:lookup_params_with_archive_details(Params0, ArcID, ArcJID, From),
Result = lookup_messages(HostType, Params),
handle_lookup_result(Result, HostType, From, IQ, Params).
Expand Down Expand Up @@ -566,6 +581,9 @@ return_error_iq(IQ, {Reason, {stacktrace, _Stacktrace}}) ->
return_error_iq(IQ, Reason);
return_error_iq(IQ, timeout) ->
{error, timeout, IQ#iq{type = error, sub_el = [mongoose_xmpp_errors:service_unavailable(<<"en">>, <<"Timeout in mod_mam_muc">>)]}};
return_error_iq(IQ, invalid_stanza_id) ->
Text = mongoose_xmpp_errors:not_acceptable(<<"en">>, <<"Invalid stanza id provided">>),
{error, invalid_stanza_id, IQ#iq{type = error, sub_el = [Text]}};
return_error_iq(IQ, item_not_found) ->
{error, item_not_found, IQ#iq{type = error, sub_el = [mongoose_xmpp_errors:item_not_found()]}};
return_error_iq(IQ, not_implemented) ->
Expand Down Expand Up @@ -600,6 +618,8 @@ report_issue({Reason, {stacktrace, Stacktrace}}, Issue, ArcJID, IQ) ->
report_issue(Reason, Issue, ArcJID, IQ) ->
report_issue(Reason, [], Issue, ArcJID, IQ).

report_issue(invalid_stanza_id, _Stacktrace, _Issue, _ArcJID, _IQ) ->
expected;
report_issue(item_not_found, _Stacktrace, _Issue, _ArcJID, _IQ) ->
expected;
report_issue(missing_with_jid, _Stacktrace, _Issue, _ArcJID, _IQ) ->
Expand Down
4 changes: 3 additions & 1 deletion src/mam/mod_mam_utils.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,9 @@ maybe_external_binary_to_mess_id(BExtMessID) ->
%% @doc Decode a message ID received from the user.
-spec external_binary_to_mess_id(binary()) -> integer().
external_binary_to_mess_id(BExtMessID) when is_binary(BExtMessID) ->
binary_to_integer(BExtMessID, 32).
try binary_to_integer(BExtMessID, 32)
catch error:badarg -> throw(invalid_stanza_id)
end.

%% -----------------------------------------------------------------------
%% XML
Expand Down