diff --git a/Cargo.lock b/Cargo.lock index db362f550d..593c837881 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -808,6 +808,21 @@ version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7e0d60973d9320722cb1206f412740e162a33b8547ea8d6be75d7cff237c7a85" +[[package]] +name = "bit-set" +version = "0.5.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0700ddab506f33b20a03b13996eccd309a48e5ff77d0d95926aa0210fb4e95f1" +dependencies = [ + "bit-vec", +] + +[[package]] +name = "bit-vec" +version = "0.6.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "349f9b6a179ed607305526ca489b34ad0a41aed5f7980fa90eb03160b69598fb" + [[package]] name = "bitflags" version = "1.3.2" @@ -2858,10 +2873,13 @@ dependencies = [ "itertools 0.13.0", "lazy_static", "msgpack", + "proptest", + "rand 0.8.5", "regex-lite", "search", "serde", "serde_repr", + "test-strategy", "types", "utils 0.1.0", ] @@ -5776,9 +5794,9 @@ dependencies = [ [[package]] name = "proc-macro2" -version = "1.0.83" +version = "1.0.89" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0b33eb56c327dec362a9e55b3ad14f9d2f0904fb5a5b03b513ab5465399e9f43" +checksum = "f139b0662de085916d1fb67d2b4169d1addddda1919e696f3252b740b629986e" dependencies = [ "unicode-ident", ] @@ -5885,6 +5903,26 @@ dependencies = [ "utils 0.1.0", ] +[[package]] +name = "proptest" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b4c2511913b88df1637da85cc8d96ec8e43a3f8bb8ccb71ee1ac240d6f3df58d" +dependencies = [ + "bit-set", + "bit-vec", + "bitflags 2.4.2", + "lazy_static", + "num-traits", + "rand 0.8.5", + "rand_chacha 0.3.1", + "rand_xorshift 0.3.0", + "regex-syntax 0.8.2", + "rusty-fork", + "tempfile", + "unarray", +] + [[package]] name = "psm" version = "0.1.21" @@ -5912,6 +5950,12 @@ version = "0.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "007d8adb5ddab6f8e3f491ac63566a7d5002cc7ed73901f72057943fa71ae1ae" +[[package]] +name = "quick-error" +version = "1.2.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a1d01941d82fa2ab50be1e79e6714289dd7cde78eba4c074bc5a4374f650dfe0" + [[package]] name = "quinn" version = "0.11.5" @@ -5990,7 +6034,7 @@ dependencies = [ "rand_jitter", "rand_os", "rand_pcg", - "rand_xorshift", + "rand_xorshift 0.1.1", "winapi", ] @@ -6161,6 +6205,15 @@ dependencies = [ "rand_core 0.3.1", ] +[[package]] +name = "rand_xorshift" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d25bf25ec5ae4a3f1b92f929810509a2f53d7dca2f50b794ff57e3face536c8f" +dependencies = [ + "rand_core 0.6.4", +] + [[package]] name = "range-set" version = "0.0.11" @@ -6638,6 +6691,18 @@ version = "1.0.14" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7ffc183a10b4478d04cbbbfc96d0873219d962dd5accaff2ffbd4ceb7df837f4" +[[package]] +name = "rusty-fork" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cb3dcc6e454c328bb824492db107ab7c0ae8fcffe4ad210136ef014458c1bc4f" +dependencies = [ + "fnv", + "quick-error", + "tempfile", + "wait-timeout", +] + [[package]] name = "ryu" version = "1.0.16" @@ -7524,6 +7589,29 @@ version = "0.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5ee073c9e4cd00e28217186dbe12796d692868f432bf2e97ee73bed0c56dfa01" +[[package]] +name = "structmeta" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2e1575d8d40908d70f6fd05537266b90ae71b15dbbe7a8b7dffa2b759306d329" +dependencies = [ + "proc-macro2", + "quote", + "structmeta-derive", + "syn 2.0.87", +] + +[[package]] +name = "structmeta-derive" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "152a0b65a590ff6c3da95cabe2353ee04e6167c896b28e3b14478c2636c922fc" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.87", +] + [[package]] name = "strum" version = "0.25.0" @@ -7707,6 +7795,18 @@ dependencies = [ "test-case-core", ] +[[package]] +name = "test-strategy" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2bf41af45e3f54cc184831d629d41d5b2bda8297e29c81add7ae4f362ed5e01b" +dependencies = [ + "proc-macro2", + "quote", + "structmeta", + "syn 2.0.87", +] + [[package]] name = "test_utils" version = "0.13.0" @@ -8205,6 +8305,12 @@ dependencies = [ "ts_export", ] +[[package]] +name = "unarray" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eaea85b334db583fe3274d12b4cd1880032beab409c0d774be044d4480ab9a94" + [[package]] name = "unicase" version = "2.7.0" @@ -8581,6 +8687,15 @@ version = "0.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5c3082ca00d5a5ef149bb8b555a72ae84c9c59f7250f013ac822ac2e49b19c64" +[[package]] +name = "wait-timeout" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9f200f5b12eb75f8c1ed65abd4b2db8a6e1b138a20de009dacee265a2498f3f6" +dependencies = [ + "libc", +] + [[package]] name = "want" version = "0.3.1" diff --git a/Cargo.toml b/Cargo.toml index bb7d48fe2d..57380b8a46 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -222,6 +222,7 @@ openssl = "0.10.66" p256 = { version = "0.13.2" } pocket-ic = "6.0.0" proc-macro2 = "1.0.83" +proptest = "1.5.0" pulldown-cmark = { version = "0.12.0", default-features = false, features = [ "html", ] } @@ -244,6 +245,7 @@ sign_in_with_email_canister = { git = "https://github.com/open-chat-labs/ic-sign sign_in_with_email_canister_test_utils = { package = "test_utils", git = "https://github.com/open-chat-labs/ic-sign-in-with-email", rev = "v0.13.0" } syn = "2.0.65" test-case = "3.3.1" +test-strategy = "0.4.0" time = "0.3.36" tokio = "1.37.0" tracing = "0.1.40" diff --git a/backend/canisters/community/CHANGELOG.md b/backend/canisters/community/CHANGELOG.md index 3d37263bb0..16071d9918 100644 --- a/backend/canisters/community/CHANGELOG.md +++ b/backend/canisters/community/CHANGELOG.md @@ -22,6 +22,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Add `bot_api_gateway` canisterId to the canister state ([#6842](https://github.com/open-chat-labs/open-chat/pull/6842)) - Simplify `inspect_message` ([#6847](https://github.com/open-chat-labs/open-chat/pull/6847)) - Avoid double iteration when updating member expiry ([#6851](https://github.com/open-chat-labs/open-chat/pull/6851)) +- Avoid cases where we were iterating over the full chat members map ([#6853](https://github.com/open-chat-labs/open-chat/pull/6853)) ### Removed diff --git a/backend/canisters/community/impl/src/jobs/import_groups.rs b/backend/canisters/community/impl/src/jobs/import_groups.rs index cb1c70ad01..ae55e8ec28 100644 --- a/backend/canisters/community/impl/src/jobs/import_groups.rs +++ b/backend/canisters/community/impl/src/jobs/import_groups.rs @@ -209,12 +209,15 @@ pub(crate) async fn process_channel_members(group_id: ChatId, channel_id: Channe let (members_to_add_to_community, local_user_index_canister_id) = mutate_state(|state| { let channel = state.data.channels.get(&channel_id).unwrap(); + let bots = channel.chat.members.bots(); let mut to_add: HashMap = HashMap::new(); - for (user_id, user_type) in channel.chat.members.iter().map(|m| (m.user_id(), m.user_type())) { - if let Some(member) = state.data.members.get_by_user_id_mut(&user_id) { + + for user_id in channel.chat.members.member_ids().iter() { + if let Some(member) = state.data.members.get_by_user_id_mut(user_id) { member.channels.insert(channel_id); } else { - to_add.insert(user_id, user_type); + let user_type = bots.get(user_id).copied().unwrap_or_default(); + to_add.insert(*user_id, user_type); } } @@ -333,7 +336,7 @@ pub(crate) fn mark_import_complete(group_id: ChatId, channel_id: ChannelId) { }, group_id, group_name: channel.chat.name.value.clone(), - members: channel.chat.members.iter().map(|m| m.user_id()).collect(), + members: channel.chat.members.member_ids().iter().copied().collect(), other_public_channels: state .data .channels diff --git a/backend/canisters/community/impl/src/lib.rs b/backend/canisters/community/impl/src/lib.rs index 32ae0da07e..285eb22355 100644 --- a/backend/canisters/community/impl/src/lib.rs +++ b/backend/canisters/community/impl/src/lib.rs @@ -15,7 +15,7 @@ use fire_and_forget_handler::FireAndForgetHandler; use gated_groups::GatePayment; use group_chat_core::AccessRulesInternal; use group_community_common::{ - Achievements, ExpiringMember, ExpiringMemberActions, ExpiringMembers, Member, Members, PaymentReceipts, PaymentRecipient, + Achievements, ExpiringMember, ExpiringMemberActions, ExpiringMembers, Members, PaymentReceipts, PaymentRecipient, PendingPayment, PendingPaymentReason, PendingPaymentsQueue, UserCache, }; use instruction_counts_log::{InstructionCountEntry, InstructionCountFunctionId, InstructionCountsLog}; @@ -634,28 +634,17 @@ impl Data { } } else if let Some(new_gate_expiry) = new_gate_expiry { // Else if the new gate has an expiry then add members to the expiry schedule. - let mut user_ids = Vec::new(); - - if let Some(channel_id) = channel_id { + let user_ids_iter = if let Some(channel_id) = channel_id { if let Some(channel) = self.channels.get_mut(&channel_id) { - user_ids = channel - .chat - .members - .iter() - .filter(|m| m.can_member_lapse()) - .map(|m| m.user_id()) - .collect(); + channel.chat.members.iter_members_who_can_lapse() + } else { + Box::new(std::iter::empty()) } } else { - user_ids = self - .members - .iter() - .filter(|m| m.can_member_lapse()) - .map(|m| m.user_id) - .collect(); - } + self.members.iter_members_who_can_lapse() + }; - for user_id in user_ids { + for user_id in user_ids_iter { self.expiring_members.push(ExpiringMember { expires: now + new_gate_expiry, channel_id, diff --git a/backend/canisters/community/impl/src/model/members.rs b/backend/canisters/community/impl/src/model/members.rs index 2eb7fc2147..44aa59d814 100644 --- a/backend/canisters/community/impl/src/model/members.rs +++ b/backend/canisters/community/impl/src/model/members.rs @@ -403,6 +403,10 @@ impl Members for CommunityMembers { fn get(&self, user_id: &UserId) -> Option<&CommunityMemberInternal> { self.get_by_user_id(user_id) } + + fn iter_members_who_can_lapse(&self) -> Box + '_> { + Box::new(self.members.values().filter(|m| m.can_member_lapse()).map(|m| m.user_id)) + } } #[derive(Serialize, Deserialize, Clone)] diff --git a/backend/canisters/community/impl/src/queries/selected_channel_initial.rs b/backend/canisters/community/impl/src/queries/selected_channel_initial.rs index f9f847be6d..3ebe71921f 100644 --- a/backend/canisters/community/impl/src/queries/selected_channel_initial.rs +++ b/backend/canisters/community/impl/src/queries/selected_channel_initial.rs @@ -1,7 +1,7 @@ use crate::{read_state, RuntimeState}; use canister_api_macros::query; use community_canister::selected_channel_initial::{Response::*, *}; -use types::{GroupMember, GroupRole}; +use std::collections::HashSet; #[query(candid = true, msgpack = true)] fn selected_channel_initial(args: Args) -> Response { @@ -29,13 +29,21 @@ fn selected_channel_initial_impl(args: Args, state: &RuntimeState) -> Response { .map(|m| m.min_visible_message_index()) .unwrap_or_default(); + let mut non_basic_members = HashSet::new(); + non_basic_members.extend(chat.members.owners().iter().copied()); + non_basic_members.extend(chat.members.admins().iter().copied()); + non_basic_members.extend(chat.members.moderators().iter().copied()); + non_basic_members.extend(chat.members.lapsed().iter().copied()); + let mut members = Vec::new(); let mut basic_members = Vec::new(); - for member in chat.members.iter().map(GroupMember::from) { - if matches!(member.role, GroupRole::Participant) && !member.lapsed { - basic_members.push(member.user_id); + for user_id in chat.members.member_ids().iter() { + if non_basic_members.contains(user_id) { + if let Some(member) = chat.members.get(user_id) { + members.push(member.into()); + } } else { - members.push(member); + basic_members.push(*user_id); } } diff --git a/backend/canisters/community/impl/src/updates/delete_channel.rs b/backend/canisters/community/impl/src/updates/delete_channel.rs index 4ef4cf7085..03d92bac56 100644 --- a/backend/canisters/community/impl/src/updates/delete_channel.rs +++ b/backend/canisters/community/impl/src/updates/delete_channel.rs @@ -76,8 +76,8 @@ fn delete_channel_impl(channel_id: ChannelId, state: &mut RuntimeState) -> Respo now, ); - for user_id in channel.chat.members.iter().map(|m| m.user_id()) { - state.data.members.mark_member_left_channel(&user_id, channel_id, now); + for user_id in channel.chat.members.member_ids() { + state.data.members.mark_member_left_channel(user_id, channel_id, now); } handle_activity_notification(state); diff --git a/backend/canisters/group/CHANGELOG.md b/backend/canisters/group/CHANGELOG.md index ef50876e94..d3d3e081e4 100644 --- a/backend/canisters/group/CHANGELOG.md +++ b/backend/canisters/group/CHANGELOG.md @@ -17,6 +17,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Improve job to delete files after messages are deleted ([#6839](https://github.com/open-chat-labs/open-chat/pull/6839)) - Add `bot_api_gateway` canisterId to the canister state ([#6842](https://github.com/open-chat-labs/open-chat/pull/6842)) - Simplify `inspect_message` ([#6847](https://github.com/open-chat-labs/open-chat/pull/6847)) +- Avoid cases where we were iterating over the full chat members map ([#6853](https://github.com/open-chat-labs/open-chat/pull/6853)) ### Removed diff --git a/backend/canisters/group/impl/src/lib.rs b/backend/canisters/group/impl/src/lib.rs index 6cb411007a..f7385dd087 100644 --- a/backend/canisters/group/impl/src/lib.rs +++ b/backend/canisters/group/impl/src/lib.rs @@ -12,9 +12,7 @@ use event_store_producer::{EventStoreClient, EventStoreClientBuilder, EventStore use event_store_producer_cdk_runtime::CdkRuntime; use fire_and_forget_handler::FireAndForgetHandler; use gated_groups::GatePayment; -use group_chat_core::{ - AddResult as AddMemberResult, GroupChatCore, GroupMemberInternal, GroupRoleInternal, InvitedUsersResult, UserInvitation, -}; +use group_chat_core::{AddResult as AddMemberResult, GroupChatCore, GroupMemberInternal, InvitedUsersResult, UserInvitation}; use group_community_common::{ Achievements, ExpiringMemberActions, ExpiringMembers, PaymentReceipts, PaymentRecipient, PendingPayment, PendingPaymentReason, PendingPaymentsQueue, UserCache, @@ -127,14 +125,7 @@ impl RuntimeState { pub fn queue_access_gate_payments(&mut self, payment: GatePayment) { // Queue a payment to each owner less the fee - let owners: Vec = self - .data - .chat - .members - .iter() - .filter(|m| matches!(m.role().value, GroupRoleInternal::Owner)) - .map(|m| m.user_id()) - .collect(); + let owners = self.data.chat.members.owners(); let owner_count = owners.len() as u128; let owner_share = (payment.amount * (100 - SNS_FEE_SHARE_PERCENT) / 100) / owner_count; @@ -144,7 +135,7 @@ impl RuntimeState { amount: owner_share, fee: payment.fee, ledger_canister: payment.ledger_canister_id, - recipient: PaymentRecipient::Member(owner), + recipient: PaymentRecipient::Member(*owner), reason: PendingPaymentReason::AccessGate, }); } @@ -400,9 +391,9 @@ impl RuntimeState { public: group_chat_core.is_public.value, date_created: group_chat_core.date_created, members: group_chat_core.members.len(), - moderators: group_chat_core.members.moderator_count(), - admins: group_chat_core.members.admin_count(), - owners: group_chat_core.members.owner_count(), + moderators: group_chat_core.members.moderators().len() as u32, + admins: group_chat_core.members.admins().len() as u32, + owners: group_chat_core.members.owners().len() as u32, blocked: group_chat_core.members.blocked().len() as u32, invited: self.data.chat.invited_users.len() as u32, chat_metrics: group_chat_core.events.metrics().hydrate(), diff --git a/backend/canisters/group/impl/src/queries/c2c_name_and_members.rs b/backend/canisters/group/impl/src/queries/c2c_name_and_members.rs index b3db867001..d6a95fa0ec 100644 --- a/backend/canisters/group/impl/src/queries/c2c_name_and_members.rs +++ b/backend/canisters/group/impl/src/queries/c2c_name_and_members.rs @@ -10,7 +10,7 @@ fn c2c_name_and_members(_args: Args) -> Response { } fn c2c_name_and_members_impl(state: &RuntimeState) -> Response { - let members = state.data.chat.members.iter().map(|p| p.user_id()).collect(); + let members = state.data.chat.members.member_ids().iter().copied().collect(); Success(SuccessResult { name: state.data.chat.name.value.clone(), diff --git a/backend/canisters/group/impl/src/queries/selected_initial.rs b/backend/canisters/group/impl/src/queries/selected_initial.rs index 4a7de73e67..1d0066d58b 100644 --- a/backend/canisters/group/impl/src/queries/selected_initial.rs +++ b/backend/canisters/group/impl/src/queries/selected_initial.rs @@ -1,7 +1,7 @@ use crate::{read_state, RuntimeState}; use canister_api_macros::query; use group_canister::selected_initial::{Response::*, *}; -use types::{GroupMember, GroupRole}; +use std::collections::HashSet; #[query(candid = true, msgpack = true)] fn selected_initial(_args: Args) -> Response { @@ -15,14 +15,22 @@ fn selected_initial_impl(state: &RuntimeState) -> Response { let chat = &state.data.chat; let last_updated = chat.details_last_updated(); + let mut non_basic_members = HashSet::new(); + non_basic_members.extend(chat.members.owners().iter().copied()); + non_basic_members.extend(chat.members.admins().iter().copied()); + non_basic_members.extend(chat.members.moderators().iter().copied()); + non_basic_members.extend(chat.members.lapsed().iter().copied()); + let mut members = Vec::new(); let mut basic_members = Vec::new(); - for member in chat.members.iter().map(GroupMember::from) { - if matches!(member.role, GroupRole::Participant) && !member.lapsed { - basic_members.push(member.user_id); + for user_id in chat.members.member_ids().iter() { + if non_basic_members.contains(user_id) { + if let Some(member) = chat.members.get(user_id) { + members.push(member.into()); + } + } else { + basic_members.push(*user_id); } - // Once website is upgraded, only push to `members` if not added to `basic_members` - members.push(member); } Success(SuccessResult { diff --git a/backend/canisters/group/impl/src/updates/c2c_delete_group.rs b/backend/canisters/group/impl/src/updates/c2c_delete_group.rs index 5fd737e363..796b6f635c 100644 --- a/backend/canisters/group/impl/src/updates/c2c_delete_group.rs +++ b/backend/canisters/group/impl/src/updates/c2c_delete_group.rs @@ -49,7 +49,7 @@ fn prepare(state: &RuntimeState) -> Result { group_index_canister_id: state.data.group_index_canister_id, deleted_by: member.user_id(), group_name: state.data.chat.name.value.clone(), - members: state.data.chat.members.iter().map(|m| m.user_id()).collect(), + members: state.data.chat.members.member_ids().iter().copied().collect(), }) } } else { diff --git a/backend/canisters/group/impl/src/updates/c2c_freeze_group.rs b/backend/canisters/group/impl/src/updates/c2c_freeze_group.rs index 6ccfad8627..a02fcb2d42 100644 --- a/backend/canisters/group/impl/src/updates/c2c_freeze_group.rs +++ b/backend/canisters/group/impl/src/updates/c2c_freeze_group.rs @@ -48,7 +48,7 @@ pub(crate) fn freeze_group_impl( handle_activity_notification(state); if return_members { - SuccessWithMembers(event, state.data.chat.members.iter().map(|p| p.user_id()).collect()) + SuccessWithMembers(event, state.data.chat.members.member_ids().iter().cloned().collect()) } else { Success(event) } diff --git a/backend/canisters/group/impl/src/updates/update_group_v2.rs b/backend/canisters/group/impl/src/updates/update_group_v2.rs index e247501b34..20cd82423e 100644 --- a/backend/canisters/group/impl/src/updates/update_group_v2.rs +++ b/backend/canisters/group/impl/src/updates/update_group_v2.rs @@ -5,7 +5,7 @@ use canister_api_macros::update; use canister_tracing_macros::trace; use group_canister::update_group_v2::*; use group_chat_core::UpdateResult; -use group_community_common::{ExpiringMember, Member}; +use group_community_common::{ExpiringMember, Members}; use group_index_canister::{c2c_make_private, c2c_update_group}; use tracing::error; use types::{AccessGateConfigInternal, CanisterId, ChatId, Document, OptionUpdate, TimestampMillis, UserId}; @@ -211,11 +211,11 @@ pub fn update_member_expiry(data: &mut Data, prev_gate_config: &Option, - pub blocked: HashSet, - pub moderator_count: u32, - pub admin_count: u32, - pub owner_count: u32, + members: HashMap, + member_ids: BTreeSet, + owners: BTreeSet, + admins: BTreeSet, + moderators: BTreeSet, + bots: BTreeMap, + lapsed: BTreeSet, + blocked: BTreeSet, + updates: BTreeSet<(TimestampMillis, UserId, MemberUpdate)>, +} + +#[derive(Serialize, Deserialize, Default)] +pub struct GroupMembersPrevious { + #[serde(serialize_with = "serialize_members", deserialize_with = "deserialize_members")] + members: HashMap, + blocked: BTreeSet, updates: BTreeSet<(TimestampMillis, UserId, MemberUpdate)>, } +impl From for GroupMembers { + fn from(value: GroupMembersPrevious) -> Self { + let mut member_ids = BTreeSet::new(); + let mut owners = BTreeSet::new(); + let mut admins = BTreeSet::new(); + let mut moderators = BTreeSet::new(); + let mut bots = BTreeMap::new(); + let mut lapsed = BTreeSet::new(); + + for member in value.members.values() { + member_ids.insert(member.user_id); + + match member.role.value { + GroupRoleInternal::Owner => owners.insert(member.user_id), + GroupRoleInternal::Admin => admins.insert(member.user_id), + GroupRoleInternal::Moderator => moderators.insert(member.user_id), + GroupRoleInternal::Member => false, + }; + + if member.lapsed.value { + lapsed.insert(member.user_id); + } + + if member.user_type.is_bot() { + bots.insert(member.user_id, member.user_type); + } + } + + GroupMembers { + members: value.members, + member_ids, + owners, + admins, + moderators, + bots, + lapsed, + blocked: value.blocked, + updates: value.updates, + } + } +} + #[derive(Serialize_repr, Deserialize_repr, Clone, Copy, Eq, PartialEq, Ord, PartialOrd)] #[repr(u8)] pub enum MemberUpdate { @@ -64,10 +120,17 @@ impl GroupMembers { GroupMembers { members: vec![(creator_user_id, member)].into_iter().collect(), - blocked: HashSet::new(), - moderator_count: 0, - admin_count: 0, - owner_count: 1, + member_ids: [creator_user_id].into_iter().collect(), + owners: [creator_user_id].into_iter().collect(), + admins: BTreeSet::new(), + moderators: BTreeSet::new(), + blocked: BTreeSet::new(), + bots: if user_type.is_bot() { + [(creator_user_id, user_type)].into_iter().collect() + } else { + BTreeMap::new() + }, + lapsed: BTreeSet::new(), updates: BTreeSet::new(), } } @@ -85,43 +148,49 @@ impl GroupMembers { AddResult::Blocked } else if let Some(limit) = self.user_limit_reached() { AddResult::MemberLimitReached(limit) - } else { - match self.members.entry(user_id) { - Vacant(e) => { - let member = GroupMemberInternal { - user_id, - date_added: now, - role: Timestamped::new(GroupRoleInternal::Member, now), - min_visible_event_index, - min_visible_message_index, - notifications_muted: Timestamped::new(notifications_muted, now), - mentions: Mentions::default(), - followed_threads: TimestampedSet::new(), - unfollowed_threads: TimestampedSet::new(), - proposal_votes: BTreeMap::default(), - suspended: Timestamped::default(), - rules_accepted: None, - user_type, - lapsed: Timestamped::new(false, now), - }; - e.insert(member.clone()); - self.updates.insert((now, user_id, MemberUpdate::Added)); - AddResult::Success(AddMemberSuccess { member, unlapse: false }) - } - _ => AddResult::AlreadyInGroup, + } else if self.member_ids.insert(user_id) { + let member = GroupMemberInternal { + user_id, + date_added: now, + role: Timestamped::new(GroupRoleInternal::Member, now), + min_visible_event_index, + min_visible_message_index, + notifications_muted: Timestamped::new(notifications_muted, now), + mentions: Mentions::default(), + followed_threads: TimestampedSet::new(), + unfollowed_threads: TimestampedSet::new(), + proposal_votes: BTreeMap::default(), + suspended: Timestamped::default(), + rules_accepted: None, + user_type, + lapsed: Timestamped::new(false, now), + }; + self.members.insert(user_id, member.clone()); + if user_type.is_bot() { + self.bots.insert(user_id, user_type); } + self.updates.insert((now, user_id, MemberUpdate::Added)); + AddResult::Success(AddMemberSuccess { member, unlapse: false }) + } else { + AddResult::AlreadyInGroup } } pub fn remove(&mut self, user_id: UserId, now: TimestampMillis) -> Option { if let Some(member) = self.members.remove(&user_id) { match member.role.value { - GroupRoleInternal::Owner => self.owner_count -= 1, - GroupRoleInternal::Admin => self.admin_count -= 1, - GroupRoleInternal::Moderator => self.moderator_count -= 1, - _ => (), + GroupRoleInternal::Owner => self.owners.remove(&user_id), + GroupRoleInternal::Admin => self.admins.remove(&user_id), + GroupRoleInternal::Moderator => self.moderators.remove(&user_id), + _ => false, + }; + if member.user_type.is_bot() { + self.bots.remove(&user_id); } - + if member.lapsed.value { + self.lapsed.remove(&user_id); + } + self.member_ids.remove(&user_id); self.updates.insert((now, user_id, MemberUpdate::Removed)); Some(member) } else { @@ -151,8 +220,8 @@ impl GroupMembers { self.blocked.iter().copied().collect() } - pub fn iter(&self) -> impl Iterator { - self.members.values() + pub fn member_ids(&self) -> &BTreeSet { + &self.member_ids } pub fn iter_mut(&mut self) -> impl Iterator { @@ -202,7 +271,7 @@ impl GroupMembers { now: TimestampMillis, ) -> ChangeRoleResult { // Is the caller authorized to change the user to this role - match self.get(&caller_id) { + match self.members.get(&caller_id) { Some(p) => { if p.suspended.value { return ChangeRoleResult::UserSuspended; @@ -217,11 +286,7 @@ impl GroupMembers { None => return ChangeRoleResult::UserNotInGroup, } - let mut owner_count = self.owner_count; - let mut admin_count = self.admin_count; - let mut moderator_count = self.moderator_count; - - let member = match self.get_mut(&user_id) { + let member = match self.members.get_mut(&user_id) { Some(p) => p, None => return ChangeRoleResult::TargetUserNotInGroup, }; @@ -232,7 +297,7 @@ impl GroupMembers { } // It is not possible to change the role of the last owner - if member.role.is_owner() && owner_count <= 1 { + if member.role.is_owner() && self.owners.len() <= 1 { return ChangeRoleResult::Invalid; } @@ -243,33 +308,37 @@ impl GroupMembers { } match prev_role { - GroupRoleInternal::Owner => owner_count -= 1, - GroupRoleInternal::Admin => admin_count -= 1, - GroupRoleInternal::Moderator => moderator_count -= 1, - _ => (), - } + GroupRoleInternal::Owner => self.owners.remove(&user_id), + GroupRoleInternal::Admin => self.admins.remove(&user_id), + GroupRoleInternal::Moderator => self.moderators.remove(&user_id), + _ => false, + }; member.role = Timestamped::new(new_role, now); match new_role { - GroupRoleInternal::Owner => owner_count += 1, - GroupRoleInternal::Admin => admin_count += 1, - GroupRoleInternal::Moderator => moderator_count += 1, - _ => (), - } + GroupRoleInternal::Owner => { + if member.lapsed.value { + self.update_lapsed(user_id, false, now); + } + self.owners.insert(user_id) + } + GroupRoleInternal::Admin => self.admins.insert(user_id), + GroupRoleInternal::Moderator => self.moderators.insert(user_id), + _ => false, + }; - self.owner_count = owner_count; - self.admin_count = admin_count; - self.moderator_count = moderator_count; self.updates.insert((now, user_id, MemberUpdate::RoleChanged)); ChangeRoleResult::Success(ChangeRoleSuccess { prev_role }) } pub fn unlapse_all(&mut self, now: TimestampMillis) { - for m in self.members.values_mut() { - if m.set_lapsed(false, now) { - self.updates.insert((now, m.user_id, MemberUpdate::Unlapsed)); + for user_id in std::mem::take(&mut self.lapsed) { + if let Some(member) = self.members.get_mut(&user_id) { + if member.set_lapsed(false, now) { + self.updates.insert((now, member.user_id, MemberUpdate::Unlapsed)); + } } } } @@ -287,6 +356,12 @@ impl GroupMembers { }; if updated { + if lapse { + self.lapsed.insert(user_id); + } else { + self.lapsed.remove(&user_id); + } + self.updates.insert(( now, user_id, @@ -295,16 +370,24 @@ impl GroupMembers { } } - pub fn owner_count(&self) -> u32 { - self.owner_count + pub fn owners(&self) -> &BTreeSet { + &self.owners } - pub fn admin_count(&self) -> u32 { - self.admin_count + pub fn admins(&self) -> &BTreeSet { + &self.admins } - pub fn moderator_count(&self) -> u32 { - self.moderator_count + pub fn moderators(&self) -> &BTreeSet { + &self.moderators + } + + pub fn bots(&self) -> &BTreeMap { + &self.bots + } + + pub fn lapsed(&self) -> &BTreeSet { + &self.lapsed } pub fn has_membership_changed(&self, since: TimestampMillis) -> bool { @@ -323,6 +406,36 @@ impl GroupMembers { pub fn last_updated(&self) -> Option { self.updates.iter().next_back().map(|(ts, _, _)| *ts) } + + #[cfg(test)] + fn check_invariants(&self) { + let mut member_ids = BTreeSet::new(); + let mut owners = BTreeSet::new(); + let mut admins = BTreeSet::new(); + let mut moderators = BTreeSet::new(); + let mut lapsed = BTreeSet::new(); + + for member in self.members.values() { + member_ids.insert(member.user_id); + + match member.role.value { + GroupRoleInternal::Owner => owners.insert(member.user_id), + GroupRoleInternal::Admin => admins.insert(member.user_id), + GroupRoleInternal::Moderator => moderators.insert(member.user_id), + GroupRoleInternal::Member => false, + }; + + if member.lapsed.value { + lapsed.insert(member.user_id); + } + } + + assert_eq!(member_ids, self.member_ids); + assert_eq!(owners, self.owners); + assert_eq!(admins, self.admins); + assert_eq!(moderators, self.moderators); + assert_eq!(lapsed, self.lapsed); + } } impl Members for GroupMembers { @@ -331,6 +444,15 @@ impl Members for GroupMembers { fn get(&self, user_id: &UserId) -> Option<&GroupMemberInternal> { self.get(user_id) } + + fn iter_members_who_can_lapse(&self) -> Box + '_> { + Box::new( + self.member_ids + .iter() + .filter(|id| !self.owners.contains(id) && !self.lapsed.contains(id)) + .copied(), + ) + } } #[allow(clippy::large_enum_variant)] diff --git a/backend/libraries/group_chat_core/src/members/proptests.rs b/backend/libraries/group_chat_core/src/members/proptests.rs new file mode 100644 index 0000000000..dc5b03582f --- /dev/null +++ b/backend/libraries/group_chat_core/src/members/proptests.rs @@ -0,0 +1,140 @@ +use crate::{GroupMembers, GroupRoleInternal}; +use candid::Principal; +use proptest::collection::vec as pvec; +use proptest::prelude::*; +use proptest::prop_oneof; +use std::collections::BTreeSet; +use test_strategy::proptest; +use types::{EventIndex, GroupPermissions, MessageIndex, TimestampMillis, UserId, UserType}; + +#[derive(Debug, Clone)] +enum Operation { + Add { + user_id: UserId, + }, + ChangeRole { + owner_index: usize, + user_index: usize, + role: GroupRoleInternal, + }, + Remove { + user_index: usize, + }, + Block { + user_index: usize, + }, + Unblock { + user_index: usize, + }, + Lapse { + user_index: usize, + }, + Unlapse { + user_index: usize, + }, + UnlapseAll, +} + +fn operation_strategy() -> impl Strategy { + prop_oneof![ + 50 => any::().prop_map(|user_index| Operation::Add { user_id: user_id(user_index) }), + 20 => (any::(), any::(), any::()) + .prop_map(|(owner_index, user_index, role_index)| Operation::ChangeRole { owner_index, user_index, role: role(role_index) }), + 10 => any::().prop_map(|user_index| Operation::Remove { user_index}), + 5 => any::().prop_map(|user_index| Operation::Block { user_index}), + 3 => any::().prop_map(|user_index| Operation::Unblock { user_index}), + 5 => any::().prop_map(|user_index| Operation::Lapse { user_index}), + 3 => any::().prop_map(|user_index| Operation::Unlapse { user_index}), + 1 => Just(Operation::UnlapseAll), + ] +} + +#[proptest(cases = 10)] +fn comprehensive(#[strategy(pvec(operation_strategy(), 100..5_000))] ops: Vec) { + let mut members = GroupMembers::new(user_id(0), UserType::User, 0); + + let mut timestamp = 1000; + for op in ops.into_iter() { + execute_operation(&mut members, op, timestamp); + timestamp += 1000; + } + + members.check_invariants(); +} + +fn execute_operation(members: &mut GroupMembers, op: Operation, timestamp: TimestampMillis) { + match op { + Operation::Add { user_id } => { + members.add( + user_id, + timestamp, + EventIndex::default(), + MessageIndex::default(), + false, + UserType::User, + ); + } + Operation::ChangeRole { + owner_index, + user_index, + role, + } => { + let owner = get(&members.owners, owner_index); + let user_id = get(&members.member_ids, user_index); + members.change_role(owner, user_id, role, &GroupPermissions::default(), false, false, timestamp); + } + Operation::Remove { user_index } => { + let user_id = get(&members.member_ids, user_index); + if members.owners.len() != 1 || members.owners.first() != Some(&user_id) { + members.remove(user_id, timestamp); + } + } + Operation::Block { user_index } => { + let user_id = get(&members.member_ids, user_index); + if members.owners.len() != 1 || members.owners.first() != Some(&user_id) { + members.remove(user_id, timestamp); + members.block(user_id, timestamp); + } + } + Operation::Unblock { user_index } => { + if !members.blocked.is_empty() { + let user_id = get(&members.blocked, user_index); + members.unblock(user_id, timestamp); + } + } + Operation::Lapse { user_index } => { + let user_id = get(&members.member_ids, user_index); + members.update_lapsed(user_id, true, timestamp); + } + Operation::Unlapse { user_index } => { + if !members.lapsed.is_empty() { + let user_id = get(&members.lapsed, user_index); + members.update_lapsed(user_id, false, timestamp); + } + } + Operation::UnlapseAll => { + members.unlapse_all(timestamp); + } + }; +} + +fn get(set: &BTreeSet, index: usize) -> UserId { + let index = index % set.len(); + *set.iter().nth(index).unwrap() +} + +fn user_id(index: usize) -> UserId { + Principal::from_slice(&index.to_be_bytes()).into() +} + +fn role(value: usize) -> GroupRoleInternal { + let index = value % 4; + + match index { + 0 => GroupRoleInternal::Owner, + 1 => GroupRoleInternal::Admin, + 2 => GroupRoleInternal::Moderator, + 3 => GroupRoleInternal::Member, + _ => unreachable!(), + } +} diff --git a/backend/libraries/group_community_common/src/member.rs b/backend/libraries/group_community_common/src/member.rs index 39b93e47c1..24a650a799 100644 --- a/backend/libraries/group_community_common/src/member.rs +++ b/backend/libraries/group_community_common/src/member.rs @@ -16,6 +16,8 @@ pub trait Members { fn get(&self, user_id: &UserId) -> Option<&Self::Member>; + fn iter_members_who_can_lapse(&self) -> Box + '_>; + fn can_member_lapse(&self, user_id: &UserId) -> bool { self.get(user_id).map_or(false, |m| m.can_member_lapse()) }