get_users_who_share_room_with_user(...) should use the users_who_share_private_rooms/users_in_public_rooms tables instead of calculating it themselves again (get_users_in_room mis-use)

[MadLittleMods]

get_users_who_share_room_with_user(...) should use the users_who_share_private_rooms/users_in_public_rooms tables instead of calculating it themselves again.

This might be a nice optimization for the presence and device list sharing code where get_users_who_share_room_with_user is used.

synapse/synapse/storage/databases/main/roommember.py

Lines 768 to 777 in 6f0c3e6

	async def get_users_who_share_room_with_user(self, user_id: str) -> Set[str]:
	"""Returns the set of users who share a room with `user_id`"""
	room_ids = await self.get_rooms_for_user(user_id)
	user_who_share_room = set()
	for room_id in room_ids:
	user_ids = await self.get_users_in_room(room_id)
	user_who_share_room.update(user_ids)
	return user_who_share_room

---

Discovered while working on #13958 and seeing code for users sharing rooms in #13966

See #13575 (comment) for the original exploration around finding get_users_in_room mis-uses.

[DMRobertson]

IIRC those tables are populated by the user directory machinery. I vaguely remember that they were a little bit dodgy or inconsistent and had caveats to them: in particular a certain class of users were excluded from the user-sharing tables I think?

synapse/synapse/storage/databases/main/user_directory.py

Lines 399 to 433 in 888a29f

	async def should_include_local_user_in_dir(self, user: str) -> bool:
	"""Certain classes of local user are omitted from the user directory.
	Is this user one of them?
	"""
	# We're opting to exclude the appservice sender (user defined by the
	# `sender_localpart` in the appservice registration) even though
	# technically it could be DM-able. In the future, this could potentially
	# be configurable per-appservice whether the appservice sender can be
	# contacted.
	if self.get_app_service_by_user_id(user) is not None: # type: ignore[attr-defined]
	return False
	# We're opting to exclude appservice users (anyone matching the user
	# namespace regex in the appservice registration) even though technically
	# they could be DM-able. In the future, this could potentially
	# be configurable per-appservice whether the appservice users can be
	# contacted.
	if self.get_if_app_services_interested_in_user(user): # type: ignore[attr-defined]
	# TODO we might want to make this configurable for each app service
	return False
	# Support users are for diagnostics and should not appear in the user directory.
	if await self.is_support_user(user): # type: ignore[attr-defined]
	return False
	# Deactivated users aren't contactable, so should not appear in the user directory.
	try:
	if await self.get_user_deactivated_status(user): # type: ignore[attr-defined]
	return False
	except StoreError:
	# No such user in the users table. No need to do this when calling
	# is_support_user---that returns False if the user is missing.
	return False
	return True

I'm struggling to find good references now for "a little bit dodgy". The best I can find is:

• Better handling of appservices in user directory #11052
• User directory should handle changes in user types #11185

For context, I did a bunch of work around the user directory a year ago as part of addressing #5677 for local users. (Regrettably we do not yet have a good fix for this for remote users at present. It is tricky to do so without loss of functionality because per-room profiles are an "accident" of the protocol; or rather, the protocol has no profile-synchronising mechanism.)

[MadLittleMods]

Ahh, good point @DMRobertson! I think this wouldn't matter for the presence stuff but it probably does matter for the device list stuff (get_device_changes_in_shared_rooms) because things need to get encrypted/decrypted for support users as well for example.

Maybe we should close this then?

[DMRobertson]

Maybe... but if this function is a bottleneck then maybe we should consider other approaches.

Another angle might be to expand the room-sharing tables to hold information about all local users, and then add an extra boolean column which indicates whether each row is relevant for the user directory.