From 07b82a217943bb4fd47fada90eafaf90d3f4b483 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 31 Jan 2019 15:16:10 +0000 Subject: [PATCH 01/58] rewrite userdir --- synapse/handlers/user_directory.py | 90 ++-------- synapse/storage/_base.py | 58 ++++++ .../storage/schema/delta/53/user_share.sql | 46 +++++ synapse/storage/user_directory.py | 165 +++++++++++------- 4 files changed, 215 insertions(+), 144 deletions(-) create mode 100644 synapse/storage/schema/delta/53/user_share.sql diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 120815b09b91..9060272f691b 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -249,7 +249,6 @@ def _handle_initial_room(self, room_id): # We sleep aggressively here as otherwise it can starve resources. # We also batch up inserts/updates, but try to avoid too many at once. to_insert = set() - to_update = set() count = 0 for user_id in user_ids: if count % self.INITIAL_ROOM_SLEEP_COUNT == 0: @@ -272,21 +271,7 @@ def _handle_initial_room(self, room_id): count += 1 user_set = (user_id, other_user_id) - - if user_set in self.initially_handled_users_share_private_room: - continue - - if user_set in self.initially_handled_users_share: - if is_public: - continue - to_update.add(user_set) - else: - to_insert.add(user_set) - - if is_public: - self.initially_handled_users_share.add(user_set) - else: - self.initially_handled_users_share_private_room.add(user_set) + to_insert.add(user_set) if len(to_insert) > self.INITIAL_ROOM_BATCH_SIZE: yield self.store.add_users_who_share_room( @@ -294,24 +279,12 @@ def _handle_initial_room(self, room_id): ) to_insert.clear() - if len(to_update) > self.INITIAL_ROOM_BATCH_SIZE: - yield self.store.update_users_who_share_room( - room_id, not is_public, to_update, - ) - to_update.clear() - if to_insert: yield self.store.add_users_who_share_room( room_id, not is_public, to_insert, ) to_insert.clear() - if to_update: - yield self.store.update_users_who_share_room( - room_id, not is_public, to_update, - ) - to_update.clear() - @defer.inlineCallbacks def _handle_deltas(self, deltas): """Called with the state deltas to process @@ -476,43 +449,22 @@ def _handle_new_user(self, room_id, user_id, profile): else: logger.debug("Not adding new user to public dir, %r", user_id) - # Now we update users who share rooms with users. We do this by getting - # all the current users in the room and seeing which aren't already - # marked in the database as sharing with `user_id` - + # Now we update users who share rooms with users. users_with_profile = yield self.state.get_current_user_in_room(room_id) to_insert = set() - to_update = set() - - is_appservice = self.store.get_if_app_services_interested_in_user(user_id) # First, if they're our user then we need to update for every user - if self.is_mine_id(user_id) and not is_appservice: - # Returns a map of other_user_id -> shared_private. We only need - # to update mappings if for users that either don't share a room - # already (aren't in the map) or, if the room is private, those that - # only share a public room. - user_ids_shared = yield self.store.get_users_who_share_room_from_dir( - user_id - ) + if self.is_mine_id(user_id): + is_appservice = self.store.get_if_app_services_interested_in_user(user_id) - for other_user_id in users_with_profile: - if user_id == other_user_id: - continue + # We don't care about appservice users. + if not is_appservice: + for other_user_id in users_with_profile: + if user_id == other_user_id: + continue - shared_is_private = user_ids_shared.get(other_user_id) - if shared_is_private is True: - # We've already marked in the database they share a private room - continue - elif shared_is_private is False: - # They already share a public room, so only update if this is - # a private room - if not is_public: - to_update.add((user_id, other_user_id)) - elif shared_is_private is None: - # This is the first time they both share a room - to_insert.add((user_id, other_user_id)) + to_update.add((user_id, other_user_id)) # Next we need to update for every local user in the room for other_user_id in users_with_profile: @@ -523,31 +475,13 @@ def _handle_new_user(self, room_id, user_id, profile): other_user_id ) if self.is_mine_id(other_user_id) and not is_appservice: - shared_is_private = yield self.store.get_if_users_share_a_room( - other_user_id, user_id, - ) - if shared_is_private is True: - # We've already marked in the database they share a private room - continue - elif shared_is_private is False: - # They already share a public room, so only update if this is - # a private room - if not is_public: - to_update.add((other_user_id, user_id)) - elif shared_is_private is None: - # This is the first time they both share a room - to_insert.add((other_user_id, user_id)) + to_insert.add((other_user_id, user_id)) if to_insert: yield self.store.add_users_who_share_room( room_id, not is_public, to_insert, ) - if to_update: - yield self.store.update_users_who_share_room( - room_id, not is_public, to_update, - ) - @defer.inlineCallbacks def _handle_remove_user(self, room_id, user_id): """Called when we might need to remove user to directory @@ -595,7 +529,7 @@ def _handle_remove_user(self, room_id, user_id): elif update_user_in_public: yield self.store.remove_from_user_in_public_room(user_id) - # Now handle users_who_share_rooms. + # Now handle users who share rooms. # Get a list of user tuples that were in the DB due to this room and # users (this includes tuples where the other user matches `user_id`) diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index 4872ff55b634..b443e6232635 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -16,7 +16,9 @@ import sys import threading import time +import itertools +from itertools import zip_longest from six import PY2, iteritems, iterkeys, itervalues from six.moves import builtins, intern, range @@ -738,6 +740,62 @@ def _simple_upsert_txn_native_upsert( ) txn.execute(sql, list(allvalues.values())) + def _simple_upsert_many_emulated_txn(self, txn, table, keys, keyvalues, values, valuesvalues): + + + if not valuesvalues: + valuesvalues = [() * len(keyvalues)] + + #def _simple_upsert_txn_emulated( + #self, txn, table, keyvalues, values, insertion_values={}, lock=True + + for keyv, valv in zip(keyvalues, valuesvalues): + + keys = {x:y for x, y in zip(keys, keyv)} + vals = {x:y for x, y in zip(values, valv)} + + _simple_upsert_txn_emulated(txn, table, keys, vals) + + + def _simple_upsert_many_native_txn(self, txn, table, keys, keyvalues, values, valuesvalues): + + allvalues = [] + allvalues.extend(keys) + allvalues.extend(values) + + if not valuesvalues: + valuesvalues = [() * len(keyvalues)] + + if not values: + latter = "NOTHING" + else: + latter = "UPDATE SET" + ", ".join(k + "=EXCLUDED." + k for k in values), + + sql = ( + "INSERT INTO %s (%s) VALUES (%s) " + "ON CONFLICT (%s) DO %s" + ) % ( + table, + ", ".join(k for k in allvalues), + ", ".join("?" for _ in allvalues), + ", ".join(keys), + latter + ) + + if isinstance(self.database_engine, PostgresEngine): + from psycopg2.extras import execute_batch + execute_batch(txn, sql, zip_longest(keyvalues, valuesvalues)) + + else: + x = zip_longest(keyvalues, valuesvalues) + + for val in x: + end = [] + for i in val: + if i: + end.extend(i) + txn.execute(sql, end) + def _simple_select_one(self, table, keyvalues, retcols, allow_none=False, desc="_simple_select_one"): """Executes a SELECT query on the named table, which is expected to diff --git a/synapse/storage/schema/delta/53/user_share.sql b/synapse/storage/schema/delta/53/user_share.sql new file mode 100644 index 000000000000..d4aee8c10a24 --- /dev/null +++ b/synapse/storage/schema/delta/53/user_share.sql @@ -0,0 +1,46 @@ +/* Copyright 2017 Vector Creations Ltd, 2019 New Vector Ltd + * + * 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. + */ + +DROP TABLE users_who_share_rooms; + +-- Table keeping track of who shares a room with who. We only keep track +-- of this for local users, so `user_id` is local users only (but we do keep track +-- of which remote users share a room) +CREATE TABLE users_who_share_public_rooms ( + user_id TEXT NOT NULL, + other_user_id TEXT NOT NULL, + room_id TEXT NOT NULL +); + + +CREATE UNIQUE INDEX users_who_share_public_rooms_u_idx ON users_who_share_public_rooms(user_id, other_user_id, room_id); +CREATE INDEX users_who_share_public_rooms_r_idx ON users_who_share_public_rooms(room_id); +CREATE INDEX users_who_share_public_rooms_o_idx ON users_who_share_public_rooms(other_user_id); + + +CREATE TABLE users_who_share_private_rooms ( + user_id TEXT NOT NULL, + other_user_id TEXT NOT NULL, + room_id TEXT NOT NULL +); + + +CREATE UNIQUE INDEX users_who_share_private_rooms_u_idx ON users_who_share_private_rooms(user_id, other_user_id, room_id); +CREATE INDEX users_who_share_private_rooms_r_idx ON users_who_share_private_rooms(room_id); +CREATE INDEX users_who_share_private_rooms_o_idx ON users_who_share_private_rooms(other_user_id); + + +-- Make sure that we populate the table initially +UPDATE user_directory_stream_pos SET stream_id = NULL; diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index ce4821226599..aaf01be3532f 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -319,8 +319,15 @@ def get_users_in_dir_due_to_room(self, room_id): desc="get_users_in_dir_due_to_room", ) - user_ids_share = yield self._simple_select_onecol( - table="users_who_share_rooms", + user_ids_share_pub = yield self._simple_select_onecol( + table="users_who_share_public_rooms", + keyvalues={"room_id": room_id}, + retcol="user_id", + desc="get_users_in_dir_due_to_room", + ) + + user_ids_share_priv = yield self._simple_select_onecol( + table="users_who_share_private_rooms", keyvalues={"room_id": room_id}, retcol="user_id", desc="get_users_in_dir_due_to_room", @@ -355,7 +362,7 @@ def get_all_local_users(self): defer.returnValue([name for name, in rows]) def add_users_who_share_room(self, room_id, share_private, user_id_tuples): - """Insert entries into the users_who_share_rooms table. The first + """Insert entries into the users_who_share_*_rooms table. The first user should be a local user. Args: @@ -364,18 +371,22 @@ def add_users_who_share_room(self, room_id, share_private, user_id_tuples): user_id_tuples([(str, str)]): iterable of 2-tuple of user IDs. """ def _add_users_who_share_room_txn(txn): - self._simple_insert_many_txn( + + if share_private: + tbl = "users_who_share_private_rooms" + else: + tbl = "users_who_share_public_rooms" + + self._simple_upsert_many_native_txn( txn, - table="users_who_share_rooms", - values=[ - { - "user_id": user_id, - "other_user_id": other_user_id, - "room_id": room_id, - "share_private": share_private, - } + table=tbl, + keys=["user_id", "other_user_id", "room_id"], + keyvalues=[ + (user_id, other_user_id, room_id) for user_id, other_user_id in user_id_tuples ], + values=(), + valuesvalues=(), ) for user_id, other_user_id in user_id_tuples: txn.call_after( @@ -390,41 +401,6 @@ def _add_users_who_share_room_txn(txn): "add_users_who_share_room", _add_users_who_share_room_txn ) - def update_users_who_share_room(self, room_id, share_private, user_id_sets): - """Updates entries in the users_who_share_rooms table. The first - user should be a local user. - - Args: - room_id (str) - share_private (bool): Is the room private - user_id_tuples([(str, str)]): iterable of 2-tuple of user IDs. - """ - def _update_users_who_share_room_txn(txn): - sql = """ - UPDATE users_who_share_rooms - SET room_id = ?, share_private = ? - WHERE user_id = ? AND other_user_id = ? - """ - txn.executemany( - sql, - ( - (room_id, share_private, uid, oid) - for uid, oid in user_id_sets - ) - ) - for user_id, other_user_id in user_id_sets: - txn.call_after( - self.get_users_who_share_room_from_dir.invalidate, - (user_id,), - ) - txn.call_after( - self.get_if_users_share_a_room.invalidate, - (user_id, other_user_id), - ) - return self.runInteraction( - "update_users_who_share_room", _update_users_who_share_room_txn - ) - def remove_user_who_share_room(self, user_id, other_user_id): """Deletes entries in the users_who_share_rooms table. The first user should be a local user. @@ -437,7 +413,15 @@ def remove_user_who_share_room(self, user_id, other_user_id): def _remove_user_who_share_room_txn(txn): self._simple_delete_txn( txn, - table="users_who_share_rooms", + table="users_who_share_private_rooms", + keyvalues={ + "user_id": user_id, + "other_user_id": other_user_id, + }, + ) + self._simple_delete_txn( + txn, + table="users_who_share_public_rooms", keyvalues={ "user_id": user_id, "other_user_id": other_user_id, @@ -456,7 +440,7 @@ def _remove_user_who_share_room_txn(txn): "remove_user_who_share_room", _remove_user_who_share_room_txn ) - @cached(max_entries=500000) + @cachedInlineCallbacks(max_entries=500000) def get_if_users_share_a_room(self, user_id, other_user_id): """Gets if users share a room. @@ -468,17 +452,39 @@ def get_if_users_share_a_room(self, user_id, other_user_id): bool|None: None if they don't share a room, otherwise whether they share a private room or not. """ - return self._simple_select_one_onecol( - table="users_who_share_rooms", + private = yield self._simple_select_one_onecol( + table="users_who_share_private_rooms", + keyvalues={ + "user_id": user_id, + "other_user_id": other_user_id, + }, + retcol="user_id", + allow_none=True, + desc="get_if_users_share_a_room", + ) + + if private is not None: + defer.returnValue(True) + + public = yield self._simple_select_one_onecol( + table="users_who_share_public_rooms", keyvalues={ "user_id": user_id, "other_user_id": other_user_id, }, - retcol="share_private", + retcol="user_id", allow_none=True, desc="get_if_users_share_a_room", ) + if public is not None: + defer.returnValue(False) + + defer.returnValue(None) + + + + @cachedInlineCallbacks(max_entries=500000, iterable=True) def get_users_who_share_room_from_dir(self, user_id): """Returns the set of users who share a room with `user_id` @@ -487,38 +493,64 @@ def get_users_who_share_room_from_dir(self, user_id): user_id(str): Must be a local user Returns: - dict: user_id -> share_private mapping + list: user_id """ rows = yield self._simple_select_list( - table="users_who_share_rooms", + table="users_who_share_private_rooms", keyvalues={ "user_id": user_id, }, - retcols=("other_user_id", "share_private",), + retcols=("other_user_id",), desc="get_users_who_share_room_with_user", ) - defer.returnValue({ - row["other_user_id"]: row["share_private"] - for row in rows - }) + pub_rows = yield self._simple_select_list( + table="users_who_share_public_rooms", + keyvalues={ + "user_id": user_id, + }, + retcols=("other_user_id",), + desc="get_users_who_share_room_with_user", + ) + + users = {} + + for user in rows: + users.add(user["other_user_id"]) + + for user in pub_rows: + users.add(user["other_user_id"]) + defer.returnValue(users) + + @defer.inlineCallbacks def get_users_in_share_dir_with_room_id(self, user_id, room_id): - """Get all user tuples that are in the users_who_share_rooms due to the - given room_id. + """ + Get all user tuples that are in the users_who_share_*_rooms + tables due to the given room_id. Returns: [(user_id, other_user_id)]: where one of the two will match the given user_id. """ sql = """ - SELECT user_id, other_user_id FROM users_who_share_rooms + SELECT user_id, other_user_id FROM users_who_share_public_rooms + WHERE room_id = ? AND (user_id = ? OR other_user_id = ?) + """ + public = yield self._execute( + "get_users_in_share_dir_with_room_id", None, sql, room_id, user_id, user_id + ) + + sql = """ + SELECT user_id, other_user_id FROM users_who_share_private_rooms WHERE room_id = ? AND (user_id = ? OR other_user_id = ?) """ - return self._execute( + private = yield self._execute( "get_users_in_share_dir_with_room_id", None, sql, room_id, user_id, user_id ) + return public + private + @defer.inlineCallbacks def get_rooms_in_common_for_users(self, user_id, other_user_id): """Given two user_ids find out the list of rooms they share. @@ -552,7 +584,8 @@ def _delete_all_from_user_dir_txn(txn): txn.execute("DELETE FROM user_directory") txn.execute("DELETE FROM user_directory_search") txn.execute("DELETE FROM users_in_public_rooms") - txn.execute("DELETE FROM users_who_share_rooms") + txn.execute("DELETE FROM users_who_share_public_rooms") + txn.execute("DELETE FROM users_who_share_private_rooms") txn.call_after(self.get_user_in_directory.invalidate_all) txn.call_after(self.get_user_in_public_room.invalidate_all) txn.call_after(self.get_users_who_share_room_from_dir.invalidate_all) @@ -678,8 +711,8 @@ def search_user_dir(self, user_id, search_term, limit): join_clause = """ LEFT JOIN users_in_public_rooms AS p USING (user_id) LEFT JOIN ( - SELECT other_user_id AS user_id FROM users_who_share_rooms - WHERE user_id = ? AND share_private + SELECT other_user_id AS user_id FROM users_who_share_private_rooms + WHERE user_id = ? ) AS s USING (user_id) """ join_args = (user_id,) From f993fd677dd689a19ae9dddc9971f6a043181610 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 31 Jan 2019 15:22:32 +0000 Subject: [PATCH 02/58] emulated --- synapse/storage/_base.py | 20 ++++++++++++++++++-- synapse/storage/user_directory.py | 2 +- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index b443e6232635..a6e67f8bb4f3 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -740,8 +740,24 @@ def _simple_upsert_txn_native_upsert( ) txn.execute(sql, list(allvalues.values())) - def _simple_upsert_many_emulated_txn(self, txn, table, keys, keyvalues, values, valuesvalues): + def _simple_upsert_many_txn(self, txn, table, keys, keyvalues, values, valuevalues): + if ( + self.database_engine.can_native_upsert + and table not in self._unsafe_to_upsert_tables + ): + return self._simple_upsert_many_txn_native_upsert( + txn, + table, + keys, keyvalues, values, valuevalues + ) + else: + return self._simple_upsert_many_txn_emulated( + txn, + table, + keys, keyvalues, values, valuevalues + ) + def _simple_upsert_many_txn_emulated(self, txn, table, keys, keyvalues, values, valuesvalues): if not valuesvalues: valuesvalues = [() * len(keyvalues)] @@ -757,7 +773,7 @@ def _simple_upsert_many_emulated_txn(self, txn, table, keys, keyvalues, values, _simple_upsert_txn_emulated(txn, table, keys, vals) - def _simple_upsert_many_native_txn(self, txn, table, keys, keyvalues, values, valuesvalues): + def _simple_upsert_many_txn_native_upsert(self, txn, table, keys, keyvalues, values, valuesvalues): allvalues = [] allvalues.extend(keys) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index aaf01be3532f..9b57c5ff8a99 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -377,7 +377,7 @@ def _add_users_who_share_room_txn(txn): else: tbl = "users_who_share_public_rooms" - self._simple_upsert_many_native_txn( + self._simple_upsert_many_txn( txn, table=tbl, keys=["user_id", "other_user_id", "room_id"], From e8dd750c5c5e2ec40bd77ab49ea97bf13b7e00b2 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 31 Jan 2019 15:25:54 +0000 Subject: [PATCH 03/58] fixes --- synapse/handlers/user_directory.py | 2 +- synapse/storage/_base.py | 2 +- synapse/storage/user_directory.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 9060272f691b..d899d1053352 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -464,7 +464,7 @@ def _handle_new_user(self, room_id, user_id, profile): if user_id == other_user_id: continue - to_update.add((user_id, other_user_id)) + to_insert.add((user_id, other_user_id)) # Next we need to update for every local user in the room for other_user_id in users_with_profile: diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index a6e67f8bb4f3..737cc5bd3eb9 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -770,7 +770,7 @@ def _simple_upsert_many_txn_emulated(self, txn, table, keys, keyvalues, values, keys = {x:y for x, y in zip(keys, keyv)} vals = {x:y for x, y in zip(values, valv)} - _simple_upsert_txn_emulated(txn, table, keys, vals) + self._simple_upsert_txn_emulated(txn, table, keys, vals) def _simple_upsert_many_txn_native_upsert(self, txn, table, keys, keyvalues, values, valuesvalues): diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 9b57c5ff8a99..635c62cde9f3 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -549,7 +549,7 @@ def get_users_in_share_dir_with_room_id(self, user_id, room_id): "get_users_in_share_dir_with_room_id", None, sql, room_id, user_id, user_id ) - return public + private + defer.returnValue(public + private) @defer.inlineCallbacks def get_rooms_in_common_for_users(self, user_id, other_user_id): From cf079f3e903d876041b228cf782e1eecacf58fbc Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 31 Jan 2019 15:28:37 +0000 Subject: [PATCH 04/58] fixes --- synapse/storage/_base.py | 6 ++---- synapse/storage/user_directory.py | 3 ++- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index 737cc5bd3eb9..269512c877a9 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -16,9 +16,7 @@ import sys import threading import time -import itertools -from itertools import zip_longest from six import PY2, iteritems, iterkeys, itervalues from six.moves import builtins, intern, range @@ -800,10 +798,10 @@ def _simple_upsert_many_txn_native_upsert(self, txn, table, keys, keyvalues, val if isinstance(self.database_engine, PostgresEngine): from psycopg2.extras import execute_batch - execute_batch(txn, sql, zip_longest(keyvalues, valuesvalues)) + execute_batch(txn, sql, zip(keyvalues, valuesvalues)) else: - x = zip_longest(keyvalues, valuesvalues) + x = zip(keyvalues, valuesvalues) for val in x: end = [] diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 635c62cde9f3..ec747948f963 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -335,7 +335,8 @@ def get_users_in_dir_due_to_room(self, room_id): user_ids = set(user_ids_dir) user_ids.update(user_ids_pub) - user_ids.update(user_ids_share) + user_ids.update(user_ids_share_pub) + user_ids.update(user_ids_share_priv) defer.returnValue(user_ids) From e818649e77763d418b3fd7e947d4924869617cf8 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 31 Jan 2019 15:37:05 +0000 Subject: [PATCH 05/58] fixes --- synapse/storage/_base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index 269512c877a9..519d073853cc 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -738,7 +738,7 @@ def _simple_upsert_txn_native_upsert( ) txn.execute(sql, list(allvalues.values())) - def _simple_upsert_many_txn(self, txn, table, keys, keyvalues, values, valuevalues): + def _simple_upsert_many_txn(self, txn, table, keys, keyvalues, values, valuesvalues): if ( self.database_engine.can_native_upsert and table not in self._unsafe_to_upsert_tables From 35b33d1e0c702929bef5abc6050afeadfeefa038 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 31 Jan 2019 15:57:51 +0000 Subject: [PATCH 06/58] fixes --- synapse/storage/_base.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index 519d073853cc..997f54e97676 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -746,13 +746,13 @@ def _simple_upsert_many_txn(self, txn, table, keys, keyvalues, values, valuesval return self._simple_upsert_many_txn_native_upsert( txn, table, - keys, keyvalues, values, valuevalues + keys, keyvalues, values, valuesvalues ) else: return self._simple_upsert_many_txn_emulated( txn, table, - keys, keyvalues, values, valuevalues + keys, keyvalues, values, valuesvalues ) def _simple_upsert_many_txn_emulated(self, txn, table, keys, keyvalues, values, valuesvalues): From c123c71db6d8006df6b84d0a594870dfa524f50f Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 31 Jan 2019 16:23:06 +0000 Subject: [PATCH 07/58] fixes --- synapse/storage/_base.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index 997f54e97676..eb02af8d525e 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -670,6 +670,9 @@ def _simple_upsert_txn_emulated( if lock: self.database_engine.lock_table(txn, table) + if not values: + values = keyvalues + def _getwhere(key): # If the value we're passing in is None (aka NULL), we need to use # IS, not =, as NULL = NULL equals NULL (False). From 1b271f2c4f6ffb50c2f150a8080ad00ecb62d480 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 31 Jan 2019 16:46:51 +0000 Subject: [PATCH 08/58] fixes --- synapse/handlers/user_directory.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index d899d1053352..c5bd54514213 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -559,13 +559,13 @@ def _handle_remove_user(self, room_id, user_id): found_public_share = j_room_id else: found_public_share = None - yield self.store.update_users_who_share_room( + yield self.store.insert_users_who_share_room( room_id, not is_public, [(user_id, other_user_id)], ) break if found_public_share: - yield self.store.update_users_who_share_room( + yield self.store.insert_users_who_share_room( room_id, not is_public, [(user_id, other_user_id)], ) From 994243e4d220d0c0df4f7f605a08f440f9182934 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 31 Jan 2019 17:11:40 +0000 Subject: [PATCH 09/58] fixes --- synapse/handlers/user_directory.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index c5bd54514213..1add06632005 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -559,13 +559,13 @@ def _handle_remove_user(self, room_id, user_id): found_public_share = j_room_id else: found_public_share = None - yield self.store.insert_users_who_share_room( + yield self.store.add_users_who_share_room( room_id, not is_public, [(user_id, other_user_id)], ) break if found_public_share: - yield self.store.insert_users_who_share_room( + yield self.store.add_users_who_share_room( room_id, not is_public, [(user_id, other_user_id)], ) From 487bdc0a2d98f09d3b8e7150c03c34ee24d952b9 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Fri, 1 Feb 2019 14:34:51 +0000 Subject: [PATCH 10/58] pep8 fixes --- synapse/storage/_base.py | 16 ++++++++-------- synapse/storage/user_directory.py | 3 --- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index eb02af8d525e..c35b07094666 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -758,23 +758,23 @@ def _simple_upsert_many_txn(self, txn, table, keys, keyvalues, values, valuesval keys, keyvalues, values, valuesvalues ) - def _simple_upsert_many_txn_emulated(self, txn, table, keys, keyvalues, values, valuesvalues): + def _simple_upsert_many_txn_emulated( + self, txn, table, keys, keyvalues, values, valuesvalues + ): if not valuesvalues: valuesvalues = [() * len(keyvalues)] - #def _simple_upsert_txn_emulated( - #self, txn, table, keyvalues, values, insertion_values={}, lock=True - for keyv, valv in zip(keyvalues, valuesvalues): - keys = {x:y for x, y in zip(keys, keyv)} - vals = {x:y for x, y in zip(values, valv)} + keys = {x: y for x, y in zip(keys, keyv)} + vals = {x: y for x, y in zip(values, valv)} self._simple_upsert_txn_emulated(txn, table, keys, vals) - - def _simple_upsert_many_txn_native_upsert(self, txn, table, keys, keyvalues, values, valuesvalues): + def _simple_upsert_many_txn_native_upsert( + self, txn, table, keys, keyvalues, values, valuesvalues + ): allvalues = [] allvalues.extend(keys) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index ec747948f963..bd3583b87d87 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -483,9 +483,6 @@ def get_if_users_share_a_room(self, user_id, other_user_id): defer.returnValue(None) - - - @cachedInlineCallbacks(max_entries=500000, iterable=True) def get_users_who_share_room_from_dir(self, user_id): """Returns the set of users who share a room with `user_id` From e7e94d74802d87ac6b2b1b983b048a37a152cab1 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Fri, 1 Feb 2019 16:03:34 +0000 Subject: [PATCH 11/58] cleanup --- synapse/storage/_base.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index c35b07094666..c53f6a8160fa 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -85,6 +85,14 @@ def __setattr__(self, name, value): def __iter__(self): return self.txn.__iter__() + def execute_batch(self, sql, args): + if isinstance(self.database_engine, PostgresEngine): + from psycopg2.extras import execute_batch + self._do_execute(execute_batch, self.txn, sql, args) + else: + for val in args: + self.execute(sql, val) + def execute(self, sql, *args): self._do_execute(self.txn.execute, sql, *args) @@ -799,19 +807,11 @@ def _simple_upsert_many_txn_native_upsert( latter ) - if isinstance(self.database_engine, PostgresEngine): - from psycopg2.extras import execute_batch - execute_batch(txn, sql, zip(keyvalues, valuesvalues)) + args = [] + for i, x in enumerate(keyvalues): + args.append(x + valuesvalues[i]) - else: - x = zip(keyvalues, valuesvalues) - - for val in x: - end = [] - for i in val: - if i: - end.extend(i) - txn.execute(sql, end) + return self.execute_batch(sql, args) def _simple_select_one(self, table, keyvalues, retcols, allow_none=False, desc="_simple_select_one"): From be4f84b68eb87690fb04f3bdeea84e1afe6337bb Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Fri, 1 Feb 2019 16:06:38 +0000 Subject: [PATCH 12/58] remove unused code --- synapse/storage/user_directory.py | 51 ------------------------------- 1 file changed, 51 deletions(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index bd3583b87d87..1c2f0d67d866 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -394,10 +394,6 @@ def _add_users_who_share_room_txn(txn): self.get_users_who_share_room_from_dir.invalidate, (user_id,), ) - txn.call_after( - self.get_if_users_share_a_room.invalidate, - (user_id, other_user_id), - ) return self.runInteraction( "add_users_who_share_room", _add_users_who_share_room_txn ) @@ -432,57 +428,11 @@ def _remove_user_who_share_room_txn(txn): self.get_users_who_share_room_from_dir.invalidate, (user_id,), ) - txn.call_after( - self.get_if_users_share_a_room.invalidate, - (user_id, other_user_id), - ) return self.runInteraction( "remove_user_who_share_room", _remove_user_who_share_room_txn ) - @cachedInlineCallbacks(max_entries=500000) - def get_if_users_share_a_room(self, user_id, other_user_id): - """Gets if users share a room. - - Args: - user_id (str): Must be a local user_id - other_user_id (str) - - Returns: - bool|None: None if they don't share a room, otherwise whether they - share a private room or not. - """ - private = yield self._simple_select_one_onecol( - table="users_who_share_private_rooms", - keyvalues={ - "user_id": user_id, - "other_user_id": other_user_id, - }, - retcol="user_id", - allow_none=True, - desc="get_if_users_share_a_room", - ) - - if private is not None: - defer.returnValue(True) - - public = yield self._simple_select_one_onecol( - table="users_who_share_public_rooms", - keyvalues={ - "user_id": user_id, - "other_user_id": other_user_id, - }, - retcol="user_id", - allow_none=True, - desc="get_if_users_share_a_room", - ) - - if public is not None: - defer.returnValue(False) - - defer.returnValue(None) - @cachedInlineCallbacks(max_entries=500000, iterable=True) def get_users_who_share_room_from_dir(self, user_id): """Returns the set of users who share a room with `user_id` @@ -587,7 +537,6 @@ def _delete_all_from_user_dir_txn(txn): txn.call_after(self.get_user_in_directory.invalidate_all) txn.call_after(self.get_user_in_public_room.invalidate_all) txn.call_after(self.get_users_who_share_room_from_dir.invalidate_all) - txn.call_after(self.get_if_users_share_a_room.invalidate_all) return self.runInteraction( "delete_all_from_user_dir", _delete_all_from_user_dir_txn ) From 84a0240fc0564e192bf48f75c2a133af8ff58315 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Fri, 1 Feb 2019 16:21:31 +0000 Subject: [PATCH 13/58] fix --- synapse/storage/_base.py | 4 ++-- synapse/storage/user_directory.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index c53f6a8160fa..29fba6843bae 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -808,8 +808,8 @@ def _simple_upsert_many_txn_native_upsert( ) args = [] - for i, x in enumerate(keyvalues): - args.append(x + valuesvalues[i]) + for i, x in enumerate(zip(keyvalues, valuesvalues)): + args.append(x[0] + x[1]) return self.execute_batch(sql, args) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 1c2f0d67d866..83cf13b4f4ea 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -387,7 +387,7 @@ def _add_users_who_share_room_txn(txn): for user_id, other_user_id in user_id_tuples ], values=(), - valuesvalues=(), + valuesvalues=None, ) for user_id, other_user_id in user_id_tuples: txn.call_after( From 060c5fb18a2459b825b84258327f1565a2496b99 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Fri, 1 Feb 2019 16:35:25 +0000 Subject: [PATCH 14/58] fix --- synapse/storage/_base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index 29fba6843bae..fb1f2c077d1c 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -811,7 +811,7 @@ def _simple_upsert_many_txn_native_upsert( for i, x in enumerate(zip(keyvalues, valuesvalues)): args.append(x[0] + x[1]) - return self.execute_batch(sql, args) + return txn.execute_batch(sql, args) def _simple_select_one(self, table, keyvalues, retcols, allow_none=False, desc="_simple_select_one"): From 766b86dbd1de73ba09ae5a2e8ec22e743ea36980 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Fri, 1 Feb 2019 16:47:33 +0000 Subject: [PATCH 15/58] fix --- synapse/storage/_base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index fb1f2c077d1c..9b4065adf9f2 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -88,7 +88,7 @@ def __iter__(self): def execute_batch(self, sql, args): if isinstance(self.database_engine, PostgresEngine): from psycopg2.extras import execute_batch - self._do_execute(execute_batch, self.txn, sql, args) + self._do_execute(lambda *x: execute_batch(self.txn, *x), sql, args) else: for val in args: self.execute(sql, val) From 1b3bc5befc311800464b377f0f543869e9df4bc2 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Mon, 4 Feb 2019 14:50:43 +0000 Subject: [PATCH 16/58] fix --- synapse/storage/_base.py | 10 +-- synapse/storage/user_directory.py | 7 +- tests/handlers/test_user_directory.py | 113 +++++++++++++++++--------- 3 files changed, 83 insertions(+), 47 deletions(-) diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index 9b4065adf9f2..e95c63573db1 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -771,10 +771,9 @@ def _simple_upsert_many_txn_emulated( ): if not valuesvalues: - valuesvalues = [() * len(keyvalues)] + valuesvalues = [() for x in range(len(keyvalues))] for keyv, valv in zip(keyvalues, valuesvalues): - keys = {x: y for x, y in zip(keys, keyv)} vals = {x: y for x, y in zip(values, valv)} @@ -789,7 +788,7 @@ def _simple_upsert_many_txn_native_upsert( allvalues.extend(values) if not valuesvalues: - valuesvalues = [() * len(keyvalues)] + valuesvalues = [[] for x in range(len(keyvalues))] if not values: latter = "NOTHING" @@ -808,8 +807,9 @@ def _simple_upsert_many_txn_native_upsert( ) args = [] - for i, x in enumerate(zip(keyvalues, valuesvalues)): - args.append(x[0] + x[1]) + + for x, y in zip(keyvalues, valuesvalues): + args.append(tuple(x) + tuple(y)) return txn.execute_batch(sql, args) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 83cf13b4f4ea..851ce86e656a 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -86,6 +86,7 @@ def add_profiles_to_user_dir(self, room_id, users_with_profile): users_with_profile (dict): Users to add to directory in the form of mapping of user_id -> ProfileInfo """ + if isinstance(self.database_engine, PostgresEngine): # We weight the loclpart most highly, then display name and finally # server name @@ -109,7 +110,7 @@ def add_profiles_to_user_dir(self, room_id, users_with_profile): INSERT INTO user_directory_search(user_id, value) VALUES (?,?) """ - args = ( + args = tuple( ( user_id, "%s %s" % (user_id, p.display_name,) if p.display_name else user_id @@ -125,7 +126,7 @@ def _add_profiles_to_user_dir_txn(txn): self._simple_insert_many_txn( txn, table="user_directory", - values=[ + values=list([ { "user_id": user_id, "room_id": room_id, @@ -133,7 +134,7 @@ def _add_profiles_to_user_dir_txn(txn): "avatar_url": profile.avatar_url, } for user_id, profile in iteritems(users_with_profile) - ] + ]) ) for user_id in users_with_profile: txn.call_after( diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 11f2bae69808..18b8aafbf4ee 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -14,78 +14,113 @@ # limitations under the License. from mock import Mock -from twisted.internet import defer - from synapse.api.constants import UserTypes -from synapse.handlers.user_directory import UserDirectoryHandler from synapse.storage.roommember import ProfileInfo +from synapse.rest.client.v1 import login, admin, room + from tests import unittest -from tests.utils import setup_test_homeserver -class UserDirectoryHandlers(object): - def __init__(self, hs): - self.user_directory_handler = UserDirectoryHandler(hs) +class UserDirectoryTestCase(unittest.HomeserverTestCase): + """ + Tests the UserDirectoryHandler. + """ + servlets = [ + login.register_servlets, + admin.register_servlets, + room.register_servlets, + ] -class UserDirectoryTestCase(unittest.TestCase): - """ Tests the UserDirectoryHandler. """ + def make_homeserver(self, reactor, clock): - @defer.inlineCallbacks - def setUp(self): - hs = yield setup_test_homeserver(self.addCleanup) - self.store = hs.get_datastore() - hs.handlers = UserDirectoryHandlers(hs) + config = self.default_config() + config.update_user_directory = True + return self.setup_test_homeserver(config=config) - self.handler = hs.get_handlers().user_directory_handler + def prepare(self, reactor, clock, hs): + hs.config.update_user_directory = True + self.store = hs.get_datastore() + self.handler = hs.get_user_directory_handler() - @defer.inlineCallbacks def test_handle_local_profile_change_with_support_user(self): support_user_id = "@support:test" - yield self.store.register( - user_id=support_user_id, - token="123", - password_hash=None, - user_type=UserTypes.SUPPORT + self.get_success( + self.store.register( + user_id=support_user_id, + token="123", + password_hash=None, + user_type=UserTypes.SUPPORT, + ) ) - yield self.handler.handle_local_profile_change(support_user_id, None) - profile = yield self.store.get_user_in_directory(support_user_id) + self.get_success( + self.handler.handle_local_profile_change(support_user_id, None) + ) + profile = self.get_success(self.store.get_user_in_directory(support_user_id)) self.assertTrue(profile is None) display_name = 'display_name' - profile_info = ProfileInfo( - avatar_url='avatar_url', - display_name=display_name, - ) + profile_info = ProfileInfo(avatar_url='avatar_url', display_name=display_name) regular_user_id = '@regular:test' - yield self.handler.handle_local_profile_change(regular_user_id, profile_info) - profile = yield self.store.get_user_in_directory(regular_user_id) + self.get_success( + self.handler.handle_local_profile_change(regular_user_id, profile_info) + ) + profile = self.get_success(self.store.get_user_in_directory(regular_user_id)) self.assertTrue(profile['display_name'] == display_name) - @defer.inlineCallbacks def test_handle_user_deactivated_support_user(self): s_user_id = "@support:test" - self.store.register( - user_id=s_user_id, - token="123", - password_hash=None, - user_type=UserTypes.SUPPORT + self.get_success( + self.store.register( + user_id=s_user_id, + token="123", + password_hash=None, + user_type=UserTypes.SUPPORT, + ) ) self.store.remove_from_user_dir = Mock() self.store.remove_from_user_in_public_room = Mock() - yield self.handler.handle_user_deactivated(s_user_id) + self.get_success(self.handler.handle_user_deactivated(s_user_id)) self.store.remove_from_user_dir.not_called() self.store.remove_from_user_in_public_room.not_called() - @defer.inlineCallbacks def test_handle_user_deactivated_regular_user(self): r_user_id = "@regular:test" - self.store.register(user_id=r_user_id, token="123", password_hash=None) + self.get_success( + self.store.register(user_id=r_user_id, token="123", password_hash=None) + ) self.store.remove_from_user_dir = Mock() self.store.remove_from_user_in_public_room = Mock() - yield self.handler.handle_user_deactivated(r_user_id) + self.get_success(self.handler.handle_user_deactivated(r_user_id)) self.store.remove_from_user_dir.called_once_with(r_user_id) self.store.remove_from_user_in_public_room.assert_called_once_with(r_user_id) + + def test_private_room(self): + """ + A user can be searched for only by people that are either in a public + room, or that share a private chat. + """ + u1 = self.register_user("user1", "pass") + u1_token = self.login(u1, "pass") + u2 = self.register_user("user2", "pass") + u2_token = self.login(u2, "pass") + u3 = self.register_user("user3", "pass") + + # NOTE: Implementation detail. We do not add users to the directory + # until they join a room. + s = self.get_success(self.handler.search_users(u1, "user2", 10)) + + room = self.helper.create_room_as(u1, is_public=False, tok=u1_token) + self.helper.invite(room, src=u1, targ=u2, tok=u1_token) + self.helper.join(room, user=u2, tok=u2_token) + + # We get one search result when searching for user2 by user1. + s = self.get_success(self.handler.search_users(u1, "user2", 10)) + self.assertEqual(len(s["results"]), 1) + + # We get NO search results when searching for user2 by user3. + s = self.get_success(self.handler.search_users(u3, "user2", 10)) + self.assertEqual(len(s["results"]), 0) From cf03ec74590bd9aa718b3090b28e3aaa45d763a9 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Mon, 4 Feb 2019 14:51:39 +0000 Subject: [PATCH 17/58] changelog --- changelog.d/4537.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/4537.feature diff --git a/changelog.d/4537.feature b/changelog.d/4537.feature new file mode 100644 index 000000000000..8f792b889071 --- /dev/null +++ b/changelog.d/4537.feature @@ -0,0 +1 @@ +The user directory has been rewritten to make it faster, with less chance of falling behind on a large server. From bd667999fd87cfa8322c9dcab158ec631cd6c108 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Mon, 4 Feb 2019 15:13:45 +0000 Subject: [PATCH 18/58] fix, maybe --- synapse/storage/_base.py | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index e95c63573db1..c726c9db308a 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -678,9 +678,6 @@ def _simple_upsert_txn_emulated( if lock: self.database_engine.lock_table(txn, table) - if not values: - values = keyvalues - def _getwhere(key): # If the value we're passing in is None (aka NULL), we need to use # IS, not =, as NULL = NULL equals NULL (False). @@ -689,17 +686,25 @@ def _getwhere(key): else: return "%s = ?" % (key,) - # First try to update. - sql = "UPDATE %s SET %s WHERE %s" % ( - table, - ", ".join("%s = ?" % (k,) for k in values), - " AND ".join(_getwhere(k) for k in keyvalues) - ) + if not values: + # Try and select. + sql = "SELECT 1 FROM %s WHERE %s" % ( + table, + " AND ".join(_getwhere(k) for k in keyvalues) + ) + sqlargs = list(keyvalues.values()) + else: + # First try to update. + sql = "UPDATE %s SET %s WHERE %s" % ( + table, + ", ".join("%s = ?" % (k,) for k in values), + " AND ".join(_getwhere(k) for k in keyvalues) + ) sqlargs = list(values.values()) + list(keyvalues.values()) txn.execute(sql, sqlargs) if txn.rowcount > 0: - # successfully updated at least one row. + # successfully updated/fetched at least one row. return False # We didn't update any rows so insert a new one From 2b3f166f239c3526c2e821cb8a986e2b8043c1c1 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Mon, 4 Feb 2019 16:07:44 +0000 Subject: [PATCH 19/58] fix, maybe --- synapse/storage/_base.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index c726c9db308a..7341075d95bb 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -693,19 +693,23 @@ def _getwhere(key): " AND ".join(_getwhere(k) for k in keyvalues) ) sqlargs = list(keyvalues.values()) + txn.execute(sql, sqlargs) + if txn.fetchall(): + # We have an existing record. + return False else: # First try to update. sql = "UPDATE %s SET %s WHERE %s" % ( table, ", ".join("%s = ?" % (k,) for k in values), " AND ".join(_getwhere(k) for k in keyvalues) - ) - sqlargs = list(values.values()) + list(keyvalues.values()) + ) + sqlargs = list(values.values()) + list(keyvalues.values()) - txn.execute(sql, sqlargs) - if txn.rowcount > 0: - # successfully updated/fetched at least one row. - return False + txn.execute(sql, sqlargs) + if txn.rowcount > 0: + # successfully updated at least one row. + return False # We didn't update any rows so insert a new one allvalues = {} From c6a68b4abe1f051f773a938231c15ce3bc11f988 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Tue, 5 Feb 2019 10:02:05 +0000 Subject: [PATCH 20/58] pep8 --- synapse/storage/_base.py | 4 ++-- tests/handlers/test_user_directory.py | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index 7341075d95bb..5060693c80ea 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -640,7 +640,7 @@ def _simple_upsert_txn( """ if ( self.database_engine.can_native_upsert - and table not in self._unsafe_to_upsert_tables + and table not in self._unsafe_to_upsert_tables and False ): return self._simple_upsert_txn_native_upsert( txn, @@ -703,7 +703,7 @@ def _getwhere(key): table, ", ".join("%s = ?" % (k,) for k in values), " AND ".join(_getwhere(k) for k in keyvalues) - ) + ) sqlargs = list(values.values()) + list(keyvalues.values()) txn.execute(sql, sqlargs) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 18b8aafbf4ee..2c0de0e048ea 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -15,10 +15,9 @@ from mock import Mock from synapse.api.constants import UserTypes +from synapse.rest.client.v1 import admin, login, room from synapse.storage.roommember import ProfileInfo -from synapse.rest.client.v1 import login, admin, room - from tests import unittest From f21dcc7eede5874b03abaaf9e2cc2e4dcf25d884 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Tue, 5 Feb 2019 10:55:46 +0000 Subject: [PATCH 21/58] fix flakiness --- synapse/storage/_base.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index 5060693c80ea..3483bc8c3292 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -759,6 +759,18 @@ def _simple_upsert_txn_native_upsert( txn.execute(sql, list(allvalues.values())) def _simple_upsert_many_txn(self, txn, table, keys, keyvalues, values, valuesvalues): + """ + Upsert, many times. + + Args: + table (str): The table to upsert into + keys (list[str]): The key column names. + keyvalues (list[list]): A list of each row's key column values. + values (list[str]): The value column names. + valuesvalues (list[list]): A list of each row's value column values. + Returns: + None + """ if ( self.database_engine.can_native_upsert and table not in self._unsafe_to_upsert_tables @@ -783,10 +795,10 @@ def _simple_upsert_many_txn_emulated( valuesvalues = [() for x in range(len(keyvalues))] for keyv, valv in zip(keyvalues, valuesvalues): - keys = {x: y for x, y in zip(keys, keyv)} - vals = {x: y for x, y in zip(values, valv)} + _keys = {x: y for x, y in zip(keys, keyv)} + _vals = {x: y for x, y in zip(values, valv)} - self._simple_upsert_txn_emulated(txn, table, keys, vals) + self._simple_upsert_txn_emulated(txn, table, _keys, _vals) def _simple_upsert_many_txn_native_upsert( self, txn, table, keys, keyvalues, values, valuesvalues From b11de34999648fe33da3f9b05e641ba593538304 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Tue, 5 Feb 2019 12:00:11 +0000 Subject: [PATCH 22/58] we dont need this here --- synapse/storage/_base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index 3483bc8c3292..c1df9d5cfd42 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -640,7 +640,7 @@ def _simple_upsert_txn( """ if ( self.database_engine.can_native_upsert - and table not in self._unsafe_to_upsert_tables and False + and table not in self._unsafe_to_upsert_tables ): return self._simple_upsert_txn_native_upsert( txn, From 2cd5abcc32641c1540ea6725f7b3c0871a147c7d Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Wed, 13 Feb 2019 23:06:19 +1100 Subject: [PATCH 23/58] black --- synapse/handlers/user_directory.py | 86 +++++++++-------- synapse/storage/user_directory.py | 148 +++++++++++++---------------- 2 files changed, 114 insertions(+), 120 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 1add06632005..27866d716cee 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -130,7 +130,7 @@ def handle_local_profile_change(self, user_id, profile): # Support users are for diagnostics and should not appear in the user directory. if not is_support: yield self.store.update_profile_in_user_dir( - user_id, profile.display_name, profile.avatar_url, None, + user_id, profile.display_name, profile.avatar_url, None ) @defer.inlineCallbacks @@ -166,8 +166,9 @@ def _unsafe_process(self): self.pos = deltas[-1]["stream_id"] # Expose current event processing position to prometheus - synapse.metrics.event_processing_positions.labels( - "user_dir").set(self.pos) + synapse.metrics.event_processing_positions.labels("user_dir").set( + self.pos + ) yield self.store.update_user_directory_stream_pos(self.pos) @@ -191,21 +192,25 @@ def _do_initial_spam(self): logger.info("Handling room %d/%d", num_processed_rooms + 1, len(room_ids)) yield self._handle_initial_room(room_id) num_processed_rooms += 1 - yield self.clock.sleep(self.INITIAL_ROOM_SLEEP_MS / 1000.) + yield self.clock.sleep(self.INITIAL_ROOM_SLEEP_MS / 1000.0) logger.info("Processed all rooms.") if self.search_all_users: num_processed_users = 0 user_ids = yield self.store.get_all_local_users() - logger.info("Doing initial update of user directory. %d users", len(user_ids)) + logger.info( + "Doing initial update of user directory. %d users", len(user_ids) + ) for user_id in user_ids: # We add profiles for all users even if they don't match the # include pattern, just in case we want to change it in future - logger.info("Handling user %d/%d", num_processed_users + 1, len(user_ids)) + logger.info( + "Handling user %d/%d", num_processed_users + 1, len(user_ids) + ) yield self._handle_local_user(user_id) num_processed_users += 1 - yield self.clock.sleep(self.INITIAL_USER_SLEEP_MS / 1000.) + yield self.clock.sleep(self.INITIAL_USER_SLEEP_MS / 1000.0) logger.info("Processed all users") @@ -224,24 +229,24 @@ def _handle_initial_room(self, room_id): if not is_in_room: return - is_public = yield self.store.is_room_world_readable_or_publicly_joinable(room_id) + is_public = yield self.store.is_room_world_readable_or_publicly_joinable( + room_id + ) users_with_profile = yield self.state.get_current_user_in_room(room_id) user_ids = set(users_with_profile) unhandled_users = user_ids - self.initially_handled_users yield self.store.add_profiles_to_user_dir( - room_id, { - user_id: users_with_profile[user_id] for user_id in unhandled_users - } + room_id, + {user_id: users_with_profile[user_id] for user_id in unhandled_users}, ) self.initially_handled_users |= unhandled_users if is_public: yield self.store.add_users_to_public_room( - room_id, - user_ids=user_ids - self.initially_handled_users_in_public + room_id, user_ids=user_ids - self.initially_handled_users_in_public ) self.initially_handled_users_in_public |= user_ids @@ -252,7 +257,7 @@ def _handle_initial_room(self, room_id): count = 0 for user_id in user_ids: if count % self.INITIAL_ROOM_SLEEP_COUNT == 0: - yield self.clock.sleep(self.INITIAL_ROOM_SLEEP_MS / 1000.) + yield self.clock.sleep(self.INITIAL_ROOM_SLEEP_MS / 1000.0) if not self.is_mine_id(user_id): count += 1 @@ -267,7 +272,7 @@ def _handle_initial_room(self, room_id): continue if count % self.INITIAL_ROOM_SLEEP_COUNT == 0: - yield self.clock.sleep(self.INITIAL_ROOM_SLEEP_MS / 1000.) + yield self.clock.sleep(self.INITIAL_ROOM_SLEEP_MS / 1000.0) count += 1 user_set = (user_id, other_user_id) @@ -275,14 +280,12 @@ def _handle_initial_room(self, room_id): if len(to_insert) > self.INITIAL_ROOM_BATCH_SIZE: yield self.store.add_users_who_share_room( - room_id, not is_public, to_insert, + room_id, not is_public, to_insert ) to_insert.clear() if to_insert: - yield self.store.add_users_who_share_room( - room_id, not is_public, to_insert, - ) + yield self.store.add_users_who_share_room(room_id, not is_public, to_insert) to_insert.clear() @defer.inlineCallbacks @@ -302,11 +305,12 @@ def _handle_deltas(self, deltas): # may have become public or not and add/remove the users in said room if typ in (EventTypes.RoomHistoryVisibility, EventTypes.JoinRules): yield self._handle_room_publicity_change( - room_id, prev_event_id, event_id, typ, + room_id, prev_event_id, event_id, typ ) elif typ == EventTypes.Member: change = yield self._get_key_change( - prev_event_id, event_id, + prev_event_id, + event_id, key_name="membership", public_value=Membership.JOIN, ) @@ -315,14 +319,16 @@ def _handle_deltas(self, deltas): # Need to check if the server left the room entirely, if so # we might need to remove all the users in that room is_in_room = yield self.store.is_host_joined( - room_id, self.server_name, + room_id, self.server_name ) if not is_in_room: logger.info("Server left room: %r", room_id) # Fetch all the users that we marked as being in user # directory due to being in the room and then check if # need to remove those users or not - user_ids = yield self.store.get_users_in_dir_due_to_room(room_id) + user_ids = yield self.store.get_users_in_dir_due_to_room( + room_id + ) for user_id in user_ids: yield self._handle_remove_user(room_id, user_id) return @@ -334,7 +340,7 @@ def _handle_deltas(self, deltas): if change is None: # Handle any profile changes yield self._handle_profile_change( - state_key, room_id, prev_event_id, event_id, + state_key, room_id, prev_event_id, event_id ) continue @@ -366,13 +372,15 @@ def _handle_room_publicity_change(self, room_id, prev_event_id, event_id, typ): if typ == EventTypes.RoomHistoryVisibility: change = yield self._get_key_change( - prev_event_id, event_id, + prev_event_id, + event_id, key_name="history_visibility", public_value="world_readable", ) elif typ == EventTypes.JoinRules: change = yield self._get_key_change( - prev_event_id, event_id, + prev_event_id, + event_id, key_name="join_rule", public_value=JoinRules.PUBLIC, ) @@ -478,9 +486,7 @@ def _handle_new_user(self, room_id, user_id, profile): to_insert.add((other_user_id, user_id)) if to_insert: - yield self.store.add_users_who_share_room( - room_id, not is_public, to_insert, - ) + yield self.store.add_users_who_share_room(room_id, not is_public, to_insert) @defer.inlineCallbacks def _handle_remove_user(self, room_id, user_id): @@ -498,15 +504,15 @@ def _handle_remove_user(self, room_id, user_id): row = yield self.store.get_user_in_public_room(user_id) update_user_in_public = row and row["room_id"] == room_id - if (update_user_in_public or update_user_dir): + if update_user_in_public or update_user_dir: # XXX: Make this faster? rooms = yield self.store.get_rooms_for_user(user_id) for j_room_id in rooms: - if (not update_user_in_public and not update_user_dir): + if not update_user_in_public and not update_user_dir: break is_in_room = yield self.store.is_host_joined( - j_room_id, self.server_name, + j_room_id, self.server_name ) if not is_in_room: @@ -534,19 +540,19 @@ def _handle_remove_user(self, room_id, user_id): # Get a list of user tuples that were in the DB due to this room and # users (this includes tuples where the other user matches `user_id`) user_tuples = yield self.store.get_users_in_share_dir_with_room_id( - user_id, room_id, + user_id, room_id ) for user_id, other_user_id in user_tuples: # For each user tuple get a list of rooms that they still share, # trying to find a private room, and update the entry in the DB - rooms = yield self.store.get_rooms_in_common_for_users(user_id, other_user_id) + rooms = yield self.store.get_rooms_in_common_for_users( + user_id, other_user_id + ) # If they dont share a room anymore, remove the mapping if not rooms: - yield self.store.remove_user_who_share_room( - user_id, other_user_id, - ) + yield self.store.remove_user_who_share_room(user_id, other_user_id) continue found_public_share = None @@ -560,13 +566,13 @@ def _handle_remove_user(self, room_id, user_id): else: found_public_share = None yield self.store.add_users_who_share_room( - room_id, not is_public, [(user_id, other_user_id)], + room_id, not is_public, [(user_id, other_user_id)] ) break if found_public_share: yield self.store.add_users_who_share_room( - room_id, not is_public, [(user_id, other_user_id)], + room_id, not is_public, [(user_id, other_user_id)] ) @defer.inlineCallbacks @@ -594,7 +600,7 @@ def _handle_profile_change(self, user_id, room_id, prev_event_id, event_id): if prev_name != new_name or prev_avatar != new_avatar: yield self.store.update_profile_in_user_dir( - user_id, new_name, new_avatar, room_id, + user_id, new_name, new_avatar, room_id ) @defer.inlineCallbacks diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 5c5691591085..10c6ff89d425 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -44,7 +44,7 @@ def is_room_world_readable_or_publicly_joinable(self, room_id): ) current_state_ids = yield self.get_filtered_current_state_ids( - room_id, StateFilter.from_types(types_to_filter), + room_id, StateFilter.from_types(types_to_filter) ) join_rules_id = current_state_ids.get((EventTypes.JoinRules, "")) @@ -74,14 +74,8 @@ def add_users_to_public_room(self, room_id, user_ids): """ yield self._simple_insert_many( table="users_in_public_rooms", - values=[ - { - "user_id": user_id, - "room_id": room_id, - } - for user_id in user_ids - ], - desc="add_users_to_public_room" + values=[{"user_id": user_id, "room_id": room_id} for user_id in user_ids], + desc="add_users_to_public_room", ) for user_id in user_ids: self.get_user_in_public_room.invalidate((user_id,)) @@ -108,7 +102,9 @@ def add_profiles_to_user_dir(self, room_id, users_with_profile): """ args = ( ( - user_id, get_localpart_from_id(user_id), get_domain_from_id(user_id), + user_id, + get_localpart_from_id(user_id), + get_domain_from_id(user_id), profile.display_name, ) for user_id, profile in iteritems(users_with_profile) @@ -121,7 +117,7 @@ def add_profiles_to_user_dir(self, room_id, users_with_profile): args = tuple( ( user_id, - "%s %s" % (user_id, p.display_name,) if p.display_name else user_id + "%s %s" % (user_id, p.display_name) if p.display_name else user_id, ) for user_id, p in iteritems(users_with_profile) ) @@ -134,20 +130,20 @@ def _add_profiles_to_user_dir_txn(txn): self._simple_insert_many_txn( txn, table="user_directory", - values=list([ - { - "user_id": user_id, - "room_id": room_id, - "display_name": profile.display_name, - "avatar_url": profile.avatar_url, - } - for user_id, profile in iteritems(users_with_profile) - ]) + values=list( + [ + { + "user_id": user_id, + "room_id": room_id, + "display_name": profile.display_name, + "avatar_url": profile.avatar_url, + } + for user_id, profile in iteritems(users_with_profile) + ] + ), ) for user_id in users_with_profile: - txn.call_after( - self.get_user_in_directory.invalidate, (user_id,) - ) + txn.call_after(self.get_user_in_directory.invalidate, (user_id,)) return self.runInteraction( "add_profiles_to_user_dir", _add_profiles_to_user_dir_txn @@ -189,9 +185,11 @@ def _update_profile_in_user_dir_txn(txn): txn.execute( sql, ( - user_id, get_localpart_from_id(user_id), - get_domain_from_id(user_id), display_name, - ) + user_id, + get_localpart_from_id(user_id), + get_domain_from_id(user_id), + display_name, + ), ) else: # TODO: Remove this code after we've bumped the minimum version @@ -209,9 +207,11 @@ def _update_profile_in_user_dir_txn(txn): txn.execute( sql, ( - user_id, get_localpart_from_id(user_id), - get_domain_from_id(user_id), display_name, - ) + user_id, + get_localpart_from_id(user_id), + get_domain_from_id(user_id), + display_name, + ), ) elif new_entry is False: sql = """ @@ -226,15 +226,16 @@ def _update_profile_in_user_dir_txn(txn): ( get_localpart_from_id(user_id), get_domain_from_id(user_id), - display_name, user_id, - ) + display_name, + user_id, + ), ) else: raise RuntimeError( "upsert returned None when 'can_native_upsert' is False" ) elif isinstance(self.database_engine, Sqlite3Engine): - value = "%s %s" % (user_id, display_name,) if display_name else user_id + value = "%s %s" % (user_id, display_name) if display_name else user_id self._simple_upsert_txn( txn, table="user_directory_search", @@ -265,29 +266,18 @@ def update_user_in_public_user_list(self, user_id, room_id): def remove_from_user_dir(self, user_id): def _remove_from_user_dir_txn(txn): self._simple_delete_txn( - txn, - table="user_directory", - keyvalues={"user_id": user_id}, + txn, table="user_directory", keyvalues={"user_id": user_id} ) self._simple_delete_txn( - txn, - table="user_directory_search", - keyvalues={"user_id": user_id}, + txn, table="user_directory_search", keyvalues={"user_id": user_id} ) self._simple_delete_txn( - txn, - table="users_in_public_rooms", - keyvalues={"user_id": user_id}, - ) - txn.call_after( - self.get_user_in_directory.invalidate, (user_id,) - ) - txn.call_after( - self.get_user_in_public_room.invalidate, (user_id,) + txn, table="users_in_public_rooms", keyvalues={"user_id": user_id} ) - return self.runInteraction( - "remove_from_user_dir", _remove_from_user_dir_txn, - ) + txn.call_after(self.get_user_in_directory.invalidate, (user_id,)) + txn.call_after(self.get_user_in_public_room.invalidate, (user_id,)) + + return self.runInteraction("remove_from_user_dir", _remove_from_user_dir_txn) @defer.inlineCallbacks def remove_from_user_in_public_room(self, user_id): @@ -380,6 +370,7 @@ def add_users_who_share_room(self, room_id, share_private, user_id_tuples): share_private (bool): Is the room private user_id_tuples([(str, str)]): iterable of 2-tuple of user IDs. """ + def _add_users_who_share_room_txn(txn): if share_private: @@ -400,9 +391,9 @@ def _add_users_who_share_room_txn(txn): ) for user_id, other_user_id in user_id_tuples: txn.call_after( - self.get_users_who_share_room_from_dir.invalidate, - (user_id,), + self.get_users_who_share_room_from_dir.invalidate, (user_id,) ) + return self.runInteraction( "add_users_who_share_room", _add_users_who_share_room_txn ) @@ -416,26 +407,20 @@ def remove_user_who_share_room(self, user_id, other_user_id): share_private (bool): Is the room private user_id_tuples([(str, str)]): iterable of 2-tuple of user IDs. """ + def _remove_user_who_share_room_txn(txn): self._simple_delete_txn( txn, table="users_who_share_private_rooms", - keyvalues={ - "user_id": user_id, - "other_user_id": other_user_id, - }, + keyvalues={"user_id": user_id, "other_user_id": other_user_id}, ) self._simple_delete_txn( txn, table="users_who_share_public_rooms", - keyvalues={ - "user_id": user_id, - "other_user_id": other_user_id, - }, + keyvalues={"user_id": user_id, "other_user_id": other_user_id}, ) txn.call_after( - self.get_users_who_share_room_from_dir.invalidate, - (user_id,), + self.get_users_who_share_room_from_dir.invalidate, (user_id,) ) return self.runInteraction( @@ -454,18 +439,14 @@ def get_users_who_share_room_from_dir(self, user_id): """ rows = yield self._simple_select_list( table="users_who_share_private_rooms", - keyvalues={ - "user_id": user_id, - }, + keyvalues={"user_id": user_id}, retcols=("other_user_id",), desc="get_users_who_share_room_with_user", ) pub_rows = yield self._simple_select_list( table="users_who_share_public_rooms", - keyvalues={ - "user_id": user_id, - }, + keyvalues={"user_id": user_id}, retcols=("other_user_id",), desc="get_users_who_share_room_with_user", ) @@ -537,6 +518,7 @@ def get_rooms_in_common_for_users(self, user_id, other_user_id): def delete_all_from_user_dir(self): """Delete the entire user directory """ + def _delete_all_from_user_dir_txn(txn): txn.execute("DELETE FROM user_directory") txn.execute("DELETE FROM user_directory_search") @@ -546,6 +528,7 @@ def _delete_all_from_user_dir_txn(txn): txn.call_after(self.get_user_in_directory.invalidate_all) txn.call_after(self.get_user_in_public_room.invalidate_all) txn.call_after(self.get_users_who_share_room_from_dir.invalidate_all) + return self.runInteraction( "delete_all_from_user_dir", _delete_all_from_user_dir_txn ) @@ -555,7 +538,7 @@ def get_user_in_directory(self, user_id): return self._simple_select_one( table="user_directory", keyvalues={"user_id": user_id}, - retcols=("room_id", "display_name", "avatar_url",), + retcols=("room_id", "display_name", "avatar_url"), allow_none=True, desc="get_user_in_directory", ) @@ -588,7 +571,9 @@ def update_user_directory_stream_pos(self, stream_id): def get_current_state_deltas(self, prev_stream_id): prev_stream_id = int(prev_stream_id) - if not self._curr_state_delta_stream_cache.has_any_entity_changed(prev_stream_id): + if not self._curr_state_delta_stream_cache.has_any_entity_changed( + prev_stream_id + ): return [] def get_current_state_deltas_txn(txn): @@ -622,7 +607,7 @@ def get_current_state_deltas_txn(txn): WHERE ? < stream_id AND stream_id <= ? ORDER BY stream_id ASC """ - txn.execute(sql, (prev_stream_id, max_stream_id,)) + txn.execute(sql, (prev_stream_id, max_stream_id)) return self.cursor_to_dict(txn) return self.runInteraction( @@ -712,8 +697,11 @@ def search_user_dir(self, user_id, search_term, limit): display_name IS NULL, avatar_url IS NULL LIMIT ? - """ % (join_clause, where_clause) - args = join_args + (full_query, exact_query, prefix_query, limit + 1,) + """ % ( + join_clause, + where_clause, + ) + args = join_args + (full_query, exact_query, prefix_query, limit + 1) elif isinstance(self.database_engine, Sqlite3Engine): search_query = _parse_query_sqlite(search_term) @@ -730,7 +718,10 @@ def search_user_dir(self, user_id, search_term, limit): display_name IS NULL, avatar_url IS NULL LIMIT ? - """ % (join_clause, where_clause) + """ % ( + join_clause, + where_clause, + ) args = join_args + (search_query, limit + 1) else: # This should be unreachable. @@ -742,10 +733,7 @@ def search_user_dir(self, user_id, search_term, limit): limited = len(results) > limit - defer.returnValue({ - "limited": limited, - "results": results, - }) + defer.returnValue({"limited": limited, "results": results}) def _parse_query_sqlite(search_term): @@ -760,7 +748,7 @@ def _parse_query_sqlite(search_term): # Pull out the individual words, discarding any non-word characters. results = re.findall(r"([\w\-]+)", search_term, re.UNICODE) - return " & ".join("(%s* OR %s)" % (result, result,) for result in results) + return " & ".join("(%s* OR %s)" % (result, result) for result in results) def _parse_query_postgres(search_term): @@ -773,7 +761,7 @@ def _parse_query_postgres(search_term): # Pull out the individual words, discarding any non-word characters. results = re.findall(r"([\w\-]+)", search_term, re.UNICODE) - both = " & ".join("(%s:* | %s)" % (result, result,) for result in results) + both = " & ".join("(%s:* | %s)" % (result, result) for result in results) exact = " & ".join("%s" % (result,) for result in results) prefix = " & ".join("%s:*" % (result,) for result in results) From ebe8bb5b0f4b6d9b4dccafa34aa73472a46b8ff2 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 02:02:42 +1100 Subject: [PATCH 24/58] some more test coverage --- synapse/handlers/user_directory.py | 2 - tests/handlers/test_user_directory.py | 83 +++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 27866d716cee..70766c17ea07 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -216,8 +216,6 @@ def _do_initial_spam(self): self.initially_handled_users = None self.initially_handled_users_in_public = None - self.initially_handled_users_share = None - self.initially_handled_users_share_private_room = None yield self.store.update_user_directory_stream_pos(new_pos) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 2c0de0e048ea..26716d4b3c86 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -123,3 +123,86 @@ def test_private_room(self): # We get NO search results when searching for user2 by user3. s = self.get_success(self.handler.search_users(u3, "user2", 10)) self.assertEqual(len(s["results"]), 0) + + def _compress_shared(self, shared): + """ + Compress a list of users who share rooms dicts to a list of tuples. + """ + r = set() + for i in shared: + r.add((i["user_id"], i["other_user_id"], i["room_id"])) + return r + + def test_initial(self): + """ + A user can be searched for only by people that are either in a public + room, or that share a private chat. + """ + u1 = self.register_user("user1", "pass") + u1_token = self.login(u1, "pass") + u2 = self.register_user("user2", "pass") + u2_token = self.login(u2, "pass") + u3 = self.register_user("user3", "pass") + u3_token = self.login(u3, "pass") + + room = self.helper.create_room_as(u1, is_public=True, tok=u1_token) + self.helper.invite(room, src=u1, targ=u2, tok=u1_token) + self.helper.join(room, user=u2, tok=u2_token) + + private_room = self.helper.create_room_as(u1, is_public=False, tok=u1_token) + self.helper.invite(private_room, src=u1, targ=u3, tok=u1_token) + self.helper.join(private_room, user=u3, tok=u3_token) + + self.get_success(self.store.update_user_directory_stream_pos(None)) + self.get_success(self.store.delete_all_from_user_dir()) + + shares_public = self.get_success( + self.store._simple_select_list( + "users_who_share_public_rooms", None, ["user_id", "other_user_id"] + ) + ) + shares_private = self.get_success( + self.store._simple_select_list( + "users_who_share_private_rooms", None, ["user_id", "other_user_id"] + ) + ) + + self.assertEqual(shares_private, []) + self.assertEqual(shares_public, []) + + # Reset the handled users caches + self.handler.initially_handled_users = set() + self.handler.initially_handled_users_in_public = set() + + d = self.handler._do_initial_spam() + + for i in range(10): + self.pump(1) + + r = self.get_success(d) + + shares_public = self.get_success( + self.store._simple_select_list( + "users_who_share_public_rooms", + None, + ["user_id", "other_user_id", "room_id"], + ) + ) + shares_private = self.get_success( + self.store._simple_select_list( + "users_who_share_private_rooms", + None, + ["user_id", "other_user_id", "room_id"], + ) + ) + + # User 1 and User 2 share public rooms + self.assertEqual( + self._compress_shared(shares_public), set([(u1, u2, room), (u2, u1, room)]) + ) + + # User 1 and User 3 share private rooms + self.assertEqual( + self._compress_shared(shares_private), + set([(u1, u3, private_room), (u3, u1, private_room)]), + ) From aa13be60c1105f1b68be2e894d8aa0b4eab03ace Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 02:30:30 +1100 Subject: [PATCH 25/58] some more test coverage --- synapse/handlers/user_directory.py | 4 +- tests/handlers/test_user_directory.py | 105 +++++++++++++++++++------- 2 files changed, 79 insertions(+), 30 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 70766c17ea07..c51f44c611db 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -51,6 +51,7 @@ class UserDirectoryHandler(object): INITIAL_USER_SLEEP_MS = 10 def __init__(self, hs): + self.hs = hs self.store = hs.get_datastore() self.state = hs.get_state_handler() self.server_name = hs.hostname @@ -58,7 +59,6 @@ def __init__(self, hs): self.notifier = hs.get_notifier() self.is_mine_id = hs.is_mine_id self.update_user_directory = hs.config.update_user_directory - self.search_all_users = hs.config.user_directory_search_all_users # When start up for the first time we need to populate the user_directory. # This is a set of user_id's we've inserted already @@ -196,7 +196,7 @@ def _do_initial_spam(self): logger.info("Processed all rooms.") - if self.search_all_users: + if self.hs.config.user_directory_search_all_users: num_processed_users = 0 user_ids = yield self.store.get_all_local_users() logger.info( diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 26716d4b3c86..cfaa3c47b79c 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -108,8 +108,7 @@ def test_private_room(self): u2_token = self.login(u2, "pass") u3 = self.register_user("user3", "pass") - # NOTE: Implementation detail. We do not add users to the directory - # until they join a room. + # We do not add users to the directory until they join a room. s = self.get_success(self.handler.search_users(u1, "user2", 10)) room = self.helper.create_room_as(u1, is_public=False, tok=u1_token) @@ -124,6 +123,10 @@ def test_private_room(self): s = self.get_success(self.handler.search_users(u3, "user2", 10)) self.assertEqual(len(s["results"]), 0) + # We get NO search results when searching for user3 by user1. + s = self.get_success(self.handler.search_users(u1, "user3", 10)) + self.assertEqual(len(s["results"]), 0) + def _compress_shared(self, shared): """ Compress a list of users who share rooms dicts to a list of tuples. @@ -133,10 +136,27 @@ def _compress_shared(self, shared): r.add((i["user_id"], i["other_user_id"], i["room_id"])) return r + def get_users_who_share_public_rooms(self): + return self.get_success( + self.store._simple_select_list( + "users_who_share_public_rooms", + None, + ["user_id", "other_user_id", "room_id"], + ) + ) + + def get_users_who_share_private_rooms(self): + return self.get_success( + self.store._simple_select_list( + "users_who_share_private_rooms", + None, + ["user_id", "other_user_id", "room_id"], + ) + ) + def test_initial(self): """ - A user can be searched for only by people that are either in a public - room, or that share a private chat. + The user directory's initial handler correctly updates the search tables. """ u1 = self.register_user("user1", "pass") u1_token = self.login(u1, "pass") @@ -156,16 +176,8 @@ def test_initial(self): self.get_success(self.store.update_user_directory_stream_pos(None)) self.get_success(self.store.delete_all_from_user_dir()) - shares_public = self.get_success( - self.store._simple_select_list( - "users_who_share_public_rooms", None, ["user_id", "other_user_id"] - ) - ) - shares_private = self.get_success( - self.store._simple_select_list( - "users_who_share_private_rooms", None, ["user_id", "other_user_id"] - ) - ) + shares_public = self.get_users_who_share_public_rooms() + shares_private = self.get_users_who_share_private_rooms() self.assertEqual(shares_private, []) self.assertEqual(shares_public, []) @@ -174,26 +186,20 @@ def test_initial(self): self.handler.initially_handled_users = set() self.handler.initially_handled_users_in_public = set() + # Do the initial handler d = self.handler._do_initial_spam() + # This takes a while, so pump it a bunch of times to get through the + # sleep delays for i in range(10): self.pump(1) - r = self.get_success(d) + self.get_success(d) - shares_public = self.get_success( - self.store._simple_select_list( - "users_who_share_public_rooms", - None, - ["user_id", "other_user_id", "room_id"], - ) - ) - shares_private = self.get_success( - self.store._simple_select_list( - "users_who_share_private_rooms", - None, - ["user_id", "other_user_id", "room_id"], - ) + shares_public = self.get_users_who_share_public_rooms() + shares_private = self.get_users_who_share_private_rooms() + in_public_room = self.get_success( + self.store.get_users_in_public_due_to_room(room) ) # User 1 and User 2 share public rooms @@ -201,8 +207,51 @@ def test_initial(self): self._compress_shared(shares_public), set([(u1, u2, room), (u2, u1, room)]) ) + # User 1 and User 2 are in the same public room + self.assertEqual(set(in_public_room), set([u1, u2])) + # User 1 and User 3 share private rooms self.assertEqual( self._compress_shared(shares_private), set([(u1, u3, private_room), (u3, u1, private_room)]), ) + + def test_initial_profile(self): + """ + + """ + self.hs.config.user_directory_search_all_users = True + + u1 = self.register_user("user1", "pass") + u1_token = self.login(u1, "pass") + u2 = self.register_user("user2", "pass") + u2_token = self.login(u2, "pass") + u3 = self.register_user("user3", "pass") + + # User 1 and User 2 join a room. User 3 never does. + room = self.helper.create_room_as(u1, is_public=True, tok=u1_token) + self.helper.invite(room, src=u1, targ=u2, tok=u1_token) + self.helper.join(room, user=u2, tok=u2_token) + + self.get_success(self.store.update_user_directory_stream_pos(None)) + self.get_success(self.store.delete_all_from_user_dir()) + + # Reset the handled users caches + self.handler.initially_handled_users = set() + self.handler.initially_handled_users_in_public = set() + + # Configure the + + d = self.handler._do_initial_spam() + + # This takes a while, so pump it a bunch of times to get through the + # sleep delays + for i in range(10): + self.pump(1) + + self.get_success(d) + + # Despite not sharing a room, search_all_users means we get a search + # result. + s = self.get_success(self.handler.search_users(u1, u3, 10)) + self.assertEqual(len(s["results"]), 1) From 3c4d418d6bbc7d4d639025f2bba3a1aaf454f7da Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 06:32:58 +1100 Subject: [PATCH 26/58] some more test coverage --- synapse/handlers/user_directory.py | 123 +++++--------------------- synapse/storage/user_directory.py | 117 ++++++++---------------- tests/handlers/test_user_directory.py | 39 ++++++-- tests/storage/test_user_directory.py | 4 +- 4 files changed, 89 insertions(+), 194 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index c51f44c611db..204525a55da2 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -15,7 +15,7 @@ import logging -from six import iteritems +from six import iteritems, iterkeys from twisted.internet import defer @@ -140,7 +140,6 @@ def handle_user_deactivated(self, user_id): # FIXME(#3714): We should probably do this in the same worker as all # the other changes. yield self.store.remove_from_user_dir(user_id) - yield self.store.remove_from_user_in_public_room(user_id) @defer.inlineCallbacks def _unsafe_process(self): @@ -236,18 +235,11 @@ def _handle_initial_room(self, room_id): unhandled_users = user_ids - self.initially_handled_users yield self.store.add_profiles_to_user_dir( - room_id, {user_id: users_with_profile[user_id] for user_id in unhandled_users}, ) self.initially_handled_users |= unhandled_users - if is_public: - yield self.store.add_users_to_public_room( - room_id, user_ids=user_ids - self.initially_handled_users_in_public - ) - self.initially_handled_users_in_public |= user_ids - # We now go and figure out the new users who share rooms with user entries # We sleep aggressively here as otherwise it can starve resources. # We also batch up inserts/updates, but try to avoid too many at once. @@ -407,14 +399,15 @@ def _handle_room_publicity_change(self, room_id, prev_event_id, event_id, typ): # ignore the change return - if change: - users_with_profile = yield self.state.get_current_user_in_room(room_id) - for user_id, profile in iteritems(users_with_profile): - yield self._handle_new_user(room_id, user_id, profile) - else: - users = yield self.store.get_users_in_public_due_to_room(room_id) - for user_id in users: - yield self._handle_remove_user(room_id, user_id) + users_with_profile = yield self.state.get_current_user_in_room(room_id) + + # Remove every user from the sharing tables for that room. + for user_id in iterkeys(users_with_profile): + yield self.store.remove_user_who_share_room(user_id, room_id) + + # Then, re-add them to the tables. + for user_id, profile in iteritems(users_with_profile): + yield self._handle_new_user(room_id, user_id, profile) @defer.inlineCallbacks def _handle_local_user(self, user_id): @@ -428,7 +421,7 @@ def _handle_local_user(self, user_id): row = yield self.store.get_user_in_directory(user_id) if not row: - yield self.store.add_profiles_to_user_dir(None, {user_id: profile}) + yield self.store.add_profiles_to_user_dir({user_id: profile}) @defer.inlineCallbacks def _handle_new_user(self, room_id, user_id, profile): @@ -442,19 +435,11 @@ def _handle_new_user(self, room_id, user_id, profile): row = yield self.store.get_user_in_directory(user_id) if not row: - yield self.store.add_profiles_to_user_dir(room_id, {user_id: profile}) + yield self.store.add_profiles_to_user_dir({user_id: profile}) is_public = yield self.store.is_room_world_readable_or_publicly_joinable( room_id ) - - if is_public: - row = yield self.store.get_user_in_public_room(user_id) - if not row: - yield self.store.add_users_to_public_room(room_id, [user_id]) - else: - logger.debug("Not adding new user to public dir, %r", user_id) - # Now we update users who share rooms with users. users_with_profile = yield self.state.get_current_user_in_room(room_id) @@ -462,10 +447,14 @@ def _handle_new_user(self, room_id, user_id, profile): # First, if they're our user then we need to update for every user if self.is_mine_id(user_id): + is_appservice = self.store.get_if_app_services_interested_in_user(user_id) # We don't care about appservice users. if not is_appservice: + # Our users are always in a room with themselves + to_insert.add((user_id, user_id)) + for other_user_id in users_with_profile: if user_id == other_user_id: continue @@ -494,84 +483,16 @@ def _handle_remove_user(self, room_id, user_id): room_id (str): room_id that user left or stopped being public that user_id (str) """ - logger.debug("Maybe removing user %r", user_id) - - row = yield self.store.get_user_in_directory(user_id) - update_user_dir = row and row["room_id"] == room_id - - row = yield self.store.get_user_in_public_room(user_id) - update_user_in_public = row and row["room_id"] == room_id + logger.debug("Removing user %r", user_id) - if update_user_in_public or update_user_dir: - # XXX: Make this faster? - rooms = yield self.store.get_rooms_for_user(user_id) - for j_room_id in rooms: - if not update_user_in_public and not update_user_dir: - break - - is_in_room = yield self.store.is_host_joined( - j_room_id, self.server_name - ) - - if not is_in_room: - continue + # Remove user from sharing tables + yield self.store.remove_user_who_share_room(user_id, room_id) - if update_user_dir: - update_user_dir = False - yield self.store.update_user_in_user_dir(user_id, j_room_id) + # Are they still in a room with members? If not, remove them entirely. + users_in_room_with = yield self.store.get_users_who_share_room_from_dir(user_id) - is_public = yield self.store.is_room_world_readable_or_publicly_joinable( - j_room_id - ) - - if update_user_in_public and is_public: - yield self.store.update_user_in_public_user_list(user_id, j_room_id) - update_user_in_public = False - - if update_user_dir: + if len(users_in_room_with) == 0: yield self.store.remove_from_user_dir(user_id) - elif update_user_in_public: - yield self.store.remove_from_user_in_public_room(user_id) - - # Now handle users who share rooms. - - # Get a list of user tuples that were in the DB due to this room and - # users (this includes tuples where the other user matches `user_id`) - user_tuples = yield self.store.get_users_in_share_dir_with_room_id( - user_id, room_id - ) - - for user_id, other_user_id in user_tuples: - # For each user tuple get a list of rooms that they still share, - # trying to find a private room, and update the entry in the DB - rooms = yield self.store.get_rooms_in_common_for_users( - user_id, other_user_id - ) - - # If they dont share a room anymore, remove the mapping - if not rooms: - yield self.store.remove_user_who_share_room(user_id, other_user_id) - continue - - found_public_share = None - for j_room_id in rooms: - is_public = yield self.store.is_room_world_readable_or_publicly_joinable( - j_room_id - ) - - if is_public: - found_public_share = j_room_id - else: - found_public_share = None - yield self.store.add_users_who_share_room( - room_id, not is_public, [(user_id, other_user_id)] - ) - break - - if found_public_share: - yield self.store.add_users_who_share_room( - room_id, not is_public, [(user_id, other_user_id)] - ) @defer.inlineCallbacks def _handle_profile_change(self, user_id, room_id, prev_event_id, event_id): diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 8b5bcff41766..78d5125b47a9 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -63,28 +63,10 @@ def is_room_world_readable_or_publicly_joinable(self, room_id): defer.returnValue(False) - @defer.inlineCallbacks - def add_users_to_public_room(self, room_id, user_ids): - """Add user to the list of users in public rooms - - Args: - room_id (str): A room_id that all users are in that is world_readable - or publically joinable - user_ids (list(str)): Users to add - """ - yield self._simple_insert_many( - table="users_in_public_rooms", - values=[{"user_id": user_id, "room_id": room_id} for user_id in user_ids], - desc="add_users_to_public_room", - ) - for user_id in user_ids: - self.get_user_in_public_room.invalidate((user_id,)) - - def add_profiles_to_user_dir(self, room_id, users_with_profile): + def add_profiles_to_user_dir(self, users_with_profile): """Add profiles to the user directory Args: - room_id (str): A room_id that all users are joined to users_with_profile (dict): Users to add to directory in the form of mapping of user_id -> ProfileInfo """ @@ -133,7 +115,7 @@ def _add_profiles_to_user_dir_txn(txn): values=[ { "user_id": user_id, - "room_id": room_id, + "room_id": None, "display_name": profile.display_name, "avatar_url": profile.avatar_url, } @@ -251,16 +233,6 @@ def _update_profile_in_user_dir_txn(txn): "update_profile_in_user_dir", _update_profile_in_user_dir_txn ) - @defer.inlineCallbacks - def update_user_in_public_user_list(self, user_id, room_id): - yield self._simple_update_one( - table="users_in_public_rooms", - keyvalues={"user_id": user_id}, - updatevalues={"room_id": room_id}, - desc="update_user_in_public_user_list", - ) - self.get_user_in_public_room.invalidate((user_id,)) - def remove_from_user_dir(self, user_id): def _remove_from_user_dir_txn(txn): self._simple_delete_txn( @@ -270,33 +242,19 @@ def _remove_from_user_dir_txn(txn): txn, table="user_directory_search", keyvalues={"user_id": user_id} ) self._simple_delete_txn( - txn, table="users_in_public_rooms", keyvalues={"user_id": user_id} + txn, + table="users_who_share_public_rooms", + keyvalues={"user_id": user_id}, + ) + self._simple_delete_txn( + txn, + table="users_who_share_public_rooms", + keyvalues={"other_user_id": user_id}, ) txn.call_after(self.get_user_in_directory.invalidate, (user_id,)) - txn.call_after(self.get_user_in_public_room.invalidate, (user_id,)) return self.runInteraction("remove_from_user_dir", _remove_from_user_dir_txn) - @defer.inlineCallbacks - def remove_from_user_in_public_room(self, user_id): - yield self._simple_delete( - table="users_in_public_rooms", - keyvalues={"user_id": user_id}, - desc="remove_from_user_in_public_room", - ) - self.get_user_in_public_room.invalidate((user_id,)) - - def get_users_in_public_due_to_room(self, room_id): - """Get all user_ids that are in the room directory because they're - in the given room_id - """ - return self._simple_select_onecol( - table="users_in_public_rooms", - keyvalues={"room_id": room_id}, - retcol="user_id", - desc="get_users_in_public_due_to_room", - ) - @defer.inlineCallbacks def get_users_in_dir_due_to_room(self, room_id): """Get all user_ids that are in the room directory because they're @@ -309,13 +267,6 @@ def get_users_in_dir_due_to_room(self, room_id): desc="get_users_in_dir_due_to_room", ) - user_ids_pub = yield self._simple_select_onecol( - table="users_in_public_rooms", - keyvalues={"room_id": room_id}, - retcol="user_id", - desc="get_users_in_dir_due_to_room", - ) - user_ids_share_pub = yield self._simple_select_onecol( table="users_who_share_public_rooms", keyvalues={"room_id": room_id}, @@ -331,7 +282,6 @@ def get_users_in_dir_due_to_room(self, room_id): ) user_ids = set(user_ids_dir) - user_ids.update(user_ids_pub) user_ids.update(user_ids_share_pub) user_ids.update(user_ids_share_priv) @@ -396,26 +346,36 @@ def _add_users_who_share_room_txn(txn): "add_users_who_share_room", _add_users_who_share_room_txn ) - def remove_user_who_share_room(self, user_id, other_user_id): - """Deletes entries in the users_who_share_rooms table. The first + def remove_user_who_share_room(self, user_id, room_id): + """ + Deletes entries in the users_who_share_*_rooms table. The first user should be a local user. Args: + user_id (str) room_id (str) - share_private (bool): Is the room private - user_id_tuples([(str, str)]): iterable of 2-tuple of user IDs. """ def _remove_user_who_share_room_txn(txn): self._simple_delete_txn( txn, table="users_who_share_private_rooms", - keyvalues={"user_id": user_id, "other_user_id": other_user_id}, + keyvalues={"user_id": user_id, "room_id": room_id}, + ) + self._simple_delete_txn( + txn, + table="users_who_share_private_rooms", + keyvalues={"other_user_id": user_id, "room_id": room_id}, ) self._simple_delete_txn( txn, table="users_who_share_public_rooms", - keyvalues={"user_id": user_id, "other_user_id": other_user_id}, + keyvalues={"user_id": user_id, "room_id": room_id}, + ) + self._simple_delete_txn( + txn, + table="users_who_share_public_rooms", + keyvalues={"other_user_id": user_id, "room_id": room_id}, ) txn.call_after( self.get_users_who_share_room_from_dir.invalidate, (user_id,) @@ -457,6 +417,9 @@ def get_users_who_share_room_from_dir(self, user_id): for user in pub_rows: users.add(user["other_user_id"]) + # Since users share a room with themselves, remove them + users.discard(user_id) + defer.returnValue(users) @defer.inlineCallbacks @@ -520,12 +483,9 @@ def delete_all_from_user_dir(self): def _delete_all_from_user_dir_txn(txn): txn.execute("DELETE FROM user_directory") txn.execute("DELETE FROM user_directory_search") - txn.execute("DELETE FROM users_in_public_rooms") txn.execute("DELETE FROM users_who_share_public_rooms") txn.execute("DELETE FROM users_who_share_private_rooms") txn.call_after(self.get_user_in_directory.invalidate_all) - txn.call_after(self.get_user_in_public_room.invalidate_all) - txn.call_after(self.get_users_who_share_room_from_dir.invalidate_all) return self.runInteraction( "delete_all_from_user_dir", _delete_all_from_user_dir_txn @@ -536,21 +496,11 @@ def get_user_in_directory(self, user_id): return self._simple_select_one( table="user_directory", keyvalues={"user_id": user_id}, - retcols=("room_id", "display_name", "avatar_url"), + retcols=("display_name", "avatar_url"), allow_none=True, desc="get_user_in_directory", ) - @cached() - def get_user_in_public_room(self, user_id): - return self._simple_select_one( - table="users_in_public_rooms", - keyvalues={"user_id": user_id}, - retcols=("room_id",), - allow_none=True, - desc="get_user_in_public_room", - ) - def get_user_directory_stream_pos(self): return self._simple_select_one_onecol( table="user_directory_stream_pos", @@ -648,13 +598,16 @@ def search_user_dir(self, user_id, search_term, limit): where_clause = "1=1" else: join_clause = """ - LEFT JOIN users_in_public_rooms AS p USING (user_id) + LEFT JOIN ( + SELECT other_user_id AS user_id FROM users_who_share_public_rooms + WHERE user_id = ? + ) AS p USING (user_id) LEFT JOIN ( SELECT other_user_id AS user_id FROM users_who_share_private_rooms WHERE user_id = ? ) AS s USING (user_id) """ - join_args = (user_id,) + join_args = (user_id, user_id) where_clause = "(s.user_id IS NOT NULL OR p.user_id IS NOT NULL)" if isinstance(self.database_engine, PostgresEngine): diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index cfaa3c47b79c..5b12b40bafd2 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -92,10 +92,8 @@ def test_handle_user_deactivated_regular_user(self): self.store.register(user_id=r_user_id, token="123", password_hash=None) ) self.store.remove_from_user_dir = Mock() - self.store.remove_from_user_in_public_room = Mock() self.get_success(self.handler.handle_user_deactivated(r_user_id)) self.store.remove_from_user_dir.called_once_with(r_user_id) - self.store.remove_from_user_in_public_room.assert_called_once_with(r_user_id) def test_private_room(self): """ @@ -110,11 +108,21 @@ def test_private_room(self): # We do not add users to the directory until they join a room. s = self.get_success(self.handler.search_users(u1, "user2", 10)) + self.assertEqual(len(s["results"]), 0) room = self.helper.create_room_as(u1, is_public=False, tok=u1_token) self.helper.invite(room, src=u1, targ=u2, tok=u1_token) self.helper.join(room, user=u2, tok=u2_token) + # Check we have populated the database correctly. + shares_public = self.get_users_who_share_public_rooms() + shares_private = self.get_users_who_share_private_rooms() + + self.assertEqual(shares_public, []) + self.assertEqual( + self._compress_shared(shares_private), set([(u1, u2, room), (u2, u1, room)]) + ) + # We get one search result when searching for user2 by user1. s = self.get_success(self.handler.search_users(u1, "user2", 10)) self.assertEqual(len(s["results"]), 1) @@ -127,12 +135,33 @@ def test_private_room(self): s = self.get_success(self.handler.search_users(u1, "user3", 10)) self.assertEqual(len(s["results"]), 0) + # User 2 then leaves. + self.helper.leave(room, user=u2, tok=u2_token) + + # Check we have removed the values. + shares_public = self.get_users_who_share_public_rooms() + shares_private = self.get_users_who_share_private_rooms() + + self.assertEqual(shares_public, []) + self.assertEqual(self._compress_shared(shares_private), set()) + + # User1 now gets no search results for any of the other users. + s = self.get_success(self.handler.search_users(u1, "user2", 10)) + self.assertEqual(len(s["results"]), 0) + + s = self.get_success(self.handler.search_users(u1, "user3", 10)) + self.assertEqual(len(s["results"]), 0) + def _compress_shared(self, shared): """ Compress a list of users who share rooms dicts to a list of tuples. """ r = set() for i in shared: + # Ignore users sharing a room with themselves + if i["user_id"] == i["other_user_id"]: + continue + r.add((i["user_id"], i["other_user_id"], i["room_id"])) return r @@ -198,18 +227,12 @@ def test_initial(self): shares_public = self.get_users_who_share_public_rooms() shares_private = self.get_users_who_share_private_rooms() - in_public_room = self.get_success( - self.store.get_users_in_public_due_to_room(room) - ) # User 1 and User 2 share public rooms self.assertEqual( self._compress_shared(shares_public), set([(u1, u2, room), (u2, u1, room)]) ) - # User 1 and User 2 are in the same public room - self.assertEqual(set(in_public_room), set([u1, u2])) - # User 1 and User 3 share private rooms self.assertEqual( self._compress_shared(shares_private), diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 0dde1ab2fea3..1228e64cdd49 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -35,16 +35,14 @@ def setUp(self): # alice and bob are both in !room_id. bobby is not but shares # a homeserver with alice. yield self.store.add_profiles_to_user_dir( - "!room:id", { ALICE: ProfileInfo(None, "alice"), BOB: ProfileInfo(None, "bob"), BOBBY: ProfileInfo(None, "bobby"), }, ) - yield self.store.add_users_to_public_room("!room:id", [ALICE, BOB]) yield self.store.add_users_who_share_room( - "!room:id", False, ((ALICE, BOB), (BOB, ALICE)) + "!room:id", False, ((ALICE, BOB), (BOB, ALICE), (BOB, BOB), (ALICE, ALICE)) ) @defer.inlineCallbacks From ff789185c813be0fde8aaaad16261b72a49f1ea1 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 06:46:43 +1100 Subject: [PATCH 27/58] fix failure --- synapse/storage/user_directory.py | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 78d5125b47a9..d499455e8f8d 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -395,32 +395,27 @@ def get_users_who_share_room_from_dir(self, user_id): Returns: list: user_id """ - rows = yield self._simple_select_list( + rows = yield self._simple_select_onecol( table="users_who_share_private_rooms", keyvalues={"user_id": user_id}, - retcols=("other_user_id",), + retcol="other_user_id", desc="get_users_who_share_room_with_user", ) - pub_rows = yield self._simple_select_list( + pub_rows = yield self._simple_select_onecol( table="users_who_share_public_rooms", keyvalues={"user_id": user_id}, - retcols=("other_user_id",), + retcol="other_user_id", desc="get_users_who_share_room_with_user", ) - users = {} + users = set(pub_rows) + users = users.update(rows) - for user in rows: - users.add(user["other_user_id"]) - - for user in pub_rows: - users.add(user["other_user_id"]) - - # Since users share a room with themselves, remove them + # Remove the user themselves from this list. users.discard(user_id) - defer.returnValue(users) + defer.returnValue(list(users)) @defer.inlineCallbacks def get_users_in_share_dir_with_room_id(self, user_id, room_id): From 47162669dce3d2e1831a5e9cd16b4bf4977d8e7e Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 07:00:12 +1100 Subject: [PATCH 28/58] fix failure --- synapse/storage/user_directory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index d499455e8f8d..b55e85ce5846 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -410,7 +410,7 @@ def get_users_who_share_room_from_dir(self, user_id): ) users = set(pub_rows) - users = users.update(rows) + users.update(rows) # Remove the user themselves from this list. users.discard(user_id) From d3e216a1342c2976f063d3943e9905cbd027fc54 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 07:13:25 +1100 Subject: [PATCH 29/58] select distinct --- synapse/storage/user_directory.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index b55e85ce5846..504eeded4761 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -593,16 +593,13 @@ def search_user_dir(self, user_id, search_term, limit): where_clause = "1=1" else: join_clause = """ - LEFT JOIN ( - SELECT other_user_id AS user_id FROM users_who_share_public_rooms - WHERE user_id = ? - ) AS p USING (user_id) + LEFT JOIN users_who_share_public_rooms as p USING (user_id) LEFT JOIN ( SELECT other_user_id AS user_id FROM users_who_share_private_rooms WHERE user_id = ? ) AS s USING (user_id) """ - join_args = (user_id, user_id) + join_args = (user_id,) where_clause = "(s.user_id IS NOT NULL OR p.user_id IS NOT NULL)" if isinstance(self.database_engine, PostgresEngine): @@ -614,7 +611,7 @@ def search_user_dir(self, user_id, search_term, limit): # The array of numbers are the weights for the various part of the # search: (domain, _, display name, localpart) sql = """ - SELECT d.user_id AS user_id, display_name, avatar_url + SELECT DISTINCT d.user_id AS user_id, display_name, avatar_url FROM user_directory_search INNER JOIN user_directory AS d USING (user_id) %s @@ -652,7 +649,7 @@ def search_user_dir(self, user_id, search_term, limit): search_query = _parse_query_sqlite(search_term) sql = """ - SELECT d.user_id AS user_id, display_name, avatar_url + SELECT DISTINCT d.user_id AS user_id, display_name, avatar_url FROM user_directory_search INNER JOIN user_directory AS d USING (user_id) %s From 75174bbd450daba2f4fad4a765005fa7e45f04d7 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 07:21:33 +1100 Subject: [PATCH 30/58] fix remote --- synapse/storage/user_directory.py | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 504eeded4761..f596c85f2753 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -260,29 +260,21 @@ def get_users_in_dir_due_to_room(self, room_id): """Get all user_ids that are in the room directory because they're in the given room_id """ - user_ids_dir = yield self._simple_select_onecol( - table="user_directory", - keyvalues={"room_id": room_id}, - retcol="user_id", - desc="get_users_in_dir_due_to_room", - ) - user_ids_share_pub = yield self._simple_select_onecol( table="users_who_share_public_rooms", keyvalues={"room_id": room_id}, - retcol="user_id", + retcol="other_user_id", desc="get_users_in_dir_due_to_room", ) user_ids_share_priv = yield self._simple_select_onecol( table="users_who_share_private_rooms", keyvalues={"room_id": room_id}, - retcol="user_id", + retcol="other_user_id", desc="get_users_in_dir_due_to_room", ) - user_ids = set(user_ids_dir) - user_ids.update(user_ids_share_pub) + user_ids = set(user_ids_share_pub) user_ids.update(user_ids_share_priv) defer.returnValue(user_ids) From f385f759aa4ab9a923b03814863e510daa30deb0 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 07:37:46 +1100 Subject: [PATCH 31/58] do a log --- synapse/handlers/user_directory.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 204525a55da2..f080155899a5 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -491,6 +491,8 @@ def _handle_remove_user(self, room_id, user_id): # Are they still in a room with members? If not, remove them entirely. users_in_room_with = yield self.store.get_users_who_share_room_from_dir(user_id) + logger.info(users_in_room_with) + if len(users_in_room_with) == 0: yield self.store.remove_from_user_dir(user_id) From a88c1d676c57b405a3d4d15762df1b3d26cdba62 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 07:45:31 +1100 Subject: [PATCH 32/58] fix remote --- synapse/storage/user_directory.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index f596c85f2753..134b56e5190e 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -389,14 +389,14 @@ def get_users_who_share_room_from_dir(self, user_id): """ rows = yield self._simple_select_onecol( table="users_who_share_private_rooms", - keyvalues={"user_id": user_id}, + keyvalues={"other_user_id": user_id}, retcol="other_user_id", desc="get_users_who_share_room_with_user", ) pub_rows = yield self._simple_select_onecol( table="users_who_share_public_rooms", - keyvalues={"user_id": user_id}, + keyvalues={"other_user_id": user_id}, retcol="other_user_id", desc="get_users_who_share_room_with_user", ) From 28239aa2b3e7c2ef7a59ca30b9e902f20805d263 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 07:56:56 +1100 Subject: [PATCH 33/58] fix remote --- synapse/handlers/user_directory.py | 4 ++-- synapse/storage/user_directory.py | 10 ++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index f080155899a5..3820952a669d 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -319,6 +319,8 @@ def _handle_deltas(self, deltas): user_ids = yield self.store.get_users_in_dir_due_to_room( room_id ) + logger.info(user_ids) + for user_id in user_ids: yield self._handle_remove_user(room_id, user_id) return @@ -491,8 +493,6 @@ def _handle_remove_user(self, room_id, user_id): # Are they still in a room with members? If not, remove them entirely. users_in_room_with = yield self.store.get_users_who_share_room_from_dir(user_id) - logger.info(users_in_room_with) - if len(users_in_room_with) == 0: yield self.store.remove_from_user_dir(user_id) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 134b56e5190e..c3f7024ca286 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -251,6 +251,16 @@ def _remove_from_user_dir_txn(txn): table="users_who_share_public_rooms", keyvalues={"other_user_id": user_id}, ) + self._simple_delete_txn( + txn, + table="users_who_share_private_rooms", + keyvalues={"user_id": user_id}, + ) + self._simple_delete_txn( + txn, + table="users_who_share_private_rooms", + keyvalues={"other_user_id": user_id}, + ) txn.call_after(self.get_user_in_directory.invalidate, (user_id,)) return self.runInteraction("remove_from_user_dir", _remove_from_user_dir_txn) From a689842b4382c301557b9d634f7177d3ab46dbcd Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 08:10:46 +1100 Subject: [PATCH 34/58] try and fix --- synapse/storage/user_directory.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index c3f7024ca286..fc0086400241 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -601,8 +601,8 @@ def search_user_dir(self, user_id, search_term, limit): WHERE user_id = ? ) AS s USING (user_id) """ - join_args = (user_id,) - where_clause = "(s.user_id IS NOT NULL OR p.user_id IS NOT NULL)" + join_args = (user_id, user_id) + where_clause = "(s.user_id IS NOT NULL OR p.user_id IS NOT NULL OR user_id != ?)" if isinstance(self.database_engine, PostgresEngine): full_query, exact_query, prefix_query = _parse_query_postgres(search_term) From 06a93b0e07a2baf4ae89f17fbe42a662d534a5cc Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 08:18:07 +1100 Subject: [PATCH 35/58] try and fix --- synapse/storage/user_directory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index fc0086400241..36ae66267272 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -602,7 +602,7 @@ def search_user_dir(self, user_id, search_term, limit): ) AS s USING (user_id) """ join_args = (user_id, user_id) - where_clause = "(s.user_id IS NOT NULL OR p.user_id IS NOT NULL OR user_id != ?)" + where_clause = "(s.user_id IS NOT NULL OR p.user_id IS NOT NULL) AND user_id != ?" if isinstance(self.database_engine, PostgresEngine): full_query, exact_query, prefix_query = _parse_query_postgres(search_term) From fab3b33944816205a9cedf6097136f22ab1da696 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 08:31:18 +1100 Subject: [PATCH 36/58] try and fix --- synapse/storage/user_directory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 36ae66267272..979c0a7b49ea 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -602,7 +602,7 @@ def search_user_dir(self, user_id, search_term, limit): ) AS s USING (user_id) """ join_args = (user_id, user_id) - where_clause = "(s.user_id IS NOT NULL OR p.user_id IS NOT NULL) AND user_id != ?" + where_clause = "(s.user_id IS NOT NULL OR p.user_id IS NOT NULL) AND s.user_id != ?" if isinstance(self.database_engine, PostgresEngine): full_query, exact_query, prefix_query = _parse_query_postgres(search_term) From 257488910e900bea42a1fd3e9bed44b1c90ece79 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 08:38:46 +1100 Subject: [PATCH 37/58] try and fix --- synapse/storage/user_directory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 979c0a7b49ea..6a232dc470b9 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -602,7 +602,7 @@ def search_user_dir(self, user_id, search_term, limit): ) AS s USING (user_id) """ join_args = (user_id, user_id) - where_clause = "(s.user_id IS NOT NULL OR p.user_id IS NOT NULL) AND s.user_id != ?" + where_clause = "(s.user_id IS NOT NULL OR p.user_id IS NOT NULL) AND p.user_id != ?" if isinstance(self.database_engine, PostgresEngine): full_query, exact_query, prefix_query = _parse_query_postgres(search_term) From a50de76f9ac22294dfd95f9d7eae716960831227 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 08:50:35 +1100 Subject: [PATCH 38/58] try and fix --- synapse/storage/user_directory.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 6a232dc470b9..e17a256514f9 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -595,14 +595,17 @@ def search_user_dir(self, user_id, search_term, limit): where_clause = "1=1" else: join_clause = """ - LEFT JOIN users_who_share_public_rooms as p USING (user_id) + LEFT JOIN ( + SELECT user_id FROM users_who_share_public_rooms + WHERE user_id != ? + ) AS p USING (user_id) LEFT JOIN ( SELECT other_user_id AS user_id FROM users_who_share_private_rooms WHERE user_id = ? ) AS s USING (user_id) """ join_args = (user_id, user_id) - where_clause = "(s.user_id IS NOT NULL OR p.user_id IS NOT NULL) AND p.user_id != ?" + where_clause = "(s.user_id IS NOT NULL OR p.user_id IS NOT NULL)" if isinstance(self.database_engine, PostgresEngine): full_query, exact_query, prefix_query = _parse_query_postgres(search_term) @@ -613,7 +616,7 @@ def search_user_dir(self, user_id, search_term, limit): # The array of numbers are the weights for the various part of the # search: (domain, _, display name, localpart) sql = """ - SELECT DISTINCT d.user_id AS user_id, display_name, avatar_url + SELECT d.user_id AS user_id, display_name, avatar_url FROM user_directory_search INNER JOIN user_directory AS d USING (user_id) %s @@ -651,7 +654,7 @@ def search_user_dir(self, user_id, search_term, limit): search_query = _parse_query_sqlite(search_term) sql = """ - SELECT DISTINCT d.user_id AS user_id, display_name, avatar_url + SELECT d.user_id AS user_id, display_name, avatar_url FROM user_directory_search INNER JOIN user_directory AS d USING (user_id) %s From 960b3f0a503f064390b68061dceb564d8f685806 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 09:00:18 +1100 Subject: [PATCH 39/58] try and fix --- synapse/storage/user_directory.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index e17a256514f9..5cdb648575f9 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -595,17 +595,14 @@ def search_user_dir(self, user_id, search_term, limit): where_clause = "1=1" else: join_clause = """ - LEFT JOIN ( - SELECT user_id FROM users_who_share_public_rooms - WHERE user_id != ? - ) AS p USING (user_id) + LEFT JOIN users_who_share_public_rooms as p USING (user_id) LEFT JOIN ( SELECT other_user_id AS user_id FROM users_who_share_private_rooms - WHERE user_id = ? + WHERE user_id = ? AND other_user_id != ? ) AS s USING (user_id) """ join_args = (user_id, user_id) - where_clause = "(s.user_id IS NOT NULL OR p.user_id IS NOT NULL)" + where_clause = "(s.user_id IS NOT NULL OR p.user_id IS NOT NULL) AND p.user_id != ?" if isinstance(self.database_engine, PostgresEngine): full_query, exact_query, prefix_query = _parse_query_postgres(search_term) From d64fbc618ca2222aeb9328409b539275b4e061df Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 09:20:08 +1100 Subject: [PATCH 40/58] try and fix --- synapse/storage/user_directory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 5cdb648575f9..e4404c367db6 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -602,7 +602,7 @@ def search_user_dir(self, user_id, search_term, limit): ) AS s USING (user_id) """ join_args = (user_id, user_id) - where_clause = "(s.user_id IS NOT NULL OR p.user_id IS NOT NULL) AND p.user_id != ?" + where_clause = "(s.user_id IS NOT NULL OR p.user_id IS NOT NULL)" if isinstance(self.database_engine, PostgresEngine): full_query, exact_query, prefix_query = _parse_query_postgres(search_term) From 7c5b66e6a5e1637e30a572de2592b98679c4250e Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 09:30:06 +1100 Subject: [PATCH 41/58] try and fix --- synapse/storage/user_directory.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index e4404c367db6..1f7c28525287 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -613,7 +613,7 @@ def search_user_dir(self, user_id, search_term, limit): # The array of numbers are the weights for the various part of the # search: (domain, _, display name, localpart) sql = """ - SELECT d.user_id AS user_id, display_name, avatar_url + SELECT DISTINCT user_id, display_name, avatar_url FROM user_directory_search INNER JOIN user_directory AS d USING (user_id) %s @@ -651,7 +651,7 @@ def search_user_dir(self, user_id, search_term, limit): search_query = _parse_query_sqlite(search_term) sql = """ - SELECT d.user_id AS user_id, display_name, avatar_url + SELECT DISTINCT user_id, display_name, avatar_url FROM user_directory_search INNER JOIN user_directory AS d USING (user_id) %s From 59334f550f62758f4c91b066807040c45c81d906 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 09:38:29 +1100 Subject: [PATCH 42/58] try and fix --- synapse/storage/user_directory.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 1f7c28525287..c4b91cbcdf05 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -399,14 +399,14 @@ def get_users_who_share_room_from_dir(self, user_id): """ rows = yield self._simple_select_onecol( table="users_who_share_private_rooms", - keyvalues={"other_user_id": user_id}, + keyvalues={"user_id": user_id}, retcol="other_user_id", desc="get_users_who_share_room_with_user", ) pub_rows = yield self._simple_select_onecol( table="users_who_share_public_rooms", - keyvalues={"other_user_id": user_id}, + keyvalues={"user_id": user_id}, retcol="other_user_id", desc="get_users_who_share_room_with_user", ) From 42d0d3efbbc7291300e165aa6eac268457fac178 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 09:46:03 +1100 Subject: [PATCH 43/58] try and fix --- synapse/storage/user_directory.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index c4b91cbcdf05..8fb8a2d0c165 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -595,7 +595,9 @@ def search_user_dir(self, user_id, search_term, limit): where_clause = "1=1" else: join_clause = """ - LEFT JOIN users_who_share_public_rooms as p USING (user_id) + LEFT JOIN ( + SELECT other_user_id AS user_id FROM users_who_share_public_rooms + ) AS p USING (user_id) LEFT JOIN ( SELECT other_user_id AS user_id FROM users_who_share_private_rooms WHERE user_id = ? AND other_user_id != ? From 81109052eeb7a851c38718e31a00cc381208ad03 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 09:54:36 +1100 Subject: [PATCH 44/58] try and fix --- synapse/storage/user_directory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 8fb8a2d0c165..33c44c3d73ca 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -623,7 +623,7 @@ def search_user_dir(self, user_id, search_term, limit): %s AND vector @@ to_tsquery('english', ?) ORDER BY - (CASE WHEN s.user_id IS NOT NULL THEN 4.0 ELSE 1.0 END) + (CASE WHEN user_id IS NOT NULL THEN 4.0 ELSE 1.0 END) * (CASE WHEN display_name IS NOT NULL THEN 1.2 ELSE 1.0 END) * (CASE WHEN avatar_url IS NOT NULL THEN 1.2 ELSE 1.0 END) * ( From d0d8a6ea4bcda7ee13f80e856eac4d8015844743 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 14 Feb 2019 10:10:55 +1100 Subject: [PATCH 45/58] try and fix --- synapse/storage/user_directory.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 33c44c3d73ca..1d40b29d414a 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -597,14 +597,13 @@ def search_user_dir(self, user_id, search_term, limit): join_clause = """ LEFT JOIN ( SELECT other_user_id AS user_id FROM users_who_share_public_rooms - ) AS p USING (user_id) - LEFT JOIN ( + UNION SELECT other_user_id AS user_id FROM users_who_share_private_rooms WHERE user_id = ? AND other_user_id != ? - ) AS s USING (user_id) + ) AS p USING (user_id) """ join_args = (user_id, user_id) - where_clause = "(s.user_id IS NOT NULL OR p.user_id IS NOT NULL)" + where_clause = "p.user_id IS NOT NULL" if isinstance(self.database_engine, PostgresEngine): full_query, exact_query, prefix_query = _parse_query_postgres(search_term) @@ -615,7 +614,7 @@ def search_user_dir(self, user_id, search_term, limit): # The array of numbers are the weights for the various part of the # search: (domain, _, display name, localpart) sql = """ - SELECT DISTINCT user_id, display_name, avatar_url + SELECT d.user_id, display_name, avatar_url FROM user_directory_search INNER JOIN user_directory AS d USING (user_id) %s @@ -623,7 +622,7 @@ def search_user_dir(self, user_id, search_term, limit): %s AND vector @@ to_tsquery('english', ?) ORDER BY - (CASE WHEN user_id IS NOT NULL THEN 4.0 ELSE 1.0 END) + (CASE WHEN d.user_id IS NOT NULL THEN 4.0 ELSE 1.0 END) * (CASE WHEN display_name IS NOT NULL THEN 1.2 ELSE 1.0 END) * (CASE WHEN avatar_url IS NOT NULL THEN 1.2 ELSE 1.0 END) * ( @@ -653,7 +652,7 @@ def search_user_dir(self, user_id, search_term, limit): search_query = _parse_query_sqlite(search_term) sql = """ - SELECT DISTINCT user_id, display_name, avatar_url + SELECT d.user_id, display_name, avatar_url FROM user_directory_search INNER JOIN user_directory AS d USING (user_id) %s From e652138ee2b3af26b7be5836629f8eb63fc2e2d8 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Mon, 25 Feb 2019 10:07:39 -0800 Subject: [PATCH 46/58] fix --- synapse/storage/user_directory.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 1d40b29d414a..91c7f0190880 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -331,13 +331,13 @@ def _add_users_who_share_room_txn(txn): self._simple_upsert_many_txn( txn, table=tbl, - keys=["user_id", "other_user_id", "room_id"], - keyvalues=[ + key_names=["user_id", "other_user_id", "room_id"], + key_values=[ (user_id, other_user_id, room_id) for user_id, other_user_id in user_id_tuples ], - values=(), - valuesvalues=None, + value_names=(), + value_values=None, ) for user_id, other_user_id in user_id_tuples: txn.call_after( From 43b71b37adaf61ddcb438b92ebe4d4a44948dcb3 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Mon, 25 Feb 2019 16:32:53 -0800 Subject: [PATCH 47/58] cleanup --- synapse/handlers/user_directory.py | 1 - synapse/storage/user_directory.py | 28 ---------------------------- 2 files changed, 29 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 3820952a669d..9954a75e2735 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -319,7 +319,6 @@ def _handle_deltas(self, deltas): user_ids = yield self.store.get_users_in_dir_due_to_room( room_id ) - logger.info(user_ids) for user_id in user_ids: yield self._handle_remove_user(room_id, user_id) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 91c7f0190880..25159105dd8b 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -419,34 +419,6 @@ def get_users_who_share_room_from_dir(self, user_id): defer.returnValue(list(users)) - @defer.inlineCallbacks - def get_users_in_share_dir_with_room_id(self, user_id, room_id): - """ - Get all user tuples that are in the users_who_share_*_rooms - tables due to the given room_id. - - Returns: - [(user_id, other_user_id)]: where one of the two will match the given - user_id. - """ - sql = """ - SELECT user_id, other_user_id FROM users_who_share_public_rooms - WHERE room_id = ? AND (user_id = ? OR other_user_id = ?) - """ - public = yield self._execute( - "get_users_in_share_dir_with_room_id", None, sql, room_id, user_id, user_id - ) - - sql = """ - SELECT user_id, other_user_id FROM users_who_share_private_rooms - WHERE room_id = ? AND (user_id = ? OR other_user_id = ?) - """ - private = yield self._execute( - "get_users_in_share_dir_with_room_id", None, sql, room_id, user_id, user_id - ) - - defer.returnValue(public + private) - @defer.inlineCallbacks def get_rooms_in_common_for_users(self, user_id, other_user_id): """Given two user_ids find out the list of rooms they share. From 6c465044682badf91f243670d1e3db5ab3d37bb4 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Mon, 25 Feb 2019 17:57:45 -0800 Subject: [PATCH 48/58] cleanup --- UPGRADE.rst | 12 ++++++------ synapse/storage/schema/delta/53/user_share.sql | 3 +++ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/UPGRADE.rst b/UPGRADE.rst index 228222d53422..fb7f41403771 100644 --- a/UPGRADE.rst +++ b/UPGRADE.rst @@ -191,12 +191,12 @@ Upgrading to v0.9.0 Application services have had a breaking API change in this version. -They can no longer register themselves with a home server using the AS HTTP API. This -decision was made because a compromised application service with free reign to register -any regex in effect grants full read/write access to the home server if a regex of ``.*`` -is used. An attack where a compromised AS re-registers itself with ``.*`` was deemed too -big of a security risk to ignore, and so the ability to register with the HS remotely has -been removed. +They can no longer register themselves with a home server using the AS HTTP API. +This decision was made because a compromised application service with free reign +to register any regex in effect grants full read/write access to the home server +if a regex of ``.*`` is used. An attack where a compromised AS re-registers +itself with ``.*`` was deemed too big of a security risk to ignore, and so the +ability to register with the HS remotely has been removed. It has been replaced by specifying a list of application service registrations in ``homeserver.yaml``:: diff --git a/synapse/storage/schema/delta/53/user_share.sql b/synapse/storage/schema/delta/53/user_share.sql index d4aee8c10a24..accb9c579fd5 100644 --- a/synapse/storage/schema/delta/53/user_share.sql +++ b/synapse/storage/schema/delta/53/user_share.sql @@ -15,6 +15,9 @@ DROP TABLE users_who_share_rooms; +-- This is no longer used because it's duplicated by the users_who_share_public_rooms +DROP TABLE users_in_public_rooms; + -- Table keeping track of who shares a room with who. We only keep track -- of this for local users, so `user_id` is local users only (but we do keep track -- of which remote users share a room) From 95ed0fa1994802e4b6d78e1881697b1ec39f439d Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Mon, 25 Feb 2019 17:58:39 -0800 Subject: [PATCH 49/58] cleanup --- UPGRADE.rst | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/UPGRADE.rst b/UPGRADE.rst index fb7f41403771..228222d53422 100644 --- a/UPGRADE.rst +++ b/UPGRADE.rst @@ -191,12 +191,12 @@ Upgrading to v0.9.0 Application services have had a breaking API change in this version. -They can no longer register themselves with a home server using the AS HTTP API. -This decision was made because a compromised application service with free reign -to register any regex in effect grants full read/write access to the home server -if a regex of ``.*`` is used. An attack where a compromised AS re-registers -itself with ``.*`` was deemed too big of a security risk to ignore, and so the -ability to register with the HS remotely has been removed. +They can no longer register themselves with a home server using the AS HTTP API. This +decision was made because a compromised application service with free reign to register +any regex in effect grants full read/write access to the home server if a regex of ``.*`` +is used. An attack where a compromised AS re-registers itself with ``.*`` was deemed too +big of a security risk to ignore, and so the ability to register with the HS remotely has +been removed. It has been replaced by specifying a list of application service registrations in ``homeserver.yaml``:: From eed6db8cc159c957bf83d996a805222ddb86cd23 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Wed, 27 Feb 2019 14:46:34 -0800 Subject: [PATCH 50/58] update comments --- .../storage/schema/delta/53/user_share.sql | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/synapse/storage/schema/delta/53/user_share.sql b/synapse/storage/schema/delta/53/user_share.sql index accb9c579fd5..57a29e66759d 100644 --- a/synapse/storage/schema/delta/53/user_share.sql +++ b/synapse/storage/schema/delta/53/user_share.sql @@ -13,37 +13,36 @@ * limitations under the License. */ +-- Old disused version of the tables below. DROP TABLE users_who_share_rooms; -- This is no longer used because it's duplicated by the users_who_share_public_rooms DROP TABLE users_in_public_rooms; --- Table keeping track of who shares a room with who. We only keep track --- of this for local users, so `user_id` is local users only (but we do keep track --- of which remote users share a room) +-- Tables keeping track of what users share rooms. This is a map of local users +-- to local or remote users, per room. If it is a local user, there will also be +-- an entry making the user in the same room as themselves. Remote users cannot +-- be in the user_id column, only the other_user_id column. +-- There are two sets of tables, those for public rooms and those for private rooms. CREATE TABLE users_who_share_public_rooms ( user_id TEXT NOT NULL, other_user_id TEXT NOT NULL, room_id TEXT NOT NULL ); - -CREATE UNIQUE INDEX users_who_share_public_rooms_u_idx ON users_who_share_public_rooms(user_id, other_user_id, room_id); -CREATE INDEX users_who_share_public_rooms_r_idx ON users_who_share_public_rooms(room_id); -CREATE INDEX users_who_share_public_rooms_o_idx ON users_who_share_public_rooms(other_user_id); - - CREATE TABLE users_who_share_private_rooms ( user_id TEXT NOT NULL, other_user_id TEXT NOT NULL, room_id TEXT NOT NULL ); +CREATE UNIQUE INDEX users_who_share_public_rooms_u_idx ON users_who_share_public_rooms(user_id, other_user_id, room_id); +CREATE INDEX users_who_share_public_rooms_r_idx ON users_who_share_public_rooms(room_id); +CREATE INDEX users_who_share_public_rooms_o_idx ON users_who_share_public_rooms(other_user_id); CREATE UNIQUE INDEX users_who_share_private_rooms_u_idx ON users_who_share_private_rooms(user_id, other_user_id, room_id); CREATE INDEX users_who_share_private_rooms_r_idx ON users_who_share_private_rooms(room_id); CREATE INDEX users_who_share_private_rooms_o_idx ON users_who_share_private_rooms(other_user_id); - --- Make sure that we populate the table initially +-- Make sure that we populate the tables initially by resetting the stream ID UPDATE user_directory_stream_pos SET stream_id = NULL; From 2bf1d6ac1338b6bbfc29ccc8b81ee29d1a6f1483 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 28 Feb 2019 13:56:53 -0800 Subject: [PATCH 51/58] cleanup --- synapse/handlers/user_directory.py | 8 ++------ synapse/storage/user_directory.py | 5 +++-- tests/handlers/test_user_directory.py | 7 ++----- 3 files changed, 7 insertions(+), 13 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 9954a75e2735..84f6f8f57405 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -63,10 +63,6 @@ def __init__(self, hs): # When start up for the first time we need to populate the user_directory. # This is a set of user_id's we've inserted already self.initially_handled_users = set() - self.initially_handled_users_in_public = set() - - self.initially_handled_users_share = set() - self.initially_handled_users_share_private_room = set() # The current position in the current_state_delta stream self.pos = None @@ -214,13 +210,13 @@ def _do_initial_spam(self): logger.info("Processed all users") self.initially_handled_users = None - self.initially_handled_users_in_public = None yield self.store.update_user_directory_stream_pos(new_pos) @defer.inlineCallbacks def _handle_initial_room(self, room_id): - """Called when we initially fill out user_directory one room at a time + """ + Called when we initially fill out user_directory one room at a time """ is_in_room = yield self.store.is_host_joined(room_id, self.server_name) if not is_in_room: diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 25159105dd8b..f03ca77b32dc 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -455,6 +455,7 @@ def _delete_all_from_user_dir_txn(txn): txn.execute("DELETE FROM users_who_share_public_rooms") txn.execute("DELETE FROM users_who_share_private_rooms") txn.call_after(self.get_user_in_directory.invalidate_all) + txn.call_after(self.get_users_who_share_room_from_dir.invalidate_all) return self.runInteraction( "delete_all_from_user_dir", _delete_all_from_user_dir_txn @@ -586,7 +587,7 @@ def search_user_dir(self, user_id, search_term, limit): # The array of numbers are the weights for the various part of the # search: (domain, _, display name, localpart) sql = """ - SELECT d.user_id, display_name, avatar_url + SELECT d.user_id AS user_id, display_name, avatar_url FROM user_directory_search INNER JOIN user_directory AS d USING (user_id) %s @@ -624,7 +625,7 @@ def search_user_dir(self, user_id, search_term, limit): search_query = _parse_query_sqlite(search_term) sql = """ - SELECT d.user_id, display_name, avatar_url + SELECT d.user_id AS user_id, display_name, avatar_url FROM user_directory_search INNER JOIN user_directory AS d USING (user_id) %s diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 5b12b40bafd2..167c87a87643 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -213,9 +213,8 @@ def test_initial(self): # Reset the handled users caches self.handler.initially_handled_users = set() - self.handler.initially_handled_users_in_public = set() - # Do the initial handler + # Do the initial population d = self.handler._do_initial_spam() # This takes a while, so pump it a bunch of times to get through the @@ -261,10 +260,8 @@ def test_initial_profile(self): # Reset the handled users caches self.handler.initially_handled_users = set() - self.handler.initially_handled_users_in_public = set() - - # Configure the + # Do the initial population d = self.handler._do_initial_spam() # This takes a while, so pump it a bunch of times to get through the From 72e74f46e7ff6842f3adb256dc8374adc211d51b Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 28 Feb 2019 14:06:18 -0800 Subject: [PATCH 52/58] cleanup --- tests/handlers/test_user_directory.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 167c87a87643..3aaaec2606cb 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -238,9 +238,11 @@ def test_initial(self): set([(u1, u3, private_room), (u3, u1, private_room)]), ) - def test_initial_profile(self): + def test_search_all_users(self): """ - + Search all users = True means that a user does not have to share a + private room with the searching user or be in a public room to be search + visible. """ self.hs.config.user_directory_search_all_users = True From c628aaa8a90bb529aa6b4c7c01bb91a1c385debc Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 28 Feb 2019 14:11:07 -0800 Subject: [PATCH 53/58] cleanup --- .../storage/schema/delta/53/user_share.sql | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/synapse/storage/schema/delta/53/user_share.sql b/synapse/storage/schema/delta/53/user_share.sql index 57a29e66759d..359adf39dcf3 100644 --- a/synapse/storage/schema/delta/53/user_share.sql +++ b/synapse/storage/schema/delta/53/user_share.sql @@ -14,35 +14,35 @@ */ -- Old disused version of the tables below. -DROP TABLE users_who_share_rooms; +DROP TABLE IF EXISTS users_who_share_rooms; -- This is no longer used because it's duplicated by the users_who_share_public_rooms -DROP TABLE users_in_public_rooms; +DROP TABLE IF EXISTS users_in_public_rooms; -- Tables keeping track of what users share rooms. This is a map of local users -- to local or remote users, per room. If it is a local user, there will also be -- an entry making the user in the same room as themselves. Remote users cannot -- be in the user_id column, only the other_user_id column. -- There are two sets of tables, those for public rooms and those for private rooms. -CREATE TABLE users_who_share_public_rooms ( +CREATE TABLE IF NOT EXISTS users_who_share_public_rooms ( user_id TEXT NOT NULL, other_user_id TEXT NOT NULL, room_id TEXT NOT NULL ); -CREATE TABLE users_who_share_private_rooms ( +CREATE TABLE IF NOT EXISTS users_who_share_private_rooms ( user_id TEXT NOT NULL, other_user_id TEXT NOT NULL, room_id TEXT NOT NULL ); -CREATE UNIQUE INDEX users_who_share_public_rooms_u_idx ON users_who_share_public_rooms(user_id, other_user_id, room_id); -CREATE INDEX users_who_share_public_rooms_r_idx ON users_who_share_public_rooms(room_id); -CREATE INDEX users_who_share_public_rooms_o_idx ON users_who_share_public_rooms(other_user_id); +CREATE UNIQUE INDEX IF NOT EXISTS users_who_share_public_rooms_u_idx ON users_who_share_public_rooms(user_id, other_user_id, room_id); +CREATE INDEX IF NOT EXISTS users_who_share_public_rooms_r_idx ON users_who_share_public_rooms(room_id); +CREATE INDEX IF NOT EXISTS users_who_share_public_rooms_o_idx ON users_who_share_public_rooms(other_user_id); -CREATE UNIQUE INDEX users_who_share_private_rooms_u_idx ON users_who_share_private_rooms(user_id, other_user_id, room_id); -CREATE INDEX users_who_share_private_rooms_r_idx ON users_who_share_private_rooms(room_id); -CREATE INDEX users_who_share_private_rooms_o_idx ON users_who_share_private_rooms(other_user_id); +CREATE UNIQUE INDEX IF NOT EXISTS users_who_share_private_rooms_u_idx ON users_who_share_private_rooms(user_id, other_user_id, room_id); +CREATE INDEX IF NOT EXISTS users_who_share_private_rooms_r_idx ON users_who_share_private_rooms(room_id); +CREATE INDEX IF NOT EXISTS users_who_share_private_rooms_o_idx ON users_who_share_private_rooms(other_user_id); -- Make sure that we populate the tables initially by resetting the stream ID UPDATE user_directory_stream_pos SET stream_id = NULL; From 843d2870eb9727e81ee7e2b104d74761cf9713a2 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 28 Feb 2019 14:23:34 -0800 Subject: [PATCH 54/58] cleanup --- synapse/handlers/user_directory.py | 3 --- synapse/storage/schema/delta/53/user_share.sql | 7 +++---- synapse/storage/user_directory.py | 4 ++-- tests/handlers/test_user_directory.py | 4 ---- 4 files changed, 5 insertions(+), 13 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 84f6f8f57405..a430dc0e98e9 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -449,9 +449,6 @@ def _handle_new_user(self, room_id, user_id, profile): # We don't care about appservice users. if not is_appservice: - # Our users are always in a room with themselves - to_insert.add((user_id, user_id)) - for other_user_id in users_with_profile: if user_id == other_user_id: continue diff --git a/synapse/storage/schema/delta/53/user_share.sql b/synapse/storage/schema/delta/53/user_share.sql index 359adf39dcf3..dbd2c0a0b275 100644 --- a/synapse/storage/schema/delta/53/user_share.sql +++ b/synapse/storage/schema/delta/53/user_share.sql @@ -20,10 +20,9 @@ DROP TABLE IF EXISTS users_who_share_rooms; DROP TABLE IF EXISTS users_in_public_rooms; -- Tables keeping track of what users share rooms. This is a map of local users --- to local or remote users, per room. If it is a local user, there will also be --- an entry making the user in the same room as themselves. Remote users cannot --- be in the user_id column, only the other_user_id column. --- There are two sets of tables, those for public rooms and those for private rooms. +-- to local or remote users, per room. Remote users cannot be in the user_id +-- column, only the other_user_id column. There are two tables, one for public +-- rooms and those for private rooms. CREATE TABLE IF NOT EXISTS users_who_share_public_rooms ( user_id TEXT NOT NULL, other_user_id TEXT NOT NULL, diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index f03ca77b32dc..2317d22ed68a 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -572,10 +572,10 @@ def search_user_dir(self, user_id, search_term, limit): SELECT other_user_id AS user_id FROM users_who_share_public_rooms UNION SELECT other_user_id AS user_id FROM users_who_share_private_rooms - WHERE user_id = ? AND other_user_id != ? + WHERE user_id = ? ) AS p USING (user_id) """ - join_args = (user_id, user_id) + join_args = (user_id,) where_clause = "p.user_id IS NOT NULL" if isinstance(self.database_engine, PostgresEngine): diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 3aaaec2606cb..d57d81aa4f23 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -158,10 +158,6 @@ def _compress_shared(self, shared): """ r = set() for i in shared: - # Ignore users sharing a room with themselves - if i["user_id"] == i["other_user_id"]: - continue - r.add((i["user_id"], i["other_user_id"], i["room_id"])) return r From f35b3cf7456d6b436d01d2fc9211a53c0f803dc8 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Fri, 1 Mar 2019 01:03:55 -0800 Subject: [PATCH 55/58] cleanup --- synapse/storage/schema/delta/53/user_share.sql | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/synapse/storage/schema/delta/53/user_share.sql b/synapse/storage/schema/delta/53/user_share.sql index dbd2c0a0b275..14424ded0c64 100644 --- a/synapse/storage/schema/delta/53/user_share.sql +++ b/synapse/storage/schema/delta/53/user_share.sql @@ -35,13 +35,13 @@ CREATE TABLE IF NOT EXISTS users_who_share_private_rooms ( room_id TEXT NOT NULL ); -CREATE UNIQUE INDEX IF NOT EXISTS users_who_share_public_rooms_u_idx ON users_who_share_public_rooms(user_id, other_user_id, room_id); -CREATE INDEX IF NOT EXISTS users_who_share_public_rooms_r_idx ON users_who_share_public_rooms(room_id); -CREATE INDEX IF NOT EXISTS users_who_share_public_rooms_o_idx ON users_who_share_public_rooms(other_user_id); +CREATE UNIQUE INDEX users_who_share_public_rooms_u_idx ON users_who_share_public_rooms(user_id, other_user_id, room_id); +CREATE INDEX users_who_share_public_rooms_r_idx ON users_who_share_public_rooms(room_id); +CREATE INDEX users_who_share_public_rooms_o_idx ON users_who_share_public_rooms(other_user_id); -CREATE UNIQUE INDEX IF NOT EXISTS users_who_share_private_rooms_u_idx ON users_who_share_private_rooms(user_id, other_user_id, room_id); -CREATE INDEX IF NOT EXISTS users_who_share_private_rooms_r_idx ON users_who_share_private_rooms(room_id); -CREATE INDEX IF NOT EXISTS users_who_share_private_rooms_o_idx ON users_who_share_private_rooms(other_user_id); +CREATE UNIQUE INDEX users_who_share_private_rooms_u_idx ON users_who_share_private_rooms(user_id, other_user_id, room_id); +CREATE INDEX users_who_share_private_rooms_r_idx ON users_who_share_private_rooms(room_id); +CREATE INDEX users_who_share_private_rooms_o_idx ON users_who_share_private_rooms(other_user_id); -- Make sure that we populate the tables initially by resetting the stream ID UPDATE user_directory_stream_pos SET stream_id = NULL; From 8e963dddb0f7fbfd43c8925de9c53d41be078dc7 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Fri, 1 Mar 2019 18:39:13 -0800 Subject: [PATCH 56/58] cleanuo --- tests/storage/test_user_directory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 1228e64cdd49..a2a652a2354f 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -42,7 +42,7 @@ def setUp(self): }, ) yield self.store.add_users_who_share_room( - "!room:id", False, ((ALICE, BOB), (BOB, ALICE), (BOB, BOB), (ALICE, ALICE)) + "!room:id", False, ((ALICE, BOB), (BOB, ALICE)) ) @defer.inlineCallbacks From a8c48b55755f4c1c4cfb0b633277bb90bbb5423b Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 7 Mar 2019 19:34:41 +1100 Subject: [PATCH 57/58] review cleanup --- synapse/handlers/user_directory.py | 9 +++++++-- tests/handlers/test_user_directory.py | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index a430dc0e98e9..c21da8343ab2 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -51,7 +51,6 @@ class UserDirectoryHandler(object): INITIAL_USER_SLEEP_MS = 10 def __init__(self, hs): - self.hs = hs self.store = hs.get_datastore() self.state = hs.get_state_handler() self.server_name = hs.hostname @@ -59,6 +58,7 @@ def __init__(self, hs): self.notifier = hs.get_notifier() self.is_mine_id = hs.is_mine_id self.update_user_directory = hs.config.update_user_directory + self.search_all_users = hs.config.user_directory_search_all_users # When start up for the first time we need to populate the user_directory. # This is a set of user_id's we've inserted already @@ -191,7 +191,7 @@ def _do_initial_spam(self): logger.info("Processed all rooms.") - if self.hs.config.user_directory_search_all_users: + if self.search_all_users: num_processed_users = 0 user_ids = yield self.store.get_all_local_users() logger.info( @@ -403,6 +403,11 @@ def _handle_room_publicity_change(self, room_id, prev_event_id, event_id, typ): yield self.store.remove_user_who_share_room(user_id, room_id) # Then, re-add them to the tables. + # NOTE: this is not the most efficient method, as handle_new_user sets + # up local_user -> other_user and other_user_whos_local -> local_user, + # which when ran over an entire room, will result in the same values + # being added multiple times. The batching upserts shouldn't make this + # too bad, though. for user_id, profile in iteritems(users_with_profile): yield self._handle_new_user(room_id, user_id, profile) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index d57d81aa4f23..ab4f09dbb61c 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -240,7 +240,7 @@ def test_search_all_users(self): private room with the searching user or be in a public room to be search visible. """ - self.hs.config.user_directory_search_all_users = True + self.handler.search_all_users = True u1 = self.register_user("user1", "pass") u1_token = self.login(u1, "pass") From 3408e8d9f95d23aa2792f7864dd4dbd177527b8c Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 7 Mar 2019 20:04:49 +1100 Subject: [PATCH 58/58] review cleanup --- tests/handlers/test_user_directory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index ab4f09dbb61c..a16a2dc67b26 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -39,7 +39,6 @@ def make_homeserver(self, reactor, clock): return self.setup_test_homeserver(config=config) def prepare(self, reactor, clock, hs): - hs.config.update_user_directory = True self.store = hs.get_datastore() self.handler = hs.get_user_directory_handler() @@ -241,6 +240,7 @@ def test_search_all_users(self): visible. """ self.handler.search_all_users = True + self.hs.config.user_directory_search_all_users = True u1 = self.register_user("user1", "pass") u1_token = self.login(u1, "pass")