Merge pull request #4178 from esl/muclight_config_fix

Muclight config fix

big_tests/tests/muc_light_SUITE.erl

@@ -48,6 +48,7 @@
          destroy_room_get_disco_items_empty/1,
          destroy_room_get_disco_items_one_left/1,
          set_config/1,
+         set_partial_config/1,
          set_config_with_custom_schema/1,
          deny_config_change_that_conflicts_with_schema/1,
          assorted_config_doesnt_lead_to_duplication/1,
@@ -169,6 +170,7 @@ groups() ->
                               destroy_room_get_disco_items_empty,
                               destroy_room_get_disco_items_one_left,
                               set_config,
+                              set_partial_config,
                               set_config_with_custom_schema,
                               deny_config_change_that_conflicts_with_schema,
                               assorted_config_doesnt_lead_to_duplication,
@@ -713,18 +715,31 @@ destroy_room_get_disco_items_one_left(Config) ->
 set_config(Config) ->
     escalus:story(Config, [{alice, 1}, {bob, 1}, {kate, 1}], fun(Alice, Bob, Kate) ->
             ConfigChange = [{<<"roomname">>, <<"The Coven">>}],
-            escalus:send(Alice, stanza_config_set(?ROOM, ConfigChange)),
-            foreach_recipient([Alice, Bob, Kate], config_msg_verify_fun(ConfigChange)),
-            escalus:assert(is_iq_result, escalus:wait_for_stanza(Alice))
+            change_room_config(Alice, [Alice, Bob, Kate], ConfigChange)
+        end).
+
+set_partial_config(Config) ->
+    escalus:story(Config, [{alice, 1}, {bob, 1}, {kate, 1}], fun(Alice, Bob, Kate) ->
+            FinalRoomName = <<"The Coven">>,
+            %% Change all the config
+            ConfigChange = [{<<"roomname">>, FinalRoomName}, {<<"subject">>, <<"Evil Ravens">>}],
+            change_room_config(Alice, [Alice, Bob, Kate], ConfigChange),
+            %% Now change only the subject
+            ConfigChange2 = [{<<"subject">>, <<"Good Ravens">>}],
+            change_room_config(Alice, [Alice, Bob, Kate], ConfigChange2),
+            %% Verify that roomname is still the original change
+            escalus:send(Alice, stanza_config_get(?ROOM, ver(1))),
+            IQRes = escalus:wait_for_stanza(Alice),
+            escalus:assert(is_iq_result, IQRes),
+            RoomName = exml_query:path(IQRes, [{element, <<"query">>}, {element, <<"roomname">>}, cdata]),
+            ?assertEqual(FinalRoomName, RoomName)
         end).
 
 set_config_with_custom_schema(Config) ->
     escalus:story(Config, [{alice, 1}, {bob, 1}, {kate, 1}], fun(Alice, Bob, Kate) ->
             ConfigChange = [{<<"background">>, <<"builtin:unicorns">>},
                             {<<"music">>, <<"builtin:rainbow">>}],
-            escalus:send(Alice, stanza_config_set(?ROOM, ConfigChange)),
-            foreach_recipient([Alice, Bob, Kate], config_msg_verify_fun(ConfigChange)),
-            escalus:assert(is_iq_result, escalus:wait_for_stanza(Alice))
+            change_room_config(Alice, [Alice, Bob, Kate], ConfigChange)
         end).
 
 deny_config_change_that_conflicts_with_schema(Config) ->
@@ -740,10 +755,7 @@ assorted_config_doesnt_lead_to_duplication(Config) ->
             ConfigChange = [{<<"subject">>, <<"Elixirs">>},
                             {<<"roomname">>, <<"The Coven">>},
                             {<<"subject">>, <<"Elixirs">>}],
-            ConfigSetStanza = stanza_config_set(?ROOM, ConfigChange),
-            escalus:send(Alice, ConfigSetStanza),
-            foreach_recipient([Alice, Bob, Kate], config_msg_verify_fun(ConfigChange)),
-            escalus:assert(is_iq_result, escalus:wait_for_stanza(Alice)),
+            change_room_config(Alice, [Alice, Bob, Kate], ConfigChange),
 
             Stanza = stanza_config_get(?ROOM, <<"oldver">>),
             VerifyFun = fun(Incoming) ->
@@ -937,6 +949,12 @@ blocking_disabled(Config) ->
 %% Subroutines
 %%--------------------------------------------------------------------
 
+-spec change_room_config(escalus:client(), [escalus:client()], list({binary(), binary()})) -> term().
+change_room_config(User, Users, ConfigChange) ->
+    escalus:send(User, stanza_config_set(?ROOM, ConfigChange)),
+    foreach_recipient(Users, config_msg_verify_fun(ConfigChange)),
+    escalus:assert(is_iq_result, escalus:wait_for_stanza(User)).
+
 -spec get_disco_rooms(User :: escalus:client()) -> list(xmlel()).
 get_disco_rooms(User) ->
     DiscoStanza = escalus_stanza:to(escalus_stanza:iq_get(?NS_DISCO_ITEMS, []), ?MUCHOST),

src/muc_light/mod_muc_light_db_rdbms.erl

@@ -657,7 +657,7 @@ create_room_transaction(HostType, {RoomU, RoomS}, Config, AffUsers, Version) ->
     insert_room(HostType, RoomU, RoomS, Version),
     RoomID = mongoose_rdbms:selected_to_integer(select_room_id(HostType, RoomU, RoomS)),
     Schema = mod_muc_light:config_schema(RoomS),
-    ConfigFields = mod_muc_light_room_config:to_binary_kv(Config, Schema),
+    {ok, ConfigFields} = mod_muc_light_room_config:to_binary_kv(Config, Schema),
     [insert_aff_tuple(HostType, RoomID, AffUser) || AffUser <- AffUsers],
     [insert_config_kv(HostType, RoomID, KV) || KV <- ConfigFields],
     ok.
@@ -708,11 +708,12 @@ set_config_transaction({RoomU, RoomS} = RoomUS, ConfigChanges, Version) ->
         {selected, [{DbRoomID, PrevVersion}]} ->
             RoomID = mongoose_rdbms:result_to_integer(DbRoomID),
             {updated, _} = update_room_version(HostType, RoomU, RoomS, Version),
+            {ok, Config} = mod_muc_light_room_config:to_binary_kv_diff(
+                             ConfigChanges, mod_muc_light:config_schema(RoomS)),
             lists:foreach(
               fun({Key, Val}) ->
                       {updated, _} = update_config(HostType, RoomID, Key, Val)
-              end, mod_muc_light_room_config:to_binary_kv(
-                     ConfigChanges, mod_muc_light:config_schema(RoomS))),
+              end, Config),
             {ok, PrevVersion};
         {selected, []} ->
             {error, not_exists}

src/muc_light/mod_muc_light_room.erl

@@ -108,7 +108,7 @@ process_request({get, #config{} = ConfigReq},
     {_, RoomS} = RoomUS,
     HostType = mongoose_acc:host_type(Acc),
     {ok, Config, RoomVersion} = mod_muc_light_db_backend:get_config(HostType, RoomUS),
-    RawConfig = mod_muc_light_room_config:to_binary_kv(Config, mod_muc_light:config_schema(RoomS)),
+    {ok, RawConfig} = mod_muc_light_room_config:to_binary_kv(Config, mod_muc_light:config_schema(RoomS)),
     {get, ConfigReq#config{ version = RoomVersion,
                             raw_config = RawConfig }};
 process_request({get, #affiliations{} = AffReq},
@@ -121,7 +121,7 @@ process_request({get, #info{} = InfoReq},
                 _From, {_, RoomS} = RoomUS, _Auth, _AffUsers, Acc) ->
     HostType = mongoose_acc:host_type(Acc),
     {ok, Config, AffUsers, RoomVersion} = mod_muc_light_db_backend:get_info(HostType, RoomUS),
-    RawConfig = mod_muc_light_room_config:to_binary_kv(Config, mod_muc_light:config_schema(RoomS)),
+    {ok, RawConfig} = mod_muc_light_room_config:to_binary_kv(Config, mod_muc_light:config_schema(RoomS)),
     {get, InfoReq#info{ version = RoomVersion, aff_users = AffUsers,
                         raw_config = RawConfig }};
 process_request({set, #config{} = ConfigReq},

src/muc_light/mod_muc_light_room_config.erl

@@ -23,7 +23,8 @@
 -module(mod_muc_light_room_config).
 
 %% API
--export([from_binary_kv_diff/2, from_binary_kv/2, to_binary_kv/2]).
+-export([from_binary_kv_diff/2, from_binary_kv/2,
+         to_binary_kv_diff/2, to_binary_kv/2]).
 
 -include("mod_muc_light.hrl").
 
@@ -40,8 +41,7 @@
 -type binary_kv() :: [{Key :: binary(), Value :: binary()}].
 
 %% User definition processing
--type schema_item() :: {FieldName :: binary(), DefaultValue :: value(),
-                                     key(), value_type()}.
+-type schema_item() :: {FieldName :: binary(), DefaultValue :: value(), key(), value_type()}.
 -type schema() :: [schema_item()]. % has to be sorted
 
 %%====================================================================
@@ -52,36 +52,40 @@
 -spec from_binary_kv_diff(RawConfig :: binary_kv(), ConfigSchema :: schema()) ->
     {ok, kv()} | validation_error().
 from_binary_kv_diff(RawConfig, ConfigSchema) ->
-    from_binary_kv_diff(lists:ukeysort(1, RawConfig), ConfigSchema, []).
-
-from_binary_kv_diff([], [], Config) ->
-    {ok, Config};
-from_binary_kv_diff(RawConfig, ConfigSchema, Config) ->
-    case take_next_kv(RawConfig, ConfigSchema) of
-        {error, Reason} ->
-            {error, Reason};
-        {value, RRawConfig, RConfigSchema, KV} ->
-            from_binary_kv(RRawConfig, RConfigSchema, [KV | Config]);
-        {default, _, _, _} ->
-            % do not populate the diff with default values
-            from_binary_kv(RawConfig, ConfigSchema, Config)
-    end.
+    take_next(lists:ukeysort(1, RawConfig), ConfigSchema, true, fun take_next_kv/2, []).
 
 -spec from_binary_kv(RawConfig :: binary_kv(), ConfigSchema :: schema()) ->
-          {ok, kv()} | validation_error().
+    {ok, kv()} | validation_error().
 from_binary_kv(RawConfig, ConfigSchema) ->
-    from_binary_kv(lists:ukeysort(1, RawConfig), ConfigSchema, []).
+    take_next(lists:ukeysort(1, RawConfig), ConfigSchema, false, fun take_next_kv/2, []).
 
-from_binary_kv([], [], Config) ->
+-spec to_binary_kv_diff(RawConfig :: kv(), ConfigSchema :: schema()) ->
+    {ok, binary_kv()} | validation_error().
+to_binary_kv_diff(RawConfig, ConfigSchema) ->
+    take_next(lists:ukeysort(1, RawConfig), ConfigSchema, true, fun take_next_binary_kv/2, []).
+
+-spec to_binary_kv(Config :: kv(), ConfigSchema :: schema()) ->
+    {ok, binary_kv()} | validation_error().
+to_binary_kv(RawConfig, ConfigSchema) ->
+    take_next(lists:ukeysort(1, RawConfig), ConfigSchema, false, fun take_next_binary_kv/2, []).
+
+take_next([], [], _, _, Config) ->
     {ok, Config};
-from_binary_kv(RawConfig, ConfigSchema, Config) ->
-    case take_next_kv(RawConfig, ConfigSchema) of
-        {error, Reason} ->
-            {error, Reason};
-        {_, RRawConfig, RConfigSchema, KV} ->
-            from_binary_kv(RRawConfig, RConfigSchema, [KV | Config])
+take_next(RawConfig, ConfigSchema, DropDefaults, TakeNext, Config) ->
+    case {DropDefaults, TakeNext(RawConfig, ConfigSchema)} of
+        {true, {default, RRawConfig, RConfigSchema, _}} ->
+            % do not populate the diff with default values
+            take_next(RRawConfig, RConfigSchema, DropDefaults, TakeNext, Config);
+        {_, {_, RRawConfig, RConfigSchema, KV}} ->
+            take_next(RRawConfig, RConfigSchema, DropDefaults, TakeNext, [KV | Config]);
+        {_, {error, Reason}} ->
+            {error, Reason}
     end.
 
+%%====================================================================
+%% Internal functions
+%%====================================================================
+
 take_next_kv([{KeyBin, ValBin} | RRawConfig], [{KeyBin, _Default, Key, Type} | RSchema]) ->
     try {value, RRawConfig, RSchema, {Key, b2value(ValBin, Type)}}
     catch _:_ -> {error, {KeyBin, type_error}}
@@ -91,14 +95,14 @@ take_next_kv(RawConfig, [{_KeyBin, Default, Key, _Type} | RSchema]) ->
 take_next_kv([{KeyBin, _} | _], _) ->
     {error, {KeyBin, not_found}}.
 
--spec to_binary_kv(Config :: kv(), ConfigSchema :: schema()) -> binary_kv().
-to_binary_kv(Config, ConfigSchema) ->
-    ConfigWithSchema = lists:zip(lists:sort(Config), lists:keysort(3, ConfigSchema)),
-    [{KeyBin, value2b(Val, Type)} || {{Key, Val}, {KeyBin, _Default, Key, Type}} <- ConfigWithSchema].
-
-%%====================================================================
-%% Internal functions
-%%====================================================================
+take_next_binary_kv([{Key, ValBin} | RRawConfig], [{KeyBin, _Default, Key, Type} | RSchema]) ->
+    try {value, RRawConfig, RSchema, {KeyBin, value2b(ValBin, Type)}}
+    catch _:_ -> {error, {KeyBin, type_error}}
+    end;
+take_next_binary_kv(RawConfig, [{KeyBin, Default, _Key, _Type} | RSchema]) ->
+    {default, RawConfig, RSchema, {KeyBin, Default}};
+take_next_binary_kv([{KeyBin, _} | _], _) ->
+    {error, {KeyBin, not_found}}.
 
 -spec b2value(ValBin :: binary(), Type :: value_type()) -> Converted :: value().
 b2value(ValBin, binary) when is_binary(ValBin) -> ValBin;