From ca319ad7e0a9ad7a79211dcbdce86f167220c752 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20D=C5=82ugosz?= Date: Wed, 21 Jun 2023 13:37:57 +0200 Subject: [PATCH 1/5] Refactor adhoc:produce_response/1 function --- src/adhoc.erl | 57 +++++++++++++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 24 deletions(-) diff --git a/src/adhoc.erl b/src/adhoc.erl index bd227aee57..ca1d9c0466 100644 --- a/src/adhoc.erl +++ b/src/adhoc.erl @@ -105,33 +105,42 @@ produce_response(#adhoc_response{lang = _Lang, actions = Actions, notes = Notes, elements = Elements}) -> - SessionID = if is_binary(ProvidedSessionID), ProvidedSessionID /= <<"">> -> - ProvidedSessionID; - true -> - USec = os:system_time(microsecond), - TS = calendar:system_time_to_rfc3339(USec, [{offset, "Z"}, - {unit, microsecond}]), - list_to_binary(TS) - end, - ActionsEls = case Actions of - [] -> - []; - _ -> - ActionsElAttrs = case DefaultAction of - <<"">> -> []; - _ -> [{<<"execute">>, DefaultAction}] - end, - [#xmlel{name = <<"actions">>, attrs = ActionsElAttrs, - children = [#xmlel{name = Action} || Action <- Actions]}] - end, - NotesEls = lists:map(fun({Type, Text}) -> - #xmlel{name = <<"note">>, - attrs = [{<<"type">>, Type}], - children = [#xmlcdata{content = Text}]} - end, Notes), + SessionID = ensure_correct_session_id(ProvidedSessionID), + ActionsEls = maybe_actions_element(Actions, DefaultAction), + NotesEls = lists:map(fun note_to_xmlel/1, Notes), #xmlel{name = <<"command">>, attrs = [{<<"xmlns">>, ?NS_COMMANDS}, {<<"sessionid">>, SessionID}, {<<"node">>, Node}, {<<"status">>, list_to_binary(atom_to_list(Status))}], children = ActionsEls ++ NotesEls ++ Elements}. + +-spec ensure_correct_session_id(binary()) -> binary(). +ensure_correct_session_id(SessionID) when is_binary(SessionID), SessionID /= <<"">> -> + SessionID; +ensure_correct_session_id(_) -> + USec = os:system_time(microsecond), + TS = calendar:system_time_to_rfc3339(USec, [{offset, "Z"}, {unit, microsecond}]), + list_to_binary(TS). + +-spec maybe_actions_element([binary()], binary()) -> [exml:element()]. +maybe_actions_element([], _DefaultAction) -> + []; +maybe_actions_element(Actions, DefaultAction) -> + ActionsElAttrs = case DefaultAction of + <<"">> -> []; + _ -> [{<<"execute">>, DefaultAction}] + end, + [#xmlel{ + name = <<"actions">>, + attrs = ActionsElAttrs, + children = [#xmlel{name = Action} || Action <- Actions] + }]. + +-spec note_to_xmlel({binary(), iodata()}) -> exml:element(). +note_to_xmlel({Type, Text}) -> + #xmlel{ + name = <<"note">>, + attrs = [{<<"type">>, Type}], + children = [#xmlcdata{content = Text}] + }. \ No newline at end of file From 06c5b94570e6ce0cbdf7e9876c348aff34c30c50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20D=C5=82ugosz?= Date: Wed, 21 Jun 2023 13:41:00 +0200 Subject: [PATCH 2/5] Update XEP-0050: Ad-Hoc Commands to version 1.3.0 --- src/adhoc.erl | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/src/adhoc.erl b/src/adhoc.erl index ca1d9c0466..c0f6da7f9b 100644 --- a/src/adhoc.erl +++ b/src/adhoc.erl @@ -25,7 +25,7 @@ -module(adhoc). -author('henoch@dtek.chalmers.se'). --xep([{xep, 50}, {version, "1.2"}]). +-xep([{xep, 50}, {version, "1.3"}]). -export([parse_request/1, produce_response/2, produce_response/4, @@ -126,21 +126,34 @@ ensure_correct_session_id(_) -> -spec maybe_actions_element([binary()], binary()) -> [exml:element()]. maybe_actions_element([], _DefaultAction) -> []; +maybe_actions_element(Actions, <<>>) -> + % If the "execute" attribute is absent, it defaults to "next". + AllActions = ensure_default_action_present(Actions, <<"next">>), + [#xmlel{ + name = <<"actions">>, + children = [#xmlel{name = Action} || Action <- AllActions] + }]; maybe_actions_element(Actions, DefaultAction) -> - ActionsElAttrs = case DefaultAction of - <<"">> -> []; - _ -> [{<<"execute">>, DefaultAction}] - end, + % A form which has an element and an "execute" attribute + % which evaluates to an action which is not allowed is invalid. + AllActions = ensure_default_action_present(Actions, DefaultAction), [#xmlel{ name = <<"actions">>, - attrs = ActionsElAttrs, - children = [#xmlel{name = Action} || Action <- Actions] + attrs = [{<<"execute">>, DefaultAction}], + children = [#xmlel{name = Action} || Action <- AllActions] }]. +-spec ensure_default_action_present([binary()], binary()) -> [exml:element()]. +ensure_default_action_present(Actions, DefaultAction) -> + case lists:member(Actions, DefaultAction) of + true -> Actions; + false -> [DefaultAction | Actions] + end. + -spec note_to_xmlel({binary(), iodata()}) -> exml:element(). note_to_xmlel({Type, Text}) -> #xmlel{ name = <<"note">>, attrs = [{<<"type">>, Type}], children = [#xmlcdata{content = Text}] - }. \ No newline at end of file + }. From 3b067e945635894417fab514cc7619492cfe7545 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20D=C5=82ugosz?= Date: Mon, 26 Jun 2023 12:27:32 +0200 Subject: [PATCH 3/5] Add test suite for adhoc module --- test/adhoc_SUITE.erl | 230 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 230 insertions(+) create mode 100644 test/adhoc_SUITE.erl diff --git a/test/adhoc_SUITE.erl b/test/adhoc_SUITE.erl new file mode 100644 index 0000000000..df7aa86c18 --- /dev/null +++ b/test/adhoc_SUITE.erl @@ -0,0 +1,230 @@ +%% @doc This suite tests API of adhoc module which implements XEP-0050: Ad-Hoc Commands +-module(adhoc_SUITE). +-compile([export_all, nowarn_export_all]). + +-include_lib("exml/include/exml.hrl"). +-include_lib("eunit/include/eunit.hrl"). +-include("jlib.hrl"). +-include("adhoc.hrl"). + +%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% +%%%% Suite configuration +%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% + +all() -> + [ + {group, basic} + ]. + +groups() -> + [ + {basic, [parallel], [ + parse_correct_request_with_form, + parse_correct_request_without_form, + parse_incorrect_request_wrong_type, + parse_incorrect_request_wrong_namespace, + produce_response_full, + produce_response_no_session_id, + produce_response_no_actions, + produce_response_no_default_action, + produce_response_default_action_not_present + ]} + ]. + +%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% +%%%% Tests +%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% + +parse_correct_request_with_form(_C) -> + parse_correct_request(sample_form()). + +parse_correct_request_without_form(_C) -> + parse_correct_request(false). + +parse_correct_request(Form) -> + % given + IqRequest = + case Form of + false -> sample_request(); + _ -> sample_request([Form]) + end, + % when + #adhoc_request{ + lang = Lang, + node = Node, + session_id = SessionID, + action = Action, + xdata = XData, + others = Others + } = adhoc:parse_request(IqRequest), + % then + ?assertEqual(Lang, <<"en-us">>), + ?assertEqual(Node, <<"node_name">>), + ?assertEqual(SessionID, <<"1">>), + ?assertEqual(Action, <<"execute">>), + ?assertEqual(XData, Form), + ?assertEqual(Others, [#xmlel{name = <<"test">>}]). + +parse_incorrect_request_wrong_type(_C) -> + % given + IqRequest = (sample_request())#iq{type = get}, + % when + {error, Error} = adhoc:parse_request(IqRequest), + % then + ?assert(is_bad_request(Error)). + +parse_incorrect_request_wrong_namespace(_C) -> + % given + IqRequest = (sample_request())#iq{xmlns = <<"wrong_namespace">>}, + % when + {error, Error} = adhoc:parse_request(IqRequest), + % then + ?assert(is_bad_request(Error)). + +produce_response_full(_C) -> + % given + Actions = [<<"next">>, <<"complete">>], + Notes = [{<<"info">>, <<"Information message.">>}], + AdditionalElements = [sample_form()], + AdhocResponse = #adhoc_response{ + node = <<"node_name">>, + session_id = <<"1234">>, + status = executing, + default_action = <<"next">>, + actions = Actions, + notes = Notes, + elements = AdditionalElements + }, + ExpectedActionsEls = [#xmlel{ + name = <<"actions">>, + attrs = [{<<"execute">>, <<"next">>}], + children = [#xmlel{name = Action} || Action <- Actions] + }], + ExpectedNotesEls = [ + #xmlel{ + name = <<"note">>, + attrs = [{<<"type">>, Type}], + children = [#xmlcdata{content = Text}] + } + || {Type, Text} <- Notes + ], + ExpectedChildren = ExpectedActionsEls ++ ExpectedNotesEls ++ AdditionalElements, + % when + #xmlel{ + name = <<"command">>, + attrs = Attrs, + children = Children + } = adhoc:produce_response(AdhocResponse), + % then + ?assertEqual(<<"1234">>, proplists:get_value(<<"sessionid">>, Attrs)), + ?assertEqual(<<"node_name">>, proplists:get_value(<<"node">>, Attrs)), + ?assertEqual(<<"executing">>, proplists:get_value(<<"status">>, Attrs)), + ?assertEqual(lists:sort(ExpectedChildren), lists:sort(Children)). + +produce_response_no_session_id(_C) -> + % given + AdhocResponse = #adhoc_response{ + session_id = <<"">> + }, + % when + #xmlel{ + name = <<"command">>, + attrs = Attrs + } = adhoc:produce_response(AdhocResponse), + % then + SessionID = proplists:get_value(<<"sessionid">>, Attrs), + ?assert(is_binary(SessionID)), + ?assertNotEqual(<<"">>, SessionID). + +produce_response_no_actions(_C) -> + % given + AdhocResponse = #adhoc_response{ + actions = [] + }, + % when + #xmlel{ + name = <<"command">>, + children = Children + } = adhoc:produce_response(AdhocResponse), + % then + ?assertEqual([], Children). + +produce_response_no_default_action(_C) -> + % given + Actions = [<<"complete">>], + AdhocResponse = #adhoc_response{ + actions = Actions + }, + ExpectedActionsEls = [#xmlel{ + name = <<"actions">>, + attrs = [], + children = [#xmlel{name = Action} || Action <- [<<"next">> | Actions]] + }], + % when + #xmlel{ + name = <<"command">>, + children = Children + } = adhoc:produce_response(AdhocResponse), + % then + ?assertEqual(lists:sort(ExpectedActionsEls), lists:sort(Children)). + +produce_response_default_action_not_present(_C) -> + % given + Actions = [<<"complete">>], + AdhocResponse = #adhoc_response{ + default_action = <<"prev">>, + actions = Actions + }, + ExpectedActionsEls = [#xmlel{ + name = <<"actions">>, + attrs = [{<<"execute">>, <<"prev">>}], + children = [#xmlel{name = Action} || Action <- [<<"prev">> | Actions]] + }], + % when + #xmlel{ + name = <<"command">>, + children = Children + } = adhoc:produce_response(AdhocResponse), + % then + ?assertEqual(lists:sort(ExpectedActionsEls), lists:sort(Children)). + +%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% +%%%% Helpers +%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% + +sample_form() -> + #xmlel{ + name = <<"x">>, + attrs = [ + {<<"xmlns">>, <<"jabber:x:data">>}, + {<<"type">>, <<"form">>} + ], + children = [] + }. + +sample_request() -> + sample_request([]). + +sample_request(MaybeForm) -> + #iq{ + type = set, + lang = <<"en-us">>, + sub_el = #xmlel{ + attrs = [ + {<<"node">>, <<"node_name">>}, + {<<"sessionid">>, <<"1">>}, + {<<"action">>, <<"execute">>} + ], + children = [#xmlel{name = <<"test">>}] ++ MaybeForm + }, + xmlns = ?NS_COMMANDS + }. + +is_bad_request(#xmlel{ + name = <<"error">>, + attrs = [{<<"code">>, <<"400">>}, {<<"type">>, <<"modify">>}], + children = [#xmlel{name = <<"bad-request">>}] +}) -> + true; +is_bad_request(_) -> + false. From 15c31020b64ef3c7e9dfc23dfdcd4fcc2017d621 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20D=C5=82ugosz?= Date: Mon, 26 Jun 2023 12:29:52 +0200 Subject: [PATCH 4/5] Code review follow-up --- src/adhoc.erl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/adhoc.erl b/src/adhoc.erl index c0f6da7f9b..15af848d26 100644 --- a/src/adhoc.erl +++ b/src/adhoc.erl @@ -116,7 +116,7 @@ produce_response(#adhoc_response{lang = _Lang, children = ActionsEls ++ NotesEls ++ Elements}. -spec ensure_correct_session_id(binary()) -> binary(). -ensure_correct_session_id(SessionID) when is_binary(SessionID), SessionID /= <<"">> -> +ensure_correct_session_id(SessionID) when is_binary(SessionID), SessionID /= <<>> -> SessionID; ensure_correct_session_id(_) -> USec = os:system_time(microsecond), @@ -143,9 +143,9 @@ maybe_actions_element(Actions, DefaultAction) -> children = [#xmlel{name = Action} || Action <- AllActions] }]. --spec ensure_default_action_present([binary()], binary()) -> [exml:element()]. +-spec ensure_default_action_present([binary()], binary()) -> [binary()]. ensure_default_action_present(Actions, DefaultAction) -> - case lists:member(Actions, DefaultAction) of + case lists:member(DefaultAction, Actions) of true -> Actions; false -> [DefaultAction | Actions] end. From a14fb58bba043095ccefafcd8c3448f8df1e597c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20D=C5=82ugosz?= <37941643+pawlooss1@users.noreply.github.com> Date: Tue, 27 Jun 2023 17:04:58 +0200 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: JanuszJakubiec <54719269+JanuszJakubiec@users.noreply.github.com> --- test/adhoc_SUITE.erl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/adhoc_SUITE.erl b/test/adhoc_SUITE.erl index df7aa86c18..bd72cdedba 100644 --- a/test/adhoc_SUITE.erl +++ b/test/adhoc_SUITE.erl @@ -124,7 +124,7 @@ produce_response_full(_C) -> produce_response_no_session_id(_C) -> % given AdhocResponse = #adhoc_response{ - session_id = <<"">> + session_id = <<>> }, % when #xmlel{ @@ -134,7 +134,7 @@ produce_response_no_session_id(_C) -> % then SessionID = proplists:get_value(<<"sessionid">>, Attrs), ?assert(is_binary(SessionID)), - ?assertNotEqual(<<"">>, SessionID). + ?assertNotEqual(<<>>, SessionID). produce_response_no_actions(_C) -> % given