From 2c7c5c223c0100362fb4fbccd74522c1fe9c8460 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 7 Oct 2021 16:56:50 -0500 Subject: [PATCH 01/14] Fix exception thrown when attempting to notify appservice sender Fix https://github.com/matrix-org/synapse/issues/11025 Before in [`user_directory_handler._handle_deltas`, we just checked for `is_support_user(user_id)`](https://github.com/matrix-org/synapse/pull/10960/files#diff-e02a9a371e03b8615b53c6b6552f76fc7d3ef58931ca64d28b3512caf305449fL232) which works just fine. Now with https://github.com/matrix-org/synapse/pull/10960, we [call `should_include_local_user_in_dir`](https://github.com/matrix-org/synapse/pull/10960/files#diff-e02a9a371e03b8615b53c6b6552f76fc7d3ef58931ca64d28b3512caf305449fR229) which does a [bunch of additional checks](https://github.com/matrix-org/synapse/blob/e79ee48313404abf8fbb7c88361e4ab1efa29a81/synapse/storage/databases/main/user_directory.py#L382-L398) which aren't all compatible with the main appservice sender (main bridge user/sender). More specifically, we can't check the `users` database table for whether the user is deactivated. In the `should_include_local_user_in_dir` checks, we should return early if we encounter a main appservice sender before the incompatible checks. --- .../storage/databases/main/user_directory.py | 4 ++++ tests/handlers/test_user_directory.py | 21 +++++++++++++++++++ tests/storage/test_user_directory.py | 15 +++++++++++++ 3 files changed, 40 insertions(+) diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index 5c713a732ee9..7957af19bfe4 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -383,6 +383,10 @@ 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? """ + # The main app service sender isn't usually contactable, so exclude them + if self.get_app_service_by_user_id(user) is not None: + return False + # App service users aren't usually contactable, so exclude them. if self.get_if_app_services_interested_in_user(user): # TODO we might want to make this configurable for each app service diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 03fd5a3e2c52..9f17662690ff 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -295,6 +295,27 @@ def test_handle_local_profile_change_with_appservice_user(self) -> None: profile = self.get_success(self.store.get_user_in_directory(as_user_id)) self.assertTrue(profile is None) + def test_handle_local_profile_change_with_appservice_sender(self) -> None: + # profile is not in directory + profile = self.get_success( + self.store.get_user_in_directory(self.appservice.sender) + ) + self.assertTrue(profile is None) + + # update profile + profile_info = ProfileInfo(avatar_url="avatar_url", display_name="4L1c3") + self.get_success( + self.handler.handle_local_profile_change( + self.appservice.sender, profile_info + ) + ) + + # profile is still not in directory + profile = self.get_success( + self.store.get_user_in_directory(self.appservice.sender) + ) + self.assertTrue(profile is None) + def test_handle_user_deactivated_support_user(self) -> None: s_user_id = "@support:test" self.get_success( diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 9f483ad681c6..3831fbc8ef2d 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -364,6 +364,21 @@ def test_population_excludes_appservice_user(self) -> None: # Check the AS user is not in the directory. self._check_room_sharing_tables(user, public, private) + def test_population_excludes_appservice_sender(self) -> None: + user = self.register_user("user", "pass") + token = self.login(user, "pass") + + # Join the AS sender to rooms owned by the normal user. + public, private = self._create_rooms_and_inject_memberships( + user, token, self.appservice.sender + ) + + # Rebuild the directory. + self._purge_and_rebuild_user_dir() + + # Check the AS sender is not in the directory. + self._check_room_sharing_tables(user, public, private) + def test_population_conceals_private_nickname(self) -> None: # Make a private room, and set a nickname within user = self.register_user("aaaa", "pass") From ede2a702111852c27cf0a640e16594fb6e3fbc3e Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 7 Oct 2021 18:14:25 -0500 Subject: [PATCH 02/14] Add changelog --- changelog.d/11026.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11026.bugfix diff --git a/changelog.d/11026.bugfix b/changelog.d/11026.bugfix new file mode 100644 index 000000000000..3bdcaf037ceb --- /dev/null +++ b/changelog.d/11026.bugfix @@ -0,0 +1 @@ +Fix exception thrown when attempting to notify appservice `sender` of new messages. From a92cad83092f6cd80f939e77d9732c6e7bccb5c7 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 8 Oct 2021 17:22:16 -0500 Subject: [PATCH 03/14] Adjust changelog to explain what/why --- changelog.d/11026.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/11026.bugfix b/changelog.d/11026.bugfix index 3bdcaf037ceb..09351e0fe035 100644 --- a/changelog.d/11026.bugfix +++ b/changelog.d/11026.bugfix @@ -1 +1 @@ -Fix exception thrown when attempting to notify appservice `sender` of new messages. +Exclude application service sender membership from user directory to fix a bug stopping the user directory from being populated. From 65232341d8a473ad63fa78b08dfcb83b9974d7a4 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 8 Oct 2021 17:28:53 -0500 Subject: [PATCH 04/14] Add better comment explanations --- synapse/storage/databases/main/user_directory.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index 7957af19bfe4..d2f85ee0c9ce 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -383,11 +383,19 @@ 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? """ - # The main app service sender isn't usually contactable, so exclude 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: return False - # App service users aren't usually contactable, so exclude them. + # 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): # TODO we might want to make this configurable for each app service return False From 8d2025c2c87e2c5251e304f5fc0f5b205300d4dd Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 8 Oct 2021 17:52:40 -0500 Subject: [PATCH 05/14] Move to more concise assertion method for the usage --- tests/handlers/test_user_directory.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index bf05be08b792..e889a8c6336e 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -230,7 +230,7 @@ def test_handle_local_profile_change_with_support_user(self) -> None: ) ) profile = self.get_success(self.store.get_user_in_directory(support_user_id)) - self.assertTrue(profile is None) + self.assertIsNone(profile) display_name = "display_name" profile_info = ProfileInfo(avatar_url="avatar_url", display_name=display_name) @@ -264,7 +264,7 @@ def test_handle_local_profile_change_with_deactivated_user(self) -> None: # profile is not in directory profile = self.get_success(self.store.get_user_in_directory(r_user_id)) - self.assertTrue(profile is None) + self.assertIsNone(profile) # update profile after deactivation self.get_success( @@ -273,7 +273,7 @@ def test_handle_local_profile_change_with_deactivated_user(self) -> None: # profile is furthermore not in directory profile = self.get_success(self.store.get_user_in_directory(r_user_id)) - self.assertTrue(profile is None) + self.assertIsNone(profile) def test_handle_local_profile_change_with_appservice_user(self) -> None: # create user @@ -283,7 +283,7 @@ def test_handle_local_profile_change_with_appservice_user(self) -> None: # profile is not in directory profile = self.get_success(self.store.get_user_in_directory(as_user_id)) - self.assertTrue(profile is None) + self.assertIsNone(profile) # update profile profile_info = ProfileInfo(avatar_url="avatar_url", display_name="4L1c3") @@ -293,14 +293,14 @@ def test_handle_local_profile_change_with_appservice_user(self) -> None: # profile is still not in directory profile = self.get_success(self.store.get_user_in_directory(as_user_id)) - self.assertTrue(profile is None) + self.assertIsNone(profile) def test_handle_local_profile_change_with_appservice_sender(self) -> None: # profile is not in directory profile = self.get_success( self.store.get_user_in_directory(self.appservice.sender) ) - self.assertTrue(profile is None) + self.assertIsNone(profile) # update profile profile_info = ProfileInfo(avatar_url="avatar_url", display_name="4L1c3") @@ -314,7 +314,7 @@ def test_handle_local_profile_change_with_appservice_sender(self) -> None: profile = self.get_success( self.store.get_user_in_directory(self.appservice.sender) ) - self.assertTrue(profile is None) + self.assertIsNone(profile) def test_handle_user_deactivated_support_user(self) -> None: s_user_id = "@support:test" From ff6212a5a5d9f5a43f8436ad6039819eb3b86bd4 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 11 Oct 2021 17:29:07 +0100 Subject: [PATCH 06/14] Add test covering appservice sender joining a room I don't think this actually improves coverage as such, but I'll sleep better at night having this case checked! --- tests/handlers/test_user_directory.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index e889a8c6336e..1b495695d9ce 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -63,7 +63,9 @@ def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: hostname="test", id="1234", namespaces={"users": [{"regex": r"@as_user.*", "exclusive": True}]}, - sender="@as:test", + # Note: this user does not match the regex above, so that tests + # can distinguish the sender from the AS user. + sender="@as_main:test", ) mock_load_appservices = Mock(return_value=[self.appservice]) @@ -179,6 +181,15 @@ def test_excludes_appservices_user(self) -> None: ) self._check_only_one_user_in_directory(user, public) + def test_excludes_appservice_sender(self) -> None: + user = self.register_user("user", "pass") + token = self.login(user, "pass") + room = self.helper.create_room_as( + self.appservice.sender, is_public=True, tok=self.appservice.token + ) + self.helper.join(room, user, tok=token) + self._check_only_one_user_in_directory(user, room) + def _create_rooms_and_inject_memberships( self, creator: str, token: str, joiner: str ) -> Tuple[str, str]: From 3c053fdd510afa9baccb6c51b16e8618ca690c72 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 11 Oct 2021 18:20:08 +0100 Subject: [PATCH 07/14] On reflection, INCLUDE the appservice senders That's the current behaviour (I didn't realise they typically don't match their own user regex). --- synapse/storage/databases/main/user_directory.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index d2f85ee0c9ce..88d6dbd2ca16 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -383,13 +383,13 @@ 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. + # We include the appservice sender in the directory. These typically + # aren't covered by `get_if_app_services_interested_in_user`. + # We need to handle it here or else we'll get a "row not found" error + # in the deactivated user check, because the sender doesn't have an + # entry in the `users` table. if self.get_app_service_by_user_id(user) is not None: - return False + return True # We're opting to exclude appservice users (anyone matching the user # namespace regex in the appservice registration) even though technically From 1b84f7dfb057091d986cfac893433ec497556cb5 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 11 Oct 2021 18:20:53 +0100 Subject: [PATCH 08/14] Update changelog --- changelog.d/11026.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/11026.bugfix b/changelog.d/11026.bugfix index 09351e0fe035..1bc8051390f8 100644 --- a/changelog.d/11026.bugfix +++ b/changelog.d/11026.bugfix @@ -1 +1 @@ -Exclude application service sender membership from user directory to fix a bug stopping the user directory from being populated. +Fix a long-standing bug where rebuilding the user directory wouldn't exclude support and disabled users. From 03db1e551d51b7018b418f57b7acade270d70a73 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 11 Oct 2021 18:28:08 +0100 Subject: [PATCH 09/14] Update tests --- tests/handlers/test_user_directory.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 1b495695d9ce..275225d6dafa 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -181,14 +181,22 @@ def test_excludes_appservices_user(self) -> None: ) self._check_only_one_user_in_directory(user, public) - def test_excludes_appservice_sender(self) -> None: + def test_includes_appservice_sender(self) -> None: user = self.register_user("user", "pass") token = self.login(user, "pass") room = self.helper.create_room_as( self.appservice.sender, is_public=True, tok=self.appservice.token ) self.helper.join(room, user, tok=token) - self._check_only_one_user_in_directory(user, room) + users = self.get_success(self.user_dir_helper.get_users_in_user_directory()) + in_public = self.get_success(self.user_dir_helper.get_users_in_public_rooms()) + in_private = self.get_success( + self.user_dir_helper.get_users_who_share_private_rooms() + ) + + self.assertEqual(users, {user, self.appservice.sender}) + self.assertEqual(set(in_public), {(user, room), (self.appservice.sender, room)}) + self.assertEqual(in_private, []) def _create_rooms_and_inject_memberships( self, creator: str, token: str, joiner: str @@ -314,18 +322,18 @@ def test_handle_local_profile_change_with_appservice_sender(self) -> None: self.assertIsNone(profile) # update profile - profile_info = ProfileInfo(avatar_url="avatar_url", display_name="4L1c3") + profile_info = ProfileInfo(avatar_url="avatar_url", display_name="beep boop") self.get_success( self.handler.handle_local_profile_change( self.appservice.sender, profile_info ) ) - # profile is still not in directory + # profile is now set profile = self.get_success( self.store.get_user_in_directory(self.appservice.sender) ) - self.assertIsNone(profile) + self.assertEqual(profile, {"display_name": "beep boop", "avatar_url": "avatar_url"}) def test_handle_user_deactivated_support_user(self) -> None: s_user_id = "@support:test" From 4c5afd032c88b25fc57a6585b2e20492c4e1a419 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 11 Oct 2021 18:33:07 +0100 Subject: [PATCH 10/14] Lint --- tests/handlers/test_user_directory.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 275225d6dafa..791ddc32bc24 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -333,7 +333,9 @@ def test_handle_local_profile_change_with_appservice_sender(self) -> None: profile = self.get_success( self.store.get_user_in_directory(self.appservice.sender) ) - self.assertEqual(profile, {"display_name": "beep boop", "avatar_url": "avatar_url"}) + self.assertEqual( + profile, {"display_name": "beep boop", "avatar_url": "avatar_url"} + ) def test_handle_user_deactivated_support_user(self) -> None: s_user_id = "@support:test" From 7648898d8a3efd7d73e478d54596279a29f965eb Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 11 Oct 2021 20:17:46 +0100 Subject: [PATCH 11/14] Ensure dir rebuild includes appservice senders --- docs/user_directory.md | 41 ++++++++++++--- .../storage/databases/main/user_directory.py | 22 ++++++++ .../main/delta/53/user_dir_populate.sql | 2 + tests/storage/test_user_directory.py | 50 +++++++++++++++---- 4 files changed, 99 insertions(+), 16 deletions(-) diff --git a/docs/user_directory.md b/docs/user_directory.md index 07fe95489133..658848f49948 100644 --- a/docs/user_directory.md +++ b/docs/user_directory.md @@ -5,11 +5,6 @@ The user directory is currently maintained based on the 'visible' users on this particular server - i.e. ones which your account shares a room with, or who are present in a publicly viewable room present on the server. -The directory info is stored in various tables, which can (typically after -DB corruption) get stale or out of sync. If this happens, for now the -solution to fix it is to execute the SQL [here](https://github.com/matrix-org/synapse/blob/master/synapse/storage/schema/main/delta/53/user_dir_populate.sql) -and then restart synapse. This should then start a background task to -flush the current tables and regenerate the directory. Data model ---------- @@ -21,7 +16,8 @@ see who. From all of these tables we exclude three types of local user: - support users - - appservice users + - appservice users (e.g. people using IRC) + - but not the "appservice sender" (e.g. the bot which bridges Matrix to IRC). - deactivated users * `user_directory`. This contains the user_id, display name and avatar we'll @@ -47,3 +43,36 @@ From all of these tables we exclude three types of local user: * `users_who_share_private_rooms`. Rows are triples `(L, M, room id)` where `L` is a local user and `M` is a local or remote user. `L` and `M` should be different, but this isn't enforced by a constraint. + + +Rebuilding the directory +------------------------ + +The directory info is stored in various tables, which can (typically after +DB corruption) get stale or out of sync. If this happens, for now the +solution to fix it is to execute the following SQL and then restart Synapse. + +```sql +-- Set up staging tables +INSERT INTO background_updates (update_name, progress_json) VALUES + ('populate_user_directory_createtables', '{}'); + +-- Run through each room and update the room sharing tables. +-- Also add directory entries for remote users. +INSERT INTO background_updates (update_name, progress_json, depends_on) VALUES + ('populate_user_directory_process_rooms', '{}', 'populate_user_directory_createtables'); + +-- Insert directory entries for all local users. +INSERT INTO background_updates (update_name, progress_json, depends_on) VALUES + ('populate_user_directory_process_users', '{}', 'populate_user_directory_process_rooms'); + +-- Insert directory entries for all appservice senders. +INSERT INTO background_updates (update_name, progress_json, depends_on) VALUES + ('populate_user_directory_process_appservice_senders', '{}', 'populate_user_directory_process_users'); + +-- Clean up staging tables +INSERT INTO background_updates (update_name, progress_json, depends_on) VALUES + ('populate_user_directory_cleanup', '{}', 'populate_user_directory_process_appservice_senders'); +``` +This should then start a background task to +flush the current tables and regenerate the directory. diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index 88d6dbd2ca16..9fe5bbc8b19e 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -28,6 +28,7 @@ if TYPE_CHECKING: from synapse.server import HomeServer + from synapse.appservice import ApplicationService from synapse.api.constants import EventTypes, HistoryVisibility, JoinRules from synapse.storage.database import DatabasePool, LoggingTransaction @@ -70,6 +71,10 @@ def __init__( "populate_user_directory_process_users", self._populate_user_directory_process_users, ) + self.db_pool.updates.register_background_update_handler( + "populate_user_directory_process_appservice_senders", + self._populate_user_directory_process_appservice_senders, + ) self.db_pool.updates.register_background_update_handler( "populate_user_directory_cleanup", self._populate_user_directory_cleanup ) @@ -379,6 +384,23 @@ def _get_next_batch(txn: LoggingTransaction) -> Optional[List[str]]: return len(users_to_work_on) + async def _populate_user_directory_process_appservice_senders( + self, progress: JsonDict, batch_size: int + ) -> int: + """Add appservice senders to the `user_directory` table. + + Appservices users don't live in the users table, so they won't be picked up + by `_populate_user_directory_process_users`. Process them explicitly here. + """ + service: "ApplicationService" + for service in self.services_cache: + await self.update_profile_in_user_dir(service.sender, None, None) + + await self.db_pool.updates._end_background_update( + "populate_user_directory_process_appservice_senders" + ) + return 1 + 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? diff --git a/synapse/storage/schema/main/delta/53/user_dir_populate.sql b/synapse/storage/schema/main/delta/53/user_dir_populate.sql index ffcc896b5887..74b921453f16 100644 --- a/synapse/storage/schema/main/delta/53/user_dir_populate.sql +++ b/synapse/storage/schema/main/delta/53/user_dir_populate.sql @@ -13,6 +13,8 @@ * limitations under the License. */ +-- For the most up-to-date version of these instructions, see docs/user_directory.md + -- Set up staging tables INSERT INTO background_updates (update_name, progress_json) VALUES ('populate_user_directory_createtables', '{}'); diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 3831fbc8ef2d..33229c5f957a 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -134,7 +134,9 @@ def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: hostname="test", id="1234", namespaces={"users": [{"regex": r"@as_user.*", "exclusive": True}]}, - sender="@as:test", + # Note: this user does not match the regex above, so that tests + # can distinguish the sender from the AS user. + sender="@as_main:test", ) mock_load_appservices = Mock(return_value=[self.appservice]) @@ -205,12 +207,22 @@ def _purge_and_rebuild_user_dir(self) -> None: self.store.db_pool.simple_insert( "background_updates", { - "update_name": "populate_user_directory_cleanup", + "update_name": "populate_user_directory_process_appservice_senders", "progress_json": "{}", "depends_on": "populate_user_directory_process_users", }, ) ) + self.get_success( + self.store.db_pool.simple_insert( + "background_updates", + { + "update_name": "populate_user_directory_cleanup", + "progress_json": "{}", + "depends_on": "populate_user_directory_process_appservice_senders", + }, + ) + ) self.wait_for_background_updates() @@ -254,7 +266,7 @@ def test_initial(self) -> None: # All three should have entries in the directory users = self.get_success(self.user_dir_helper.get_users_in_user_directory()) - self.assertEqual(users, {u1, u2, u3}) + self.assertEqual(users, {u1, u2, u3, self.appservice.sender}) # The next three tests (test_population_excludes_*) all set up # - A normal user included in the user dir @@ -290,7 +302,7 @@ def _check_room_sharing_tables( ) -> None: # After rebuilding the directory, we should only see the normal user. users = self.get_success(self.user_dir_helper.get_users_in_user_directory()) - self.assertEqual(users, {normal_user}) + self.assertEqual(users, {normal_user, self.appservice.sender}) in_public_rooms = self.get_success( self.user_dir_helper.get_users_in_public_rooms() ) @@ -364,7 +376,7 @@ def test_population_excludes_appservice_user(self) -> None: # Check the AS user is not in the directory. self._check_room_sharing_tables(user, public, private) - def test_population_excludes_appservice_sender(self) -> None: + def test_population_includes_appservice_sender(self) -> None: user = self.register_user("user", "pass") token = self.login(user, "pass") @@ -376,8 +388,25 @@ def test_population_excludes_appservice_sender(self) -> None: # Rebuild the directory. self._purge_and_rebuild_user_dir() - # Check the AS sender is not in the directory. - self._check_room_sharing_tables(user, public, private) + # Check the AS sender is in the directory. + users = self.get_success(self.user_dir_helper.get_users_in_user_directory()) + self.assertEqual(users, {user, self.appservice.sender}) + in_public_rooms = self.get_success( + self.user_dir_helper.get_users_in_public_rooms() + ) + self.assertEqual( + set(in_public_rooms), {(user, public), (self.appservice.sender, public)} + ) + in_private_rooms = self.get_success( + self.user_dir_helper.get_users_who_share_private_rooms() + ) + self.assertEqual( + self.user_dir_helper._compress_shared(in_private_rooms), + { + (user, self.appservice.sender, private), + (self.appservice.sender, user, private), + }, + ) def test_population_conceals_private_nickname(self) -> None: # Make a private room, and set a nickname within @@ -407,15 +436,16 @@ async def mocked_process_users(*args: Any, **kwargs: Any) -> int: self._purge_and_rebuild_user_dir() # Local users are ignored by the scan over rooms - users = self.get_success(self.user_dir_helper.get_profiles_in_user_directory()) - self.assertEqual(users, {}) + users = self.get_success(self.user_dir_helper.get_users_in_user_directory()) + self.assertEqual(users, {self.appservice.sender}) # Do a full rebuild including the scan over the `users` table. The local # user should appear with their profile name. self._purge_and_rebuild_user_dir() users = self.get_success(self.user_dir_helper.get_profiles_in_user_directory()) self.assertEqual( - users, {user: ProfileInfo(display_name="aaaa", avatar_url=None)} + users[user], + ProfileInfo(display_name="aaaa", avatar_url=None), ) From 8550c0a4d3098c245c791b368b7a7886b77a39f6 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 11 Oct 2021 20:43:23 +0100 Subject: [PATCH 12/14] Desperate attempt to make test pass on CI. It's fine locally! --- tests/handlers/test_user_directory.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 791ddc32bc24..0f4c84d4e03b 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -188,6 +188,9 @@ def test_includes_appservice_sender(self) -> None: self.appservice.sender, is_public=True, tok=self.appservice.token ) self.helper.join(room, user, tok=token) + + # Will this make the test pass in CI? It's fine locally. + self.wait_for_background_updates() users = self.get_success(self.user_dir_helper.get_users_in_user_directory()) in_public = self.get_success(self.user_dir_helper.get_users_in_public_rooms()) in_private = self.get_success( From 04078ff514cf3bc049dc9a5ac5741d7d938b27ef Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 11 Oct 2021 21:20:52 +0100 Subject: [PATCH 13/14] Please do dump logs --- .github/workflows/tests.yml | 3 +++ .github/workflows/twisted_trunk.yml | 1 + 2 files changed, 4 insertions(+) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 96c39dd9a4bc..e8a2afa16ba2 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -122,6 +122,7 @@ jobs: # Note: Dumps to workflow logs instead of using actions/upload-artifact # This keeps logs colocated with failing jobs # It also ignores find's exit code; this is a best effort affair + if: $ {{ always() }} run: >- find _trial_temp -name '*.log' -exec echo "::group::{}" \; @@ -146,6 +147,7 @@ jobs: # Note: Dumps to workflow logs instead of using actions/upload-artifact # This keeps logs colocated with failing jobs # It also ignores find's exit code; this is a best effort affair + if: $ {{ always() }} run: >- find _trial_temp -name '*.log' -exec echo "::group::{}" \; @@ -176,6 +178,7 @@ jobs: # Note: Dumps to workflow logs instead of using actions/upload-artifact # This keeps logs colocated with failing jobs # It also ignores find's exit code; this is a best effort affair + if: $ {{ always() }} run: >- find _trial_temp -name '*.log' -exec echo "::group::{}" \; diff --git a/.github/workflows/twisted_trunk.yml b/.github/workflows/twisted_trunk.yml index b5c729888f57..637835b29ac1 100644 --- a/.github/workflows/twisted_trunk.yml +++ b/.github/workflows/twisted_trunk.yml @@ -36,6 +36,7 @@ jobs: # Note: Dumps to workflow logs instead of using actions/upload-artifact # This keeps logs colocated with failing jobs # It also ignores find's exit code; this is a best effort affair + if: $ {{ always() }} run: >- find _trial_temp -name '*.log' -exec echo "::group::{}" \; From 9fec1cc212ac703b3c51fbb68d7aa9fbbf301b45 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 11 Oct 2021 22:12:14 +0100 Subject: [PATCH 14/14] Fix typo --- .github/workflows/tests.yml | 6 +++--- .github/workflows/twisted_trunk.yml | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index e8a2afa16ba2..27dcb733c07d 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -122,7 +122,7 @@ jobs: # Note: Dumps to workflow logs instead of using actions/upload-artifact # This keeps logs colocated with failing jobs # It also ignores find's exit code; this is a best effort affair - if: $ {{ always() }} + if: ${{ always() }} run: >- find _trial_temp -name '*.log' -exec echo "::group::{}" \; @@ -147,7 +147,7 @@ jobs: # Note: Dumps to workflow logs instead of using actions/upload-artifact # This keeps logs colocated with failing jobs # It also ignores find's exit code; this is a best effort affair - if: $ {{ always() }} + if: ${{ always() }} run: >- find _trial_temp -name '*.log' -exec echo "::group::{}" \; @@ -178,7 +178,7 @@ jobs: # Note: Dumps to workflow logs instead of using actions/upload-artifact # This keeps logs colocated with failing jobs # It also ignores find's exit code; this is a best effort affair - if: $ {{ always() }} + if: ${{ always() }} run: >- find _trial_temp -name '*.log' -exec echo "::group::{}" \; diff --git a/.github/workflows/twisted_trunk.yml b/.github/workflows/twisted_trunk.yml index 637835b29ac1..14ad7a852634 100644 --- a/.github/workflows/twisted_trunk.yml +++ b/.github/workflows/twisted_trunk.yml @@ -36,7 +36,7 @@ jobs: # Note: Dumps to workflow logs instead of using actions/upload-artifact # This keeps logs colocated with failing jobs # It also ignores find's exit code; this is a best effort affair - if: $ {{ always() }} + if: ${{ always() }} run: >- find _trial_temp -name '*.log' -exec echo "::group::{}" \;