Find and parse refactor

src/event_pusher/mod_event_pusher_push.erl

@@ -236,16 +236,16 @@ parse_form(Form) ->
 
 -spec parse_form_fields(exml:element()) -> invalid_form | form().
 parse_form_fields(Form) ->
-    case mongoose_data_forms:form_to_kvs(Form, <<"submit">>) of
-        invalid ->
-            invalid_form;
-        KVs ->
-            ParsedKVs = lists:map(fun parse_kv/1, KVs),
+    case mongoose_data_forms:parse_form(Form) of
+        #{kvs := KVs, type := <<"submit">>} ->
+            ParsedKVs = lists:map(fun parse_kv/1, maps:to_list(KVs)),
             FormTypeKV = {<<"FORM_TYPE">>, ?NS_PUBSUB_PUB_OPTIONS},
             case lists:member(FormTypeKV, ParsedKVs) andalso not lists:member(invalid_field, ParsedKVs) of
                 true -> ParsedKVs -- [FormTypeKV];
                 false -> invalid_form
-            end
+            end;
+        _ ->
+            invalid_form
     end.
 
 parse_kv({K, [V]}) -> {K, V};

src/inbox/mod_inbox.erl

@@ -510,9 +510,9 @@ expand_limit(Max) ->
 form_to_params(_, undefined) ->
     #{ order => desc };
 form_to_params(HostType, FormEl) ->
-    ParsedFields = mongoose_data_forms:form_to_kvs(FormEl),
+    #{kvs := ParsedFields} = mongoose_data_forms:parse_form_fields(FormEl),
     ?LOG_DEBUG(#{what => inbox_parsed_form_fields, parsed_fields => ParsedFields}),
-    fields_to_params(HostType, ParsedFields, #{ order => desc }).
+    fields_to_params(HostType, maps:to_list(ParsedFields), #{ order => desc }).
 
 -spec fields_to_params(mongooseim:host_type(),
                        [{Var :: binary(), Values :: [binary()]}], Acc :: get_inbox_params()) ->
@@ -579,8 +579,6 @@ fields_to_params(HostType, [{<<"box">>, [Value]} | RFields], Acc) ->
             fields_to_params(HostType, RFields, Acc#{ box => Value })
     end;
 
-fields_to_params(HostType, [{<<"FORM_TYPE">>, _} | RFields], Acc) ->
-    fields_to_params(HostType, RFields, Acc);
 fields_to_params(_, [{Invalid, [InvalidFieldVal]} | _], _) ->
     ?LOG_WARNING(#{what => inbox_invalid_form_field, reason => unknown_field,
                    field => Invalid, value => InvalidFieldVal}),

src/mam/mam_iq.erl

@@ -148,13 +148,14 @@ form_to_lookup_params(#iq{sub_el = QueryEl} = IQ, MaxResultLimit, DefaultResultL
                is_simple => maybe_enforce_simple(KVs, EnforceSimple)},
     maybe_add_extra_lookup_params(Module, Params, IQ).
 
--spec query_to_map(exml:element()) -> mongoose_data_forms:kv_map() | invalid.
+-spec query_to_map(exml:element()) -> mongoose_data_forms:kv_map().
 query_to_map(QueryEl) ->
     case mongoose_data_forms:find_form(QueryEl) of
         undefined ->
             #{};
         Form ->
-            mongoose_data_forms:form_to_map(Form)
+            #{kvs := KVs} = mongoose_data_forms:parse_form_fields(Form),
+            KVs
     end.
 
 -spec common_lookup_params(exml:element(), non_neg_integer(), non_neg_integer()) ->

src/mam/mod_mam_utils.erl

@@ -637,9 +637,9 @@ borders(AfterID, BeforeID, FromID, ToID) ->
 
 -spec form_field_mess_id(mongoose_data_forms:kv_map(), binary()) -> 'undefined' | integer().
 form_field_mess_id(KVs, Name) ->
-    case maps:get(Name, KVs, undefined) of
-        [BExtMessID] -> external_binary_to_mess_id(BExtMessID);
-        undefined -> undefined
+    case KVs of
+        #{Name := [BExtMessID]} -> external_binary_to_mess_id(BExtMessID);
+        #{} -> undefined
     end.
 
 -spec form_decode_optimizations(mongoose_data_forms:kv_map()) -> boolean().

src/mod_caps.erl

@@ -596,24 +596,18 @@ concat_identities(Els) ->
 
 concat_info(Els) ->
     lists:sort(lists:flatmap(fun(El) ->
-                                     case mongoose_data_forms:is_form(El) of
-                                         true ->
-                                             KVs = mongoose_data_forms:form_to_kvs(El, <<"result">>),
-                                             [concat_xdata_fields(KVs)];
-                                         false ->
-                                             []
-                                     end
+                                     concat_xdata_fields(mongoose_data_forms:parse_form(El))
                              end,
                              Els)).
 
-concat_xdata_fields(KVs) ->
-    {FormType, Res} = lists:foldl(fun({<<"FORM_TYPE">>, [FormType]}, {_, VarFields}) ->
-                                          {FormType, VarFields};
-                                     ({Var, Values}, {FormType, VarFields}) ->
-                                          NewField = [[V, $<] || V <- [Var | lists:sort(Values)]],
-                                          {FormType, [NewField | VarFields]}
-                                  end, {<<>>, []}, KVs),
-    [FormType, $<, lists:sort(Res)].
+concat_xdata_fields(#{type := <<"result">>, kvs := KVs, ns := NS}) ->
+    Res = maps:fold(fun(Var, Values, VarFields) ->
+                            NewField = [[V, $<] || V <- [Var | lists:sort(Values)]],
+                            [NewField | VarFields]
+                    end, [], KVs),
+    [[NS, $<, lists:sort(Res)]];
+concat_xdata_fields(_) ->
+    [].
 
 gb_trees_fold(F, Acc, Tree) ->
     Iter = gb_trees:iterator(Tree),

src/mod_muc.erl

@@ -1137,42 +1137,31 @@ iq_set_unregister_info(HostType, MucHost, From, _Lang) ->
                               jid:jid(), exml:element(), ejabberd:lang())
             -> {'error', exml:element()} | {'result', []}.
 process_iq_register_set(HostType, MucHost, From, SubEl, Lang) ->
-    #xmlel{children = Els} = SubEl,
     case xml:get_subtag(SubEl, <<"remove">>) of
         false ->
-            case get_submitted_form(xml:remove_cdata(Els)) of
-                {ok, XEl} ->
-                    FormType = mongoose_data_forms:form_type(XEl),
-                    process_register(FormType, HostType, MucHost, From, Lang, XEl);
-                no_form ->
-                    {error, mongoose_xmpp_errors:bad_request()}
+            case mongoose_data_forms:find_and_parse_form(SubEl) of
+                #{type := <<"cancel">>} ->
+                    {result, []};
+                #{type := <<"submit">>, kvs := KVs} ->
+                    process_register(HostType, MucHost, From, Lang, KVs);
+                {error, Msg} ->
+                    {error, mongoose_xmpp_errors:bad_request(Lang, Msg)};
+                _ ->
+                    {error, mongoose_xmpp_errors:bad_request(Lang, <<"Invalid form type">>)}
             end;
         _ ->
             iq_set_unregister_info(HostType, MucHost, From, Lang)
     end.
 
-get_submitted_form([El]) ->
-    case mongoose_data_forms:is_form(El, [<<"submit">>, <<"cancel">>]) of
-        true -> {ok, El};
-        false -> no_form
-    end;
-get_submitted_form(_) ->
-    no_form.
-
--spec process_register(Type :: binary(),
-                       HostType :: host_type(), MucHost :: jid:server(),
-                       From :: jid:jid(), Lang :: ejabberd:lang(), XEl :: exml:element()) ->
+-spec process_register(HostType :: host_type(), MucHost :: jid:server(),
+                       From :: jid:jid(), Lang :: ejabberd:lang(),
+                       KVs :: mongoose_data_forms:kv_map()) ->
     {error, exml:element()} | {result, []}.
-process_register(<<"cancel">>, _HostType, _Host, _From, _Lang, _XEl) ->
-    {result, []};
-process_register(<<"submit">>, HostType, MucHost, From, Lang, XEl) ->
-    case mongoose_data_forms:form_to_map(XEl) of
-        #{<<"nick">> := [Nick]} ->
-            iq_set_register_info(HostType, MucHost, From, Nick, Lang);
-        _ ->
-            ErrText = <<"You must fill in field \"Nickname\" in the form">>,
-            {error, mongoose_xmpp_errors:not_acceptable(Lang, ErrText)}
-    end.
+process_register(HostType, MucHost, From, Lang, #{<<"nick">> := [Nick]}) ->
+    iq_set_register_info(HostType, MucHost, From, Nick, Lang);
+process_register(_HostType, _MucHost, _From, Lang, #{}) ->
+    ErrText = <<"You must fill in field \"Nickname\" in the form">>,
+    {error, mongoose_xmpp_errors:not_acceptable(Lang, ErrText)}.
 
 -spec iq_get_vcard(ejabberd:lang()) -> [exml:element(), ...].
 iq_get_vcard(Lang) ->

src/mod_muc_room.erl

@@ -3186,21 +3186,23 @@ process_authorized_iq_owner(From, set, Lang, SubEl, StateData, StateName) ->
             add_to_log(room_existence, destroyed, StateData),
             destroy_room(SubEl1, StateData);
         [XEl] ->
-            case {form_type(XEl), StateName} of
-                {<<"cancel">>, locked_state} ->
+            case {mongoose_data_forms:parse_form(XEl), StateName} of
+                {#{type := <<"cancel">>}, locked_state} ->
                     ?LOG_INFO(ls(#{what => muc_cancel_locked,
-                                   text => <<"Received cancel before the room was configured - destroy room">>,
+                                   text => <<"Received cancel before the room was configured "
+                                             "- destroy room">>,
                                    from_jid => jid:to_binary(From)}, StateData)),
                     add_to_log(room_existence, destroyed, StateData),
                     destroy_room(XEl, StateData);
-                {<<"cancel">>, normal_state} ->
+                {#{type := <<"cancel">>}, normal_state} ->
                     %% received cancel when room was configured - continue without changes
                     {result, [], StateData};
-                {<<"submit">>, _} ->
-                    KVs = mongoose_data_forms:form_to_kvs(XEl),
-                    process_authorized_submit_owner(From, KVs, StateData);
+                {#{type := <<"submit">>, kvs := KVs}, _} ->
+                    process_authorized_submit_owner(From, maps:to_list(KVs), StateData);
+                {{error, Msg}, _} ->
+                    {error, mongoose_xmpp_errors:bad_request(Lang, Msg)};
                 _ ->
-                    {error, mongoose_xmpp_errors:bad_request()}
+                    {error, mongoose_xmpp_errors:bad_request(Lang, <<"Invalid form contents">>)}
             end;
         Items ->
             process_admin_items_set(From, Items, Lang, StateData)
@@ -3221,12 +3223,6 @@ process_authorized_iq_owner(From, get, Lang, SubEl, StateData, _StateName) ->
             end
     end.
 
-form_type(Elem) ->
-    case mongoose_data_forms:is_form(Elem) of
-        true -> mongoose_data_forms:form_type(Elem);
-        false -> undefined
-    end.
-
 -spec process_authorized_submit_owner(From ::jid:jid(), [{binary(), [binary()]}],
                                       StateData :: state()) ->
     {error, exml:element()} | {result, [exml:element() | jlib:xmlcdata()], state() | stop}.
@@ -3580,9 +3576,6 @@ set_xoption([{<<"muc#roomconfig_getmemberlist">>, Val} | Opts], Config) ->
     end;
 set_xoption([{<<"muc#roomconfig_enablelogging">>, [Val]} | Opts], Config) ->
     ?SET_BOOL_XOPT(logging, Val);
-set_xoption([{<<"FORM_TYPE">>, _} | Opts], Config) ->
-    %% Ignore our FORM_TYPE
-    set_xoption(Opts, Config);
 set_xoption([_ | _Opts], _Config) ->
     {error, mongoose_xmpp_errors:bad_request()}.
 
@@ -3915,24 +3908,30 @@ disco_item(User=#user{nick=Nick}, RoomJID) ->
            | {role, BRole :: binary(), RoomNick :: mod_muc:nick()}
            | {error, any()}
            | ok.
-check_voice_approval(From, [XEl], _Lang, StateData) ->
-    Fields = mongoose_data_forms:form_to_kvs(XEl),
-    [BRole] = proplists:get_value(<<"muc#role">>, Fields, [undefined]),
-    case Fields of
-        [_Form, _Role] ->
-            case catch binary_to_role(BRole) of
-                {'EXIT', _} -> {error, mongoose_xmpp_errors:bad_request()};
-                _ -> {form, BRole}
+check_voice_approval(From, [XEl], Lang, StateData) ->
+    case mongoose_data_forms:parse_form(XEl) of
+        #{kvs := #{<<"muc#role">> := [BRole]} = KVs} ->
+            case {get_role(From, StateData) =:= moderator,
+                  maps:find(<<"muc#request_allow">>, KVs),
+                  maps:find(<<"muc#roomnick">>, KVs)} of
+                {_, error, error} ->
+                    case catch binary_to_role(BRole) of
+                        {'EXIT', _} -> {error, mongoose_xmpp_errors:bad_request()};
+                        _ -> {form, BRole}
+                    end;
+                {false, _, _} ->
+                    {error, mongoose_xmpp_errors:not_allowed()};
+                {true, {ok, [<<"true">>]}, error} ->
+                    {error, mongoose_xmpp_errors:bad_request()};
+                {true, {ok, [<<"true">>]}, {ok, [RoomNick]}} ->
+                    {role, BRole, RoomNick};
+                {true, _, _} ->
+                    ok
             end;
+        {error, Msg} ->
+            {error, mongoose_xmpp_errors:bad_request(Lang, Msg)};
         _ ->
-            case {get_role(From, StateData),
-                  proplists:get_value(<<"muc#request_allow">>, Fields),
-                  proplists:get_value(<<"muc#roomnick">>, Fields)} of
-                {moderator, [<<"true">>], undefined} -> {error, mongoose_xmpp_errors:bad_request()};
-                {moderator, [<<"true">>], [RoomNick]} -> {role, BRole, RoomNick};
-                {moderator, _, _} -> ok;
-                _ -> {error, mongoose_xmpp_errors:not_allowed()}
-            end
+            {error, mongoose_xmpp_errors:bad_request(Lang, <<"MUC Role was not provided">>)}
     end.
 
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

src/mongoose_data_forms.erl

@@ -1,12 +1,11 @@
 -module(mongoose_data_forms).
 -xep([{xep, 4}, {version, "2.13.1"}]).
+-xep([{xep, 68}, {version, "1.3.0"}]).
 
 %% Form processing
--export([is_form/1, is_form/2,
-         form_type/1,
-         find_form/1, find_form/2,
-         form_to_kvs/1, form_to_kvs/2,
-         form_to_map/1, form_to_map/2]).
+-export([find_and_parse_form/1, find_form/1, find_form/2,
+         parse_form/1, parse_form_fields/1,
+         is_form/1, is_form/2]).
 
 %% Form construction
 -export([form/1]).
@@ -19,26 +18,25 @@
 -type field() :: #{var => binary(), type => binary(), label => binary(),
                    values => [binary()], options => [option()]}.
 -type option() :: binary() | {binary(), binary()}.
--type kv_list() :: [{binary(), [binary()]}].
+
+-type parsed_form() :: #{type := binary() | undefined, ns => binary(), kvs := kv_map()}.
 -type kv_map() :: #{binary() => [binary()]}.
 
--export_type([form/0, field/0, option/0, kv_list/0, kv_map/0]).
+-export_type([form/0, field/0, option/0, kv_map/0]).
 
--ignore_xref([form_to_map/2]). % exported for consistency, might be used later
+-ignore_xref([is_form/1]). % exported for consistency, might be used later
 
 %% Form processing
 
--spec is_form(exml:element()) -> boolean().
-is_form(#xmlel{name = Name} = Elem) ->
-    Name =:= <<"x">> andalso exml_query:attr(Elem, <<"xmlns">>) =:= ?NS_XDATA.
-
--spec is_form(exml:element(), [binary()]) -> boolean().
-is_form(Elem, Types) ->
-    is_form(Elem) andalso lists:member(form_type(Elem), Types).
-
--spec form_type(exml:element()) -> binary() | undefined.
-form_type(Form) ->
-    exml_query:attr(Form, <<"type">>).
+%% @doc Find a form in subelements, and then parse its fields
+-spec find_and_parse_form(exml:element()) -> parsed_form() | {error, binary()}.
+find_and_parse_form(Parent) ->
+    case find_form(Parent) of
+        undefined ->
+            {error, <<"Form not found">>};
+        Form ->
+            parse_form_fields(Form)
+    end.
 
 -spec find_form(exml:element()) -> exml:element() | undefined.
 find_form(Parent) ->
@@ -48,31 +46,48 @@ find_form(Parent) ->
 find_form(Parent, Default) ->
     exml_query:subelement_with_name_and_ns(Parent, <<"x">>, ?NS_XDATA, Default).
 
--spec form_to_kvs(exml:element(), binary()) -> kv_list() | invalid.
-form_to_kvs(FormEl, Type) ->
-    case form_type(FormEl) of
-        Type -> form_to_kvs(FormEl);
-        _ -> invalid
+%% @doc Check if the element is a form, and then parse its fields
+-spec parse_form(exml:element()) -> parsed_form() | {error, binary()}.
+parse_form(Elem) ->
+    case is_form(Elem) of
+        true ->
+            parse_form_fields(Elem);
+        false ->
+            {error, <<"Invalid form element">>}
     end.
 
--spec form_to_kvs(exml:element()) -> kv_list().
-form_to_kvs(FormEl) ->
-    form_fields_to_kvs(FormEl#xmlel.children).
-
--spec form_to_map(exml:element(), binary()) -> kv_map() | invalid.
-form_to_map(FormEl, Type) ->
-    case form_type(FormEl) of
-        Type -> form_to_map(FormEl);
-        _ -> invalid
+%% @doc Parse the form fields without checking that it is a form element
+-spec parse_form_fields(exml:element()) -> parsed_form().
+parse_form_fields(Elem) ->
+    M = case form_type(Elem) of
+            undefined -> #{};
+            Type -> #{type => Type}
+        end,
+    KVs = form_fields_to_kvs(Elem#xmlel.children),
+    case maps:take(<<"FORM_TYPE">>, KVs) of
+        {[NS], FKVs} ->
+            M#{ns => NS, kvs => FKVs};
+        _ ->
+            % Multiple values of FORM_TYPE don't make sense in context of XEP-0068,
+            % but according to XEP-004 the form is still valid
+            M#{kvs => KVs}
     end.
 
--spec form_to_map(exml:element()) -> kv_map().
-form_to_map(FormEl) ->
-    maps:from_list(form_fields_to_kvs(FormEl#xmlel.children)).
+-spec is_form(exml:element()) -> boolean().
+is_form(#xmlel{name = Name} = Elem) ->
+    Name =:= <<"x">> andalso exml_query:attr(Elem, <<"xmlns">>) =:= ?NS_XDATA.
+
+-spec is_form(exml:element(), [binary()]) -> boolean().
+is_form(Elem, Types) ->
+    is_form(Elem) andalso lists:member(form_type(Elem), Types).
+
+-spec form_type(exml:element()) -> binary() | undefined.
+form_type(Form) ->
+    exml_query:attr(Form, <<"type">>, <<"form">>).
 
--spec form_fields_to_kvs([exml:element()]) -> kv_list().
+-spec form_fields_to_kvs([exml:element()]) -> kv_map().
 form_fields_to_kvs(Fields) ->
-    lists:flatmap(fun form_field_to_kv/1, Fields).
+    maps:from_list(lists:flatmap(fun form_field_to_kv/1, Fields)).
 
 form_field_to_kv(FieldEl = #xmlel{name = <<"field">>}) ->
     case exml_query:attr(FieldEl, <<"var">>) of