From 887de61e413c74e6511a0359b72b01a67b313c03 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 27 Aug 2020 13:36:01 -0400 Subject: [PATCH 1/3] Convert simple_delete to async/await. --- changelog.d/8191.misc | 1 + synapse/storage/database.py | 42 ++++++++++++++++++- .../storage/databases/main/group_server.py | 28 +++++++------ .../storage/databases/main/registration.py | 29 +++++++------ tests/storage/test_event_push_actions.py | 6 ++- 5 files changed, 75 insertions(+), 31 deletions(-) create mode 100644 changelog.d/8191.misc diff --git a/changelog.d/8191.misc b/changelog.d/8191.misc new file mode 100644 index 000000000000..dfe4c03171d6 --- /dev/null +++ b/changelog.d/8191.misc @@ -0,0 +1 @@ +Convert various parts of the codebase to async/await. diff --git a/synapse/storage/database.py b/synapse/storage/database.py index ba4c0c9af6d1..e5367e6dd663 100644 --- a/synapse/storage/database.py +++ b/synapse/storage/database.py @@ -1451,13 +1451,37 @@ def simple_delete_one_txn( if txn.rowcount > 1: raise StoreError(500, "More than one row matched (%s)" % (table,)) - def simple_delete(self, table: str, keyvalues: Dict[str, Any], desc: str): - return self.runInteraction(desc, self.simple_delete_txn, table, keyvalues) + async def simple_delete( + self, table: str, keyvalues: Dict[str, Any], desc: str + ) -> int: + """Executes a DELETE query on the named table. + + Filters rows by the key-value pairs. + + Args: + table: string giving the table name + keyvalues: dict of column names and values to select the row with + + Returns: + The number of deleted rows. + """ + return await self.runInteraction(desc, self.simple_delete_txn, table, keyvalues) @staticmethod def simple_delete_txn( txn: LoggingTransaction, table: str, keyvalues: Dict[str, Any] ) -> int: + """Executes a DELETE query on the named table. + + Filters rows by the key-value pairs. + + Args: + table: string giving the table name + keyvalues: dict of column names and values to select the row with + + Returns: + The number of deleted rows. + """ sql = "DELETE FROM %s WHERE %s" % ( table, " AND ".join("%s = ?" % (k,) for k in keyvalues), @@ -1474,6 +1498,20 @@ async def simple_delete_many( keyvalues: Dict[str, Any], desc: str, ) -> int: + """Executes a DELETE query on the named table. + + Filters rows by if value of `column` is in `iterable`. + + Args: + txn: Transaction object + table: string giving the table name + column: column name to test for inclusion against `iterable` + iterable: list + keyvalues: dict of column names and values to select the rows with + + Returns: + Number rows deleted + """ return await self.runInteraction( desc, self.simple_delete_many_txn, table, column, iterable, keyvalues ) diff --git a/synapse/storage/databases/main/group_server.py b/synapse/storage/databases/main/group_server.py index e3ead71853ea..34f1d6e190b5 100644 --- a/synapse/storage/databases/main/group_server.py +++ b/synapse/storage/databases/main/group_server.py @@ -728,11 +728,13 @@ def _add_room_to_summary_txn( }, ) - def remove_room_from_summary(self, group_id, room_id, category_id): + async def remove_room_from_summary( + self, group_id: str, room_id: str, category_id: str + ) -> int: if category_id is None: category_id = _DEFAULT_CATEGORY_ID - return self.db_pool.simple_delete( + return await self.db_pool.simple_delete( table="group_summary_rooms", keyvalues={ "group_id": group_id, @@ -766,8 +768,8 @@ def upsert_group_category(self, group_id, category_id, profile, is_public): desc="upsert_group_category", ) - def remove_group_category(self, group_id, category_id): - return self.db_pool.simple_delete( + async def remove_group_category(self, group_id: str, category_id: str) -> int: + return await self.db_pool.simple_delete( table="group_room_categories", keyvalues={"group_id": group_id, "category_id": category_id}, desc="remove_group_category", @@ -797,8 +799,8 @@ def upsert_group_role(self, group_id, role_id, profile, is_public): desc="upsert_group_role", ) - def remove_group_role(self, group_id, role_id): - return self.db_pool.simple_delete( + async def remove_group_role(self, group_id: str, role_id: str) -> int: + return await self.db_pool.simple_delete( table="group_roles", keyvalues={"group_id": group_id, "role_id": role_id}, desc="remove_group_role", @@ -928,11 +930,13 @@ def _add_user_to_summary_txn( }, ) - def remove_user_from_summary(self, group_id, user_id, role_id): + async def remove_user_from_summary( + self, group_id: str, user_id: str, role_id: str + ) -> int: if role_id is None: role_id = _DEFAULT_ROLE_ID - return self.db_pool.simple_delete( + return await self.db_pool.simple_delete( table="group_summary_users", keyvalues={"group_id": group_id, "role_id": role_id, "user_id": user_id}, desc="remove_user_from_summary", @@ -1250,16 +1254,16 @@ async def update_remote_attestion( desc="update_remote_attestion", ) - def remove_attestation_renewal(self, group_id, user_id): + async def remove_attestation_renewal(self, group_id: str, user_id: str) -> int: """Remove an attestation that we thought we should renew, but actually shouldn't. Ideally this would never get called as we would never incorrectly try and do attestations for local users on local groups. Args: - group_id (str) - user_id (str) + group_id + user_id """ - return self.db_pool.simple_delete( + return await self.db_pool.simple_delete( table="group_attestations_renewals", keyvalues={"group_id": group_id, "user_id": user_id}, desc="remove_attestation_renewal", diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 48bda66f3ec3..3ae9e77e09a5 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -529,21 +529,21 @@ async def user_get_threepids(self, user_id): "user_get_threepids", ) - def user_delete_threepid(self, user_id, medium, address): - return self.db_pool.simple_delete( + async def user_delete_threepid(self, user_id, medium, address) -> None: + await self.db_pool.simple_delete( "user_threepids", keyvalues={"user_id": user_id, "medium": medium, "address": address}, desc="user_delete_threepid", ) - def user_delete_threepids(self, user_id: str): + async def user_delete_threepids(self, user_id: str) -> None: """Delete all threepid this user has bound Args: user_id: The user id to delete all threepids of """ - return self.db_pool.simple_delete( + await self.db_pool.simple_delete( "user_threepids", keyvalues={"user_id": user_id}, desc="user_delete_threepids", @@ -598,21 +598,20 @@ async def user_get_bound_threepids(self, user_id: str) -> List[Dict[str, Any]]: desc="user_get_bound_threepids", ) - def remove_user_bound_threepid(self, user_id, medium, address, id_server): + async def remove_user_bound_threepid( + self, user_id: str, medium: str, address: str, id_server: str + ) -> None: """The server proxied an unbind request to the given identity server on behalf of the given user, so we remove the mapping of threepid to identity server. Args: - user_id (str) - medium (str) - address (str) - id_server (str) - - Returns: - Deferred + user_id + medium + address + id_server """ - return self.db_pool.simple_delete( + await self.db_pool.simple_delete( table="user_threepid_id_server", keyvalues={ "user_id": user_id, @@ -1248,14 +1247,14 @@ def add_user_pending_deactivation(self, user_id): desc="add_user_pending_deactivation", ) - def del_user_pending_deactivation(self, user_id): + async def del_user_pending_deactivation(self, user_id: str) -> None: """ Removes the given user to the table of users who need to be parted from all the rooms they're in, effectively marking that user as fully deactivated. """ # XXX: This should be simple_delete_one but we failed to put a unique index on # the table, so somehow duplicate entries have ended up in it. - return self.db_pool.simple_delete( + await self.db_pool.simple_delete( "users_pending_deactivation", keyvalues={"user_id": user_id}, desc="del_user_pending_deactivation", diff --git a/tests/storage/test_event_push_actions.py b/tests/storage/test_event_push_actions.py index 238bad5b4513..0e7427e57abe 100644 --- a/tests/storage/test_event_push_actions.py +++ b/tests/storage/test_event_push_actions.py @@ -123,8 +123,10 @@ def _mark_read(stream, depth): yield _inject_actions(6, PlAIN_NOTIF) yield _rotate(7) - yield self.store.db_pool.simple_delete( - table="event_push_actions", keyvalues={"1": 1}, desc="" + yield defer.ensureDeferred( + self.store.db_pool.simple_delete( + table="event_push_actions", keyvalues={"1": 1}, desc="" + ) ) yield _assert_counts(1, 0) From 8e06c4667cbb2d80ca3552f1e3afd150f228eaac Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 27 Aug 2020 13:49:53 -0400 Subject: [PATCH 2/3] Add documentation for 'desc' property in many places. --- synapse/storage/database.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/synapse/storage/database.py b/synapse/storage/database.py index e5367e6dd663..8e9592f7be64 100644 --- a/synapse/storage/database.py +++ b/synapse/storage/database.py @@ -614,6 +614,7 @@ async def execute( """Runs a single query for a result set. Args: + desc: description of the transaction, for logging and metrics decoder - The function which can resolve the cursor results to something meaningful. query - The query string to execute @@ -649,7 +650,7 @@ async def simple_insert( or_ignore: bool stating whether an exception should be raised when a conflicting row already exists. If True, False will be returned by the function instead - desc: string giving a description of the transaction + desc: description of the transaction, for logging and metrics Returns: Whether the row was inserted or not. Only useful when `or_ignore` is True @@ -686,7 +687,7 @@ async def simple_insert_many( Args: table: string giving the table name values: dict of new column names and values for them - desc: string giving a description of the transaction + desc: description of the transaction, for logging and metrics """ await self.runInteraction(desc, self.simple_insert_many_txn, table, values) @@ -700,7 +701,6 @@ def simple_insert_many_txn( txn: The transaction to use. table: string giving the table name values: dict of new column names and values for them - desc: string giving a description of the transaction """ if not values: return @@ -755,6 +755,7 @@ async def simple_upsert( keyvalues: The unique key columns and their new values values: The nonunique columns and their new values insertion_values: additional key/values to use only when inserting + desc: description of the transaction, for logging and metrics lock: True to lock the table when doing the upsert. Returns: Native upserts always return None. Emulated upserts return True if a @@ -1081,6 +1082,7 @@ async def simple_select_one( retcols: list of strings giving the names of the columns to return allow_none: If true, return None instead of failing if the SELECT statement returns no rows + desc: description of the transaction, for logging and metrics """ return await self.runInteraction( desc, self.simple_select_one_txn, table, keyvalues, retcols, allow_none @@ -1166,6 +1168,7 @@ async def simple_select_onecol( table: table name keyvalues: column names and values to select the rows with retcol: column whos value we wish to retrieve. + desc: description of the transaction, for logging and metrics Returns: Results in a list @@ -1190,6 +1193,7 @@ async def simple_select_list( column names and values to select the rows with, or None to not apply a WHERE clause. retcols: the names of the columns to return + desc: description of the transaction, for logging and metrics Returns: A list of dictionaries. @@ -1249,8 +1253,10 @@ async def simple_select_many_batch( table: string giving the table name column: column name to test for inclusion against `iterable` iterable: list - keyvalues: dict of column names and values to select the rows with retcols: list of strings giving the names of the columns to return + keyvalues: dict of column names and values to select the rows with + desc: description of the transaction, for logging and metrics + batch_size: the number of rows for each select query """ results = [] # type: List[Dict[str, Any]] @@ -1367,6 +1373,7 @@ async def simple_update_one( table: string giving the table name keyvalues: dict of column names and values to select the row with updatevalues: dict giving column names and values to update + desc: description of the transaction, for logging and metrics """ await self.runInteraction( desc, self.simple_update_one_txn, table, keyvalues, updatevalues @@ -1426,6 +1433,7 @@ async def simple_delete_one( Args: table: string giving the table name keyvalues: dict of column names and values to select the row with + desc: description of the transaction, for logging and metrics """ await self.runInteraction(desc, self.simple_delete_one_txn, table, keyvalues) @@ -1461,6 +1469,7 @@ async def simple_delete( Args: table: string giving the table name keyvalues: dict of column names and values to select the row with + desc: description of the transaction, for logging and metrics Returns: The number of deleted rows. @@ -1503,11 +1512,11 @@ async def simple_delete_many( Filters rows by if value of `column` is in `iterable`. Args: - txn: Transaction object table: string giving the table name column: column name to test for inclusion against `iterable` iterable: list keyvalues: dict of column names and values to select the rows with + desc: description of the transaction, for logging and metrics Returns: Number rows deleted From 843f00daea1f2f88ef4667567db37ec97251e229 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 27 Aug 2020 13:50:55 -0400 Subject: [PATCH 3/3] Clarify comment. --- synapse/storage/database.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/database.py b/synapse/storage/database.py index 8e9592f7be64..7ab370efef15 100644 --- a/synapse/storage/database.py +++ b/synapse/storage/database.py @@ -1247,7 +1247,7 @@ async def simple_select_many_batch( """Executes a SELECT query on the named table, which may return zero or more rows, returning the result as a list of dicts. - Filters rows by if value of `column` is in `iterable`. + Filters rows by whether the value of `column` is in `iterable`. Args: table: string giving the table name @@ -1297,7 +1297,7 @@ def simple_select_many_txn( """Executes a SELECT query on the named table, which may return zero or more rows, returning the result as a list of dicts. - Filters rows by if value of `column` is in `iterable`. + Filters rows by whether the value of `column` is in `iterable`. Args: txn: Transaction object