matrix-rust-sdk icon indicating copy to clipboard operation
matrix-rust-sdk copied to clipboard

`room::Common::members{_no_sync}()` only returns active members with the Sled and IndexedDb stores

Open zecakeh opened this issue 1 year ago • 4 comments

The title is pretty self-explanatory. We were trying to get all members from the store except those for Membership::Leave so we called room::Common::members() and tried to filter it and realized that it doesn't return banned or left members although we were sure there were some.

After some investigating, it turns out that room::Common::members_no_sync() calls BaseRoom::members() that in turn calls StateStore::get_user_ids() to get the list of user IDs in the room.

However, both with the Sled and IndexedDb stores, this list is built from the get_joined_user_ids() and get_invited_user_ids() lists so it returns the same list as calling BaseRoom::active_members().

The memory store doesn't have that issue because it builds the list from the member events for the given room. It's probably what the other stores should do.

It could probably be improved by adding directly a StateStore::get_room_members() method, to avoid requesting the store once to get the member events and filter the user IDs, and then iteratively get the member events one after the other. If the store cannot request all events directly, it should be left as an implementation detail.

Alternatively we could have left, banned and knocked lists (the last two are probably interesting to have anyway) and build the list from it, but it would break again when a new membership is added.

zecakeh avatar Aug 07 '22 09:08 zecakeh

I think "members of the room" is a somewhat fuzzy concept. If you know how it's represented it's natural to consider every user who has an m.room.member state event, for the purposes of showing a member list you likely want to include invited users and those who knocked, but kicked users are generally not shown while banned users are usually hidden away somewhere but still accessible (so you can review that list and un-ban people).

Maybe we can have a bitset RoomMemberFilter with flags for joined, invited, knocked, left, kicked and banned with the Default being joined | invited | knocked?

jplatte avatar Aug 07 '22 12:08 jplatte

Maybe we can have a bitset RoomMemberFilter with flags for joined, invited, knocked, left, kicked and banned with the Default being joined | invited | knocked?

I like that idea. It avoids to have methods for all the different states.

zecakeh avatar Aug 08 '22 07:08 zecakeh

Do we want to deprecate all the other *_members methods and only keep one pair of sync/no-sync methods with the filter?

Should we also have a single get_user_ids() method on the StateStore trait with the same filter? And in the BaseRoom methods?

zecakeh avatar Aug 14 '22 08:08 zecakeh

I think all of those things make sense.

jplatte avatar Sep 22 '22 09:09 jplatte