From 1e8908d6cd9548593968b42510f6b9eae4505c12 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 16 May 2023 14:47:24 -0700 Subject: [PATCH 01/15] update `get_profileinfo` to read from column `full_user_id` --- synapse/handlers/admin.py | 2 +- synapse/handlers/auth.py | 2 +- synapse/handlers/deactivate_account.py | 2 +- synapse/handlers/profile.py | 10 +++++----- synapse/handlers/register.py | 2 +- synapse/module_api/__init__.py | 4 +++- synapse/storage/databases/main/profile.py | 4 ++-- 7 files changed, 14 insertions(+), 12 deletions(-) diff --git a/synapse/handlers/admin.py b/synapse/handlers/admin.py index b06f25b03c21..119c7f838481 100644 --- a/synapse/handlers/admin.py +++ b/synapse/handlers/admin.py @@ -89,7 +89,7 @@ async def get_user(self, user: UserID) -> Optional[JsonDict]: } # Add additional user metadata - profile = await self._store.get_profileinfo(user.localpart) + profile = await self._store.get_profileinfo(user) threepids = await self._store.user_get_threepids(user.to_string()) external_ids = [ ({"auth_provider": auth_provider, "external_id": external_id}) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 59e340974d93..743b6a226229 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -1757,7 +1757,7 @@ async def complete_sso_login( return user_profile_data = await self.store.get_profileinfo( - UserID.from_string(registered_user_id).localpart + UserID.from_string(registered_user_id) ) # Store any extra attributes which will be passed in the login response. diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py index f299b89a1b2f..67adeae6a7a6 100644 --- a/synapse/handlers/deactivate_account.py +++ b/synapse/handlers/deactivate_account.py @@ -297,5 +297,5 @@ async def activate_account(self, user_id: str) -> None: # Add the user to the directory, if necessary. Note that # this must be done after the user is re-activated, because # deactivated users are excluded from the user directory. - profile = await self.store.get_profileinfo(user.localpart) + profile = await self.store.get_profileinfo(user) await self.user_directory_handler.handle_local_profile_change(user_id, profile) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index a9160c87e304..797145e6a88a 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -67,7 +67,7 @@ async def get_profile(self, user_id: str, ignore_backoff: bool = True) -> JsonDi target_user = UserID.from_string(user_id) if self.hs.is_mine(target_user): - profileinfo = await self.store.get_profileinfo(target_user.localpart) + profileinfo = await self.store.get_profileinfo(target_user) if profileinfo.display_name is None: raise SynapseError(404, "Profile was not found", Codes.NOT_FOUND) @@ -147,7 +147,7 @@ async def set_displayname( raise AuthError(400, "Cannot set another user's displayname") if not by_admin and not self.hs.config.registration.enable_set_displayname: - profile = await self.store.get_profileinfo(target_user.localpart) + profile = await self.store.get_profileinfo(target_user) if profile.display_name: raise SynapseError( 400, @@ -180,7 +180,7 @@ async def set_displayname( await self.store.set_profile_displayname(target_user, displayname_to_set) - profile = await self.store.get_profileinfo(target_user.localpart) + profile = await self.store.get_profileinfo(target_user) await self.user_directory_handler.handle_local_profile_change( target_user.to_string(), profile ) @@ -241,7 +241,7 @@ async def set_avatar_url( raise AuthError(400, "Cannot set another user's avatar_url") if not by_admin and not self.hs.config.registration.enable_set_avatar_url: - profile = await self.store.get_profileinfo(target_user.localpart) + profile = await self.store.get_profileinfo(target_user) if profile.avatar_url: raise SynapseError( 400, "Changing avatar is disabled on this server", Codes.FORBIDDEN @@ -272,7 +272,7 @@ async def set_avatar_url( await self.store.set_profile_avatar_url(target_user, avatar_url_to_set) - profile = await self.store.get_profileinfo(target_user.localpart) + profile = await self.store.get_profileinfo(target_user) await self.user_directory_handler.handle_local_profile_change( target_user.to_string(), profile ) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index c80946c2e976..a2d3f03061fc 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -315,7 +315,7 @@ async def register_user( approved=approved, ) - profile = await self.store.get_profileinfo(localpart) + profile = await self.store.get_profileinfo(user) await self.user_directory_handler.handle_local_profile_change( user_id, profile ) diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 0e9f366cba62..9c09d18a1bd5 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -647,7 +647,9 @@ async def get_profile_for_user(self, localpart: str) -> ProfileInfo: Returns: The profile information (i.e. display name and avatar URL). """ - return await self._store.get_profileinfo(localpart) + server_name = self._hs.hostname + user_id = UserID.from_string(f"@{localpart}:{server_name}") + return await self._store.get_profileinfo(user_id) async def get_threepids_for_user(self, user_id: str) -> List[Dict[str, str]]: """Look up the threepids (email addresses and phone numbers) associated with the diff --git a/synapse/storage/databases/main/profile.py b/synapse/storage/databases/main/profile.py index 65c92bef51cf..cf75da14e4f5 100644 --- a/synapse/storage/databases/main/profile.py +++ b/synapse/storage/databases/main/profile.py @@ -137,11 +137,11 @@ def _final_batch(txn: LoggingTransaction, lower_bound_id: str) -> None: return 50 - async def get_profileinfo(self, user_localpart: str) -> ProfileInfo: + async def get_profileinfo(self, user_id: UserID) -> ProfileInfo: try: profile = await self.db_pool.simple_select_one( table="profiles", - keyvalues={"user_id": user_localpart}, + keyvalues={"full_user_id": user_id.to_string()}, retcols=("displayname", "avatar_url"), desc="get_profileinfo", ) From 02981c39ed8285fddd5a02a40ce2ef8d6900f807 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 16 May 2023 16:06:35 -0700 Subject: [PATCH 02/15] update `get_profile_displayname` to read from column `full_user_id` --- synapse/handlers/account_validity.py | 2 +- synapse/handlers/profile.py | 8 ++------ synapse/push/mailer.py | 2 +- synapse/storage/databases/main/profile.py | 4 ++-- tests/handlers/test_profile.py | 20 ++++---------------- tests/module_api/test_api.py | 6 ++++-- tests/storage/test_profile.py | 8 ++------ 7 files changed, 16 insertions(+), 34 deletions(-) diff --git a/synapse/handlers/account_validity.py b/synapse/handlers/account_validity.py index 4aa4ebf7e4a8..f1a7a05df6bc 100644 --- a/synapse/handlers/account_validity.py +++ b/synapse/handlers/account_validity.py @@ -164,7 +164,7 @@ async def _send_renewal_email(self, user_id: str, expiration_ts: int) -> None: try: user_display_name = await self.store.get_profile_displayname( - UserID.from_string(user_id).localpart + UserID.from_string(user_id) ) if user_display_name is None: user_display_name = user_id diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 797145e6a88a..43fabea446b2 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -99,9 +99,7 @@ async def get_profile(self, user_id: str, ignore_backoff: bool = True) -> JsonDi async def get_displayname(self, target_user: UserID) -> Optional[str]: if self.hs.is_mine(target_user): try: - displayname = await self.store.get_profile_displayname( - target_user.localpart - ) + displayname = await self.store.get_profile_displayname(target_user) except StoreError as e: if e.code == 404: raise SynapseError(404, "Profile was not found", Codes.NOT_FOUND) @@ -369,9 +367,7 @@ async def on_profile_query(self, args: JsonDict) -> JsonDict: response = {} try: if just_field is None or just_field == "displayname": - response["displayname"] = await self.store.get_profile_displayname( - user.localpart - ) + response["displayname"] = await self.store.get_profile_displayname(user) if just_field is None or just_field == "avatar_url": response["avatar_url"] = await self.store.get_profile_avatar_url( diff --git a/synapse/push/mailer.py b/synapse/push/mailer.py index 491a09b71d54..79e0627b6a66 100644 --- a/synapse/push/mailer.py +++ b/synapse/push/mailer.py @@ -247,7 +247,7 @@ async def send_notification_mail( try: user_display_name = await self.store.get_profile_displayname( - UserID.from_string(user_id).localpart + UserID.from_string(user_id) ) if user_display_name is None: user_display_name = user_id diff --git a/synapse/storage/databases/main/profile.py b/synapse/storage/databases/main/profile.py index cf75da14e4f5..f4b61c1d138c 100644 --- a/synapse/storage/databases/main/profile.py +++ b/synapse/storage/databases/main/profile.py @@ -156,10 +156,10 @@ async def get_profileinfo(self, user_id: UserID) -> ProfileInfo: avatar_url=profile["avatar_url"], display_name=profile["displayname"] ) - async def get_profile_displayname(self, user_localpart: str) -> Optional[str]: + async def get_profile_displayname(self, user_id: UserID) -> Optional[str]: return await self.db_pool.simple_select_one_onecol( table="profiles", - keyvalues={"user_id": user_localpart}, + keyvalues={"full_user_id": user_id.to_string()}, retcol="displayname", desc="get_profile_displayname", ) diff --git a/tests/handlers/test_profile.py b/tests/handlers/test_profile.py index 64a9a22afeca..df567cd03df5 100644 --- a/tests/handlers/test_profile.py +++ b/tests/handlers/test_profile.py @@ -80,11 +80,7 @@ def test_set_my_name(self) -> None: ) self.assertEqual( - ( - self.get_success( - self.store.get_profile_displayname(self.frank.localpart) - ) - ), + (self.get_success(self.store.get_profile_displayname(self.frank))), "Frank Jr.", ) @@ -96,11 +92,7 @@ def test_set_my_name(self) -> None: ) self.assertEqual( - ( - self.get_success( - self.store.get_profile_displayname(self.frank.localpart) - ) - ), + (self.get_success(self.store.get_profile_displayname(self.frank))), "Frank", ) @@ -112,7 +104,7 @@ def test_set_my_name(self) -> None: ) self.assertIsNone( - self.get_success(self.store.get_profile_displayname(self.frank.localpart)) + self.get_success(self.store.get_profile_displayname(self.frank)) ) def test_set_my_name_if_disabled(self) -> None: @@ -122,11 +114,7 @@ def test_set_my_name_if_disabled(self) -> None: self.get_success(self.store.set_profile_displayname(self.frank, "Frank")) self.assertEqual( - ( - self.get_success( - self.store.get_profile_displayname(self.frank.localpart) - ) - ), + (self.get_success(self.store.get_profile_displayname(self.frank))), "Frank", ) diff --git a/tests/module_api/test_api.py b/tests/module_api/test_api.py index bff7114cd89c..b3310abe1b31 100644 --- a/tests/module_api/test_api.py +++ b/tests/module_api/test_api.py @@ -28,7 +28,7 @@ from synapse.rest import admin from synapse.rest.client import login, notifications, presence, profile, room from synapse.server import HomeServer -from synapse.types import JsonDict, create_requester +from synapse.types import JsonDict, UserID, create_requester from synapse.util import Clock from tests.events.test_presence_router import send_presence_update, sync_presence @@ -103,7 +103,9 @@ def test_can_register_user(self) -> None: self.assertEqual(email["added_at"], 0) # Check that the displayname was assigned - displayname = self.get_success(self.store.get_profile_displayname("bob")) + displayname = self.get_success( + self.store.get_profile_displayname(UserID.from_string("@bob:test")) + ) self.assertEqual(displayname, "Bobberino") def test_can_register_admin_user(self) -> None: diff --git a/tests/storage/test_profile.py b/tests/storage/test_profile.py index f9cf0fcb82ed..7eecbfc2c51a 100644 --- a/tests/storage/test_profile.py +++ b/tests/storage/test_profile.py @@ -35,18 +35,14 @@ def test_displayname(self) -> None: self.assertEqual( "Frank", - ( - self.get_success( - self.store.get_profile_displayname(self.u_frank.localpart) - ) - ), + (self.get_success(self.store.get_profile_displayname(self.u_frank))), ) # test set to None self.get_success(self.store.set_profile_displayname(self.u_frank, None)) self.assertIsNone( - self.get_success(self.store.get_profile_displayname(self.u_frank.localpart)) + self.get_success(self.store.get_profile_displayname(self.u_frank)) ) def test_avatar_url(self) -> None: From 0bedee1e29e455c07691f4027cab740ac44257d0 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 16 May 2023 16:19:01 -0700 Subject: [PATCH 03/15] update `get_profile_avatar_url` to read from column `full_user_id` --- synapse/handlers/profile.py | 8 ++------ synapse/storage/databases/main/profile.py | 4 ++-- tests/handlers/test_profile.py | 8 ++++---- tests/storage/test_profile.py | 8 ++------ 4 files changed, 10 insertions(+), 18 deletions(-) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 43fabea446b2..a7f8c5e636f8 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -192,9 +192,7 @@ async def set_displayname( async def get_avatar_url(self, target_user: UserID) -> Optional[str]: if self.hs.is_mine(target_user): try: - avatar_url = await self.store.get_profile_avatar_url( - target_user.localpart - ) + avatar_url = await self.store.get_profile_avatar_url(target_user) except StoreError as e: if e.code == 404: raise SynapseError(404, "Profile was not found", Codes.NOT_FOUND) @@ -370,9 +368,7 @@ async def on_profile_query(self, args: JsonDict) -> JsonDict: response["displayname"] = await self.store.get_profile_displayname(user) if just_field is None or just_field == "avatar_url": - response["avatar_url"] = await self.store.get_profile_avatar_url( - user.localpart - ) + response["avatar_url"] = await self.store.get_profile_avatar_url(user) except StoreError as e: if e.code == 404: raise SynapseError(404, "Profile was not found", Codes.NOT_FOUND) diff --git a/synapse/storage/databases/main/profile.py b/synapse/storage/databases/main/profile.py index f4b61c1d138c..28fba4a90a9f 100644 --- a/synapse/storage/databases/main/profile.py +++ b/synapse/storage/databases/main/profile.py @@ -164,10 +164,10 @@ async def get_profile_displayname(self, user_id: UserID) -> Optional[str]: desc="get_profile_displayname", ) - async def get_profile_avatar_url(self, user_localpart: str) -> Optional[str]: + async def get_profile_avatar_url(self, user_id: UserID) -> Optional[str]: return await self.db_pool.simple_select_one_onecol( table="profiles", - keyvalues={"user_id": user_localpart}, + keyvalues={"full_user_id": user_id.to_string()}, retcol="avatar_url", desc="get_profile_avatar_url", ) diff --git a/tests/handlers/test_profile.py b/tests/handlers/test_profile.py index df567cd03df5..196ceb0b82d0 100644 --- a/tests/handlers/test_profile.py +++ b/tests/handlers/test_profile.py @@ -189,7 +189,7 @@ def test_set_my_avatar(self) -> None: ) self.assertEqual( - (self.get_success(self.store.get_profile_avatar_url(self.frank.localpart))), + (self.get_success(self.store.get_profile_avatar_url(self.frank))), "http://my.server/pic.gif", ) @@ -203,7 +203,7 @@ def test_set_my_avatar(self) -> None: ) self.assertEqual( - (self.get_success(self.store.get_profile_avatar_url(self.frank.localpart))), + (self.get_success(self.store.get_profile_avatar_url(self.frank))), "http://my.server/me.png", ) @@ -217,7 +217,7 @@ def test_set_my_avatar(self) -> None: ) self.assertIsNone( - (self.get_success(self.store.get_profile_avatar_url(self.frank.localpart))), + (self.get_success(self.store.get_profile_avatar_url(self.frank))), ) def test_set_my_avatar_if_disabled(self) -> None: @@ -229,7 +229,7 @@ def test_set_my_avatar_if_disabled(self) -> None: ) self.assertEqual( - (self.get_success(self.store.get_profile_avatar_url(self.frank.localpart))), + (self.get_success(self.store.get_profile_avatar_url(self.frank))), "http://my.server/me.png", ) diff --git a/tests/storage/test_profile.py b/tests/storage/test_profile.py index 7eecbfc2c51a..574a7cf29a34 100644 --- a/tests/storage/test_profile.py +++ b/tests/storage/test_profile.py @@ -54,18 +54,14 @@ def test_avatar_url(self) -> None: self.assertEqual( "http://my.site/here", - ( - self.get_success( - self.store.get_profile_avatar_url(self.u_frank.localpart) - ) - ), + (self.get_success(self.store.get_profile_avatar_url(self.u_frank))), ) # test set to None self.get_success(self.store.set_profile_avatar_url(self.u_frank, None)) self.assertIsNone( - self.get_success(self.store.get_profile_avatar_url(self.u_frank.localpart)) + self.get_success(self.store.get_profile_avatar_url(self.u_frank)) ) def test_profiles_bg_migration(self) -> None: From 14d7253f1433a893bc96eb8eed8c0cbc11b0267a Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Mon, 22 May 2023 13:34:36 -0700 Subject: [PATCH 04/15] add functions to validate constraint/add check on tables `profiles` and `user_filters` --- synapse/storage/schema/__init__.py | 2 +- .../78/01_validate_and_update_profiles.py | 85 ++++++++++++++++++ .../78/02_validate_and_update_user_filters.py | 89 +++++++++++++++++++ 3 files changed, 175 insertions(+), 1 deletion(-) create mode 100644 synapse/storage/schema/main/delta/78/01_validate_and_update_profiles.py create mode 100644 synapse/storage/schema/main/delta/78/02_validate_and_update_user_filters.py diff --git a/synapse/storage/schema/__init__.py b/synapse/storage/schema/__init__.py index df2cc31ca65a..07a5a3fafcd4 100644 --- a/synapse/storage/schema/__init__.py +++ b/synapse/storage/schema/__init__.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -SCHEMA_VERSION = 77 # remember to update the list below when updating +SCHEMA_VERSION = 78 # remember to update the list below when updating """Represents the expectations made by the codebase about the database schema This should be incremented whenever the codebase changes its requirements on the diff --git a/synapse/storage/schema/main/delta/78/01_validate_and_update_profiles.py b/synapse/storage/schema/main/delta/78/01_validate_and_update_profiles.py new file mode 100644 index 000000000000..8283cd1f0a13 --- /dev/null +++ b/synapse/storage/schema/main/delta/78/01_validate_and_update_profiles.py @@ -0,0 +1,85 @@ +# Copyright 2023 The Matrix.org Foundation C.I.C +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from synapse.config.homeserver import HomeServerConfig +from synapse.storage.database import LoggingTransaction +from synapse.storage.engines import BaseDatabaseEngine, PostgresEngine + + +def run_upgrade( + cur: LoggingTransaction, + database_engine: BaseDatabaseEngine, + config: HomeServerConfig, +) -> None: + hostname = config.server.server_name + + if isinstance(database_engine, PostgresEngine): + # check if the constraint can be validated + check_sql = """ + SELECT user_id from profiles WHERE full_user_id IS NULL + """ + cur.execute(check_sql) + res = cur.fetchall() + + if res: + # there are rows the background job missed, finish them here before we validate the constraint + process_rows_sql = """ + UPDATE profiles + SET full_user_id = '@' || user_id || ? + WHERE user_id IN (SELECT user_id FROM profiles WHERE + full_user_id IS NULL) + """ + cur.execute(process_rows_sql, (f":{hostname}",)) + + # Now we can validate + validate_sql = """ + ALTER TABLE profiles VALIDATE CONSTRAINT full_user_id_not_null + """ + cur.execute(validate_sql) + + else: + # in SQLite we need to rewrite the table to add the constraint + cur.execute("DROP TABLE IF EXISTS temp_profiles") + + create_sql = """ + CREATE TABLE temp_profiles ( + full_user_id text NOT NULL, + user_id text, + displayname text, + avatar_url text, + UNIQUE (full_user_id), + UNIQUE (user_id) + ) + """ + cur.execute(create_sql) + + copy_sql = """ + INSERT INTO temp_profiles ( + user_id, + displayname, + avatar_url, + full_user_id) + SELECT user_id, displayname, avatar_url, '@' || user_id || ':' || ? FROM profiles + """ + cur.execute(copy_sql, (f"{hostname}",)) + + drop_sql = """ + DROP TABLE profiles + """ + cur.execute(drop_sql) + + rename_sql = """ + ALTER TABLE temp_profiles RENAME to profiles + """ + cur.execute(rename_sql) diff --git a/synapse/storage/schema/main/delta/78/02_validate_and_update_user_filters.py b/synapse/storage/schema/main/delta/78/02_validate_and_update_user_filters.py new file mode 100644 index 000000000000..1a4427785566 --- /dev/null +++ b/synapse/storage/schema/main/delta/78/02_validate_and_update_user_filters.py @@ -0,0 +1,89 @@ +# Copyright 2023 The Matrix.org Foundation C.I.C +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from synapse.config.homeserver import HomeServerConfig +from synapse.storage.database import LoggingTransaction +from synapse.storage.engines import BaseDatabaseEngine, PostgresEngine + + +def run_upgrade( + cur: LoggingTransaction, + database_engine: BaseDatabaseEngine, + config: HomeServerConfig, +) -> None: + hostname = config.server.server_name + + if isinstance(database_engine, PostgresEngine): + # check if the constraint can be validated + check_sql = """ + SELECT user_id from user_filters WHERE full_user_id IS NULL + """ + cur.execute(check_sql) + res = cur.fetchall() + + if res: + # there are rows the background job missed, finish them here before we validate constraint + process_rows_sql = """ + UPDATE user_filters + SET full_user_id = '@' || user_id || ? + WHERE user_id IN (SELECT user_id FROM user_filters + WHERE full_user_id IS NULL) + """ + cur.execute(process_rows_sql, (f":{hostname}",)) + + # Now we can validate + validate_sql = """ + ALTER TABLE user_filters VALIDATE CONSTRAINT full_user_id_not_null + """ + cur.execute(validate_sql) + + else: + cur.execute("DROP TABLE IF EXISTS temp_user_filters") + create_sql = """ + CREATE TABLE temp_user_filters ( + full_user_id text NOT NULL, + user_id text NOT NULL, + filter_id bigint NOT NULL, + filter_json bytea NOT NULL, + UNIQUE (full_user_id), + UNIQUE (user_id) + ) + """ + cur.execute(create_sql) + + index_sql = """ + CREATE UNIQUE INDEX IF NOT EXISTS user_filters_unique ON + temp_user_filters (user_id, filter_id) + """ + cur.execute(index_sql) + + copy_sql = """ + INSERT INTO temp_user_filters ( + user_id, + filter_id, + filter_json, + full_user_id) + SELECT user_id, filter_id, filter_json, '@' || user_id || ':' || ? FROM user_filters + """ + cur.execute(copy_sql, (f"{hostname}",)) + + drop_sql = """ + DROP TABLE user_filters + """ + cur.execute(drop_sql) + + rename_sql = """ + ALTER TABLE temp_user_filters RENAME to user_filters + """ + cur.execute(rename_sql) From 382ba52895a51fa1d03731b4cd306b3963d53f51 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Mon, 22 May 2023 13:39:18 -0700 Subject: [PATCH 05/15] update `get_user_filter` and `add_user_filter` to read from column `full_user_id` rather than `user_id` --- synapse/api/filtering.py | 4 ++-- synapse/rest/client/filter.py | 2 +- synapse/rest/client/sync.py | 2 +- synapse/storage/databases/main/filtering.py | 12 +++++----- tests/api/test_filtering.py | 25 +++++---------------- tests/rest/client/test_filter.py | 4 +++- 6 files changed, 19 insertions(+), 30 deletions(-) diff --git a/synapse/api/filtering.py b/synapse/api/filtering.py index de7c56bc0fa3..e66b822d449a 100644 --- a/synapse/api/filtering.py +++ b/synapse/api/filtering.py @@ -165,9 +165,9 @@ def __init__(self, hs: "HomeServer"): self.DEFAULT_FILTER_COLLECTION = FilterCollection(hs, {}) async def get_user_filter( - self, user_localpart: str, filter_id: Union[int, str] + self, user_id: UserID, filter_id: Union[int, str] ) -> "FilterCollection": - result = await self.store.get_user_filter(user_localpart, filter_id) + result = await self.store.get_user_filter(user_id, filter_id) return FilterCollection(self._hs, result) def add_user_filter(self, user_id: UserID, user_filter: JsonDict) -> Awaitable[int]: diff --git a/synapse/rest/client/filter.py b/synapse/rest/client/filter.py index 04561f36d7a1..5da1e511a281 100644 --- a/synapse/rest/client/filter.py +++ b/synapse/rest/client/filter.py @@ -58,7 +58,7 @@ async def on_GET( try: filter_collection = await self.filtering.get_user_filter( - user_localpart=target_user.localpart, filter_id=filter_id_int + user_id=target_user, filter_id=filter_id_int ) except StoreError as e: if e.code != 404: diff --git a/synapse/rest/client/sync.py b/synapse/rest/client/sync.py index 03b05789456b..d7854ed4fd9d 100644 --- a/synapse/rest/client/sync.py +++ b/synapse/rest/client/sync.py @@ -178,7 +178,7 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: else: try: filter_collection = await self.filtering.get_user_filter( - user.localpart, filter_id + user, filter_id ) except StoreError as err: if err.code != 404: diff --git a/synapse/storage/databases/main/filtering.py b/synapse/storage/databases/main/filtering.py index da31eb44dc96..3b86d23ce9ea 100644 --- a/synapse/storage/databases/main/filtering.py +++ b/synapse/storage/databases/main/filtering.py @@ -145,7 +145,7 @@ def _final_batch(txn: LoggingTransaction, lower_bound_id: str) -> None: @cached(num_args=2) async def get_user_filter( - self, user_localpart: str, filter_id: Union[int, str] + self, user_id: UserID, filter_id: Union[int, str] ) -> JsonDict: # filter_id is BIGINT UNSIGNED, so if it isn't a number, fail # with a coherent error message rather than 500 M_UNKNOWN. @@ -156,7 +156,7 @@ async def get_user_filter( def_json = await self.db_pool.simple_select_one_onecol( table="user_filters", - keyvalues={"user_id": user_localpart, "filter_id": filter_id}, + keyvalues={"full_user_id": user_id.to_string(), "filter_id": filter_id}, retcol="filter_json", allow_none=False, desc="get_user_filter", @@ -172,15 +172,15 @@ async def add_user_filter(self, user_id: UserID, user_filter: JsonDict) -> int: def _do_txn(txn: LoggingTransaction) -> int: sql = ( "SELECT filter_id FROM user_filters " - "WHERE user_id = ? AND filter_json = ?" + "WHERE full_user_id = ? AND filter_json = ?" ) - txn.execute(sql, (user_id.localpart, bytearray(def_json))) + txn.execute(sql, (user_id.to_string(), bytearray(def_json))) filter_id_response = txn.fetchone() if filter_id_response is not None: return filter_id_response[0] - sql = "SELECT MAX(filter_id) FROM user_filters WHERE user_id = ?" - txn.execute(sql, (user_id.localpart,)) + sql = "SELECT MAX(filter_id) FROM user_filters WHERE full_user_id = ?" + txn.execute(sql, (user_id.to_string(),)) max_id = cast(Tuple[Optional[int]], txn.fetchone())[0] if max_id is None: filter_id = 0 diff --git a/tests/api/test_filtering.py b/tests/api/test_filtering.py index 222449baac81..19d6c48ec6d4 100644 --- a/tests/api/test_filtering.py +++ b/tests/api/test_filtering.py @@ -35,7 +35,6 @@ user_id = UserID.from_string("@test_user:test") user2_id = UserID.from_string("@test_user2:test") -user_localpart = "test_user" class FilteringTestCase(unittest.HomeserverTestCase): @@ -455,9 +454,7 @@ def test_filter_presence_match(self) -> None: ] user_filter = self.get_success( - self.filtering.get_user_filter( - user_localpart=user_localpart, filter_id=filter_id - ) + self.filtering.get_user_filter(user_id=user_id, filter_id=filter_id) ) results = self.get_success(user_filter.filter_presence(presence_states)) @@ -485,9 +482,7 @@ def test_filter_presence_no_match(self) -> None: ] user_filter = self.get_success( - self.filtering.get_user_filter( - user_localpart=user_localpart + "2", filter_id=filter_id - ) + self.filtering.get_user_filter(user_id=user2_id, filter_id=filter_id) ) results = self.get_success(user_filter.filter_presence(presence_states)) @@ -504,9 +499,7 @@ def test_filter_room_state_match(self) -> None: events = [event] user_filter = self.get_success( - self.filtering.get_user_filter( - user_localpart=user_localpart, filter_id=filter_id - ) + self.filtering.get_user_filter(user_id=user_id, filter_id=filter_id) ) results = self.get_success(user_filter.filter_room_state(events=events)) @@ -525,9 +518,7 @@ def test_filter_room_state_no_match(self) -> None: events = [event] user_filter = self.get_success( - self.filtering.get_user_filter( - user_localpart=user_localpart, filter_id=filter_id - ) + self.filtering.get_user_filter(user_id=user_id, filter_id=filter_id) ) results = self.get_success(user_filter.filter_room_state(events)) @@ -609,9 +600,7 @@ def test_add_filter(self) -> None: user_filter_json, ( self.get_success( - self.datastore.get_user_filter( - user_localpart=user_localpart, filter_id=0 - ) + self.datastore.get_user_filter(user_id=user_id, filter_id=0) ) ), ) @@ -626,9 +615,7 @@ def test_get_filter(self) -> None: ) filter = self.get_success( - self.filtering.get_user_filter( - user_localpart=user_localpart, filter_id=filter_id - ) + self.filtering.get_user_filter(user_id=user_id, filter_id=filter_id) ) self.assertEqual(filter.get_filter_json(), user_filter_json) diff --git a/tests/rest/client/test_filter.py b/tests/rest/client/test_filter.py index 9faa9de05076..a2d5d340be35 100644 --- a/tests/rest/client/test_filter.py +++ b/tests/rest/client/test_filter.py @@ -46,7 +46,9 @@ def test_add_filter(self) -> None: self.assertEqual(channel.code, 200) self.assertEqual(channel.json_body, {"filter_id": "0"}) filter = self.get_success( - self.store.get_user_filter(user_localpart="apple", filter_id=0) + self.store.get_user_filter( + user_id=UserID.from_string(FilterTestCase.user_id), filter_id=0 + ) ) self.pump() self.assertEqual(filter, self.EXAMPLE_FILTER) From 3559530f4cebdc188fdcd53a4b91cdaeff0a7548 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Mon, 22 May 2023 13:39:30 -0700 Subject: [PATCH 06/15] test upgrade functions --- tests/storage/test_profile.py | 63 ++++++++++++++++++++++++++++ tests/storage/test_user_filters.py | 66 +++++++++++++++++++++++++++++- 2 files changed, 128 insertions(+), 1 deletion(-) diff --git a/tests/storage/test_profile.py b/tests/storage/test_profile.py index 574a7cf29a34..a8c9e8c7895c 100644 --- a/tests/storage/test_profile.py +++ b/tests/storage/test_profile.py @@ -11,6 +11,8 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +import importlib.util + from twisted.test.proto_helpers import MemoryReactor from synapse.server import HomeServer @@ -69,6 +71,7 @@ def test_profiles_bg_migration(self) -> None: Test background job that copies entries from column user_id to full_user_id, adding the hostname in the process. """ + updater = self.hs.get_datastores().main.db_pool.updates # drop the constraint so we can insert nulls in full_user_id to populate the test @@ -124,3 +127,63 @@ def f(txn: LoggingTransaction) -> None: ) self.assertEqual(len(res), len(expected_values)) self.assertEqual(res, expected_values) + + def test_upgrade_function(self) -> None: + # drop the constraint so we can insert nulls in full_user_id to populate the test + if isinstance(self.store.database_engine, PostgresEngine): + + def f(txn: LoggingTransaction) -> None: + txn.execute( + "ALTER TABLE profiles DROP CONSTRAINT full_user_id_not_null" + ) + + self.get_success(self.store.db_pool.runInteraction("", f)) + + for i in range(0, 10): + self.get_success( + self.store.db_pool.simple_insert( + "profiles", + {"user_id": f"hello{i:02}"}, + ) + ) + + # re-add the constraint so that when it's validated it actually exists + if isinstance(self.store.database_engine, PostgresEngine): + + def f(txn: LoggingTransaction) -> None: + txn.execute( + "ALTER TABLE profiles ADD CONSTRAINT full_user_id_not_null CHECK (full_user_id IS NOT NULL) NOT VALID" + ) + + self.get_success(self.store.db_pool.runInteraction("", f)) + + expected_values = [] + for i in range(0, 10): + expected_values.append((f"@hello{i:02}:{self.hs.hostname}",)) + + # get the upgrade function and run it on the db we just prepared + module_name = "synapse.storage.v78_0_1_validate_and_update_profiles" + absolute_path = "../../synapse/storage/schema/main/delta/78/01_validate_and_update_profiles.py" + + spec = importlib.util.spec_from_file_location(module_name, absolute_path) + assert spec is not None + module = importlib.util.module_from_spec(spec) + assert spec.loader is not None + spec.loader.exec_module(module) + + self.get_success( + self.store.db_pool.runInteraction( + "", + module.run_upgrade, + self.store.database_engine, + self.hs.config, + ) + ) + + res = self.get_success( + self.store.db_pool.execute( + "", None, "SELECT full_user_id from profiles ORDER BY full_user_id" + ) + ) + self.assertEqual(len(res), len(expected_values)) + self.assertEqual(res, expected_values) diff --git a/tests/storage/test_user_filters.py b/tests/storage/test_user_filters.py index bab802f56ec6..0f88ec1a6a10 100644 --- a/tests/storage/test_user_filters.py +++ b/tests/storage/test_user_filters.py @@ -11,7 +11,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. - +import importlib.util from twisted.test.proto_helpers import MemoryReactor @@ -92,3 +92,67 @@ def f(txn: LoggingTransaction) -> None: ) self.assertEqual(len(res), len(expected_values)) self.assertEqual(res, expected_values) + + def test_upgrade_function(self) -> None: + # drop the constraint so we can insert nulls in full_user_id to populate the test + if isinstance(self.store.database_engine, PostgresEngine): + + def f(txn: LoggingTransaction) -> None: + txn.execute( + "ALTER TABLE user_filters DROP CONSTRAINT full_user_id_not_null" + ) + + self.get_success(self.store.db_pool.runInteraction("", f)) + + for i in range(0, 70): + self.get_success( + self.store.db_pool.simple_insert( + "user_filters", + { + "user_id": f"hello{i:02}", + "filter_id": i, + "filter_json": bytearray(i), + }, + ) + ) + + # re-add the constraint so that when it's validated it actually exists + if isinstance(self.store.database_engine, PostgresEngine): + + def f(txn: LoggingTransaction) -> None: + txn.execute( + "ALTER TABLE user_filters ADD CONSTRAINT full_user_id_not_null CHECK (full_user_id IS NOT NULL) NOT VALID" + ) + + self.get_success(self.store.db_pool.runInteraction("", f)) + + # get the upgrade function and run the upgrade on the db we just prepared + module_name = "synapse.storage.v78_0_2_validate_and_update_user_filters" + absolute_path = "../../synapse/storage/schema/main/delta/78/02_validate_and_update_user_filters.py" + + spec = importlib.util.spec_from_file_location(module_name, absolute_path) + assert spec is not None + module = importlib.util.module_from_spec(spec) + assert spec.loader is not None + spec.loader.exec_module(module) + + self.get_success( + self.store.db_pool.runInteraction( + "", + module.run_upgrade, + self.store.database_engine, + self.hs.config, + ) + ) + + expected_values = [] + for i in range(0, 70): + expected_values.append((f"@hello{i:02}:{self.hs.hostname}",)) + + res = self.get_success( + self.store.db_pool.execute( + "", None, "SELECT full_user_id from user_filters ORDER BY full_user_id" + ) + ) + self.assertEqual(len(res), len(expected_values)) + self.assertEqual(res, expected_values) From ab51a415325baaa87757a90569091c4ec0eead9c Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Mon, 22 May 2023 14:10:12 -0700 Subject: [PATCH 07/15] newsfragement + fix stray space --- changelog.d/15649.misc | 1 + tests/storage/test_profile.py | 2 -- tests/storage/test_user_filters.py | 1 + 3 files changed, 2 insertions(+), 2 deletions(-) create mode 100644 changelog.d/15649.misc diff --git a/changelog.d/15649.misc b/changelog.d/15649.misc new file mode 100644 index 000000000000..fca38abe0f60 --- /dev/null +++ b/changelog.d/15649.misc @@ -0,0 +1 @@ +Read from column `full_user_id` rather than `user_id` of tables `profiles` and `user_filters`. diff --git a/tests/storage/test_profile.py b/tests/storage/test_profile.py index a8c9e8c7895c..14ff2083b7ee 100644 --- a/tests/storage/test_profile.py +++ b/tests/storage/test_profile.py @@ -12,7 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. import importlib.util - from twisted.test.proto_helpers import MemoryReactor from synapse.server import HomeServer @@ -73,7 +72,6 @@ def test_profiles_bg_migration(self) -> None: """ updater = self.hs.get_datastores().main.db_pool.updates - # drop the constraint so we can insert nulls in full_user_id to populate the test if isinstance(self.store.database_engine, PostgresEngine): diff --git a/tests/storage/test_user_filters.py b/tests/storage/test_user_filters.py index 0f88ec1a6a10..5b17871501b7 100644 --- a/tests/storage/test_user_filters.py +++ b/tests/storage/test_user_filters.py @@ -11,6 +11,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. + import importlib.util from twisted.test.proto_helpers import MemoryReactor From 39861db55e0b33e7caee5d6292509102fa79570d Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Mon, 22 May 2023 14:15:16 -0700 Subject: [PATCH 08/15] add space back --- tests/storage/test_profile.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/storage/test_profile.py b/tests/storage/test_profile.py index 14ff2083b7ee..a802ba69b523 100644 --- a/tests/storage/test_profile.py +++ b/tests/storage/test_profile.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import importlib.util + from twisted.test.proto_helpers import MemoryReactor from synapse.server import HomeServer From 17fa2c0331db68dd9ef94e857ab69e9ffdbcab11 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 31 May 2023 10:05:31 -0700 Subject: [PATCH 09/15] fix sql alignment --- .../78/01_validate_and_update_profiles.py | 59 ++++++++--------- .../78/02_validate_and_update_user_filters.py | 65 ++++++++++--------- 2 files changed, 63 insertions(+), 61 deletions(-) diff --git a/synapse/storage/schema/main/delta/78/01_validate_and_update_profiles.py b/synapse/storage/schema/main/delta/78/01_validate_and_update_profiles.py index 8283cd1f0a13..e3884fd8ec09 100644 --- a/synapse/storage/schema/main/delta/78/01_validate_and_update_profiles.py +++ b/synapse/storage/schema/main/delta/78/01_validate_and_update_profiles.py @@ -27,25 +27,26 @@ def run_upgrade( if isinstance(database_engine, PostgresEngine): # check if the constraint can be validated check_sql = """ - SELECT user_id from profiles WHERE full_user_id IS NULL - """ + SELECT user_id from profiles WHERE full_user_id IS NULL + """ cur.execute(check_sql) res = cur.fetchall() if res: # there are rows the background job missed, finish them here before we validate the constraint process_rows_sql = """ - UPDATE profiles - SET full_user_id = '@' || user_id || ? - WHERE user_id IN (SELECT user_id FROM profiles WHERE - full_user_id IS NULL) - """ + UPDATE profiles + SET full_user_id = '@' || user_id || ? + WHERE user_id IN ( + SELECT user_id FROM profiles WHERE full_user_id IS NULL + ) + """ cur.execute(process_rows_sql, (f":{hostname}",)) # Now we can validate validate_sql = """ - ALTER TABLE profiles VALIDATE CONSTRAINT full_user_id_not_null - """ + ALTER TABLE profiles VALIDATE CONSTRAINT full_user_id_not_null + """ cur.execute(validate_sql) else: @@ -53,33 +54,33 @@ def run_upgrade( cur.execute("DROP TABLE IF EXISTS temp_profiles") create_sql = """ - CREATE TABLE temp_profiles ( - full_user_id text NOT NULL, - user_id text, - displayname text, - avatar_url text, - UNIQUE (full_user_id), - UNIQUE (user_id) - ) - """ + CREATE TABLE temp_profiles ( + full_user_id text NOT NULL, + user_id text, + displayname text, + avatar_url text, + UNIQUE (full_user_id), + UNIQUE (user_id) + ) + """ cur.execute(create_sql) copy_sql = """ - INSERT INTO temp_profiles ( - user_id, - displayname, - avatar_url, - full_user_id) - SELECT user_id, displayname, avatar_url, '@' || user_id || ':' || ? FROM profiles - """ + INSERT INTO temp_profiles ( + user_id, + displayname, + avatar_url, + full_user_id) + SELECT user_id, displayname, avatar_url, '@' || user_id || ':' || ? FROM profiles + """ cur.execute(copy_sql, (f"{hostname}",)) drop_sql = """ - DROP TABLE profiles - """ + DROP TABLE profiles + """ cur.execute(drop_sql) rename_sql = """ - ALTER TABLE temp_profiles RENAME to profiles - """ + ALTER TABLE temp_profiles RENAME to profiles + """ cur.execute(rename_sql) diff --git a/synapse/storage/schema/main/delta/78/02_validate_and_update_user_filters.py b/synapse/storage/schema/main/delta/78/02_validate_and_update_user_filters.py index 1a4427785566..2753f9151407 100644 --- a/synapse/storage/schema/main/delta/78/02_validate_and_update_user_filters.py +++ b/synapse/storage/schema/main/delta/78/02_validate_and_update_user_filters.py @@ -27,63 +27,64 @@ def run_upgrade( if isinstance(database_engine, PostgresEngine): # check if the constraint can be validated check_sql = """ - SELECT user_id from user_filters WHERE full_user_id IS NULL - """ + SELECT user_id from user_filters WHERE full_user_id IS NULL + """ cur.execute(check_sql) res = cur.fetchall() if res: # there are rows the background job missed, finish them here before we validate constraint process_rows_sql = """ - UPDATE user_filters - SET full_user_id = '@' || user_id || ? - WHERE user_id IN (SELECT user_id FROM user_filters - WHERE full_user_id IS NULL) - """ + UPDATE user_filters + SET full_user_id = '@' || user_id || ? + WHERE user_id IN ( + SELECT user_id FROM user_filters WHERE full_user_id IS NULL + ) + """ cur.execute(process_rows_sql, (f":{hostname}",)) # Now we can validate validate_sql = """ - ALTER TABLE user_filters VALIDATE CONSTRAINT full_user_id_not_null - """ + ALTER TABLE user_filters VALIDATE CONSTRAINT full_user_id_not_null + """ cur.execute(validate_sql) else: cur.execute("DROP TABLE IF EXISTS temp_user_filters") create_sql = """ - CREATE TABLE temp_user_filters ( - full_user_id text NOT NULL, - user_id text NOT NULL, - filter_id bigint NOT NULL, - filter_json bytea NOT NULL, - UNIQUE (full_user_id), - UNIQUE (user_id) - ) - """ + CREATE TABLE temp_user_filters ( + full_user_id text NOT NULL, + user_id text NOT NULL, + filter_id bigint NOT NULL, + filter_json bytea NOT NULL, + UNIQUE (full_user_id), + UNIQUE (user_id) + ) + """ cur.execute(create_sql) index_sql = """ - CREATE UNIQUE INDEX IF NOT EXISTS user_filters_unique ON - temp_user_filters (user_id, filter_id) - """ + CREATE UNIQUE INDEX IF NOT EXISTS user_filters_unique ON + temp_user_filters (user_id, filter_id) + """ cur.execute(index_sql) copy_sql = """ - INSERT INTO temp_user_filters ( - user_id, - filter_id, - filter_json, - full_user_id) - SELECT user_id, filter_id, filter_json, '@' || user_id || ':' || ? FROM user_filters - """ + INSERT INTO temp_user_filters ( + user_id, + filter_id, + filter_json, + full_user_id) + SELECT user_id, filter_id, filter_json, '@' || user_id || ':' || ? FROM user_filters + """ cur.execute(copy_sql, (f"{hostname}",)) drop_sql = """ - DROP TABLE user_filters - """ + DROP TABLE user_filters + """ cur.execute(drop_sql) rename_sql = """ - ALTER TABLE temp_user_filters RENAME to user_filters - """ + ALTER TABLE temp_user_filters RENAME to user_filters + """ cur.execute(rename_sql) From 27bde22f970387d909e7515ba1d919730dac3721 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 31 May 2023 10:25:13 -0700 Subject: [PATCH 10/15] remove tests --- tests/storage/test_profile.py | 61 ---------------------------- tests/storage/test_user_filters.py | 65 ------------------------------ 2 files changed, 126 deletions(-) diff --git a/tests/storage/test_profile.py b/tests/storage/test_profile.py index a802ba69b523..f9edbdd4f5d6 100644 --- a/tests/storage/test_profile.py +++ b/tests/storage/test_profile.py @@ -11,7 +11,6 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -import importlib.util from twisted.test.proto_helpers import MemoryReactor @@ -126,63 +125,3 @@ def f(txn: LoggingTransaction) -> None: ) self.assertEqual(len(res), len(expected_values)) self.assertEqual(res, expected_values) - - def test_upgrade_function(self) -> None: - # drop the constraint so we can insert nulls in full_user_id to populate the test - if isinstance(self.store.database_engine, PostgresEngine): - - def f(txn: LoggingTransaction) -> None: - txn.execute( - "ALTER TABLE profiles DROP CONSTRAINT full_user_id_not_null" - ) - - self.get_success(self.store.db_pool.runInteraction("", f)) - - for i in range(0, 10): - self.get_success( - self.store.db_pool.simple_insert( - "profiles", - {"user_id": f"hello{i:02}"}, - ) - ) - - # re-add the constraint so that when it's validated it actually exists - if isinstance(self.store.database_engine, PostgresEngine): - - def f(txn: LoggingTransaction) -> None: - txn.execute( - "ALTER TABLE profiles ADD CONSTRAINT full_user_id_not_null CHECK (full_user_id IS NOT NULL) NOT VALID" - ) - - self.get_success(self.store.db_pool.runInteraction("", f)) - - expected_values = [] - for i in range(0, 10): - expected_values.append((f"@hello{i:02}:{self.hs.hostname}",)) - - # get the upgrade function and run it on the db we just prepared - module_name = "synapse.storage.v78_0_1_validate_and_update_profiles" - absolute_path = "../../synapse/storage/schema/main/delta/78/01_validate_and_update_profiles.py" - - spec = importlib.util.spec_from_file_location(module_name, absolute_path) - assert spec is not None - module = importlib.util.module_from_spec(spec) - assert spec.loader is not None - spec.loader.exec_module(module) - - self.get_success( - self.store.db_pool.runInteraction( - "", - module.run_upgrade, - self.store.database_engine, - self.hs.config, - ) - ) - - res = self.get_success( - self.store.db_pool.execute( - "", None, "SELECT full_user_id from profiles ORDER BY full_user_id" - ) - ) - self.assertEqual(len(res), len(expected_values)) - self.assertEqual(res, expected_values) diff --git a/tests/storage/test_user_filters.py b/tests/storage/test_user_filters.py index 5b17871501b7..bab802f56ec6 100644 --- a/tests/storage/test_user_filters.py +++ b/tests/storage/test_user_filters.py @@ -12,7 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -import importlib.util from twisted.test.proto_helpers import MemoryReactor @@ -93,67 +92,3 @@ def f(txn: LoggingTransaction) -> None: ) self.assertEqual(len(res), len(expected_values)) self.assertEqual(res, expected_values) - - def test_upgrade_function(self) -> None: - # drop the constraint so we can insert nulls in full_user_id to populate the test - if isinstance(self.store.database_engine, PostgresEngine): - - def f(txn: LoggingTransaction) -> None: - txn.execute( - "ALTER TABLE user_filters DROP CONSTRAINT full_user_id_not_null" - ) - - self.get_success(self.store.db_pool.runInteraction("", f)) - - for i in range(0, 70): - self.get_success( - self.store.db_pool.simple_insert( - "user_filters", - { - "user_id": f"hello{i:02}", - "filter_id": i, - "filter_json": bytearray(i), - }, - ) - ) - - # re-add the constraint so that when it's validated it actually exists - if isinstance(self.store.database_engine, PostgresEngine): - - def f(txn: LoggingTransaction) -> None: - txn.execute( - "ALTER TABLE user_filters ADD CONSTRAINT full_user_id_not_null CHECK (full_user_id IS NOT NULL) NOT VALID" - ) - - self.get_success(self.store.db_pool.runInteraction("", f)) - - # get the upgrade function and run the upgrade on the db we just prepared - module_name = "synapse.storage.v78_0_2_validate_and_update_user_filters" - absolute_path = "../../synapse/storage/schema/main/delta/78/02_validate_and_update_user_filters.py" - - spec = importlib.util.spec_from_file_location(module_name, absolute_path) - assert spec is not None - module = importlib.util.module_from_spec(spec) - assert spec.loader is not None - spec.loader.exec_module(module) - - self.get_success( - self.store.db_pool.runInteraction( - "", - module.run_upgrade, - self.store.database_engine, - self.hs.config, - ) - ) - - expected_values = [] - for i in range(0, 70): - expected_values.append((f"@hello{i:02}:{self.hs.hostname}",)) - - res = self.get_success( - self.store.db_pool.execute( - "", None, "SELECT full_user_id from user_filters ORDER BY full_user_id" - ) - ) - self.assertEqual(len(res), len(expected_values)) - self.assertEqual(res, expected_values) From ea60b0c94105f4217f27390c270f341a565ed8e5 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 31 May 2023 10:33:57 -0700 Subject: [PATCH 11/15] fix stray line --- tests/storage/test_profile.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/storage/test_profile.py b/tests/storage/test_profile.py index f9edbdd4f5d6..f4afdfbbb393 100644 --- a/tests/storage/test_profile.py +++ b/tests/storage/test_profile.py @@ -70,7 +70,6 @@ def test_profiles_bg_migration(self) -> None: Test background job that copies entries from column user_id to full_user_id, adding the hostname in the process. """ - updater = self.hs.get_datastores().main.db_pool.updates # drop the constraint so we can insert nulls in full_user_id to populate the test if isinstance(self.store.database_engine, PostgresEngine): From cf458a7c5caa1e3509a65f8b741803dc77af0bd6 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 31 May 2023 10:34:37 -0700 Subject: [PATCH 12/15] fix stray line pt 2 --- tests/storage/test_profile.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/storage/test_profile.py b/tests/storage/test_profile.py index f4afdfbbb393..fe5bb7791336 100644 --- a/tests/storage/test_profile.py +++ b/tests/storage/test_profile.py @@ -71,6 +71,7 @@ def test_profiles_bg_migration(self) -> None: the hostname in the process. """ updater = self.hs.get_datastores().main.db_pool.updates + # drop the constraint so we can insert nulls in full_user_id to populate the test if isinstance(self.store.database_engine, PostgresEngine): From 2236845cd592429322b42f51ed9ab59f743c30d2 Mon Sep 17 00:00:00 2001 From: Shay Date: Fri, 2 Jun 2023 11:15:55 -0700 Subject: [PATCH 13/15] Update synapse/storage/schema/main/delta/78/01_validate_and_update_profiles.py Co-authored-by: Eric Eastwood --- .../schema/main/delta/78/01_validate_and_update_profiles.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/storage/schema/main/delta/78/01_validate_and_update_profiles.py b/synapse/storage/schema/main/delta/78/01_validate_and_update_profiles.py index e3884fd8ec09..1653696ccbc6 100644 --- a/synapse/storage/schema/main/delta/78/01_validate_and_update_profiles.py +++ b/synapse/storage/schema/main/delta/78/01_validate_and_update_profiles.py @@ -50,7 +50,8 @@ def run_upgrade( cur.execute(validate_sql) else: - # in SQLite we need to rewrite the table to add the constraint + # in SQLite we need to rewrite the table to add the constraint. + # First drop any temporary table that might be here from a previous failed migration. cur.execute("DROP TABLE IF EXISTS temp_profiles") create_sql = """ From c0841e4fff640209c4fc1c68d94e8c2d27afe75d Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Fri, 2 Jun 2023 11:23:48 -0700 Subject: [PATCH 14/15] add some docstrings to the update function --- .../schema/main/delta/78/01_validate_and_update_profiles.py | 5 +++++ .../main/delta/78/02_validate_and_update_user_filters.py | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/synapse/storage/schema/main/delta/78/01_validate_and_update_profiles.py b/synapse/storage/schema/main/delta/78/01_validate_and_update_profiles.py index e3884fd8ec09..fd0b7b5810d3 100644 --- a/synapse/storage/schema/main/delta/78/01_validate_and_update_profiles.py +++ b/synapse/storage/schema/main/delta/78/01_validate_and_update_profiles.py @@ -22,6 +22,11 @@ def run_upgrade( database_engine: BaseDatabaseEngine, config: HomeServerConfig, ) -> None: + """ + Part 3 of a multi-step migration to drop the column `user_id` and replace it with + `full_user_id`. See the database schema docs for more information on the full + migration steps. + """ hostname = config.server.server_name if isinstance(database_engine, PostgresEngine): diff --git a/synapse/storage/schema/main/delta/78/02_validate_and_update_user_filters.py b/synapse/storage/schema/main/delta/78/02_validate_and_update_user_filters.py index 2753f9151407..8ef63335e7c3 100644 --- a/synapse/storage/schema/main/delta/78/02_validate_and_update_user_filters.py +++ b/synapse/storage/schema/main/delta/78/02_validate_and_update_user_filters.py @@ -22,6 +22,11 @@ def run_upgrade( database_engine: BaseDatabaseEngine, config: HomeServerConfig, ) -> None: + """ + Part 3 of a multi-step migration to drop the column `user_id` and replace it with + `full_user_id`. See the database schema docs for more information on the full + migration steps. + """ hostname = config.server.server_name if isinstance(database_engine, PostgresEngine): From 8561a54a1cefcbce9807aa4f47d538f597b55523 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Fri, 2 Jun 2023 14:40:04 -0700 Subject: [PATCH 15/15] update list of schema changes for 78 --- synapse/storage/schema/__init__.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/synapse/storage/schema/__init__.py b/synapse/storage/schema/__init__.py index 48a8ddee029b..fc190a8b13cf 100644 --- a/synapse/storage/schema/__init__.py +++ b/synapse/storage/schema/__init__.py @@ -103,6 +103,9 @@ Changes in SCHEMA_VERSION = 77 - (Postgres) Add NOT VALID CHECK (full_user_id IS NOT NULL) to tables profiles and user_filters + +Changes in SCHEMA_VERSION = 78 + - Validate check (full_user_id IS NOT NULL) on tables profiles and user_filters """