Avoid cases where we were iterating over the full chat members map (#…

…6853)

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"

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
 

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<UserId, UserType> = 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

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,

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<dyn Iterator<Item = UserId> + '_> {
+        Box::new(self.members.values().filter(|m| m.can_member_lapse()).map(|m| m.user_id))
+    }
 }
 
 #[derive(Serialize, Deserialize, Clone)]

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);
             }
         }
 

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);

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