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

Muclight config fix #4178

Merged
merged 3 commits into from
Nov 23, 2023
Merged

Muclight config fix #4178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d60a9e4
Regression test for room config defaults insertion
NelsonVides Nov 22, 2023
f2533fc
Fix room config diff calculations
NelsonVides Nov 22, 2023
29d8186
Room config unify from and to binaries
NelsonVides Nov 22, 2023
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
38 changes: 28 additions & 10 deletions big_tests/tests/muc_light_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) ->
Expand All @@ -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) ->
Expand Down Expand Up @@ -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),
Expand Down
7 changes: 4 additions & 3 deletions src/muc_light/mod_muc_light_db_rdbms.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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}
Expand Down
4 changes: 2 additions & 2 deletions src/muc_light/mod_muc_light_room.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand All @@ -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},
Expand Down
72 changes: 38 additions & 34 deletions src/muc_light/mod_muc_light_room_config.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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").

Expand All @@ -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

%%====================================================================
Expand All @@ -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}}
Expand All @@ -91,14 +95,14 @@
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}}

Check warning on line 100 in src/muc_light/mod_muc_light_room_config.erl

View check run for this annotation

Codecov / codecov/patch

src/muc_light/mod_muc_light_room_config.erl#L100 

Added line #L100 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to write "Why so complicated" and I after I see the old take_next_kv doing the same.
{error, _} is not covered by tests even :)

take_next logic is kinda hard to follow, in the perfect world it should be just simple maps:merge(NewConfig, maps:merge(OldConfig, Default))
or something.

end;
take_next_binary_kv(RawConfig, [{KeyBin, Default, _Key, _Type} | RSchema]) ->
{default, RawConfig, RSchema, {KeyBin, Default}};
take_next_binary_kv([{KeyBin, _} | _], _) ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor naming inconsistency

Suggested change
take_next_binary_kv([{KeyBin, _} | _], _) ->
take_next_binary_kv([{Key, _} | _], _) ->

{error, {KeyBin, not_found}}.

Check warning on line 105 in src/muc_light/mod_muc_light_room_config.erl

View check run for this annotation

Codecov / codecov/patch

src/muc_light/mod_muc_light_room_config.erl#L105 

Added line #L105 was not covered by tests

-spec b2value(ValBin :: binary(), Type :: value_type()) -> Converted :: value().
b2value(ValBin, binary) when is_binary(ValBin) -> ValBin;
Expand Down