Verify membership when test if name is ambiguous by multisme · Pull Request #5780 · matrix-org/matrix-rust-sdk

multisme · 2025-10-13T20:22:14Z

Verify if the membership is not Leave when checking if a name in a room is ambiguous.

Signed-off-by:
multi

multisme · 2025-10-13T20:22:46Z

It's pretty straightforward but should I had some tests?

codecov · 2025-10-13T20:38:39Z

Codecov Report

❌ Patch coverage is 98.79518% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.47%. Comparing base (a4e68ba) to head (2892f60).
⚠️ Report is 480 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-base/src/room/members.rs	98.79%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5780      +/-   ##
==========================================
+ Coverage   88.45%   88.47%   +0.02%     
==========================================
  Files         360      360              
  Lines      100322   100400      +78     
  Branches   100322   100400      +78     
==========================================
+ Hits        88737    88830      +93     
+ Misses       7420     7403      -17     
- Partials     4165     4167       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

codspeed-hq · 2025-10-13T20:40:21Z

CodSpeed Performance Report

Merging #5780 will not alter performance

_{Comparing multisme:fix_check_if_user_really_ambiguous (2892f60) with main (cd9f433)}

Summary

✅ 50 untouched

stefanceriu · 2025-10-14T07:25:22Z

crates/matrix-sdk-base/src/room/members.rs

-            .is_some_and(|s| is_display_name_ambiguous(&display_name, s));
+        let membership = event.membership();
+        let display_name_ambiguous = users_display_names.get(&display_name).is_some_and(|s| {
+            is_display_name_ambiguous(&display_name, s) && *membership != MembershipState::Leave


So we don't want to disambiguate the name of a left user, why is that? I see the AmbiguityCache has a concept of active members so what are we trying to achieve here?

It's because of the spec. It only sees member with an join or an invite membership as needing disambiguation

I updated the code to follow more precisely the spec.

Why not updating is_display_name_ambiguous to include this membership constraint? It seems to be really part of it.

bnjbvr

Thanks for the patch! Can you add a test please? Ideally it would show the case that's failing on main (but supposed to work), and that would be working with your patch now.

bnjbvr · 2025-10-14T13:35:06Z

crates/matrix-sdk-base/src/room/members.rs

+            is_display_name_ambiguous(&display_name, s)
+                && (*membership == MembershipState::Join || *membership == MembershipState::Invite)


Can you write the second part with a match statement, please? This will make it so that we explicitly handle all the membership cases, and we don't forget to think about handling it, would we have a new membership kind later on.

if !is_display_name_ambiguous(&display_name, s) { return false; } match *membership { // handle all variants explicitly here }

I think it's clearly worth adding this logic inside is_display_name_ambiguous.

multisme · 2025-10-14T19:09:48Z

@bnjbvr
I have a problem here. There is a test that is failing that should be failing

matrix-rust-sdk/crates/matrix-sdk-base/src/room/members.rs

Lines 615 to 622 in 7a93c57

    
           assert_eq!(room.get_member(me).await.unwrap().expect("Me user").display_name(), Some("Me")); 
        
           assert_eq!( 
        
               room.get_member(mewto).await.unwrap().expect("Mewto user").display_name(), 
        
               Some("Me") 
        
           ); 
        
           assert!(room.get_member(me).await.unwrap().expect("Me user").name_ambiguous()); 
        
           assert!(room.get_member(mewto).await.unwrap().expect("Mewto user").name_ambiguous());

Unless I misunderstood, the name should be here ambiguous and the test should on line 621 should not be failing. Should I make a new issue ?

bnjbvr · 2025-10-15T08:25:46Z

It sounds like the code you've changed introduced a regression, so, no, we can't merge this as is and open a new issue. Instead, one should investigate the issue and attempt to fix it :)

multisme · 2025-10-15T12:27:35Z

So i checked, and it seems that when using saving_changes to store the member events in MemoryStore, the HashMap display_names: HashMap<OwnedRoomId, HashMap<DisplayName, BTreeSet<OwnedUserId>>> is never updated unless you expressly do it. But this is the Hashmap used by is_the_display_name_ambiguous.
If this is not the behavior expected, I can make an issue and a PR for making sure that hashmap is populated, wait for it to get merged, and then finish this PR.

multisme · 2025-10-15T12:29:40Z

@bnjbvr
So i checked, and it seems that when using saving_changes to store the member events in MemoryStore, the HashMap display_names: HashMap<OwnedRoomId, HashMap<DisplayName, BTreeSet<OwnedUserId>>> is never updated unless you expressly do it. But this is the Hashmap used by is_the_display_name_ambiguous.
If this is not the behavior expected, I can make an issue and a PR for making sure that hashmap is populated, wait for it to get merged, and then finish this PR.

multisme · 2025-10-15T14:43:31Z

I made a mistake anyway, I should check the membership of every RoomMember with the same displayname, I'm getting back into IT.

…emberRoomInfo

multisme · 2025-11-14T21:52:12Z

@poljar Should I make an issue regarding this?

So i checked, and it seems that when using saving_changes to store the member events in MemoryStore, the HashMap display_names: HashMap<OwnedRoomId, HashMap<DisplayName, BTreeSet<OwnedUserId>>> is never updated unless you expressly do it. But this is the Hashmap used by is_the_display_name_ambiguous. If this is not the behavior expected, I can make an issue and a PR for making sure that hashmap is populated, wait for it to get merged, and then finish this PR.

zecakeh · 2025-12-12T21:18:41Z

Just my 2 cents, but why don't we keep only the active members in the display names cache, rather than having unnecessary data in it, and then use a list of active members to filter it?

multisme · 2025-12-17T21:24:51Z

tbh that would make sense, but that would be a breaking change.

zecakeh · 2025-12-18T08:56:52Z

Why would this be a breaking change? Isn't this data used only internally when building a RoomMember?

Hywan

Nice contribution, thank you.

I think we should merge the logic inside is_display_name_ambiguous to make less error-prone. What do you think?

Hywan · 2025-12-19T12:43:39Z

crates/matrix-sdk-base/src/room/members.rs

-            .is_some_and(|s| is_display_name_ambiguous(&display_name, s));
+        let membership = event.membership();
+        let display_name_ambiguous = users_display_names.get(&display_name).is_some_and(|s| {
+            is_display_name_ambiguous(&display_name, s) && *membership != MembershipState::Leave


Why not updating is_display_name_ambiguous to include this membership constraint? It seems to be really part of it.

Hywan · 2025-12-19T12:44:35Z

crates/matrix-sdk-base/src/room/members.rs

+            is_display_name_ambiguous(&display_name, s)
+                && (*membership == MembershipState::Join || *membership == MembershipState::Invite)


I think it's clearly worth adding this logic inside is_display_name_ambiguous.

Hywan · 2025-12-19T12:45:19Z

crates/matrix-sdk-base/src/room/members.rs

        let display_name = event.display_name();
        let membership = event.membership();
+
+        println!("{:?} {:?}", users_display_names, &display_name);


Should be removed.

Ok I will update is_display_name_ambiguous to take a MemberEvent instead of a string

multisme · 2025-12-21T21:39:31Z

I messed up the history big will force push because of it.

Hywan

My comments don't seem to be addressed. Is it on purpose :-)?

Hywan · 2026-01-06T11:06:04Z

crates/matrix-sdk-base/src/room/members.rs

-            .is_some_and(|s| is_display_name_ambiguous(&display_name, s));
+
+        let display_name_ambiguous = users_display_names.get(&display_name).is_some_and(|s| {
+            // s.filter(|n| )


should be removed

No, taking care of that now.

Ok I looked back into it, and the reason I didn't remove it was that I would need to make is_display_name_ambiguous accept the list of current active users, which imply a lot of changes in the matrix-sdk-base/src/store/ambiguity_map.rs, so I wanted to confirm before starting that work.

You can as new commits and see where it goes. If it's too complex, we will take another path. Thanks for asking.

multisme · 2026-01-15T11:20:05Z

@Hywan I have created a new function get_active_users_display_name to replace get_users_display_name in the proper situations but in the case of IndexDB store, I have to either do two queries or write a migration. Writing a migration sounds cleaner but it feels like I should check before starting.

multisme requested a review from a team as a code owner October 13, 2025 20:22

multisme requested review from stefanceriu and removed request for a team October 13, 2025 20:22

multisme force-pushed the fix_check_if_user_really_ambiguous branch from b1cd305 to 47488fd Compare October 13, 2025 20:24

stefanceriu reviewed Oct 14, 2025

View reviewed changes

multisme marked this pull request as draft October 14, 2025 08:22

multisme marked this pull request as ready for review October 14, 2025 13:16

multisme requested a review from stefanceriu October 14, 2025 13:17

bnjbvr requested changes Oct 14, 2025

View reviewed changes

bnjbvr removed the request for review from stefanceriu October 14, 2025 13:35

multisme marked this pull request as draft October 14, 2025 18:49

multisme force-pushed the fix_check_if_user_really_ambiguous branch from 6314fbb to 7a93c57 Compare October 14, 2025 18:52

multisme force-pushed the fix_check_if_user_really_ambiguous branch from b932ac2 to 1f759a6 Compare October 16, 2025 12:28

multisme marked this pull request as ready for review October 16, 2025 13:18

multisme added 7 commits October 17, 2025 10:43

Verify membership when test if name is ambiguous

b5316ef

Following the spec more precisely

932cea5

Separate both conditions for readability

79beb6c

Non working tests

7cd077d

Setting the map_ambiguity's changes key in the tests

48f8549

Solved the membership check by creating an active_users variable in M…

cd9e7e8

…emberRoomInfo

Formating imports

2892f60

multisme force-pushed the fix_check_if_user_really_ambiguous branch from 52fad95 to 2892f60 Compare October 17, 2025 08:43

multisme requested a review from bnjbvr October 17, 2025 09:32

poljar requested review from poljar and removed request for bnjbvr October 24, 2025 15:19

Hywan requested review from Hywan and removed request for poljar December 19, 2025 11:26

Hywan requested changes Dec 19, 2025

View reviewed changes

multisme requested a review from a team as a code owner December 21, 2025 21:31

multisme requested review from poljar and removed request for a team December 21, 2025 21:31

multisme force-pushed the fix_check_if_user_really_ambiguous branch from cce393c to 2892f60 Compare December 23, 2025 10:50

Hywan reviewed Jan 6, 2026

View reviewed changes

multisme added 2 commits January 6, 2026 18:37

Add new get_active_users_with_display_name trait

4ca8502

Add new get_active_users_with_display_name functions to sqlite store

b84b451

multisme marked this pull request as draft January 15, 2026 10:01

		is_display_name_ambiguous(&display_name, s)
		&& (membership == MembershipState::Join \|\| membership == MembershipState::Invite)

Conversation

multisme commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

multisme commented Oct 13, 2025

Uh oh!

codecov bot commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codspeed-hq bot commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #5780 will not alter performance

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

multisme Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnjbvr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

multisme commented Oct 14, 2025

Uh oh!

bnjbvr commented Oct 15, 2025

Uh oh!

multisme commented Oct 15, 2025

Uh oh!

multisme commented Oct 15, 2025

Uh oh!

multisme commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

multisme commented Nov 14, 2025

Uh oh!

zecakeh commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

multisme commented Dec 17, 2025

Uh oh!

zecakeh commented Dec 18, 2025

Uh oh!

Hywan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

multisme commented Dec 21, 2025

Uh oh!

Hywan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

multisme Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

multisme commented Oct 13, 2025 •

edited

Loading

codecov bot commented Oct 13, 2025 •

edited

Loading

codspeed-hq bot commented Oct 13, 2025 •

edited

Loading

multisme Oct 14, 2025 •

edited

Loading

multisme commented Oct 15, 2025 •

edited

Loading

zecakeh commented Dec 12, 2025 •

edited

Loading

multisme Jan 6, 2026 •

edited

Loading