From abaa93c1583800155093ab75f6c072c8d83200d0 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 8 Nov 2018 14:06:44 +0000 Subject: Add test to assert set_e2e_device_keys correctly returns False on no-op --- tests/storage/test_end_to_end_keys.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) (limited to 'tests/storage') diff --git a/tests/storage/test_end_to_end_keys.py b/tests/storage/test_end_to_end_keys.py index 8f0aaece40..b83f7336d3 100644 --- a/tests/storage/test_end_to_end_keys.py +++ b/tests/storage/test_end_to_end_keys.py @@ -44,6 +44,21 @@ class EndToEndKeyStoreTestCase(tests.unittest.TestCase): dev = res["user"]["device"] self.assertDictContainsSubset({"keys": json, "device_display_name": None}, dev) + @defer.inlineCallbacks + def test_reupload_key(self): + now = 1470174257070 + json = {"key": "value"} + + yield self.store.store_device("user", "device", None) + + changed = yield self.store.set_e2e_device_keys("user", "device", now, json) + self.assertTrue(changed) + + # If we try to upload the same key then we should be told nothing + # changed + changed = yield self.store.set_e2e_device_keys("user", "device", now, json) + self.assertFalse(changed) + @defer.inlineCallbacks def test_get_key_with_device_name(self): now = 1470174257070 -- cgit 1.5.1 From 835779f7fb8e8b84c3f8e371528d6ea08d0c373f Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 15 Nov 2018 11:08:27 -0700 Subject: Add option to track MAU stats (but not limit people) (#3830) --- changelog.d/3830.feature | 1 + synapse/app/homeserver.py | 2 +- synapse/config/server.py | 6 +++ synapse/storage/monthly_active_users.py | 74 ++++++++++++++++-------------- tests/storage/test_monthly_active_users.py | 25 ++++++++++ tests/test_mau.py | 18 ++++++++ tests/utils.py | 1 + 7 files changed, 92 insertions(+), 35 deletions(-) create mode 100644 changelog.d/3830.feature (limited to 'tests/storage') diff --git a/changelog.d/3830.feature b/changelog.d/3830.feature new file mode 100644 index 0000000000..af472cf763 --- /dev/null +++ b/changelog.d/3830.feature @@ -0,0 +1 @@ +Add option to track MAU stats (but not limit people) diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index 415374a2ce..3e4dea2f19 100755 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -535,7 +535,7 @@ def run(hs): current_mau_count = 0 reserved_count = 0 store = hs.get_datastore() - if hs.config.limit_usage_by_mau: + if hs.config.limit_usage_by_mau or hs.config.mau_stats_only: current_mau_count = yield store.get_monthly_active_count() reserved_count = yield store.get_registered_reserved_users_count() current_mau_gauge.set(float(current_mau_count)) diff --git a/synapse/config/server.py b/synapse/config/server.py index c1c7c0105e..5ff9ac288d 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -77,6 +77,7 @@ class ServerConfig(Config): self.max_mau_value = config.get( "max_mau_value", 0, ) + self.mau_stats_only = config.get("mau_stats_only", False) self.mau_limits_reserved_threepids = config.get( "mau_limit_reserved_threepids", [] @@ -372,6 +373,11 @@ class ServerConfig(Config): # max_mau_value: 50 # mau_trial_days: 2 # + # If enabled, the metrics for the number of monthly active users will + # be populated, however no one will be limited. If limit_usage_by_mau + # is true, this is implied to be true. + # mau_stats_only: False + # # Sometimes the server admin will want to ensure certain accounts are # never blocked by mau checking. These accounts are specified here. # diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index cf4104dc2e..c353b11c9a 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -96,37 +96,38 @@ class MonthlyActiveUsersStore(SQLBaseStore): txn.execute(sql, query_args) - # If MAU user count still exceeds the MAU threshold, then delete on - # a least recently active basis. - # Note it is not possible to write this query using OFFSET due to - # incompatibilities in how sqlite and postgres support the feature. - # sqlite requires 'LIMIT -1 OFFSET ?', the LIMIT must be present - # While Postgres does not require 'LIMIT', but also does not support - # negative LIMIT values. So there is no way to write it that both can - # support - safe_guard = self.hs.config.max_mau_value - len(self.reserved_users) - # Must be greater than zero for postgres - safe_guard = safe_guard if safe_guard > 0 else 0 - query_args = [safe_guard] - - base_sql = """ - DELETE FROM monthly_active_users - WHERE user_id NOT IN ( - SELECT user_id FROM monthly_active_users - ORDER BY timestamp DESC - LIMIT ? + if self.hs.config.limit_usage_by_mau: + # If MAU user count still exceeds the MAU threshold, then delete on + # a least recently active basis. + # Note it is not possible to write this query using OFFSET due to + # incompatibilities in how sqlite and postgres support the feature. + # sqlite requires 'LIMIT -1 OFFSET ?', the LIMIT must be present + # While Postgres does not require 'LIMIT', but also does not support + # negative LIMIT values. So there is no way to write it that both can + # support + safe_guard = self.hs.config.max_mau_value - len(self.reserved_users) + # Must be greater than zero for postgres + safe_guard = safe_guard if safe_guard > 0 else 0 + query_args = [safe_guard] + + base_sql = """ + DELETE FROM monthly_active_users + WHERE user_id NOT IN ( + SELECT user_id FROM monthly_active_users + ORDER BY timestamp DESC + LIMIT ? + ) + """ + # Need if/else since 'AND user_id NOT IN ({})' fails on Postgres + # when len(reserved_users) == 0. Works fine on sqlite. + if len(self.reserved_users) > 0: + query_args.extend(self.reserved_users) + sql = base_sql + """ AND user_id NOT IN ({})""".format( + ','.join(questionmarks) ) - """ - # Need if/else since 'AND user_id NOT IN ({})' fails on Postgres - # when len(reserved_users) == 0. Works fine on sqlite. - if len(self.reserved_users) > 0: - query_args.extend(self.reserved_users) - sql = base_sql + """ AND user_id NOT IN ({})""".format( - ','.join(questionmarks) - ) - else: - sql = base_sql - txn.execute(sql, query_args) + else: + sql = base_sql + txn.execute(sql, query_args) yield self.runInteraction("reap_monthly_active_users", _reap_users) # It seems poor to invalidate the whole cache, Postgres supports @@ -252,8 +253,7 @@ class MonthlyActiveUsersStore(SQLBaseStore): Args: user_id(str): the user_id to query """ - - if self.hs.config.limit_usage_by_mau: + if self.hs.config.limit_usage_by_mau or self.hs.config.mau_stats_only: # Trial users and guests should not be included as part of MAU group is_guest = yield self.is_guest(user_id) if is_guest: @@ -271,8 +271,14 @@ class MonthlyActiveUsersStore(SQLBaseStore): # but only update if we have not previously seen the user for # LAST_SEEN_GRANULARITY ms if last_seen_timestamp is None: - count = yield self.get_monthly_active_count() - if count < self.hs.config.max_mau_value: + # In the case where mau_stats_only is True and limit_usage_by_mau is + # False, there is no point in checking get_monthly_active_count - it + # adds no value and will break the logic if max_mau_value is exceeded. + if not self.hs.config.limit_usage_by_mau: yield self.upsert_monthly_active_user(user_id) + else: + count = yield self.get_monthly_active_count() + if count < self.hs.config.max_mau_value: + yield self.upsert_monthly_active_user(user_id) elif now - last_seen_timestamp > LAST_SEEN_GRANULARITY: yield self.upsert_monthly_active_user(user_id) diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py index 832e379a83..8664bc3d54 100644 --- a/tests/storage/test_monthly_active_users.py +++ b/tests/storage/test_monthly_active_users.py @@ -220,3 +220,28 @@ class MonthlyActiveUsersTestCase(HomeserverTestCase): self.store.user_add_threepid(user2, "email", user2_email, now, now) count = self.store.get_registered_reserved_users_count() self.assertEquals(self.get_success(count), len(threepids)) + + def test_track_monthly_users_without_cap(self): + self.hs.config.limit_usage_by_mau = False + self.hs.config.mau_stats_only = True + self.hs.config.max_mau_value = 1 # should not matter + + count = self.store.get_monthly_active_count() + self.assertEqual(0, self.get_success(count)) + + self.store.upsert_monthly_active_user("@user1:server") + self.store.upsert_monthly_active_user("@user2:server") + self.pump() + + count = self.store.get_monthly_active_count() + self.assertEqual(2, self.get_success(count)) + + def test_no_users_when_not_tracking(self): + self.hs.config.limit_usage_by_mau = False + self.hs.config.mau_stats_only = False + self.store.upsert_monthly_active_user = Mock() + + self.store.populate_monthly_active_users("@user:sever") + self.pump() + + self.store.upsert_monthly_active_user.assert_not_called() diff --git a/tests/test_mau.py b/tests/test_mau.py index 0afdeb0818..04f95c942f 100644 --- a/tests/test_mau.py +++ b/tests/test_mau.py @@ -171,6 +171,24 @@ class TestMauLimit(unittest.HomeserverTestCase): self.assertEqual(e.code, 403) self.assertEqual(e.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) + def test_tracked_but_not_limited(self): + self.hs.config.max_mau_value = 1 # should not matter + self.hs.config.limit_usage_by_mau = False + self.hs.config.mau_stats_only = True + + # Simply being able to create 2 users indicates that the + # limit was not reached. + token1 = self.create_user("kermit1") + self.do_sync_for_user(token1) + token2 = self.create_user("kermit2") + self.do_sync_for_user(token2) + + # We do want to verify that the number of tracked users + # matches what we want though + count = self.store.get_monthly_active_count() + self.reactor.advance(100) + self.assertEqual(2, self.successResultOf(count)) + def create_user(self, localpart): request_data = json.dumps( { diff --git a/tests/utils.py b/tests/utils.py index 67ab916f30..52ab762010 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -134,6 +134,7 @@ def default_config(name): config.hs_disabled_limit_type = "" config.max_mau_value = 50 config.mau_trial_days = 0 + config.mau_stats_only = False config.mau_limits_reserved_threepids = [] config.admin_contact = None config.rc_messages_per_second = 10000 -- cgit 1.5.1 From 30da50a5b80e63c05e4b7ca637e3be9dd88dea59 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Fri, 7 Dec 2018 14:44:46 +0100 Subject: Initialise user displayname from SAML2 data (#4272) When we register a new user from SAML2 data, initialise their displayname correctly. --- changelog.d/4272.feature | 1 + synapse/handlers/register.py | 23 ++++++++++++++++------- synapse/rest/client/v1/login.py | 5 +++++ synapse/rest/saml2/response_resource.py | 3 +++ synapse/storage/registration.py | 20 +++++++++++++------- tests/storage/test_monthly_active_users.py | 2 +- 6 files changed, 39 insertions(+), 15 deletions(-) create mode 100644 changelog.d/4272.feature (limited to 'tests/storage') diff --git a/changelog.d/4272.feature b/changelog.d/4272.feature new file mode 100644 index 0000000000..7a8f286957 --- /dev/null +++ b/changelog.d/4272.feature @@ -0,0 +1 @@ +SAML2 authentication: Initialise user display name from SAML2 data diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 0f87c4610e..ba39e67f6f 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -126,6 +126,7 @@ class RegistrationHandler(BaseHandler): make_guest=False, admin=False, threepid=None, + default_display_name=None, ): """Registers a new client on the server. @@ -140,6 +141,8 @@ class RegistrationHandler(BaseHandler): since it offers no means of associating a device_id with the access_token. Instead you should call auth_handler.issue_access_token after registration. + default_display_name (unicode|None): if set, the new user's displayname + will be set to this. Defaults to 'localpart'. Returns: A tuple of (user_id, access_token). Raises: @@ -169,6 +172,13 @@ class RegistrationHandler(BaseHandler): user = UserID(localpart, self.hs.hostname) user_id = user.to_string() + if was_guest: + # If the user was a guest then they already have a profile + default_display_name = None + + elif default_display_name is None: + default_display_name = localpart + token = None if generate_token: token = self.macaroon_gen.generate_access_token(user_id) @@ -178,10 +188,7 @@ class RegistrationHandler(BaseHandler): password_hash=password_hash, was_guest=was_guest, make_guest=make_guest, - create_profile_with_localpart=( - # If the user was a guest then they already have a profile - None if was_guest else user.localpart - ), + create_profile_with_displayname=default_display_name, admin=admin, ) @@ -203,13 +210,15 @@ class RegistrationHandler(BaseHandler): yield self.check_user_id_not_appservice_exclusive(user_id) if generate_token: token = self.macaroon_gen.generate_access_token(user_id) + if default_display_name is None: + default_display_name = localpart try: yield self.store.register( user_id=user_id, token=token, password_hash=password_hash, make_guest=make_guest, - create_profile_with_localpart=user.localpart, + create_profile_with_displayname=default_display_name, ) except SynapseError: # if user id is taken, just generate another @@ -300,7 +309,7 @@ class RegistrationHandler(BaseHandler): user_id=user_id, password_hash="", appservice_id=service_id, - create_profile_with_localpart=user.localpart, + create_profile_with_displayname=user.localpart, ) defer.returnValue(user_id) @@ -478,7 +487,7 @@ class RegistrationHandler(BaseHandler): user_id=user_id, token=token, password_hash=password_hash, - create_profile_with_localpart=user.localpart, + create_profile_with_displayname=user.localpart, ) else: yield self._auth_handler.delete_access_tokens_for_user(user_id) diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py index b7c5b58b01..e9d3032498 100644 --- a/synapse/rest/client/v1/login.py +++ b/synapse/rest/client/v1/login.py @@ -451,6 +451,7 @@ class SSOAuthHandler(object): @defer.inlineCallbacks def on_successful_auth( self, username, request, client_redirect_url, + user_display_name=None, ): """Called once the user has successfully authenticated with the SSO. @@ -467,6 +468,9 @@ class SSOAuthHandler(object): client_redirect_url (unicode): the redirect_url the client gave us when it first started the process. + user_display_name (unicode|None): if set, and we have to register a new user, + we will set their displayname to this. + Returns: Deferred[none]: Completes once we have handled the request. """ @@ -478,6 +482,7 @@ class SSOAuthHandler(object): yield self._registration_handler.register( localpart=localpart, generate_token=False, + default_display_name=user_display_name, ) ) diff --git a/synapse/rest/saml2/response_resource.py b/synapse/rest/saml2/response_resource.py index ad2ed157b5..69fb77b322 100644 --- a/synapse/rest/saml2/response_resource.py +++ b/synapse/rest/saml2/response_resource.py @@ -66,6 +66,9 @@ class SAML2ResponseResource(Resource): raise CodeMessageException(400, "uid not in SAML2 response") username = saml2_auth.ava["uid"][0] + + displayName = saml2_auth.ava.get("displayName", [None])[0] return self._sso_auth_handler.on_successful_auth( username, request, relay_state, + user_display_name=displayName, ) diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py index 80d76bf9d7..3d55441e33 100644 --- a/synapse/storage/registration.py +++ b/synapse/storage/registration.py @@ -22,6 +22,7 @@ from twisted.internet import defer from synapse.api.errors import Codes, StoreError from synapse.storage import background_updates from synapse.storage._base import SQLBaseStore +from synapse.types import UserID from synapse.util.caches.descriptors import cached, cachedInlineCallbacks @@ -167,7 +168,7 @@ class RegistrationStore(RegistrationWorkerStore, def register(self, user_id, token=None, password_hash=None, was_guest=False, make_guest=False, appservice_id=None, - create_profile_with_localpart=None, admin=False): + create_profile_with_displayname=None, admin=False): """Attempts to register an account. Args: @@ -181,8 +182,8 @@ class RegistrationStore(RegistrationWorkerStore, make_guest (boolean): True if the the new user should be guest, false to add a regular user account. appservice_id (str): The ID of the appservice registering the user. - create_profile_with_localpart (str): Optionally create a profile for - the given localpart. + create_profile_with_displayname (unicode): Optionally create a profile for + the user, setting their displayname to the given value Raises: StoreError if the user_id could not be registered. """ @@ -195,7 +196,7 @@ class RegistrationStore(RegistrationWorkerStore, was_guest, make_guest, appservice_id, - create_profile_with_localpart, + create_profile_with_displayname, admin ) @@ -208,9 +209,11 @@ class RegistrationStore(RegistrationWorkerStore, was_guest, make_guest, appservice_id, - create_profile_with_localpart, + create_profile_with_displayname, admin, ): + user_id_obj = UserID.from_string(user_id) + now = int(self.clock.time()) next_id = self._access_tokens_id_gen.get_next() @@ -273,12 +276,15 @@ class RegistrationStore(RegistrationWorkerStore, (next_id, user_id, token,) ) - if create_profile_with_localpart: + if create_profile_with_displayname: # set a default displayname serverside to avoid ugly race # between auto-joins and clients trying to set displaynames + # + # *obviously* the 'profiles' table uses localpart for user_id + # while everything else uses the full mxid. txn.execute( "INSERT INTO profiles(user_id, displayname) VALUES (?,?)", - (create_profile_with_localpart, create_profile_with_localpart) + (user_id_obj.localpart, create_profile_with_displayname) ) self._invalidate_cache_and_stream( diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py index 8664bc3d54..9618d57463 100644 --- a/tests/storage/test_monthly_active_users.py +++ b/tests/storage/test_monthly_active_users.py @@ -149,7 +149,7 @@ class MonthlyActiveUsersTestCase(HomeserverTestCase): def test_populate_monthly_users_is_guest(self): # Test that guest users are not added to mau list - user_id = "user_id" + user_id = "@user_id:host" self.store.register( user_id=user_id, token="123", password_hash=None, make_guest=True ) -- cgit 1.5.1 From d2f7c4e6b1efbdd3275d02a19220a10cf00a8f66 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Fri, 14 Dec 2018 18:20:59 +0000 Subject: create support user (#4141) Allow for the creation of a support user. A support user can access the server, join rooms, interact with other users, but does not appear in the user directory nor does it contribute to monthly active user limits. --- changelog.d/4141.feature | 1 + docs/admin_api/register_api.rst | 11 ++- synapse/_scripts/register_new_matrix_user.py | 19 ++++- synapse/api/auth.py | 5 +- synapse/api/constants.py | 8 ++ synapse/handlers/register.py | 15 +++- synapse/handlers/room.py | 2 +- synapse/handlers/user_directory.py | 45 ++++++----- synapse/rest/client/v1/admin.py | 11 ++- synapse/storage/monthly_active_users.py | 30 ++++++- synapse/storage/registration.py | 38 ++++++++- .../schema/delta/53/add_user_type_to_users.sql | 19 +++++ tests/api/test_auth.py | 2 + tests/handlers/test_register.py | 30 ++++++- tests/handlers/test_user_directory.py | 91 ++++++++++++++++++++++ tests/rest/client/v1/test_admin.py | 33 ++++++-- tests/storage/test_monthly_active_users.py | 34 +++++++- tests/storage/test_registration.py | 22 ++++++ tests/unittest.py | 1 + tests/utils.py | 1 - 20 files changed, 371 insertions(+), 47 deletions(-) create mode 100644 changelog.d/4141.feature create mode 100644 synapse/storage/schema/delta/53/add_user_type_to_users.sql create mode 100644 tests/handlers/test_user_directory.py (limited to 'tests/storage') diff --git a/changelog.d/4141.feature b/changelog.d/4141.feature new file mode 100644 index 0000000000..632d3547cb --- /dev/null +++ b/changelog.d/4141.feature @@ -0,0 +1 @@ +Special-case a support user for use in verifying behaviour of a given server. The support user does not appear in user directory or monthly active user counts. diff --git a/docs/admin_api/register_api.rst b/docs/admin_api/register_api.rst index 16d65c86b3..084e74ebf5 100644 --- a/docs/admin_api/register_api.rst +++ b/docs/admin_api/register_api.rst @@ -39,13 +39,13 @@ As an example:: } The MAC is the hex digest output of the HMAC-SHA1 algorithm, with the key being -the shared secret and the content being the nonce, user, password, and either -the string "admin" or "notadmin", each separated by NULs. For an example of -generation in Python:: +the shared secret and the content being the nonce, user, password, either the +string "admin" or "notadmin", and optionally the user_type +each separated by NULs. For an example of generation in Python:: import hmac, hashlib - def generate_mac(nonce, user, password, admin=False): + def generate_mac(nonce, user, password, admin=False, user_type=None): mac = hmac.new( key=shared_secret, @@ -59,5 +59,8 @@ generation in Python:: mac.update(password.encode('utf8')) mac.update(b"\x00") mac.update(b"admin" if admin else b"notadmin") + if user_type: + mac.update(b"\x00") + mac.update(user_type.encode('utf8')) return mac.hexdigest() diff --git a/synapse/_scripts/register_new_matrix_user.py b/synapse/_scripts/register_new_matrix_user.py index 70cecde486..4c3abf06fe 100644 --- a/synapse/_scripts/register_new_matrix_user.py +++ b/synapse/_scripts/register_new_matrix_user.py @@ -35,6 +35,7 @@ def request_registration( server_location, shared_secret, admin=False, + user_type=None, requests=_requests, _print=print, exit=sys.exit, @@ -65,6 +66,9 @@ def request_registration( mac.update(password.encode('utf8')) mac.update(b"\x00") mac.update(b"admin" if admin else b"notadmin") + if user_type: + mac.update(b"\x00") + mac.update(user_type.encode('utf8')) mac = mac.hexdigest() @@ -74,6 +78,7 @@ def request_registration( "password": password, "mac": mac, "admin": admin, + "user_type": user_type, } _print("Sending registration request...") @@ -91,7 +96,7 @@ def request_registration( _print("Success!") -def register_new_user(user, password, server_location, shared_secret, admin): +def register_new_user(user, password, server_location, shared_secret, admin, user_type): if not user: try: default_user = getpass.getuser() @@ -129,7 +134,8 @@ def register_new_user(user, password, server_location, shared_secret, admin): else: admin = False - request_registration(user, password, server_location, shared_secret, bool(admin)) + request_registration(user, password, server_location, shared_secret, + bool(admin), user_type) def main(): @@ -154,6 +160,12 @@ def main(): default=None, help="New password for user. Will prompt if omitted.", ) + parser.add_argument( + "-t", + "--user_type", + default=None, + help="User type as specified in synapse.api.constants.UserTypes", + ) admin_group = parser.add_mutually_exclusive_group() admin_group.add_argument( "-a", @@ -208,7 +220,8 @@ def main(): if args.admin or args.no_admin: admin = args.admin - register_new_user(args.user, args.password, args.server_url, secret, admin) + register_new_user(args.user, args.password, args.server_url, secret, + admin, args.user_type) if __name__ == "__main__": diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 5309899703..b8a9af7158 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -802,9 +802,10 @@ class Auth(object): threepid should never be set at the same time. """ - # Never fail an auth check for the server notices users + # Never fail an auth check for the server notices users or support user # This can be a problem where event creation is prohibited due to blocking - if user_id == self.hs.config.server_notices_mxid: + is_support = yield self.store.is_support_user(user_id) + if user_id == self.hs.config.server_notices_mxid or is_support: return if self.hs.config.hs_disabled: diff --git a/synapse/api/constants.py b/synapse/api/constants.py index f20e0fcf0b..b7f25a42a2 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -119,3 +119,11 @@ KNOWN_ROOM_VERSIONS = { ServerNoticeMsgType = "m.server_notice" ServerNoticeLimitReached = "m.server_notice.usage_limit_reached" + + +class UserTypes(object): + """Allows for user type specific behaviour. With the benefit of hindsight + 'admin' and 'guest' users should also be UserTypes. Normal users are type None + """ + SUPPORT = "support" + ALL_USER_TYPES = (SUPPORT) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index ba39e67f6f..21c17c59a0 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -126,6 +126,7 @@ class RegistrationHandler(BaseHandler): make_guest=False, admin=False, threepid=None, + user_type=None, default_display_name=None, ): """Registers a new client on the server. @@ -141,6 +142,8 @@ class RegistrationHandler(BaseHandler): since it offers no means of associating a device_id with the access_token. Instead you should call auth_handler.issue_access_token after registration. + user_type (str|None): type of user. One of the values from + api.constants.UserTypes, or None for a normal user. default_display_name (unicode|None): if set, the new user's displayname will be set to this. Defaults to 'localpart'. Returns: @@ -190,6 +193,7 @@ class RegistrationHandler(BaseHandler): make_guest=make_guest, create_profile_with_displayname=default_display_name, admin=admin, + user_type=user_type, ) if self.hs.config.user_directory_search_all_users: @@ -242,9 +246,16 @@ class RegistrationHandler(BaseHandler): # auto-join the user to any rooms we're supposed to dump them into fake_requester = create_requester(user_id) - # try to create the room if we're the first user on the server + # try to create the room if we're the first real user on the server. Note + # that an auto-generated support user is not a real user and will never be + # the user to create the room should_auto_create_rooms = False - if self.hs.config.autocreate_auto_join_rooms: + is_support = yield self.store.is_support_user(user_id) + # There is an edge case where the first user is the support user, then + # the room is never created, though this seems unlikely and + # recoverable from given the support user being involved in the first + # place. + if self.hs.config.autocreate_auto_join_rooms and not is_support: count = yield self.store.count_all_users() should_auto_create_rooms = count == 1 for r in self.hs.config.auto_join_rooms: diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 3928faa6e7..581e96c743 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -433,7 +433,7 @@ class RoomCreationHandler(BaseHandler): """ user_id = requester.user.to_string() - self.auth.check_auth_blocking(user_id) + yield self.auth.check_auth_blocking(user_id) if not self.spam_checker.user_may_create_room(user_id): raise SynapseError(403, "You are not permitted to create rooms") diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index f11b430126..3c40999338 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -125,9 +125,12 @@ class UserDirectoryHandler(object): """ # FIXME(#3714): We should probably do this in the same worker as all # the other changes. - yield self.store.update_profile_in_user_dir( - user_id, profile.display_name, profile.avatar_url, None, - ) + is_support = yield self.store.is_support_user(user_id) + # 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, + ) @defer.inlineCallbacks def handle_user_deactivated(self, user_id): @@ -329,14 +332,7 @@ class UserDirectoryHandler(object): public_value=Membership.JOIN, ) - if change is None: - # Handle any profile changes - yield self._handle_profile_change( - state_key, room_id, prev_event_id, event_id, - ) - continue - - if not change: + if change is False: # 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( @@ -354,16 +350,25 @@ class UserDirectoryHandler(object): else: logger.debug("Server is still in room: %r", room_id) - if change: # The user joined - event = yield self.store.get_event(event_id, allow_none=True) - profile = ProfileInfo( - avatar_url=event.content.get("avatar_url"), - display_name=event.content.get("displayname"), - ) + is_support = yield self.store.is_support_user(state_key) + if not is_support: + if change is None: + # Handle any profile changes + yield self._handle_profile_change( + state_key, room_id, prev_event_id, event_id, + ) + continue + + if change: # The user joined + event = yield self.store.get_event(event_id, allow_none=True) + profile = ProfileInfo( + avatar_url=event.content.get("avatar_url"), + display_name=event.content.get("displayname"), + ) - yield self._handle_new_user(room_id, state_key, profile) - else: # The user left - yield self._handle_remove_user(room_id, state_key) + yield self._handle_new_user(room_id, state_key, profile) + else: # The user left + yield self._handle_remove_user(room_id, state_key) else: logger.debug("Ignoring irrelevant type: %r", typ) diff --git a/synapse/rest/client/v1/admin.py b/synapse/rest/client/v1/admin.py index 41534b8c2a..82433a2aa9 100644 --- a/synapse/rest/client/v1/admin.py +++ b/synapse/rest/client/v1/admin.py @@ -23,7 +23,7 @@ from six.moves import http_client from twisted.internet import defer -from synapse.api.constants import Membership +from synapse.api.constants import Membership, UserTypes from synapse.api.errors import AuthError, Codes, NotFoundError, SynapseError from synapse.http.servlet import ( assert_params_in_dict, @@ -158,6 +158,11 @@ class UserRegisterServlet(ClientV1RestServlet): raise SynapseError(400, "Invalid password") admin = body.get("admin", None) + user_type = body.get("user_type", None) + + if user_type is not None and user_type not in UserTypes.ALL_USER_TYPES: + raise SynapseError(400, "Invalid user type") + got_mac = body["mac"] want_mac = hmac.new( @@ -171,6 +176,9 @@ class UserRegisterServlet(ClientV1RestServlet): want_mac.update(password) want_mac.update(b"\x00") want_mac.update(b"admin" if admin else b"notadmin") + if user_type: + want_mac.update(b"\x00") + want_mac.update(user_type.encode('utf8')) want_mac = want_mac.hexdigest() if not hmac.compare_digest( @@ -189,6 +197,7 @@ class UserRegisterServlet(ClientV1RestServlet): password=body["password"], admin=bool(admin), generate_token=False, + user_type=user_type, ) result = yield register._create_registration_details(user_id, body) diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index 479e01ddc1..d6fc8edd4c 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -55,9 +55,12 @@ class MonthlyActiveUsersStore(SQLBaseStore): txn, tp["medium"], tp["address"] ) + if user_id: - self.upsert_monthly_active_user_txn(txn, user_id) - reserved_user_list.append(user_id) + is_support = self.is_support_user_txn(txn, user_id) + if not is_support: + self.upsert_monthly_active_user_txn(txn, user_id) + reserved_user_list.append(user_id) else: logger.warning( "mau limit reserved threepid %s not found in db" % tp @@ -182,6 +185,18 @@ class MonthlyActiveUsersStore(SQLBaseStore): Args: user_id (str): user to add/update """ + # Support user never to be included in MAU stats. Note I can't easily call this + # from upsert_monthly_active_user_txn because then I need a _txn form of + # is_support_user which is complicated because I want to cache the result. + # Therefore I call it here and ignore the case where + # upsert_monthly_active_user_txn is called directly from + # _initialise_reserved_users reasoning that it would be very strange to + # include a support user in this context. + + is_support = yield self.is_support_user(user_id) + if is_support: + return + is_insert = yield self.runInteraction( "upsert_monthly_active_user", self.upsert_monthly_active_user_txn, user_id @@ -200,6 +215,16 @@ class MonthlyActiveUsersStore(SQLBaseStore): in a database thread rather than the main thread, and we can't call txn.call_after because txn may not be a LoggingTransaction. + We consciously do not call is_support_txn from this method because it + is not possible to cache the response. is_support_txn will be false in + almost all cases, so it seems reasonable to call it only for + upsert_monthly_active_user and to call is_support_txn manually + for cases where upsert_monthly_active_user_txn is called directly, + like _initialise_reserved_users + + In short, don't call this method with support users. (Support users + should not appear in the MAU stats). + Args: txn (cursor): user_id (str): user to add/update @@ -208,6 +233,7 @@ class MonthlyActiveUsersStore(SQLBaseStore): bool: True if a new entry was created, False if an existing one was updated. """ + # Am consciously deciding to lock the table on the basis that is ought # never be a big table and alternative approaches (batching multiple # upserts into a single txn) introduced a lot of extra complexity. diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py index 3d55441e33..10c3b9757f 100644 --- a/synapse/storage/registration.py +++ b/synapse/storage/registration.py @@ -19,6 +19,7 @@ from six.moves import range from twisted.internet import defer +from synapse.api.constants import UserTypes from synapse.api.errors import Codes, StoreError from synapse.storage import background_updates from synapse.storage._base import SQLBaseStore @@ -168,7 +169,7 @@ class RegistrationStore(RegistrationWorkerStore, def register(self, user_id, token=None, password_hash=None, was_guest=False, make_guest=False, appservice_id=None, - create_profile_with_displayname=None, admin=False): + create_profile_with_displayname=None, admin=False, user_type=None): """Attempts to register an account. Args: @@ -184,6 +185,10 @@ class RegistrationStore(RegistrationWorkerStore, appservice_id (str): The ID of the appservice registering the user. create_profile_with_displayname (unicode): Optionally create a profile for the user, setting their displayname to the given value + admin (boolean): is an admin user? + user_type (str|None): type of user. One of the values from + api.constants.UserTypes, or None for a normal user. + Raises: StoreError if the user_id could not be registered. """ @@ -197,7 +202,8 @@ class RegistrationStore(RegistrationWorkerStore, make_guest, appservice_id, create_profile_with_displayname, - admin + admin, + user_type ) def _register( @@ -211,6 +217,7 @@ class RegistrationStore(RegistrationWorkerStore, appservice_id, create_profile_with_displayname, admin, + user_type, ): user_id_obj = UserID.from_string(user_id) @@ -247,6 +254,7 @@ class RegistrationStore(RegistrationWorkerStore, "is_guest": 1 if make_guest else 0, "appservice_id": appservice_id, "admin": 1 if admin else 0, + "user_type": user_type, } ) else: @@ -260,6 +268,7 @@ class RegistrationStore(RegistrationWorkerStore, "is_guest": 1 if make_guest else 0, "appservice_id": appservice_id, "admin": 1 if admin else 0, + "user_type": user_type, } ) except self.database_engine.module.IntegrityError: @@ -456,6 +465,31 @@ class RegistrationStore(RegistrationWorkerStore, defer.returnValue(res if res else False) + @cachedInlineCallbacks() + def is_support_user(self, user_id): + """Determines if the user is of type UserTypes.SUPPORT + + Args: + user_id (str): user id to test + + Returns: + Deferred[bool]: True if user is of type UserTypes.SUPPORT + """ + res = yield self.runInteraction( + "is_support_user", self.is_support_user_txn, user_id + ) + defer.returnValue(res) + + def is_support_user_txn(self, txn, user_id): + res = self._simple_select_one_onecol_txn( + txn=txn, + table="users", + keyvalues={"name": user_id}, + retcol="user_type", + allow_none=True, + ) + return True if res == UserTypes.SUPPORT else False + @defer.inlineCallbacks def user_add_threepid(self, user_id, medium, address, validated_at, added_at): yield self._simple_upsert("user_threepids", { diff --git a/synapse/storage/schema/delta/53/add_user_type_to_users.sql b/synapse/storage/schema/delta/53/add_user_type_to_users.sql new file mode 100644 index 0000000000..88ec2f83e5 --- /dev/null +++ b/synapse/storage/schema/delta/53/add_user_type_to_users.sql @@ -0,0 +1,19 @@ +/* Copyright 2018 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. + */ + +/* The type of the user: NULL for a regular user, or one of the constants in + * synapse.api.constants.UserTypes + */ +ALTER TABLE users ADD COLUMN user_type TEXT DEFAULT NULL; diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index 379e9c4ab1..69dc40428b 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -50,6 +50,8 @@ class AuthTestCase(unittest.TestCase): # this is overridden for the appservice tests self.store.get_app_service_by_token = Mock(return_value=None) + self.store.is_support_user = Mock(return_value=defer.succeed(False)) + @defer.inlineCallbacks def test_get_user_by_req_user_valid_token(self): user_info = {"name": self.test_user, "token_id": "ditto", "device_id": "device"} diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index 23bce6ee7d..eb70e1daa6 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -17,7 +17,8 @@ from mock import Mock from twisted.internet import defer -from synapse.api.errors import ResourceLimitError +from synapse.api.constants import UserTypes +from synapse.api.errors import ResourceLimitError, SynapseError from synapse.handlers.register import RegistrationHandler from synapse.types import RoomAlias, UserID, create_requester @@ -64,6 +65,7 @@ class RegistrationTestCase(unittest.TestCase): requester, frank.localpart, "Frankie" ) self.assertEquals(result_user_id, user_id) + self.assertTrue(result_token is not None) self.assertEquals(result_token, 'secret') @defer.inlineCallbacks @@ -82,7 +84,7 @@ class RegistrationTestCase(unittest.TestCase): requester, local_part, None ) self.assertEquals(result_user_id, user_id) - self.assertEquals(result_token, 'secret') + self.assertTrue(result_token is not None) @defer.inlineCallbacks def test_mau_limits_when_disabled(self): @@ -169,6 +171,20 @@ class RegistrationTestCase(unittest.TestCase): rooms = yield self.store.get_rooms_for_user(res[0]) self.assertEqual(len(rooms), 0) + @defer.inlineCallbacks + def test_auto_create_auto_join_rooms_when_support_user_exists(self): + room_alias_str = "#room:test" + self.hs.config.auto_join_rooms = [room_alias_str] + + self.store.is_support_user = Mock(return_value=True) + res = yield self.handler.register(localpart='support') + rooms = yield self.store.get_rooms_for_user(res[0]) + self.assertEqual(len(rooms), 0) + directory_handler = self.hs.get_handlers().directory_handler + room_alias = RoomAlias.from_string(room_alias_str) + with self.assertRaises(SynapseError): + yield directory_handler.get_association(room_alias) + @defer.inlineCallbacks def test_auto_create_auto_join_where_no_consent(self): self.hs.config.user_consent_at_registration = True @@ -179,3 +195,13 @@ class RegistrationTestCase(unittest.TestCase): yield self.handler.post_consent_actions(res[0]) rooms = yield self.store.get_rooms_for_user(res[0]) self.assertEqual(len(rooms), 0) + + @defer.inlineCallbacks + def test_register_support_user(self): + res = yield self.handler.register(localpart='user', user_type=UserTypes.SUPPORT) + self.assertTrue(self.store.is_support_user(res[0])) + + @defer.inlineCallbacks + def test_register_not_support_user(self): + res = yield self.handler.register(localpart='user') + self.assertFalse(self.store.is_support_user(res[0])) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py new file mode 100644 index 0000000000..11f2bae698 --- /dev/null +++ b/tests/handlers/test_user_directory.py @@ -0,0 +1,91 @@ +# -*- coding: utf-8 -*- +# Copyright 2018 New Vector +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +from 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 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.TestCase): + """ Tests the UserDirectoryHandler. """ + + @defer.inlineCallbacks + def setUp(self): + hs = yield setup_test_homeserver(self.addCleanup) + self.store = hs.get_datastore() + hs.handlers = UserDirectoryHandlers(hs) + + self.handler = hs.get_handlers().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 + ) + + yield self.handler.handle_local_profile_change(support_user_id, None) + profile = yield 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, + ) + 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.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.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.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.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.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) diff --git a/tests/rest/client/v1/test_admin.py b/tests/rest/client/v1/test_admin.py index e38eb628a9..407bf0ac4c 100644 --- a/tests/rest/client/v1/test_admin.py +++ b/tests/rest/client/v1/test_admin.py @@ -19,6 +19,7 @@ import json from mock import Mock +from synapse.api.constants import UserTypes from synapse.rest.client.v1.admin import register_servlets from tests import unittest @@ -147,7 +148,9 @@ class UserRegisterTestCase(unittest.HomeserverTestCase): nonce = channel.json_body["nonce"] want_mac = hmac.new(key=b"shared", digestmod=hashlib.sha1) - want_mac.update(nonce.encode('ascii') + b"\x00bob\x00abc123\x00admin") + want_mac.update( + nonce.encode('ascii') + b"\x00bob\x00abc123\x00admin\x00support" + ) want_mac = want_mac.hexdigest() body = json.dumps( @@ -156,6 +159,7 @@ class UserRegisterTestCase(unittest.HomeserverTestCase): "username": "bob", "password": "abc123", "admin": True, + "user_type": UserTypes.SUPPORT, "mac": want_mac, } ) @@ -174,7 +178,9 @@ class UserRegisterTestCase(unittest.HomeserverTestCase): nonce = channel.json_body["nonce"] want_mac = hmac.new(key=b"shared", digestmod=hashlib.sha1) - want_mac.update(nonce.encode('ascii') + b"\x00bob\x00abc123\x00admin") + want_mac.update( + nonce.encode('ascii') + b"\x00bob\x00abc123\x00admin" + ) want_mac = want_mac.hexdigest() body = json.dumps( @@ -202,8 +208,8 @@ class UserRegisterTestCase(unittest.HomeserverTestCase): def test_missing_parts(self): """ Synapse will complain if you don't give nonce, username, password, and - mac. Admin is optional. Additional checks are done for length and - type. + mac. Admin and user_types are optional. Additional checks are done for length + and type. """ def nonce(): @@ -260,7 +266,7 @@ class UserRegisterTestCase(unittest.HomeserverTestCase): self.assertEqual('Invalid username', channel.json_body["error"]) # - # Username checks + # Password checks # # Must be present @@ -296,3 +302,20 @@ class UserRegisterTestCase(unittest.HomeserverTestCase): self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) self.assertEqual('Invalid password', channel.json_body["error"]) + + # + # user_type check + # + + # Invalid user_type + body = json.dumps({ + "nonce": nonce(), + "username": "a", + "password": "1234", + "user_type": "invalid"} + ) + request, channel = self.make_request("POST", self.url, body.encode('utf8')) + self.render(request) + + self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual('Invalid user type', channel.json_body["error"]) diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py index 9618d57463..9605301b59 100644 --- a/tests/storage/test_monthly_active_users.py +++ b/tests/storage/test_monthly_active_users.py @@ -16,6 +16,8 @@ from mock import Mock from twisted.internet import defer +from synapse.api.constants import UserTypes + from tests.unittest import HomeserverTestCase FORTY_DAYS = 40 * 24 * 60 * 60 @@ -28,6 +30,7 @@ class MonthlyActiveUsersTestCase(HomeserverTestCase): self.store = hs.get_datastore() hs.config.limit_usage_by_mau = True hs.config.max_mau_value = 50 + # Advance the clock a bit reactor.advance(FORTY_DAYS) @@ -39,14 +42,23 @@ class MonthlyActiveUsersTestCase(HomeserverTestCase): user1_email = "user1@matrix.org" user2 = "@user2:server" user2_email = "user2@matrix.org" + user3 = "@user3:server" + user3_email = "user3@matrix.org" + threepids = [ {'medium': 'email', 'address': user1_email}, {'medium': 'email', 'address': user2_email}, + {'medium': 'email', 'address': user3_email}, ] - user_num = len(threepids) + # -1 because user3 is a support user and does not count + user_num = len(threepids) - 1 self.store.register(user_id=user1, token="123", password_hash=None) self.store.register(user_id=user2, token="456", password_hash=None) + self.store.register( + user_id=user3, token="789", + password_hash=None, user_type=UserTypes.SUPPORT + ) self.pump() now = int(self.hs.get_clock().time_msec()) @@ -60,7 +72,7 @@ class MonthlyActiveUsersTestCase(HomeserverTestCase): active_count = self.store.get_monthly_active_count() - # Test total counts + # Test total counts, ensure user3 (support user) is not counted self.assertEquals(self.get_success(active_count), user_num) # Test user is marked as active @@ -221,6 +233,24 @@ class MonthlyActiveUsersTestCase(HomeserverTestCase): count = self.store.get_registered_reserved_users_count() self.assertEquals(self.get_success(count), len(threepids)) + def test_support_user_not_add_to_mau_limits(self): + support_user_id = "@support:test" + count = self.store.get_monthly_active_count() + self.pump() + self.assertEqual(self.get_success(count), 0) + + self.store.register( + user_id=support_user_id, + token="123", + password_hash=None, + user_type=UserTypes.SUPPORT + ) + + self.store.upsert_monthly_active_user(support_user_id) + count = self.store.get_monthly_active_count() + self.pump() + self.assertEqual(self.get_success(count), 0) + def test_track_monthly_users_without_cap(self): self.hs.config.limit_usage_by_mau = False self.hs.config.mau_stats_only = True diff --git a/tests/storage/test_registration.py b/tests/storage/test_registration.py index 3dfb7b903a..cb3cc4d2e5 100644 --- a/tests/storage/test_registration.py +++ b/tests/storage/test_registration.py @@ -16,6 +16,8 @@ from twisted.internet import defer +from synapse.api.constants import UserTypes + from tests import unittest from tests.utils import setup_test_homeserver @@ -99,6 +101,26 @@ class RegistrationStoreTestCase(unittest.TestCase): user = yield self.store.get_user_by_access_token(self.tokens[0]) self.assertIsNone(user, "access token was not deleted without device_id") + @defer.inlineCallbacks + def test_is_support_user(self): + TEST_USER = "@test:test" + SUPPORT_USER = "@support:test" + + res = yield self.store.is_support_user(None) + self.assertFalse(res) + yield self.store.register(user_id=TEST_USER, token="123", password_hash=None) + res = yield self.store.is_support_user(TEST_USER) + self.assertFalse(res) + + yield self.store.register( + user_id=SUPPORT_USER, + token="456", + password_hash=None, + user_type=UserTypes.SUPPORT + ) + res = yield self.store.is_support_user(SUPPORT_USER) + self.assertTrue(res) + class TokenGenerator: def __init__(self): diff --git a/tests/unittest.py b/tests/unittest.py index 092c930396..78d2f740f9 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -373,6 +373,7 @@ class HomeserverTestCase(TestCase): nonce_str += b"\x00admin" else: nonce_str += b"\x00notadmin" + want_mac.update(nonce.encode('ascii') + b"\x00" + nonce_str) want_mac = want_mac.hexdigest() diff --git a/tests/utils.py b/tests/utils.py index 04796a9b30..38e689983d 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -140,7 +140,6 @@ def default_config(name): config.rc_messages_per_second = 10000 config.rc_message_burst_count = 10000 config.saml2_enabled = False - config.use_frozen_dicts = False # we need a sane default_room_version, otherwise attempts to create rooms will -- cgit 1.5.1 From 7960c26fda8dd8d2d1333b45ef4f5c644930a619 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Wed, 9 Jan 2019 22:26:25 +1100 Subject: Fix adding new rows instead of updating them if one of the key values is a NULL in upserts. (#4369) --- changelog.d/4369.bugfix | 1 + synapse/storage/_base.py | 10 +++++- tests/storage/test_client_ips.py | 71 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 1 deletion(-) create mode 100644 changelog.d/4369.bugfix (limited to 'tests/storage') diff --git a/changelog.d/4369.bugfix b/changelog.d/4369.bugfix new file mode 100644 index 0000000000..a78d557932 --- /dev/null +++ b/changelog.d/4369.bugfix @@ -0,0 +1 @@ +Prevent users with access tokens predating the introduction of device IDs from creating spurious entries in the user_ips table. diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index 1d3069b143..865b5e915a 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -547,11 +547,19 @@ class SQLBaseStore(object): if lock: self.database_engine.lock_table(txn, table) + 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). + if keyvalues[key] is None: + return "%s IS ?" % (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("%s = ?" % (k,) for k in keyvalues) + " AND ".join(_getwhere(k) for k in keyvalues) ) sqlargs = list(values.values()) + list(keyvalues.values()) diff --git a/tests/storage/test_client_ips.py b/tests/storage/test_client_ips.py index 4577e9422b..858efe4992 100644 --- a/tests/storage/test_client_ips.py +++ b/tests/storage/test_client_ips.py @@ -62,6 +62,77 @@ class ClientIpStoreTestCase(unittest.HomeserverTestCase): r, ) + def test_insert_new_client_ip_none_device_id(self): + """ + An insert with a device ID of NULL will not create a new entry, but + update an existing entry in the user_ips table. + """ + self.reactor.advance(12345678) + + user_id = "@user:id" + + # Add & trigger the storage loop + self.get_success( + self.store.insert_client_ip( + user_id, "access_token", "ip", "user_agent", None + ) + ) + self.reactor.advance(200) + self.pump(0) + + result = self.get_success( + self.store._simple_select_list( + table="user_ips", + keyvalues={"user_id": user_id}, + retcols=["access_token", "ip", "user_agent", "device_id", "last_seen"], + desc="get_user_ip_and_agents", + ) + ) + + self.assertEqual( + result, + [ + { + 'access_token': 'access_token', + 'ip': 'ip', + 'user_agent': 'user_agent', + 'device_id': None, + 'last_seen': 12345678000, + } + ], + ) + + # Add another & trigger the storage loop + self.get_success( + self.store.insert_client_ip( + user_id, "access_token", "ip", "user_agent", None + ) + ) + self.reactor.advance(10) + self.pump(0) + + result = self.get_success( + self.store._simple_select_list( + table="user_ips", + keyvalues={"user_id": user_id}, + retcols=["access_token", "ip", "user_agent", "device_id", "last_seen"], + desc="get_user_ip_and_agents", + ) + ) + # Only one result, has been upserted. + self.assertEqual( + result, + [ + { + 'access_token': 'access_token', + 'ip': 'ip', + 'user_agent': 'user_agent', + 'device_id': None, + 'last_seen': 12345878000, + } + ], + ) + def test_disabled_monthly_active_user(self): self.hs.config.limit_usage_by_mau = False self.hs.config.max_mau_value = 50 -- cgit 1.5.1 From e79ba9eb34697cb6205c50282882b294c0acafa7 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 24 Jan 2019 09:28:16 +0000 Subject: Fix tests --- tests/storage/test_redaction.py | 5 ++++- tests/storage/test_roommember.py | 3 ++- tests/storage/test_state.py | 3 ++- tests/test_visibility.py | 4 ++++ tests/utils.py | 3 ++- 5 files changed, 14 insertions(+), 4 deletions(-) (limited to 'tests/storage') diff --git a/tests/storage/test_redaction.py b/tests/storage/test_redaction.py index 02bf975fbf..3957561b1e 100644 --- a/tests/storage/test_redaction.py +++ b/tests/storage/test_redaction.py @@ -18,7 +18,7 @@ from mock import Mock from twisted.internet import defer -from synapse.api.constants import EventTypes, Membership +from synapse.api.constants import EventTypes, Membership, RoomVersions from synapse.types import RoomID, UserID from tests import unittest @@ -52,6 +52,7 @@ class RedactionTestCase(unittest.TestCase): content = {"membership": membership} content.update(extra_content) builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": EventTypes.Member, "sender": user.to_string(), @@ -74,6 +75,7 @@ class RedactionTestCase(unittest.TestCase): self.depth += 1 builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": EventTypes.Message, "sender": user.to_string(), @@ -94,6 +96,7 @@ class RedactionTestCase(unittest.TestCase): @defer.inlineCallbacks def inject_redaction(self, room, event_id, user, reason): builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": EventTypes.Redaction, "sender": user.to_string(), diff --git a/tests/storage/test_roommember.py b/tests/storage/test_roommember.py index 978c66133d..7fa2f4fd70 100644 --- a/tests/storage/test_roommember.py +++ b/tests/storage/test_roommember.py @@ -18,7 +18,7 @@ from mock import Mock from twisted.internet import defer -from synapse.api.constants import EventTypes, Membership +from synapse.api.constants import EventTypes, Membership, RoomVersions from synapse.types import RoomID, UserID from tests import unittest @@ -50,6 +50,7 @@ class RoomMemberStoreTestCase(unittest.TestCase): @defer.inlineCallbacks def inject_room_member(self, room, user, membership, replaces_state=None): builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": EventTypes.Member, "sender": user.to_string(), diff --git a/tests/storage/test_state.py b/tests/storage/test_state.py index 086a39d834..a1f99134dc 100644 --- a/tests/storage/test_state.py +++ b/tests/storage/test_state.py @@ -17,7 +17,7 @@ import logging from twisted.internet import defer -from synapse.api.constants import EventTypes, Membership +from synapse.api.constants import EventTypes, Membership, RoomVersions from synapse.storage.state import StateFilter from synapse.types import RoomID, UserID @@ -52,6 +52,7 @@ class StateStoreTestCase(tests.unittest.TestCase): @defer.inlineCallbacks def inject_state_event(self, room, sender, typ, state_key, content): builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": typ, "sender": sender.to_string(), diff --git a/tests/test_visibility.py b/tests/test_visibility.py index 2eea3b098b..82d63ce00e 100644 --- a/tests/test_visibility.py +++ b/tests/test_visibility.py @@ -17,6 +17,7 @@ import logging from twisted.internet import defer from twisted.internet.defer import succeed +from synapse.api.constants import RoomVersions from synapse.events import FrozenEvent from synapse.visibility import filter_events_for_server @@ -124,6 +125,7 @@ class FilterEventsForServerTestCase(tests.unittest.TestCase): def inject_visibility(self, user_id, visibility): content = {"history_visibility": visibility} builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": "m.room.history_visibility", "sender": user_id, @@ -144,6 +146,7 @@ class FilterEventsForServerTestCase(tests.unittest.TestCase): content = {"membership": membership} content.update(extra_content) builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": "m.room.member", "sender": user_id, @@ -165,6 +168,7 @@ class FilterEventsForServerTestCase(tests.unittest.TestCase): if content is None: content = {"body": "testytest"} builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": "m.room.message", "sender": user_id, diff --git a/tests/utils.py b/tests/utils.py index 08d6faa0a6..1aa8f98094 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -26,7 +26,7 @@ from six.moves.urllib import parse as urlparse from twisted.internet import defer, reactor -from synapse.api.constants import EventTypes +from synapse.api.constants import EventTypes, RoomVersions from synapse.api.errors import CodeMessageException, cs_error from synapse.config.server import ServerConfig from synapse.federation.transport import server @@ -622,6 +622,7 @@ def create_room(hs, room_id, creator_id): event_creation_handler = hs.get_event_creation_handler() builder = event_builder_factory.new( + RoomVersions.V1, { "type": EventTypes.Create, "state_key": "", -- cgit 1.5.1 From 58f6c4818337364dd9c6bf01062e7b0dadcb8a25 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Thu, 24 Jan 2019 21:31:54 +1100 Subject: Use native UPSERTs where possible (#4306) --- .coveragerc | 6 +- .gitignore | 6 +- changelog.d/4306.misc | 1 + synapse/storage/_base.py | 148 +++++++++++++++++++++++++++++++++--- synapse/storage/client_ips.py | 5 +- synapse/storage/engines/__init__.py | 2 +- synapse/storage/engines/postgres.py | 14 ++++ synapse/storage/engines/sqlite.py | 96 +++++++++++++++++++++++ synapse/storage/engines/sqlite3.py | 87 --------------------- synapse/storage/pusher.py | 9 ++- synapse/storage/user_directory.py | 55 ++++++++++---- tests/storage/test_base.py | 1 + tests/test_server.py | 12 ++- tests/unittest.py | 12 ++- tox.ini | 1 + 15 files changed, 325 insertions(+), 130 deletions(-) create mode 100644 changelog.d/4306.misc create mode 100644 synapse/storage/engines/sqlite.py delete mode 100644 synapse/storage/engines/sqlite3.py (limited to 'tests/storage') diff --git a/.coveragerc b/.coveragerc index 9873a30738..e9460a340a 100644 --- a/.coveragerc +++ b/.coveragerc @@ -1,11 +1,7 @@ [run] branch = True parallel = True -source = synapse - -[paths] -source= - coverage +include = synapse/* [report] precision = 2 diff --git a/.gitignore b/.gitignore index d739595c3a..1033124f1d 100644 --- a/.gitignore +++ b/.gitignore @@ -25,9 +25,9 @@ homeserver*.pid *.tls.dh *.tls.key -.coverage -.coverage.* -!.coverage.rc +.coverage* +coverage.* +!.coveragerc htmlcov demo/*/*.db diff --git a/changelog.d/4306.misc b/changelog.d/4306.misc new file mode 100644 index 0000000000..58130b6190 --- /dev/null +++ b/changelog.d/4306.misc @@ -0,0 +1 @@ +Synapse will now take advantage of native UPSERT functionality in PostgreSQL 9.5+ and SQLite 3.24+. diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index 865b5e915a..254fdc04c6 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -192,6 +192,41 @@ class SQLBaseStore(object): self.database_engine = hs.database_engine + # A set of tables that are not safe to use native upserts in. + self._unsafe_to_upsert_tables = {"user_ips"} + + if self.database_engine.can_native_upsert: + # Check ASAP (and then later, every 1s) to see if we have finished + # background updates of tables that aren't safe to update. + self._clock.call_later(0.0, self._check_safe_to_upsert) + + @defer.inlineCallbacks + def _check_safe_to_upsert(self): + """ + Is it safe to use native UPSERT? + + If there are background updates, we will need to wait, as they may be + the addition of indexes that set the UNIQUE constraint that we require. + + If the background updates have not completed, wait a second and check again. + """ + updates = yield self._simple_select_list( + "background_updates", + keyvalues=None, + retcols=["update_name"], + desc="check_background_updates", + ) + updates = [x["update_name"] for x in updates] + + # The User IPs table in schema #53 was missing a unique index, which we + # run as a background update. + if "user_ips_device_unique_index" not in updates: + self._unsafe_to_upsert_tables.discard("user_id") + + # If there's any tables left to check, reschedule to run. + if self._unsafe_to_upsert_tables: + self._clock.call_later(1.0, self._check_safe_to_upsert) + def start_profiling(self): self._previous_loop_ts = self._clock.time_msec() @@ -494,8 +529,15 @@ class SQLBaseStore(object): txn.executemany(sql, vals) @defer.inlineCallbacks - def _simple_upsert(self, table, keyvalues, values, - insertion_values={}, desc="_simple_upsert", lock=True): + def _simple_upsert( + self, + table, + keyvalues, + values, + insertion_values={}, + desc="_simple_upsert", + lock=True + ): """ `lock` should generally be set to True (the default), but can be set @@ -516,16 +558,21 @@ class SQLBaseStore(object): inserting lock (bool): True to lock the table when doing the upsert. Returns: - Deferred(bool): True if a new entry was created, False if an - existing one was updated. + Deferred(None or bool): Native upserts always return None. Emulated + upserts return True if a new entry was created, False if an existing + one was updated. """ attempts = 0 while True: try: result = yield self.runInteraction( desc, - self._simple_upsert_txn, table, keyvalues, values, insertion_values, - lock=lock + self._simple_upsert_txn, + table, + keyvalues, + values, + insertion_values, + lock=lock, ) defer.returnValue(result) except self.database_engine.module.IntegrityError as e: @@ -537,12 +584,59 @@ class SQLBaseStore(object): # presumably we raced with another transaction: let's retry. logger.warn( - "IntegrityError when upserting into %s; retrying: %s", - table, e + "%s when upserting into %s; retrying: %s", e.__name__, table, e ) - def _simple_upsert_txn(self, txn, table, keyvalues, values, insertion_values={}, - lock=True): + def _simple_upsert_txn( + self, + txn, + table, + keyvalues, + values, + insertion_values={}, + lock=True, + ): + """ + Pick the UPSERT method which works best on the platform. Either the + native one (Pg9.5+, recent SQLites), or fall back to an emulated method. + + Args: + txn: The transaction to use. + table (str): The table to upsert into + keyvalues (dict): The unique key tables and their new values + values (dict): The nonunique columns and their new values + insertion_values (dict): additional key/values to use only when + inserting + lock (bool): True to lock the table when doing the upsert. + Returns: + Deferred(None or bool): Native upserts always return None. Emulated + upserts return True if a new entry was created, False if an existing + one was updated. + """ + if ( + self.database_engine.can_native_upsert + and table not in self._unsafe_to_upsert_tables + ): + return self._simple_upsert_txn_native_upsert( + txn, + table, + keyvalues, + values, + insertion_values=insertion_values, + ) + else: + return self._simple_upsert_txn_emulated( + txn, + table, + keyvalues, + values, + insertion_values=insertion_values, + lock=lock, + ) + + def _simple_upsert_txn_emulated( + self, txn, table, keyvalues, values, insertion_values={}, lock=True + ): # We need to lock the table :(, unless we're *really* careful if lock: self.database_engine.lock_table(txn, table) @@ -577,12 +671,44 @@ class SQLBaseStore(object): sql = "INSERT INTO %s (%s) VALUES (%s)" % ( table, ", ".join(k for k in allvalues), - ", ".join("?" for _ in allvalues) + ", ".join("?" for _ in allvalues), ) txn.execute(sql, list(allvalues.values())) # successfully inserted return True + def _simple_upsert_txn_native_upsert( + self, txn, table, keyvalues, values, insertion_values={} + ): + """ + Use the native UPSERT functionality in recent PostgreSQL versions. + + Args: + table (str): The table to upsert into + keyvalues (dict): The unique key tables and their new values + values (dict): The nonunique columns and their new values + insertion_values (dict): additional key/values to use only when + inserting + Returns: + None + """ + allvalues = {} + allvalues.update(keyvalues) + allvalues.update(values) + allvalues.update(insertion_values) + + sql = ( + "INSERT INTO %s (%s) VALUES (%s) " + "ON CONFLICT (%s) DO UPDATE SET %s" + ) % ( + table, + ", ".join(k for k in allvalues), + ", ".join("?" for _ in allvalues), + ", ".join(k for k in keyvalues), + ", ".join(k + "=EXCLUDED." + k for k in values), + ) + txn.execute(sql, list(allvalues.values())) + 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/client_ips.py b/synapse/storage/client_ips.py index b228a20ac2..091d7116c5 100644 --- a/synapse/storage/client_ips.py +++ b/synapse/storage/client_ips.py @@ -257,7 +257,10 @@ class ClientIpStore(background_updates.BackgroundUpdateStore): ) def _update_client_ips_batch_txn(self, txn, to_update): - self.database_engine.lock_table(txn, "user_ips") + if "user_ips" in self._unsafe_to_upsert_tables or ( + not self.database_engine.can_native_upsert + ): + self.database_engine.lock_table(txn, "user_ips") for entry in iteritems(to_update): (user_id, access_token, ip), (user_agent, device_id, last_seen) = entry diff --git a/synapse/storage/engines/__init__.py b/synapse/storage/engines/__init__.py index e2f9de8451..ff5ef97ca8 100644 --- a/synapse/storage/engines/__init__.py +++ b/synapse/storage/engines/__init__.py @@ -18,7 +18,7 @@ import platform from ._base import IncorrectDatabaseSetup from .postgres import PostgresEngine -from .sqlite3 import Sqlite3Engine +from .sqlite import Sqlite3Engine SUPPORTED_MODULE = { "sqlite3": Sqlite3Engine, diff --git a/synapse/storage/engines/postgres.py b/synapse/storage/engines/postgres.py index 42225f8a2a..4004427c7b 100644 --- a/synapse/storage/engines/postgres.py +++ b/synapse/storage/engines/postgres.py @@ -38,6 +38,13 @@ class PostgresEngine(object): return sql.replace("?", "%s") def on_new_connection(self, db_conn): + + # Get the version of PostgreSQL that we're using. As per the psycopg2 + # docs: The number is formed by converting the major, minor, and + # revision numbers into two-decimal-digit numbers and appending them + # together. For example, version 8.1.5 will be returned as 80105 + self._version = db_conn.server_version + db_conn.set_isolation_level( self.module.extensions.ISOLATION_LEVEL_REPEATABLE_READ ) @@ -54,6 +61,13 @@ class PostgresEngine(object): cursor.close() + @property + def can_native_upsert(self): + """ + Can we use native UPSERTs? This requires PostgreSQL 9.5+. + """ + return self._version >= 90500 + def is_deadlock(self, error): if isinstance(error, self.module.DatabaseError): # https://www.postgresql.org/docs/current/static/errcodes-appendix.html diff --git a/synapse/storage/engines/sqlite.py b/synapse/storage/engines/sqlite.py new file mode 100644 index 0000000000..c64d73ff21 --- /dev/null +++ b/synapse/storage/engines/sqlite.py @@ -0,0 +1,96 @@ +# -*- coding: utf-8 -*- +# Copyright 2015, 2016 OpenMarket 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. + +import struct +import threading +from sqlite3 import sqlite_version_info + +from synapse.storage.prepare_database import prepare_database + + +class Sqlite3Engine(object): + single_threaded = True + + def __init__(self, database_module, database_config): + self.module = database_module + + # The current max state_group, or None if we haven't looked + # in the DB yet. + self._current_state_group_id = None + self._current_state_group_id_lock = threading.Lock() + + @property + def can_native_upsert(self): + """ + Do we support native UPSERTs? This requires SQLite3 3.24+, plus some + more work we haven't done yet to tell what was inserted vs updated. + """ + return sqlite_version_info >= (3, 24, 0) + + def check_database(self, txn): + pass + + def convert_param_style(self, sql): + return sql + + def on_new_connection(self, db_conn): + prepare_database(db_conn, self, config=None) + db_conn.create_function("rank", 1, _rank) + + def is_deadlock(self, error): + return False + + def is_connection_closed(self, conn): + return False + + def lock_table(self, txn, table): + return + + def get_next_state_group_id(self, txn): + """Returns an int that can be used as a new state_group ID + """ + # We do application locking here since if we're using sqlite then + # we are a single process synapse. + with self._current_state_group_id_lock: + if self._current_state_group_id is None: + txn.execute("SELECT COALESCE(max(id), 0) FROM state_groups") + self._current_state_group_id = txn.fetchone()[0] + + self._current_state_group_id += 1 + return self._current_state_group_id + + +# Following functions taken from: https://github.com/coleifer/peewee + +def _parse_match_info(buf): + bufsize = len(buf) + return [struct.unpack('@I', buf[i:i + 4])[0] for i in range(0, bufsize, 4)] + + +def _rank(raw_match_info): + """Handle match_info called w/default args 'pcx' - based on the example rank + function http://sqlite.org/fts3.html#appendix_a + """ + match_info = _parse_match_info(raw_match_info) + score = 0.0 + p, c = match_info[:2] + for phrase_num in range(p): + phrase_info_idx = 2 + (phrase_num * c * 3) + for col_num in range(c): + col_idx = phrase_info_idx + (col_num * 3) + x1, x2 = match_info[col_idx:col_idx + 2] + if x1 > 0: + score += float(x1) / x2 + return score diff --git a/synapse/storage/engines/sqlite3.py b/synapse/storage/engines/sqlite3.py deleted file mode 100644 index 19949fc474..0000000000 --- a/synapse/storage/engines/sqlite3.py +++ /dev/null @@ -1,87 +0,0 @@ -# -*- coding: utf-8 -*- -# Copyright 2015, 2016 OpenMarket 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. - -import struct -import threading - -from synapse.storage.prepare_database import prepare_database - - -class Sqlite3Engine(object): - single_threaded = True - - def __init__(self, database_module, database_config): - self.module = database_module - - # The current max state_group, or None if we haven't looked - # in the DB yet. - self._current_state_group_id = None - self._current_state_group_id_lock = threading.Lock() - - def check_database(self, txn): - pass - - def convert_param_style(self, sql): - return sql - - def on_new_connection(self, db_conn): - prepare_database(db_conn, self, config=None) - db_conn.create_function("rank", 1, _rank) - - def is_deadlock(self, error): - return False - - def is_connection_closed(self, conn): - return False - - def lock_table(self, txn, table): - return - - def get_next_state_group_id(self, txn): - """Returns an int that can be used as a new state_group ID - """ - # We do application locking here since if we're using sqlite then - # we are a single process synapse. - with self._current_state_group_id_lock: - if self._current_state_group_id is None: - txn.execute("SELECT COALESCE(max(id), 0) FROM state_groups") - self._current_state_group_id = txn.fetchone()[0] - - self._current_state_group_id += 1 - return self._current_state_group_id - - -# Following functions taken from: https://github.com/coleifer/peewee - -def _parse_match_info(buf): - bufsize = len(buf) - return [struct.unpack('@I', buf[i:i + 4])[0] for i in range(0, bufsize, 4)] - - -def _rank(raw_match_info): - """Handle match_info called w/default args 'pcx' - based on the example rank - function http://sqlite.org/fts3.html#appendix_a - """ - match_info = _parse_match_info(raw_match_info) - score = 0.0 - p, c = match_info[:2] - for phrase_num in range(p): - phrase_info_idx = 2 + (phrase_num * c * 3) - for col_num in range(c): - col_idx = phrase_info_idx + (col_num * 3) - x1, x2 = match_info[col_idx:col_idx + 2] - if x1 > 0: - score += float(x1) / x2 - return score diff --git a/synapse/storage/pusher.py b/synapse/storage/pusher.py index 2743b52bad..134297e284 100644 --- a/synapse/storage/pusher.py +++ b/synapse/storage/pusher.py @@ -215,7 +215,7 @@ class PusherStore(PusherWorkerStore): with self._pushers_id_gen.get_next() as stream_id: # no need to lock because `pushers` has a unique key on # (app_id, pushkey, user_name) so _simple_upsert will retry - newly_inserted = yield self._simple_upsert( + yield self._simple_upsert( table="pushers", keyvalues={ "app_id": app_id, @@ -238,7 +238,12 @@ class PusherStore(PusherWorkerStore): lock=False, ) - if newly_inserted: + user_has_pusher = self.get_if_user_has_pusher.cache.get( + (user_id,), None, update_metrics=False + ) + + if user_has_pusher is not True: + # invalidate, since we the user might not have had a pusher before yield self.runInteraction( "add_pusher", self._invalidate_cache_and_stream, diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index a8781b0e5d..ce48212265 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -168,14 +168,14 @@ class UserDirectoryStore(SQLBaseStore): if isinstance(self.database_engine, PostgresEngine): # We weight the localpart most highly, then display name and finally # server name - if new_entry: + if self.database_engine.can_native_upsert: sql = """ INSERT INTO user_directory_search(user_id, vector) VALUES (?, setweight(to_tsvector('english', ?), 'A') || setweight(to_tsvector('english', ?), 'D') || setweight(to_tsvector('english', COALESCE(?, '')), 'B') - ) + ) ON CONFLICT (user_id) DO UPDATE SET vector=EXCLUDED.vector """ txn.execute( sql, @@ -185,20 +185,45 @@ class UserDirectoryStore(SQLBaseStore): ) ) else: - sql = """ - UPDATE user_directory_search - SET vector = setweight(to_tsvector('english', ?), 'A') - || setweight(to_tsvector('english', ?), 'D') - || setweight(to_tsvector('english', COALESCE(?, '')), 'B') - WHERE user_id = ? - """ - txn.execute( - sql, - ( - get_localpart_from_id(user_id), get_domain_from_id(user_id), - display_name, user_id, + # TODO: Remove this code after we've bumped the minimum version + # of postgres to always support upserts, so we can get rid of + # `new_entry` usage + if new_entry is True: + sql = """ + INSERT INTO user_directory_search(user_id, vector) + VALUES (?, + setweight(to_tsvector('english', ?), 'A') + || setweight(to_tsvector('english', ?), 'D') + || setweight(to_tsvector('english', COALESCE(?, '')), 'B') + ) + """ + txn.execute( + sql, + ( + user_id, get_localpart_from_id(user_id), + get_domain_from_id(user_id), display_name, + ) + ) + elif new_entry is False: + sql = """ + UPDATE user_directory_search + SET vector = setweight(to_tsvector('english', ?), 'A') + || setweight(to_tsvector('english', ?), 'D') + || setweight(to_tsvector('english', COALESCE(?, '')), 'B') + WHERE user_id = ? + """ + txn.execute( + sql, + ( + get_localpart_from_id(user_id), + get_domain_from_id(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 self._simple_upsert_txn( diff --git a/tests/storage/test_base.py b/tests/storage/test_base.py index 829f47d2e8..452d76ddd5 100644 --- a/tests/storage/test_base.py +++ b/tests/storage/test_base.py @@ -49,6 +49,7 @@ class SQLBaseStoreTestCase(unittest.TestCase): self.db_pool.runWithConnection = runWithConnection config = Mock() + config._enable_native_upserts = False config.event_cache_size = 1 config.database_config = {"name": "sqlite3"} hs = TestHomeServer( diff --git a/tests/test_server.py b/tests/test_server.py index 634a8fbca5..08fb3fe02f 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -19,7 +19,7 @@ from six import StringIO from twisted.internet.defer import Deferred from twisted.python.failure import Failure -from twisted.test.proto_helpers import AccumulatingProtocol, MemoryReactorClock +from twisted.test.proto_helpers import AccumulatingProtocol from twisted.web.resource import Resource from twisted.web.server import NOT_DONE_YET @@ -30,12 +30,18 @@ from synapse.util import Clock from synapse.util.logcontext import make_deferred_yieldable from tests import unittest -from tests.server import FakeTransport, make_request, render, setup_test_homeserver +from tests.server import ( + FakeTransport, + ThreadedMemoryReactorClock, + make_request, + render, + setup_test_homeserver, +) class JsonResourceTests(unittest.TestCase): def setUp(self): - self.reactor = MemoryReactorClock() + self.reactor = ThreadedMemoryReactorClock() self.hs_clock = Clock(self.reactor) self.homeserver = setup_test_homeserver( self.addCleanup, http_client=None, clock=self.hs_clock, reactor=self.reactor diff --git a/tests/unittest.py b/tests/unittest.py index 78d2f740f9..cda549c783 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -96,7 +96,7 @@ class TestCase(unittest.TestCase): method = getattr(self, methodName) - level = getattr(method, "loglevel", getattr(self, "loglevel", logging.ERROR)) + level = getattr(method, "loglevel", getattr(self, "loglevel", logging.WARNING)) @around(self) def setUp(orig): @@ -333,7 +333,15 @@ class HomeserverTestCase(TestCase): """ kwargs = dict(kwargs) kwargs.update(self._hs_args) - return setup_test_homeserver(self.addCleanup, *args, **kwargs) + hs = setup_test_homeserver(self.addCleanup, *args, **kwargs) + stor = hs.get_datastore() + + # Run the database background updates. + if hasattr(stor, "do_next_background_update"): + while not self.get_success(stor.has_completed_background_updates()): + self.get_success(stor.do_next_background_update(1)) + + return hs def pump(self, by=0.0): """ diff --git a/tox.ini b/tox.ini index a0f5486829..9b2d78ed6d 100644 --- a/tox.ini +++ b/tox.ini @@ -149,4 +149,5 @@ deps = codecov commands = coverage combine + coverage xml codecov -X gcov -- cgit 1.5.1 From be6a7e47fa75d72466a356c254376b8eb0707bb2 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 25 Jan 2019 10:23:51 +0000 Subject: Revert "Require event format version to parse or create events" --- changelog.d/4447.misc | 1 - changelog.d/4451.misc | 1 - synapse/events/__init__.py | 39 +---------------- synapse/events/builder.py | 51 +---------------------- synapse/federation/federation_base.py | 9 ++-- synapse/federation/federation_client.py | 74 ++++++++++++--------------------- synapse/federation/federation_server.py | 41 ++++++------------ synapse/federation/transport/server.py | 4 +- synapse/handlers/federation.py | 72 +++++++++++++------------------- synapse/handlers/message.py | 10 +---- synapse/replication/http/federation.py | 8 +--- synapse/replication/http/send_event.py | 8 +--- synapse/storage/events_worker.py | 8 +++- tests/storage/test_redaction.py | 5 +-- tests/storage/test_roommember.py | 3 +- tests/storage/test_state.py | 3 +- tests/test_visibility.py | 4 -- tests/utils.py | 3 +- 18 files changed, 91 insertions(+), 253 deletions(-) delete mode 100644 changelog.d/4447.misc delete mode 100644 changelog.d/4451.misc (limited to 'tests/storage') diff --git a/changelog.d/4447.misc b/changelog.d/4447.misc deleted file mode 100644 index 43f8963614..0000000000 --- a/changelog.d/4447.misc +++ /dev/null @@ -1 +0,0 @@ -Add infrastructure to support different event formats diff --git a/changelog.d/4451.misc b/changelog.d/4451.misc deleted file mode 100644 index 43f8963614..0000000000 --- a/changelog.d/4451.misc +++ /dev/null @@ -1 +0,0 @@ -Add infrastructure to support different event formats diff --git a/synapse/events/__init__.py b/synapse/events/__init__.py index c3e6caf597..888296933b 100644 --- a/synapse/events/__init__.py +++ b/synapse/events/__init__.py @@ -18,11 +18,7 @@ from distutils.util import strtobool import six -from synapse.api.constants import ( - KNOWN_EVENT_FORMAT_VERSIONS, - KNOWN_ROOM_VERSIONS, - EventFormatVersions, -) +from synapse.api.constants import EventFormatVersions from synapse.util.caches import intern_dict from synapse.util.frozenutils import freeze @@ -244,36 +240,3 @@ class FrozenEvent(EventBase): self.get("type", None), self.get("state_key", None), ) - - -def room_version_to_event_format(room_version): - """Converts a room version string to the event format - - Args: - room_version (str) - - Returns: - int - """ - if room_version not in KNOWN_ROOM_VERSIONS: - raise - - return EventFormatVersions.V1 - - -def event_type_from_format_version(format_version): - """Returns the python type to use to construct an Event object for the - given event format version. - - Args: - format_version (int): The event format version - - Returns: - type: A type that can be initialized as per the initializer of - `FrozenEvent` - """ - if format_version not in KNOWN_EVENT_FORMAT_VERSIONS: - raise Exception( - "No event format %r" % (format_version,) - ) - return FrozenEvent diff --git a/synapse/events/builder.py b/synapse/events/builder.py index 7e63371095..e662eaef10 100644 --- a/synapse/events/builder.py +++ b/synapse/events/builder.py @@ -15,39 +15,12 @@ import copy -from synapse.api.constants import RoomVersions from synapse.types import EventID from synapse.util.stringutils import random_string from . import EventBase, FrozenEvent, _event_dict_property -def get_event_builder(room_version, key_values={}, internal_metadata_dict={}): - """Generate an event builder appropriate for the given room version - - Args: - room_version (str): Version of the room that we're creating an - event builder for - key_values (dict): Fields used as the basis of the new event - internal_metadata_dict (dict): Used to create the `_EventInternalMetadata` - object. - - Returns: - EventBuilder - """ - if room_version in { - RoomVersions.V1, - RoomVersions.V2, - RoomVersions.VDH_TEST, - RoomVersions.STATE_V2_TEST, - }: - return EventBuilder(key_values, internal_metadata_dict) - else: - raise Exception( - "No event format defined for version %r" % (room_version,) - ) - - class EventBuilder(EventBase): def __init__(self, key_values={}, internal_metadata_dict={}): signatures = copy.deepcopy(key_values.pop("signatures", {})) @@ -85,29 +58,7 @@ class EventBuilderFactory(object): return e_id.to_string() - def new(self, room_version, key_values={}): - """Generate an event builder appropriate for the given room version - - Args: - room_version (str): Version of the room that we're creating an - event builder for - key_values (dict): Fields used as the basis of the new event - - Returns: - EventBuilder - """ - - # There's currently only the one event version defined - if room_version not in { - RoomVersions.V1, - RoomVersions.V2, - RoomVersions.VDH_TEST, - RoomVersions.STATE_V2_TEST, - }: - raise Exception( - "No event format defined for version %r" % (room_version,) - ) - + def new(self, key_values={}): key_values["event_id"] = self.create_event_id() time_now = int(self.clock.time_msec()) diff --git a/synapse/federation/federation_base.py b/synapse/federation/federation_base.py index 5c31e5f85f..d749bfdd3a 100644 --- a/synapse/federation/federation_base.py +++ b/synapse/federation/federation_base.py @@ -23,7 +23,7 @@ from twisted.internet.defer import DeferredList from synapse.api.constants import MAX_DEPTH, EventTypes, Membership from synapse.api.errors import Codes, SynapseError from synapse.crypto.event_signing import check_event_content_hash -from synapse.events import event_type_from_format_version +from synapse.events import FrozenEvent from synapse.events.utils import prune_event from synapse.http.servlet import assert_params_in_dict from synapse.types import get_domain_from_id @@ -302,12 +302,11 @@ def _is_invite_via_3pid(event): ) -def event_from_pdu_json(pdu_json, event_format_version, outlier=False): +def event_from_pdu_json(pdu_json, outlier=False): """Construct a FrozenEvent from an event json received over federation Args: pdu_json (object): pdu as received over federation - event_format_version (int): The event format version outlier (bool): True to mark this event as an outlier Returns: @@ -331,8 +330,8 @@ def event_from_pdu_json(pdu_json, event_format_version, outlier=False): elif depth > MAX_DEPTH: raise SynapseError(400, "Depth too large", Codes.BAD_JSON) - event = event_type_from_format_version(event_format_version)( - pdu_json, + event = FrozenEvent( + pdu_json ) event.internal_metadata.outlier = outlier diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 4b25f891ca..777deabdf7 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -38,7 +38,6 @@ from synapse.api.errors import ( SynapseError, ) from synapse.crypto.event_signing import add_hashes_and_signatures -from synapse.events import room_version_to_event_format from synapse.federation.federation_base import FederationBase, event_from_pdu_json from synapse.util import logcontext, unwrapFirstError from synapse.util.caches.expiringcache import ExpiringCache @@ -170,13 +169,13 @@ class FederationClient(FederationBase): @defer.inlineCallbacks @log_function - def backfill(self, dest, room_id, limit, extremities): + def backfill(self, dest, context, limit, extremities): """Requests some more historic PDUs for the given context from the given destination server. Args: dest (str): The remote home server to ask. - room_id (str): The room_id to backfill. + context (str): The context to backfill. limit (int): The maximum number of PDUs to return. extremities (list): List of PDU id and origins of the first pdus we have seen from the context @@ -191,15 +190,12 @@ class FederationClient(FederationBase): return transaction_data = yield self.transport_layer.backfill( - dest, room_id, extremities, limit) + dest, context, extremities, limit) logger.debug("backfill transaction_data=%s", repr(transaction_data)) - room_version = yield self.store.get_room_version(room_id) - format_ver = room_version_to_event_format(room_version) - pdus = [ - event_from_pdu_json(p, format_ver, outlier=False) + event_from_pdu_json(p, outlier=False) for p in transaction_data["pdus"] ] @@ -243,8 +239,6 @@ class FederationClient(FederationBase): pdu_attempts = self.pdu_destination_tried.setdefault(event_id, {}) - format_ver = room_version_to_event_format(room_version) - signed_pdu = None for destination in destinations: now = self._clock.time_msec() @@ -260,7 +254,7 @@ class FederationClient(FederationBase): logger.debug("transaction_data %r", transaction_data) pdu_list = [ - event_from_pdu_json(p, format_ver, outlier=outlier) + event_from_pdu_json(p, outlier=outlier) for p in transaction_data["pdus"] ] @@ -354,16 +348,12 @@ class FederationClient(FederationBase): destination, room_id, event_id=event_id, ) - room_version = yield self.store.get_room_version(room_id) - format_ver = room_version_to_event_format(room_version) - pdus = [ - event_from_pdu_json(p, format_ver, outlier=True) - for p in result["pdus"] + event_from_pdu_json(p, outlier=True) for p in result["pdus"] ] auth_chain = [ - event_from_pdu_json(p, format_ver, outlier=True) + event_from_pdu_json(p, outlier=True) for p in result.get("auth_chain", []) ] @@ -371,6 +361,8 @@ class FederationClient(FederationBase): ev.event_id for ev in itertools.chain(pdus, auth_chain) ]) + room_version = yield self.store.get_room_version(room_id) + signed_pdus = yield self._check_sigs_and_hash_and_fetch( destination, [p for p in pdus if p.event_id not in seen_events], @@ -469,14 +461,13 @@ class FederationClient(FederationBase): destination, room_id, event_id, ) - room_version = yield self.store.get_room_version(room_id) - format_ver = room_version_to_event_format(room_version) - auth_chain = [ - event_from_pdu_json(p, format_ver, outlier=True) + event_from_pdu_json(p, outlier=True) for p in res["auth_chain"] ] + room_version = yield self.store.get_room_version(room_id) + signed_auth = yield self._check_sigs_and_hash_and_fetch( destination, auth_chain, outlier=True, room_version=room_version, @@ -566,9 +557,9 @@ class FederationClient(FederationBase): params (dict[str, str|Iterable[str]]): Query parameters to include in the request. Return: - Deferred[tuple[str, FrozenEvent, int]]: resolves to a tuple of - `(origin, event, event_format)` where origin is the remote - homeserver which generated the event. + Deferred[tuple[str, FrozenEvent]]: resolves to a tuple of `origin` + and event where origin is the remote homeserver which generated + the event. Fails with a ``SynapseError`` if the chosen remote server returns a 300/400 code. @@ -588,11 +579,6 @@ class FederationClient(FederationBase): destination, room_id, user_id, membership, params, ) - # Note: If not supplied, the room version may be either v1 or v2, - # however either way the event format version will be v1. - room_version = ret.get("room_version", RoomVersions.V1) - event_format = room_version_to_event_format(room_version) - pdu_dict = ret.get("event", None) if not isinstance(pdu_dict, dict): raise InvalidResponseError("Bad 'event' field in response") @@ -612,7 +598,7 @@ class FederationClient(FederationBase): pdu_dict.pop("origin_server_ts", None) pdu_dict.pop("unsigned", None) - builder = self.event_builder_factory.new(room_version, pdu_dict) + builder = self.event_builder_factory.new(pdu_dict) add_hashes_and_signatures( builder, self.hs.hostname, @@ -621,14 +607,14 @@ class FederationClient(FederationBase): ev = builder.build() defer.returnValue( - (destination, ev, event_format) + (destination, ev) ) return self._try_destination_list( "make_" + membership, destinations, send_request, ) - def send_join(self, destinations, pdu, event_format_version): + def send_join(self, destinations, pdu): """Sends a join event to one of a list of homeservers. Doing so will cause the remote server to add the event to the graph, @@ -638,7 +624,6 @@ class FederationClient(FederationBase): destinations (str): Candidate homeservers which are probably participating in the room. pdu (BaseEvent): event to be sent - event_format_version (int): The event format version Return: Deferred: resolves to a dict with members ``origin`` (a string @@ -684,12 +669,12 @@ class FederationClient(FederationBase): logger.debug("Got content: %s", content) state = [ - event_from_pdu_json(p, event_format_version, outlier=True) + event_from_pdu_json(p, outlier=True) for p in content.get("state", []) ] auth_chain = [ - event_from_pdu_json(p, event_format_version, outlier=True) + event_from_pdu_json(p, outlier=True) for p in content.get("auth_chain", []) ] @@ -767,10 +752,7 @@ class FederationClient(FederationBase): logger.debug("Got response to send_invite: %s", pdu_dict) - room_version = yield self.store.get_room_version(room_id) - format_ver = room_version_to_event_format(room_version) - - pdu = event_from_pdu_json(pdu_dict, format_ver) + pdu = event_from_pdu_json(pdu_dict) # Check signatures are correct. pdu = yield self._check_sigs_and_hash(pdu) @@ -848,14 +830,13 @@ class FederationClient(FederationBase): content=send_content, ) - room_version = yield self.store.get_room_version(room_id) - format_ver = room_version_to_event_format(room_version) - auth_chain = [ - event_from_pdu_json(e, format_ver) + event_from_pdu_json(e) for e in content["auth_chain"] ] + room_version = yield self.store.get_room_version(room_id) + signed_auth = yield self._check_sigs_and_hash_and_fetch( destination, auth_chain, outlier=True, room_version=room_version, ) @@ -899,14 +880,13 @@ class FederationClient(FederationBase): timeout=timeout, ) - room_version = yield self.store.get_room_version(room_id) - format_ver = room_version_to_event_format(room_version) - events = [ - event_from_pdu_json(e, format_ver) + event_from_pdu_json(e) for e in content.get("events", []) ] + room_version = yield self.store.get_room_version(room_id) + signed_events = yield self._check_sigs_and_hash_and_fetch( destination, events, outlier=False, room_version=room_version, ) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 4aa04b9588..cb729c69ea 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -34,7 +34,6 @@ from synapse.api.errors import ( SynapseError, ) from synapse.crypto.event_signing import compute_event_signature -from synapse.events import room_version_to_event_format from synapse.federation.federation_base import FederationBase, event_from_pdu_json from synapse.federation.persistence import TransactionActions from synapse.federation.units import Edu, Transaction @@ -179,13 +178,14 @@ class FederationServer(FederationBase): continue try: - room_version = yield self.store.get_room_version(room_id) - format_ver = room_version_to_event_format(room_version) + # In future we will actually use the room version to parse the + # PDU into an event. + yield self.store.get_room_version(room_id) except NotFoundError: logger.info("Ignoring PDU for unknown room_id: %s", room_id) continue - event = event_from_pdu_json(p, format_ver) + event = event_from_pdu_json(p) pdus_by_room.setdefault(room_id, []).append(event) pdu_results = {} @@ -370,9 +370,7 @@ class FederationServer(FederationBase): @defer.inlineCallbacks def on_invite_request(self, origin, content, room_version): - format_ver = room_version_to_event_format(room_version) - - pdu = event_from_pdu_json(content, format_ver) + pdu = event_from_pdu_json(content) origin_host, _ = parse_server_name(origin) yield self.check_server_matches_acl(origin_host, pdu.room_id) ret_pdu = yield self.handler.on_invite_request(origin, pdu) @@ -380,12 +378,9 @@ class FederationServer(FederationBase): defer.returnValue({"event": ret_pdu.get_pdu_json(time_now)}) @defer.inlineCallbacks - def on_send_join_request(self, origin, content, room_id): + def on_send_join_request(self, origin, content): logger.debug("on_send_join_request: content: %s", content) - - room_version = yield self.store.get_room_version(room_id) - format_ver = room_version_to_event_format(room_version) - pdu = event_from_pdu_json(content, format_ver) + pdu = event_from_pdu_json(content) origin_host, _ = parse_server_name(origin) yield self.check_server_matches_acl(origin_host, pdu.room_id) @@ -405,22 +400,13 @@ class FederationServer(FederationBase): origin_host, _ = parse_server_name(origin) yield self.check_server_matches_acl(origin_host, room_id) pdu = yield self.handler.on_make_leave_request(room_id, user_id) - - room_version = yield self.store.get_room_version(room_id) - time_now = self._clock.time_msec() - defer.returnValue({ - "event": pdu.get_pdu_json(time_now), - "room_version": room_version, - }) + defer.returnValue({"event": pdu.get_pdu_json(time_now)}) @defer.inlineCallbacks - def on_send_leave_request(self, origin, content, room_id): + def on_send_leave_request(self, origin, content): logger.debug("on_send_leave_request: content: %s", content) - - room_version = yield self.store.get_room_version(room_id) - format_ver = room_version_to_event_format(room_version) - pdu = event_from_pdu_json(content, format_ver) + pdu = event_from_pdu_json(content) origin_host, _ = parse_server_name(origin) yield self.check_server_matches_acl(origin_host, pdu.room_id) @@ -466,14 +452,13 @@ class FederationServer(FederationBase): origin_host, _ = parse_server_name(origin) yield self.check_server_matches_acl(origin_host, room_id) - room_version = yield self.store.get_room_version(room_id) - format_ver = room_version_to_event_format(room_version) - auth_chain = [ - event_from_pdu_json(e, format_ver) + event_from_pdu_json(e) for e in content["auth_chain"] ] + room_version = yield self.store.get_room_version(room_id) + signed_auth = yield self._check_sigs_and_hash_and_fetch( origin, auth_chain, outlier=True, room_version=room_version, ) diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py index 67ae0212c3..4557a9e66e 100644 --- a/synapse/federation/transport/server.py +++ b/synapse/federation/transport/server.py @@ -469,7 +469,7 @@ class FederationSendLeaveServlet(BaseFederationServlet): @defer.inlineCallbacks def on_PUT(self, origin, content, query, room_id, event_id): - content = yield self.handler.on_send_leave_request(origin, content, room_id) + content = yield self.handler.on_send_leave_request(origin, content) defer.returnValue((200, content)) @@ -487,7 +487,7 @@ class FederationSendJoinServlet(BaseFederationServlet): def on_PUT(self, origin, content, query, context, event_id): # TODO(paul): assert that context/event_id parsed from path actually # match those given in content - content = yield self.handler.on_send_join_request(origin, content, context) + content = yield self.handler.on_send_join_request(origin, content) defer.returnValue((200, content)) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index a4b771049c..453d393ce1 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -1061,7 +1061,7 @@ class FederationHandler(BaseHandler): """ logger.debug("Joining %s to %s", joinee, room_id) - origin, event, event_format_version = yield self._make_and_verify_event( + origin, event = yield self._make_and_verify_event( target_hosts, room_id, joinee, @@ -1091,9 +1091,7 @@ class FederationHandler(BaseHandler): target_hosts.insert(0, origin) except ValueError: pass - ret = yield self.federation_client.send_join( - target_hosts, event, event_format_version, - ) + ret = yield self.federation_client.send_join(target_hosts, event) origin = ret["origin"] state = ret["state"] @@ -1166,18 +1164,13 @@ class FederationHandler(BaseHandler): """ event_content = {"membership": Membership.JOIN} - room_version = yield self.store.get_room_version(room_id) - - builder = self.event_builder_factory.new( - room_version, - { - "type": EventTypes.Member, - "content": event_content, - "room_id": room_id, - "sender": user_id, - "state_key": user_id, - } - ) + builder = self.event_builder_factory.new({ + "type": EventTypes.Member, + "content": event_content, + "room_id": room_id, + "sender": user_id, + "state_key": user_id, + }) try: event, context = yield self.event_creation_handler.create_new_client_event( @@ -1311,7 +1304,7 @@ class FederationHandler(BaseHandler): @defer.inlineCallbacks def do_remotely_reject_invite(self, target_hosts, room_id, user_id): - origin, event, event_format_version = yield self._make_and_verify_event( + origin, event = yield self._make_and_verify_event( target_hosts, room_id, user_id, @@ -1343,7 +1336,7 @@ class FederationHandler(BaseHandler): @defer.inlineCallbacks def _make_and_verify_event(self, target_hosts, room_id, user_id, membership, content={}, params=None): - origin, event, format_ver = yield self.federation_client.make_membership_event( + origin, pdu = yield self.federation_client.make_membership_event( target_hosts, room_id, user_id, @@ -1352,7 +1345,9 @@ class FederationHandler(BaseHandler): params=params, ) - logger.debug("Got response to make_%s: %s", membership, event) + logger.debug("Got response to make_%s: %s", membership, pdu) + + event = pdu # We should assert some things. # FIXME: Do this in a nicer way @@ -1360,7 +1355,7 @@ class FederationHandler(BaseHandler): assert(event.user_id == user_id) assert(event.state_key == user_id) assert(event.room_id == room_id) - defer.returnValue((origin, event, format_ver)) + defer.returnValue((origin, event)) @defer.inlineCallbacks @log_function @@ -1369,17 +1364,13 @@ class FederationHandler(BaseHandler): leave event for the room and return that. We do *not* persist or process it until the other server has signed it and sent it back. """ - room_version = yield self.store.get_room_version(room_id) - builder = self.event_builder_factory.new( - room_version, - { - "type": EventTypes.Member, - "content": {"membership": Membership.LEAVE}, - "room_id": room_id, - "sender": user_id, - "state_key": user_id, - } - ) + builder = self.event_builder_factory.new({ + "type": EventTypes.Member, + "content": {"membership": Membership.LEAVE}, + "room_id": room_id, + "sender": user_id, + "state_key": user_id, + }) event, context = yield self.event_creation_handler.create_new_client_event( builder=builder, @@ -2275,16 +2266,14 @@ class FederationHandler(BaseHandler): } if (yield self.auth.check_host_in_room(room_id, self.hs.hostname)): - room_version = yield self.store.get_room_version(room_id) - builder = self.event_builder_factory.new(room_version, event_dict) - + builder = self.event_builder_factory.new(event_dict) EventValidator().validate_new(builder) event, context = yield self.event_creation_handler.create_new_client_event( builder=builder ) event, context = yield self.add_display_name_to_third_party_invite( - room_version, event_dict, event, context + event_dict, event, context ) try: @@ -2315,18 +2304,14 @@ class FederationHandler(BaseHandler): Returns: Deferred: resolves (to None) """ - room_version = yield self.store.get_room_version(room_id) - - # NB: event_dict has a particular specced format we might need to fudge - # if we change event formats too much. - builder = self.event_builder_factory.new(room_version, event_dict) + builder = self.event_builder_factory.new(event_dict) event, context = yield self.event_creation_handler.create_new_client_event( builder=builder, ) event, context = yield self.add_display_name_to_third_party_invite( - room_version, event_dict, event, context + event_dict, event, context ) try: @@ -2346,8 +2331,7 @@ class FederationHandler(BaseHandler): yield member_handler.send_membership_event(None, event, context) @defer.inlineCallbacks - def add_display_name_to_third_party_invite(self, room_version, event_dict, - event, context): + def add_display_name_to_third_party_invite(self, event_dict, event, context): key = ( EventTypes.ThirdPartyInvite, event.content["third_party_invite"]["signed"]["token"] @@ -2371,7 +2355,7 @@ class FederationHandler(BaseHandler): # auth checks. If we need the invite and don't have it then the # auth check code will explode appropriately. - builder = self.event_builder_factory.new(room_version, event_dict) + builder = self.event_builder_factory.new(event_dict) EventValidator().validate_new(builder) event, context = yield self.event_creation_handler.create_new_client_event( builder=builder, diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 7aaa4fba33..a7cd779b02 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -278,15 +278,7 @@ class EventCreationHandler(object): """ yield self.auth.check_auth_blocking(requester.user.to_string()) - if event_dict["type"] == EventTypes.Create and event_dict["state_key"] == "": - room_version = event_dict["content"]["room_version"] - else: - try: - room_version = yield self.store.get_room_version(event_dict["room_id"]) - except NotFoundError: - raise AuthError(403, "Unknown room") - - builder = self.event_builder_factory.new(room_version, event_dict) + builder = self.event_builder_factory.new(event_dict) self.validator.validate_new(builder) diff --git a/synapse/replication/http/federation.py b/synapse/replication/http/federation.py index 2e16c69666..64a79da162 100644 --- a/synapse/replication/http/federation.py +++ b/synapse/replication/http/federation.py @@ -17,7 +17,7 @@ import logging from twisted.internet import defer -from synapse.events import event_type_from_format_version +from synapse.events import FrozenEvent from synapse.events.snapshot import EventContext from synapse.http.servlet import parse_json_object_from_request from synapse.replication.http._base import ReplicationEndpoint @@ -70,7 +70,6 @@ class ReplicationFederationSendEventsRestServlet(ReplicationEndpoint): event_payloads.append({ "event": event.get_pdu_json(), - "event_format_version": event.format_version, "internal_metadata": event.internal_metadata.get_dict(), "rejected_reason": event.rejected_reason, "context": serialized_context, @@ -95,12 +94,9 @@ class ReplicationFederationSendEventsRestServlet(ReplicationEndpoint): event_and_contexts = [] for event_payload in event_payloads: event_dict = event_payload["event"] - format_ver = content["event_format_version"] internal_metadata = event_payload["internal_metadata"] rejected_reason = event_payload["rejected_reason"] - - EventType = event_type_from_format_version(format_ver) - event = EventType(event_dict, internal_metadata, rejected_reason) + event = FrozenEvent(event_dict, internal_metadata, rejected_reason) context = yield EventContext.deserialize( self.store, event_payload["context"], diff --git a/synapse/replication/http/send_event.py b/synapse/replication/http/send_event.py index 3635015eda..5b52c91650 100644 --- a/synapse/replication/http/send_event.py +++ b/synapse/replication/http/send_event.py @@ -17,7 +17,7 @@ import logging from twisted.internet import defer -from synapse.events import event_type_from_format_version +from synapse.events import FrozenEvent from synapse.events.snapshot import EventContext from synapse.http.servlet import parse_json_object_from_request from synapse.replication.http._base import ReplicationEndpoint @@ -74,7 +74,6 @@ class ReplicationSendEventRestServlet(ReplicationEndpoint): payload = { "event": event.get_pdu_json(), - "event_format_version": event.format_version, "internal_metadata": event.internal_metadata.get_dict(), "rejected_reason": event.rejected_reason, "context": serialized_context, @@ -91,12 +90,9 @@ class ReplicationSendEventRestServlet(ReplicationEndpoint): content = parse_json_object_from_request(request) event_dict = content["event"] - format_ver = content["event_format_version"] internal_metadata = content["internal_metadata"] rejected_reason = content["rejected_reason"] - - EventType = event_type_from_format_version(format_ver) - event = EventType(event_dict, internal_metadata, rejected_reason) + event = FrozenEvent(event_dict, internal_metadata, rejected_reason) requester = Requester.deserialize(self.store, content["requester"]) context = yield EventContext.deserialize(self.store, content["context"]) diff --git a/synapse/storage/events_worker.py b/synapse/storage/events_worker.py index 0a0ca58fc4..599f892858 100644 --- a/synapse/storage/events_worker.py +++ b/synapse/storage/events_worker.py @@ -23,7 +23,7 @@ from twisted.internet import defer from synapse.api.constants import EventFormatVersions from synapse.api.errors import NotFoundError -from synapse.events import FrozenEvent, event_type_from_format_version # noqa: F401 +from synapse.events import FrozenEvent # these are only included to make the type annotations work from synapse.events.snapshot import EventContext # noqa: F401 from synapse.events.utils import prune_event @@ -412,7 +412,11 @@ class EventsWorkerStore(SQLBaseStore): # of a event format version, so it must be a V1 event. format_version = EventFormatVersions.V1 - original_ev = event_type_from_format_version(format_version)( + # TODO: When we implement new event formats we'll need to use a + # different event python type + assert format_version == EventFormatVersions.V1 + + original_ev = FrozenEvent( event_dict=d, internal_metadata_dict=internal_metadata, rejected_reason=rejected_reason, diff --git a/tests/storage/test_redaction.py b/tests/storage/test_redaction.py index 3957561b1e..02bf975fbf 100644 --- a/tests/storage/test_redaction.py +++ b/tests/storage/test_redaction.py @@ -18,7 +18,7 @@ from mock import Mock from twisted.internet import defer -from synapse.api.constants import EventTypes, Membership, RoomVersions +from synapse.api.constants import EventTypes, Membership from synapse.types import RoomID, UserID from tests import unittest @@ -52,7 +52,6 @@ class RedactionTestCase(unittest.TestCase): content = {"membership": membership} content.update(extra_content) builder = self.event_builder_factory.new( - RoomVersions.V1, { "type": EventTypes.Member, "sender": user.to_string(), @@ -75,7 +74,6 @@ class RedactionTestCase(unittest.TestCase): self.depth += 1 builder = self.event_builder_factory.new( - RoomVersions.V1, { "type": EventTypes.Message, "sender": user.to_string(), @@ -96,7 +94,6 @@ class RedactionTestCase(unittest.TestCase): @defer.inlineCallbacks def inject_redaction(self, room, event_id, user, reason): builder = self.event_builder_factory.new( - RoomVersions.V1, { "type": EventTypes.Redaction, "sender": user.to_string(), diff --git a/tests/storage/test_roommember.py b/tests/storage/test_roommember.py index 7fa2f4fd70..978c66133d 100644 --- a/tests/storage/test_roommember.py +++ b/tests/storage/test_roommember.py @@ -18,7 +18,7 @@ from mock import Mock from twisted.internet import defer -from synapse.api.constants import EventTypes, Membership, RoomVersions +from synapse.api.constants import EventTypes, Membership from synapse.types import RoomID, UserID from tests import unittest @@ -50,7 +50,6 @@ class RoomMemberStoreTestCase(unittest.TestCase): @defer.inlineCallbacks def inject_room_member(self, room, user, membership, replaces_state=None): builder = self.event_builder_factory.new( - RoomVersions.V1, { "type": EventTypes.Member, "sender": user.to_string(), diff --git a/tests/storage/test_state.py b/tests/storage/test_state.py index a1f99134dc..086a39d834 100644 --- a/tests/storage/test_state.py +++ b/tests/storage/test_state.py @@ -17,7 +17,7 @@ import logging from twisted.internet import defer -from synapse.api.constants import EventTypes, Membership, RoomVersions +from synapse.api.constants import EventTypes, Membership from synapse.storage.state import StateFilter from synapse.types import RoomID, UserID @@ -52,7 +52,6 @@ class StateStoreTestCase(tests.unittest.TestCase): @defer.inlineCallbacks def inject_state_event(self, room, sender, typ, state_key, content): builder = self.event_builder_factory.new( - RoomVersions.V1, { "type": typ, "sender": sender.to_string(), diff --git a/tests/test_visibility.py b/tests/test_visibility.py index 82d63ce00e..2eea3b098b 100644 --- a/tests/test_visibility.py +++ b/tests/test_visibility.py @@ -17,7 +17,6 @@ import logging from twisted.internet import defer from twisted.internet.defer import succeed -from synapse.api.constants import RoomVersions from synapse.events import FrozenEvent from synapse.visibility import filter_events_for_server @@ -125,7 +124,6 @@ class FilterEventsForServerTestCase(tests.unittest.TestCase): def inject_visibility(self, user_id, visibility): content = {"history_visibility": visibility} builder = self.event_builder_factory.new( - RoomVersions.V1, { "type": "m.room.history_visibility", "sender": user_id, @@ -146,7 +144,6 @@ class FilterEventsForServerTestCase(tests.unittest.TestCase): content = {"membership": membership} content.update(extra_content) builder = self.event_builder_factory.new( - RoomVersions.V1, { "type": "m.room.member", "sender": user_id, @@ -168,7 +165,6 @@ class FilterEventsForServerTestCase(tests.unittest.TestCase): if content is None: content = {"body": "testytest"} builder = self.event_builder_factory.new( - RoomVersions.V1, { "type": "m.room.message", "sender": user_id, diff --git a/tests/utils.py b/tests/utils.py index 2dfcb70a93..df73c539c3 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -26,7 +26,7 @@ from six.moves.urllib import parse as urlparse from twisted.internet import defer, reactor -from synapse.api.constants import EventTypes, RoomVersions +from synapse.api.constants import EventTypes from synapse.api.errors import CodeMessageException, cs_error from synapse.config.server import ServerConfig from synapse.federation.transport import server @@ -624,7 +624,6 @@ def create_room(hs, room_id, creator_id): event_creation_handler = hs.get_event_creation_handler() builder = event_builder_factory.new( - RoomVersions.V1, { "type": EventTypes.Create, "state_key": "", -- cgit 1.5.1 From 9770ed91c223051d940f773e0b1fa2746003eb4e Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 24 Jan 2019 09:28:16 +0000 Subject: Fix tests --- tests/storage/test_redaction.py | 5 ++++- tests/storage/test_roommember.py | 3 ++- tests/storage/test_state.py | 3 ++- tests/test_visibility.py | 4 ++++ tests/utils.py | 3 ++- 5 files changed, 14 insertions(+), 4 deletions(-) (limited to 'tests/storage') diff --git a/tests/storage/test_redaction.py b/tests/storage/test_redaction.py index 02bf975fbf..3957561b1e 100644 --- a/tests/storage/test_redaction.py +++ b/tests/storage/test_redaction.py @@ -18,7 +18,7 @@ from mock import Mock from twisted.internet import defer -from synapse.api.constants import EventTypes, Membership +from synapse.api.constants import EventTypes, Membership, RoomVersions from synapse.types import RoomID, UserID from tests import unittest @@ -52,6 +52,7 @@ class RedactionTestCase(unittest.TestCase): content = {"membership": membership} content.update(extra_content) builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": EventTypes.Member, "sender": user.to_string(), @@ -74,6 +75,7 @@ class RedactionTestCase(unittest.TestCase): self.depth += 1 builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": EventTypes.Message, "sender": user.to_string(), @@ -94,6 +96,7 @@ class RedactionTestCase(unittest.TestCase): @defer.inlineCallbacks def inject_redaction(self, room, event_id, user, reason): builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": EventTypes.Redaction, "sender": user.to_string(), diff --git a/tests/storage/test_roommember.py b/tests/storage/test_roommember.py index 978c66133d..7fa2f4fd70 100644 --- a/tests/storage/test_roommember.py +++ b/tests/storage/test_roommember.py @@ -18,7 +18,7 @@ from mock import Mock from twisted.internet import defer -from synapse.api.constants import EventTypes, Membership +from synapse.api.constants import EventTypes, Membership, RoomVersions from synapse.types import RoomID, UserID from tests import unittest @@ -50,6 +50,7 @@ class RoomMemberStoreTestCase(unittest.TestCase): @defer.inlineCallbacks def inject_room_member(self, room, user, membership, replaces_state=None): builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": EventTypes.Member, "sender": user.to_string(), diff --git a/tests/storage/test_state.py b/tests/storage/test_state.py index 086a39d834..a1f99134dc 100644 --- a/tests/storage/test_state.py +++ b/tests/storage/test_state.py @@ -17,7 +17,7 @@ import logging from twisted.internet import defer -from synapse.api.constants import EventTypes, Membership +from synapse.api.constants import EventTypes, Membership, RoomVersions from synapse.storage.state import StateFilter from synapse.types import RoomID, UserID @@ -52,6 +52,7 @@ class StateStoreTestCase(tests.unittest.TestCase): @defer.inlineCallbacks def inject_state_event(self, room, sender, typ, state_key, content): builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": typ, "sender": sender.to_string(), diff --git a/tests/test_visibility.py b/tests/test_visibility.py index 2eea3b098b..82d63ce00e 100644 --- a/tests/test_visibility.py +++ b/tests/test_visibility.py @@ -17,6 +17,7 @@ import logging from twisted.internet import defer from twisted.internet.defer import succeed +from synapse.api.constants import RoomVersions from synapse.events import FrozenEvent from synapse.visibility import filter_events_for_server @@ -124,6 +125,7 @@ class FilterEventsForServerTestCase(tests.unittest.TestCase): def inject_visibility(self, user_id, visibility): content = {"history_visibility": visibility} builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": "m.room.history_visibility", "sender": user_id, @@ -144,6 +146,7 @@ class FilterEventsForServerTestCase(tests.unittest.TestCase): content = {"membership": membership} content.update(extra_content) builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": "m.room.member", "sender": user_id, @@ -165,6 +168,7 @@ class FilterEventsForServerTestCase(tests.unittest.TestCase): if content is None: content = {"body": "testytest"} builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": "m.room.message", "sender": user_id, diff --git a/tests/utils.py b/tests/utils.py index df73c539c3..2dfcb70a93 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -26,7 +26,7 @@ from six.moves.urllib import parse as urlparse from twisted.internet import defer, reactor -from synapse.api.constants import EventTypes +from synapse.api.constants import EventTypes, RoomVersions from synapse.api.errors import CodeMessageException, cs_error from synapse.config.server import ServerConfig from synapse.federation.transport import server @@ -624,6 +624,7 @@ def create_room(hs, room_id, creator_id): event_creation_handler = hs.get_event_creation_handler() builder = event_builder_factory.new( + RoomVersions.V1, { "type": EventTypes.Create, "state_key": "", -- cgit 1.5.1 From 7072fe30846c47849c3cb142acef03ef3825a892 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Mon, 28 Jan 2019 15:43:32 +0000 Subject: Fix UPSERTs on SQLite 3.24+ (#4477) --- changelog.d/4306.misc | 2 +- changelog.d/4471.misc | 2 +- changelog.d/4477.misc | 1 + synapse/storage/_base.py | 10 ++++++++-- synapse/storage/engines/sqlite.py | 10 +++------- synapse/storage/monthly_active_users.py | 12 +++++++++--- tests/storage/test_base.py | 7 +++++-- tests/storage/test_monthly_active_users.py | 4 ++-- 8 files changed, 30 insertions(+), 18 deletions(-) create mode 100644 changelog.d/4477.misc (limited to 'tests/storage') diff --git a/changelog.d/4306.misc b/changelog.d/4306.misc index 7f48b02fbf..58130b6190 100644 --- a/changelog.d/4306.misc +++ b/changelog.d/4306.misc @@ -1 +1 @@ -Synapse will now take advantage of native UPSERT functionality in PostgreSQL 9.5+. +Synapse will now take advantage of native UPSERT functionality in PostgreSQL 9.5+ and SQLite 3.24+. diff --git a/changelog.d/4471.misc b/changelog.d/4471.misc index 7f48b02fbf..994801fd1e 100644 --- a/changelog.d/4471.misc +++ b/changelog.d/4471.misc @@ -1 +1 @@ -Synapse will now take advantage of native UPSERT functionality in PostgreSQL 9.5+. + Synapse will now take advantage of native UPSERT functionality in PostgreSQL 9.5+ and SQLite 3.24+. diff --git a/changelog.d/4477.misc b/changelog.d/4477.misc new file mode 100644 index 0000000000..58130b6190 --- /dev/null +++ b/changelog.d/4477.misc @@ -0,0 +1 @@ +Synapse will now take advantage of native UPSERT functionality in PostgreSQL 9.5+ and SQLite 3.24+. diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index f62f70b9f1..5109bc3e2e 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -27,7 +27,7 @@ from twisted.internet import defer from synapse.api.errors import StoreError from synapse.metrics.background_process_metrics import run_as_background_process -from synapse.storage.engines import PostgresEngine +from synapse.storage.engines import PostgresEngine, Sqlite3Engine from synapse.util.caches.descriptors import Cache from synapse.util.logcontext import LoggingContext, PreserveLoggingContext from synapse.util.stringutils import exception_to_unicode @@ -196,6 +196,12 @@ class SQLBaseStore(object): # A set of tables that are not safe to use native upserts in. self._unsafe_to_upsert_tables = {"user_ips"} + # We add the user_directory_search table to the blacklist on SQLite + # because the existing search table does not have an index, making it + # unsafe to use native upserts. + if isinstance(self.database_engine, Sqlite3Engine): + self._unsafe_to_upsert_tables.add("user_directory_search") + if self.database_engine.can_native_upsert: # Check ASAP (and then later, every 1s) to see if we have finished # background updates of tables that aren't safe to update. @@ -230,7 +236,7 @@ class SQLBaseStore(object): self._unsafe_to_upsert_tables.discard("user_ips") # If there's any tables left to check, reschedule to run. - if self._unsafe_to_upsert_tables: + if self.updates: self._clock.call_later( 15.0, run_as_background_process, diff --git a/synapse/storage/engines/sqlite.py b/synapse/storage/engines/sqlite.py index 31b8449ca1..059ab81055 100644 --- a/synapse/storage/engines/sqlite.py +++ b/synapse/storage/engines/sqlite.py @@ -33,14 +33,10 @@ class Sqlite3Engine(object): @property def can_native_upsert(self): """ - Do we support native UPSERTs? + Do we support native UPSERTs? This requires SQLite3 3.24+, plus some + more work we haven't done yet to tell what was inserted vs updated. """ - # SQLite3 3.24+ supports them, but empirically the unit tests don't work - # when its enabled. - # FIXME: Figure out what is wrong so we can re-enable native upserts - - # return self.module.sqlite_version_info >= (3, 24, 0) - return False + return self.module.sqlite_version_info >= (3, 24, 0) def check_database(self, txn): pass diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index d6fc8edd4c..9e7e09b8c1 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -197,15 +197,21 @@ class MonthlyActiveUsersStore(SQLBaseStore): if is_support: return - is_insert = yield self.runInteraction( + yield self.runInteraction( "upsert_monthly_active_user", self.upsert_monthly_active_user_txn, user_id ) - if is_insert: - self.user_last_seen_monthly_active.invalidate((user_id,)) + user_in_mau = self.user_last_seen_monthly_active.cache.get( + (user_id,), + None, + update_metrics=False + ) + if user_in_mau is None: self.get_monthly_active_count.invalidate(()) + self.user_last_seen_monthly_active.invalidate((user_id,)) + def upsert_monthly_active_user_txn(self, txn, user_id): """Updates or inserts monthly active user member diff --git a/tests/storage/test_base.py b/tests/storage/test_base.py index 452d76ddd5..f18db8c384 100644 --- a/tests/storage/test_base.py +++ b/tests/storage/test_base.py @@ -49,14 +49,17 @@ class SQLBaseStoreTestCase(unittest.TestCase): self.db_pool.runWithConnection = runWithConnection config = Mock() - config._enable_native_upserts = False + config._disable_native_upserts = True config.event_cache_size = 1 config.database_config = {"name": "sqlite3"} + engine = create_engine(config.database_config) + fake_engine = Mock(wraps=engine) + fake_engine.can_native_upsert = False hs = TestHomeServer( "test", db_pool=self.db_pool, config=config, - database_engine=create_engine(config.database_config), + database_engine=fake_engine, ) self.datastore = SQLBaseStore(None, hs) diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py index 9605301b59..d6569a82bb 100644 --- a/tests/storage/test_monthly_active_users.py +++ b/tests/storage/test_monthly_active_users.py @@ -18,12 +18,12 @@ from twisted.internet import defer from synapse.api.constants import UserTypes -from tests.unittest import HomeserverTestCase +from tests import unittest FORTY_DAYS = 40 * 24 * 60 * 60 -class MonthlyActiveUsersTestCase(HomeserverTestCase): +class MonthlyActiveUsersTestCase(unittest.HomeserverTestCase): def make_homeserver(self, reactor, clock): hs = self.setup_test_homeserver() -- cgit 1.5.1 From 3f189c902ea1146a497512049aa38fe9a0a91169 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Wed, 30 Jan 2019 10:53:17 +0000 Subject: Fix flake8 (#4519) --- changelog.d/4519.misc | 1 + synapse/_scripts/register_new_matrix_user.py | 4 +- synapse/handlers/directory.py | 4 +- synapse/handlers/federation.py | 2 +- synapse/push/clientformat.py | 2 +- synapse/storage/__init__.py | 2 +- synapse/storage/events.py | 168 +++++++++++++-------------- synapse/storage/events_worker.py | 2 +- tests/storage/test_background_update.py | 2 +- tests/storage/test_end_to_end_keys.py | 3 - tests/storage/test_keys.py | 3 - tests/storage/test_state.py | 3 - 12 files changed, 94 insertions(+), 102 deletions(-) create mode 100644 changelog.d/4519.misc (limited to 'tests/storage') diff --git a/changelog.d/4519.misc b/changelog.d/4519.misc new file mode 100644 index 0000000000..897e783d28 --- /dev/null +++ b/changelog.d/4519.misc @@ -0,0 +1 @@ +Fix code to comply with linting in PyFlakes 3.7.1. diff --git a/synapse/_scripts/register_new_matrix_user.py b/synapse/_scripts/register_new_matrix_user.py index 4c3abf06fe..6e93f5a0c6 100644 --- a/synapse/_scripts/register_new_matrix_user.py +++ b/synapse/_scripts/register_new_matrix_user.py @@ -46,7 +46,7 @@ def request_registration( # Get the nonce r = requests.get(url, verify=False) - if r.status_code is not 200: + if r.status_code != 200: _print("ERROR! Received %d %s" % (r.status_code, r.reason)) if 400 <= r.status_code < 500: try: @@ -84,7 +84,7 @@ def request_registration( _print("Sending registration request...") r = requests.post(url, json=data, verify=False) - if r.status_code is not 200: + if r.status_code != 200: _print("ERROR! Received %d %s" % (r.status_code, r.reason)) if 400 <= r.status_code < 500: try: diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index 0699731c13..6bb254f899 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -57,8 +57,8 @@ class DirectoryHandler(BaseHandler): # general association creation for both human users and app services for wchar in string.whitespace: - if wchar in room_alias.localpart: - raise SynapseError(400, "Invalid characters in room alias") + if wchar in room_alias.localpart: + raise SynapseError(400, "Invalid characters in room alias") if not self.hs.is_mine(room_alias): raise SynapseError(400, "Room alias must be local") diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index f89dabb9eb..083f2e0ac3 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -102,7 +102,7 @@ class FederationHandler(BaseHandler): self.hs = hs - self.store = hs.get_datastore() # type: synapse.storage.DataStore + self.store = hs.get_datastore() self.federation_client = hs.get_federation_client() self.state_handler = hs.get_state_handler() self.server_name = hs.hostname diff --git a/synapse/push/clientformat.py b/synapse/push/clientformat.py index ecbf364a5e..8bd96b1178 100644 --- a/synapse/push/clientformat.py +++ b/synapse/push/clientformat.py @@ -84,7 +84,7 @@ def _rule_to_template(rule): templaterule["pattern"] = thecond["pattern"] if unscoped_rule_id: - templaterule['rule_id'] = unscoped_rule_id + templaterule['rule_id'] = unscoped_rule_id if 'default' in rule: templaterule['default'] = rule['default'] return templaterule diff --git a/synapse/storage/__init__.py b/synapse/storage/__init__.py index 24329879e5..42cd3c83ad 100644 --- a/synapse/storage/__init__.py +++ b/synapse/storage/__init__.py @@ -317,7 +317,7 @@ class DataStore(RoomMemberStore, RoomStore, thirty_days_ago_in_secs)) for row in txn: - if row[0] is 'unknown': + if row[0] == 'unknown': pass results[row[0]] = row[1] diff --git a/synapse/storage/events.py b/synapse/storage/events.py index 3e1915fb87..81b250480d 100644 --- a/synapse/storage/events.py +++ b/synapse/storage/events.py @@ -904,106 +904,106 @@ class EventsStore(StateGroupWorkerStore, EventFederationStore, EventsWorkerStore def _update_current_state_txn(self, txn, state_delta_by_room, max_stream_order): for room_id, current_state_tuple in iteritems(state_delta_by_room): - to_delete, to_insert = current_state_tuple - - # First we add entries to the current_state_delta_stream. We - # do this before updating the current_state_events table so - # that we can use it to calculate the `prev_event_id`. (This - # allows us to not have to pull out the existing state - # unnecessarily). - sql = """ - INSERT INTO current_state_delta_stream - (stream_id, room_id, type, state_key, event_id, prev_event_id) - SELECT ?, ?, ?, ?, ?, ( - SELECT event_id FROM current_state_events - WHERE room_id = ? AND type = ? AND state_key = ? - ) - """ - txn.executemany(sql, ( - ( - max_stream_order, room_id, etype, state_key, None, - room_id, etype, state_key, - ) - for etype, state_key in to_delete - # We sanity check that we're deleting rather than updating - if (etype, state_key) not in to_insert - )) - txn.executemany(sql, ( - ( - max_stream_order, room_id, etype, state_key, ev_id, - room_id, etype, state_key, - ) - for (etype, state_key), ev_id in iteritems(to_insert) - )) + to_delete, to_insert = current_state_tuple - # Now we actually update the current_state_events table - - txn.executemany( - "DELETE FROM current_state_events" - " WHERE room_id = ? AND type = ? AND state_key = ?", - ( - (room_id, etype, state_key) - for etype, state_key in itertools.chain(to_delete, to_insert) - ), + # First we add entries to the current_state_delta_stream. We + # do this before updating the current_state_events table so + # that we can use it to calculate the `prev_event_id`. (This + # allows us to not have to pull out the existing state + # unnecessarily). + sql = """ + INSERT INTO current_state_delta_stream + (stream_id, room_id, type, state_key, event_id, prev_event_id) + SELECT ?, ?, ?, ?, ?, ( + SELECT event_id FROM current_state_events + WHERE room_id = ? AND type = ? AND state_key = ? ) - - self._simple_insert_many_txn( - txn, - table="current_state_events", - values=[ - { - "event_id": ev_id, - "room_id": room_id, - "type": key[0], - "state_key": key[1], - } - for key, ev_id in iteritems(to_insert) - ], + """ + txn.executemany(sql, ( + ( + max_stream_order, room_id, etype, state_key, None, + room_id, etype, state_key, ) - - txn.call_after( - self._curr_state_delta_stream_cache.entity_has_changed, - room_id, max_stream_order, + for etype, state_key in to_delete + # We sanity check that we're deleting rather than updating + if (etype, state_key) not in to_insert + )) + txn.executemany(sql, ( + ( + max_stream_order, room_id, etype, state_key, ev_id, + room_id, etype, state_key, ) + for (etype, state_key), ev_id in iteritems(to_insert) + )) - # Invalidate the various caches - - # Figure out the changes of membership to invalidate the - # `get_rooms_for_user` cache. - # We find out which membership events we may have deleted - # and which we have added, then we invlidate the caches for all - # those users. - members_changed = set( - state_key - for ev_type, state_key in itertools.chain(to_delete, to_insert) - if ev_type == EventTypes.Member - ) + # Now we actually update the current_state_events table - for member in members_changed: - self._invalidate_cache_and_stream( - txn, self.get_rooms_for_user_with_stream_ordering, (member,) - ) + txn.executemany( + "DELETE FROM current_state_events" + " WHERE room_id = ? AND type = ? AND state_key = ?", + ( + (room_id, etype, state_key) + for etype, state_key in itertools.chain(to_delete, to_insert) + ), + ) - for host in set(get_domain_from_id(u) for u in members_changed): - self._invalidate_cache_and_stream( - txn, self.is_host_joined, (room_id, host) - ) - self._invalidate_cache_and_stream( - txn, self.was_host_joined, (room_id, host) - ) + self._simple_insert_many_txn( + txn, + table="current_state_events", + values=[ + { + "event_id": ev_id, + "room_id": room_id, + "type": key[0], + "state_key": key[1], + } + for key, ev_id in iteritems(to_insert) + ], + ) + + txn.call_after( + self._curr_state_delta_stream_cache.entity_has_changed, + room_id, max_stream_order, + ) + + # Invalidate the various caches + + # Figure out the changes of membership to invalidate the + # `get_rooms_for_user` cache. + # We find out which membership events we may have deleted + # and which we have added, then we invlidate the caches for all + # those users. + members_changed = set( + state_key + for ev_type, state_key in itertools.chain(to_delete, to_insert) + if ev_type == EventTypes.Member + ) + for member in members_changed: self._invalidate_cache_and_stream( - txn, self.get_users_in_room, (room_id,) + txn, self.get_rooms_for_user_with_stream_ordering, (member,) ) + for host in set(get_domain_from_id(u) for u in members_changed): self._invalidate_cache_and_stream( - txn, self.get_room_summary, (room_id,) + txn, self.is_host_joined, (room_id, host) ) - self._invalidate_cache_and_stream( - txn, self.get_current_state_ids, (room_id,) + txn, self.was_host_joined, (room_id, host) ) + self._invalidate_cache_and_stream( + txn, self.get_users_in_room, (room_id,) + ) + + self._invalidate_cache_and_stream( + txn, self.get_room_summary, (room_id,) + ) + + self._invalidate_cache_and_stream( + txn, self.get_current_state_ids, (room_id,) + ) + def _update_forward_extremities_txn(self, txn, new_forward_extremities, max_stream_order): for room_id, new_extrem in iteritems(new_forward_extremities): diff --git a/synapse/storage/events_worker.py b/synapse/storage/events_worker.py index ebe1429acb..57dae324c7 100644 --- a/synapse/storage/events_worker.py +++ b/synapse/storage/events_worker.py @@ -220,7 +220,7 @@ class EventsWorkerStore(SQLBaseStore): defer.returnValue(events) def _invalidate_get_event_cache(self, event_id): - self._get_event_cache.invalidate((event_id,)) + self._get_event_cache.invalidate((event_id,)) def _get_events_from_cache(self, events, allow_rejected, update_metrics=True): """Fetch events from the caches diff --git a/tests/storage/test_background_update.py b/tests/storage/test_background_update.py index 81403727c5..5568a607c7 100644 --- a/tests/storage/test_background_update.py +++ b/tests/storage/test_background_update.py @@ -11,7 +11,7 @@ class BackgroundUpdateTestCase(unittest.TestCase): def setUp(self): hs = yield setup_test_homeserver( self.addCleanup - ) # type: synapse.server.HomeServer + ) self.store = hs.get_datastore() self.clock = hs.get_clock() diff --git a/tests/storage/test_end_to_end_keys.py b/tests/storage/test_end_to_end_keys.py index b83f7336d3..11fb8c0c19 100644 --- a/tests/storage/test_end_to_end_keys.py +++ b/tests/storage/test_end_to_end_keys.py @@ -20,9 +20,6 @@ import tests.utils class EndToEndKeyStoreTestCase(tests.unittest.TestCase): - def __init__(self, *args, **kwargs): - super(EndToEndKeyStoreTestCase, self).__init__(*args, **kwargs) - self.store = None # type: synapse.storage.DataStore @defer.inlineCallbacks def setUp(self): diff --git a/tests/storage/test_keys.py b/tests/storage/test_keys.py index 47f4a8ceac..0d2dc9f325 100644 --- a/tests/storage/test_keys.py +++ b/tests/storage/test_keys.py @@ -22,9 +22,6 @@ import tests.utils class KeyStoreTestCase(tests.unittest.TestCase): - def __init__(self, *args, **kwargs): - super(KeyStoreTestCase, self).__init__(*args, **kwargs) - self.store = None # type: synapse.storage.keys.KeyStore @defer.inlineCallbacks def setUp(self): diff --git a/tests/storage/test_state.py b/tests/storage/test_state.py index a1f99134dc..99cd3e09eb 100644 --- a/tests/storage/test_state.py +++ b/tests/storage/test_state.py @@ -28,9 +28,6 @@ logger = logging.getLogger(__name__) class StateStoreTestCase(tests.unittest.TestCase): - def __init__(self, *args, **kwargs): - super(StateStoreTestCase, self).__init__(*args, **kwargs) - self.store = None # type: synapse.storage.DataStore @defer.inlineCallbacks def setUp(self): -- cgit 1.5.1 From a06614bd2aba5d751e002433c88e8ba1ba02c50b Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Wed, 20 Feb 2019 23:03:30 +1100 Subject: UPSERT many functionality (#4644) --- .travis.yml | 2 +- changelog.d/4644.misc | 1 + synapse/storage/_base.py | 146 ++++++++++++++++++++++++++++++++++++++++---- tests/storage/test__base.py | 88 ++++++++++++++++++++++++++ 4 files changed, 224 insertions(+), 13 deletions(-) create mode 100644 changelog.d/4644.misc (limited to 'tests/storage') diff --git a/.travis.yml b/.travis.yml index d88f10324f..5d763123a0 100644 --- a/.travis.yml +++ b/.travis.yml @@ -89,7 +89,7 @@ install: - psql -At -U postgres -c 'select version();' || true - pip install tox - + # if we don't have python3.6 in this environment, travis unhelpfully gives us # a `python3.6` on our path which does nothing but spit out a warning. Tox # tries to run it (even if we're not running a py36 env), so the build logs diff --git a/changelog.d/4644.misc b/changelog.d/4644.misc new file mode 100644 index 0000000000..84137c3412 --- /dev/null +++ b/changelog.d/4644.misc @@ -0,0 +1 @@ +Introduce upsert batching functionality in the database layer. diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index f1a5366b95..3d895da43c 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -106,6 +106,14 @@ class LoggingTransaction(object): 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(lambda *x: execute_batch(self.txn, *x), sql, args) + else: + for val in args: + self.execute(sql, val) + def execute(self, sql, *args): self._do_execute(self.txn.execute, sql, *args) @@ -699,20 +707,34 @@ class SQLBaseStore(object): 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) - ) - sqlargs = list(values.values()) + list(keyvalues.values()) + if not values: + # If `values` is empty, then all of the values we care about are in + # the unique key, so there is nothing to UPDATE. We can just do a + # SELECT instead to see if it exists. + sql = "SELECT 1 FROM %s WHERE %s" % ( + table, + " 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()) - txn.execute(sql, sqlargs) - if txn.rowcount > 0: - # successfully updated 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 + # We didn't find any existing rows, so insert a new one allvalues = {} allvalues.update(keyvalues) allvalues.update(values) @@ -759,6 +781,106 @@ class SQLBaseStore(object): ) txn.execute(sql, list(allvalues.values())) + def _simple_upsert_many_txn( + self, txn, table, key_names, key_values, value_names, value_values + ): + """ + Upsert, many times. + + Args: + table (str): The table to upsert into + key_names (list[str]): The key column names. + key_values (list[list]): A list of each row's key column values. + value_names (list[str]): The value column names. If empty, no + values will be used, even if value_values is provided. + value_values (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 + ): + return self._simple_upsert_many_txn_native_upsert( + txn, table, key_names, key_values, value_names, value_values + ) + else: + return self._simple_upsert_many_txn_emulated( + txn, table, key_names, key_values, value_names, value_values + ) + + def _simple_upsert_many_txn_emulated( + self, txn, table, key_names, key_values, value_names, value_values + ): + """ + Upsert, many times, but without native UPSERT support or batching. + + Args: + table (str): The table to upsert into + key_names (list[str]): The key column names. + key_values (list[list]): A list of each row's key column values. + value_names (list[str]): The value column names. If empty, no + values will be used, even if value_values is provided. + value_values (list[list]): A list of each row's value column values. + Returns: + None + """ + # No value columns, therefore make a blank list so that the following + # zip() works correctly. + if not value_names: + value_values = [() for x in range(len(key_values))] + + for keyv, valv in zip(key_values, value_values): + _keys = {x: y for x, y in zip(key_names, keyv)} + _vals = {x: y for x, y in zip(value_names, valv)} + + self._simple_upsert_txn_emulated(txn, table, _keys, _vals) + + def _simple_upsert_many_txn_native_upsert( + self, txn, table, key_names, key_values, value_names, value_values + ): + """ + Upsert, many times, using batching where possible. + + Args: + table (str): The table to upsert into + key_names (list[str]): The key column names. + key_values (list[list]): A list of each row's key column values. + value_names (list[str]): The value column names. If empty, no + values will be used, even if value_values is provided. + value_values (list[list]): A list of each row's value column values. + Returns: + None + """ + allnames = [] + allnames.extend(key_names) + allnames.extend(value_names) + + if not value_names: + # No value columns, therefore make a blank list so that the + # following zip() works correctly. + latter = "NOTHING" + value_values = [() for x in range(len(key_values))] + else: + latter = ( + "UPDATE SET " + ", ".join(k + "=EXCLUDED." + k for k in value_names) + ) + + sql = "INSERT INTO %s (%s) VALUES (%s) ON CONFLICT (%s) DO %s" % ( + table, + ", ".join(k for k in allnames), + ", ".join("?" for _ in allnames), + ", ".join(key_names), + latter, + ) + + args = [] + + for x, y in zip(key_values, value_values): + args.append(tuple(x) + tuple(y)) + + return txn.execute_batch(sql, args) + 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/tests/storage/test__base.py b/tests/storage/test__base.py index 52eb05bfbf..dd49a14524 100644 --- a/tests/storage/test__base.py +++ b/tests/storage/test__base.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- # Copyright 2015, 2016 OpenMarket Ltd +# Copyright 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. @@ -314,3 +315,90 @@ class CacheDecoratorTestCase(unittest.TestCase): self.assertEquals(callcount[0], 2) self.assertEquals(callcount2[0], 3) + + +class UpsertManyTests(unittest.HomeserverTestCase): + def prepare(self, reactor, clock, hs): + self.storage = hs.get_datastore() + + self.table_name = "table_" + hs.get_secrets().token_hex(6) + self.get_success( + self.storage.runInteraction( + "create", + lambda x, *a: x.execute(*a), + "CREATE TABLE %s (id INTEGER, username TEXT, value TEXT)" + % (self.table_name,), + ) + ) + self.get_success( + self.storage.runInteraction( + "index", + lambda x, *a: x.execute(*a), + "CREATE UNIQUE INDEX %sindex ON %s(id, username)" + % (self.table_name, self.table_name), + ) + ) + + def _dump_to_tuple(self, res): + for i in res: + yield (i["id"], i["username"], i["value"]) + + def test_upsert_many(self): + """ + Upsert_many will perform the upsert operation across a batch of data. + """ + # Add some data to an empty table + key_names = ["id", "username"] + value_names = ["value"] + key_values = [[1, "user1"], [2, "user2"]] + value_values = [["hello"], ["there"]] + + self.get_success( + self.storage.runInteraction( + "test", + self.storage._simple_upsert_many_txn, + self.table_name, + key_names, + key_values, + value_names, + value_values, + ) + ) + + # Check results are what we expect + res = self.get_success( + self.storage._simple_select_list( + self.table_name, None, ["id, username, value"] + ) + ) + self.assertEqual( + set(self._dump_to_tuple(res)), + set([(1, "user1", "hello"), (2, "user2", "there")]), + ) + + # Update only user2 + key_values = [[2, "user2"]] + value_values = [["bleb"]] + + self.get_success( + self.storage.runInteraction( + "test", + self.storage._simple_upsert_many_txn, + self.table_name, + key_names, + key_values, + value_names, + value_values, + ) + ) + + # Check results are what we expect + res = self.get_success( + self.storage._simple_select_list( + self.table_name, None, ["id, username, value"] + ) + ) + self.assertEqual( + set(self._dump_to_tuple(res)), + set([(1, "user1", "hello"), (2, "user2", "bleb")]), + ) -- cgit 1.5.1