From e2ed1b7155bbd38d4a2752073056c112464b3644 Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Fri, 5 Aug 2022 16:59:09 +0200 Subject: Use literals in place of `HTTPStatus` constants in tests (#13463) --- tests/handlers/test_room_member.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'tests/handlers/test_room_member.py') diff --git a/tests/handlers/test_room_member.py b/tests/handlers/test_room_member.py index 254e7e4b80..b4e1405aee 100644 --- a/tests/handlers/test_room_member.py +++ b/tests/handlers/test_room_member.py @@ -1,4 +1,3 @@ -from http import HTTPStatus from unittest.mock import Mock, patch from twisted.test.proto_helpers import MemoryReactor @@ -260,7 +259,7 @@ class TestReplicatedJoinsLimitedByPerRoomRateLimiter(RedisMultiWorkerStreamTestC f"/_matrix/client/v3/rooms/{self.room_id}/join", access_token=self.bob_token, ) - self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) + self.assertEqual(channel.code, 200, channel.json_body) # wait for join to arrive over replication self.replicate() -- cgit 1.5.1 From f3fba4914d63bd9ee0573b61c9631b58ec263070 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 19 Aug 2022 08:25:24 -0400 Subject: Reduce the number of tests using TCP replication. (#13543) Uses Redis replication in additional test cases (instead of TCP replication). A small step towards dropping TCP replication. --- changelog.d/13543.misc | 1 + tests/handlers/test_room_member.py | 4 +- tests/module_api/test_api.py | 7 -- tests/replication/_base.py | 90 ++++++++--------------- tests/replication/tcp/test_handler.py | 4 +- tests/replication/test_sharded_event_persister.py | 7 -- 6 files changed, 36 insertions(+), 77 deletions(-) create mode 100644 changelog.d/13543.misc (limited to 'tests/handlers/test_room_member.py') diff --git a/changelog.d/13543.misc b/changelog.d/13543.misc new file mode 100644 index 0000000000..0300f46cd8 --- /dev/null +++ b/changelog.d/13543.misc @@ -0,0 +1 @@ +Reduce the number of tests using legacy TCP replication. diff --git a/tests/handlers/test_room_member.py b/tests/handlers/test_room_member.py index b4e1405aee..1d13ed1e88 100644 --- a/tests/handlers/test_room_member.py +++ b/tests/handlers/test_room_member.py @@ -14,7 +14,7 @@ from synapse.server import HomeServer from synapse.types import UserID, create_requester from synapse.util import Clock -from tests.replication._base import RedisMultiWorkerStreamTestCase +from tests.replication._base import BaseMultiWorkerStreamTestCase from tests.server import make_request from tests.test_utils import make_awaitable from tests.unittest import FederatingHomeserverTestCase, override_config @@ -216,7 +216,7 @@ class TestJoinsLimitedByPerRoomRateLimiter(FederatingHomeserverTestCase): # - trying to remote-join again. -class TestReplicatedJoinsLimitedByPerRoomRateLimiter(RedisMultiWorkerStreamTestCase): +class TestReplicatedJoinsLimitedByPerRoomRateLimiter(BaseMultiWorkerStreamTestCase): servlets = [ synapse.rest.admin.register_servlets, synapse.rest.client.login.register_servlets, diff --git a/tests/module_api/test_api.py b/tests/module_api/test_api.py index 106159fa65..02cef6f876 100644 --- a/tests/module_api/test_api.py +++ b/tests/module_api/test_api.py @@ -30,7 +30,6 @@ from tests.replication._base import BaseMultiWorkerStreamTestCase from tests.test_utils import simple_async_mock from tests.test_utils.event_injection import inject_member_event from tests.unittest import HomeserverTestCase, override_config -from tests.utils import USE_POSTGRES_FOR_TESTS class ModuleApiTestCase(HomeserverTestCase): @@ -738,11 +737,6 @@ class ModuleApiTestCase(HomeserverTestCase): class ModuleApiWorkerTestCase(BaseMultiWorkerStreamTestCase): """For testing ModuleApi functionality in a multi-worker setup""" - # Testing stream ID replication from the main to worker processes requires postgres - # (due to needing `MultiWriterIdGenerator`). - if not USE_POSTGRES_FOR_TESTS: - skip = "Requires Postgres" - servlets = [ admin.register_servlets, login.register_servlets, @@ -752,7 +746,6 @@ class ModuleApiWorkerTestCase(BaseMultiWorkerStreamTestCase): def default_config(self): conf = super().default_config() - conf["redis"] = {"enabled": "true"} conf["stream_writers"] = {"presence": ["presence_writer"]} conf["instance_map"] = { "presence_writer": {"host": "testserv", "port": 1001}, diff --git a/tests/replication/_base.py b/tests/replication/_base.py index 970d5e533b..ce53f808db 100644 --- a/tests/replication/_base.py +++ b/tests/replication/_base.py @@ -24,11 +24,11 @@ from synapse.http.site import SynapseRequest, SynapseSite from synapse.replication.http import ReplicationRestResource from synapse.replication.tcp.client import ReplicationDataHandler from synapse.replication.tcp.handler import ReplicationCommandHandler -from synapse.replication.tcp.protocol import ClientReplicationStreamProtocol -from synapse.replication.tcp.resource import ( - ReplicationStreamProtocolFactory, +from synapse.replication.tcp.protocol import ( + ClientReplicationStreamProtocol, ServerReplicationStreamProtocol, ) +from synapse.replication.tcp.resource import ReplicationStreamProtocolFactory from synapse.server import HomeServer from tests import unittest @@ -220,15 +220,34 @@ class BaseStreamTestCase(unittest.HomeserverTestCase): class BaseMultiWorkerStreamTestCase(unittest.HomeserverTestCase): """Base class for tests running multiple workers. + Enables Redis, providing a fake Redis server. + Automatically handle HTTP replication requests from workers to master, unlike `BaseStreamTestCase`. """ + if not hiredis: + skip = "Requires hiredis" + + if not USE_POSTGRES_FOR_TESTS: + # Redis replication only takes place on Postgres + skip = "Requires Postgres" + + def default_config(self) -> Dict[str, Any]: + """ + Overrides the default config to enable Redis. + Even if the test only uses make_worker_hs, the main process needs Redis + enabled otherwise it won't create a Fake Redis server to listen on the + Redis port and accept fake TCP connections. + """ + base = super().default_config() + base["redis"] = {"enabled": True} + return base + def setUp(self): super().setUp() # build a replication server - self.server_factory = ReplicationStreamProtocolFactory(self.hs) self.streamer = self.hs.get_replication_streamer() # Fake in memory Redis server that servers can connect to. @@ -247,15 +266,14 @@ class BaseMultiWorkerStreamTestCase(unittest.HomeserverTestCase): # handling inbound HTTP requests to that instance. self._hs_to_site = {self.hs: self.site} - if self.hs.config.redis.redis_enabled: - # Handle attempts to connect to fake redis server. - self.reactor.add_tcp_client_callback( - "localhost", - 6379, - self.connect_any_redis_attempts, - ) + # Handle attempts to connect to fake redis server. + self.reactor.add_tcp_client_callback( + "localhost", + 6379, + self.connect_any_redis_attempts, + ) - self.hs.get_replication_command_handler().start_replication(self.hs) + self.hs.get_replication_command_handler().start_replication(self.hs) # When we see a connection attempt to the master replication listener we # automatically set up the connection. This is so that tests don't @@ -339,27 +357,6 @@ class BaseMultiWorkerStreamTestCase(unittest.HomeserverTestCase): store = worker_hs.get_datastores().main store.db_pool._db_pool = self.database_pool._db_pool - # Set up TCP replication between master and the new worker if we don't - # have Redis support enabled. - if not worker_hs.config.redis.redis_enabled: - repl_handler = ReplicationCommandHandler(worker_hs) - client = ClientReplicationStreamProtocol( - worker_hs, - "client", - "test", - self.clock, - repl_handler, - ) - server = self.server_factory.buildProtocol( - IPv4Address("TCP", "127.0.0.1", 0) - ) - - client_transport = FakeTransport(server, self.reactor) - client.makeConnection(client_transport) - - server_transport = FakeTransport(client, self.reactor) - server.makeConnection(server_transport) - # Set up a resource for the worker resource = ReplicationRestResource(worker_hs) @@ -378,8 +375,7 @@ class BaseMultiWorkerStreamTestCase(unittest.HomeserverTestCase): reactor=self.reactor, ) - if worker_hs.config.redis.redis_enabled: - worker_hs.get_replication_command_handler().start_replication(worker_hs) + worker_hs.get_replication_command_handler().start_replication(worker_hs) return worker_hs @@ -582,27 +578,3 @@ class FakeRedisPubSubProtocol(Protocol): def connectionLost(self, reason): self._server.remove_subscriber(self) - - -class RedisMultiWorkerStreamTestCase(BaseMultiWorkerStreamTestCase): - """ - A test case that enables Redis, providing a fake Redis server. - """ - - if not hiredis: - skip = "Requires hiredis" - - if not USE_POSTGRES_FOR_TESTS: - # Redis replication only takes place on Postgres - skip = "Requires Postgres" - - def default_config(self) -> Dict[str, Any]: - """ - Overrides the default config to enable Redis. - Even if the test only uses make_worker_hs, the main process needs Redis - enabled otherwise it won't create a Fake Redis server to listen on the - Redis port and accept fake TCP connections. - """ - base = super().default_config() - base["redis"] = {"enabled": True} - return base diff --git a/tests/replication/tcp/test_handler.py b/tests/replication/tcp/test_handler.py index e6a19eafd5..1e299d2d67 100644 --- a/tests/replication/tcp/test_handler.py +++ b/tests/replication/tcp/test_handler.py @@ -12,10 +12,10 @@ # See the License for the specific language governing permissions and # limitations under the License. -from tests.replication._base import RedisMultiWorkerStreamTestCase +from tests.replication._base import BaseMultiWorkerStreamTestCase -class ChannelsTestCase(RedisMultiWorkerStreamTestCase): +class ChannelsTestCase(BaseMultiWorkerStreamTestCase): def test_subscribed_to_enough_redis_channels(self) -> None: # The default main process is subscribed to the USER_IP channel. self.assertCountEqual( diff --git a/tests/replication/test_sharded_event_persister.py b/tests/replication/test_sharded_event_persister.py index a7ca68069e..541d390286 100644 --- a/tests/replication/test_sharded_event_persister.py +++ b/tests/replication/test_sharded_event_persister.py @@ -20,7 +20,6 @@ from synapse.storage.util.id_generators import MultiWriterIdGenerator from tests.replication._base import BaseMultiWorkerStreamTestCase from tests.server import make_request -from tests.utils import USE_POSTGRES_FOR_TESTS logger = logging.getLogger(__name__) @@ -28,11 +27,6 @@ logger = logging.getLogger(__name__) class EventPersisterShardTestCase(BaseMultiWorkerStreamTestCase): """Checks event persisting sharding works""" - # Event persister sharding requires postgres (due to needing - # `MultiWriterIdGenerator`). - if not USE_POSTGRES_FOR_TESTS: - skip = "Requires Postgres" - servlets = [ admin.register_servlets_for_client_rest_resource, room.register_servlets, @@ -50,7 +44,6 @@ class EventPersisterShardTestCase(BaseMultiWorkerStreamTestCase): def default_config(self): conf = super().default_config() - conf["redis"] = {"enabled": "true"} conf["stream_writers"] = {"events": ["worker1", "worker2"]} conf["instance_map"] = { "worker1": {"host": "testserv", "port": 1001}, -- cgit 1.5.1 From 682dfcfc0db05d9c99b7615d950997535df4d533 Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 30 Aug 2022 11:58:38 +0200 Subject: Fix that user cannot `/forget` rooms after the last member has left (#13546) --- changelog.d/13546.bugfix | 1 + synapse/handlers/room_member.py | 7 ++- tests/handlers/test_room_member.py | 93 +++++++++++++++++++++++++++++++++++++- tests/storage/test_roommember.py | 4 +- 4 files changed, 99 insertions(+), 6 deletions(-) create mode 100644 changelog.d/13546.bugfix (limited to 'tests/handlers/test_room_member.py') diff --git a/changelog.d/13546.bugfix b/changelog.d/13546.bugfix new file mode 100644 index 0000000000..83bc3a61d2 --- /dev/null +++ b/changelog.d/13546.bugfix @@ -0,0 +1 @@ +Fix bug that user cannot `/forget` rooms after the last member has left the room. \ No newline at end of file diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 709682622f..e726997d83 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -1925,8 +1925,11 @@ class RoomMemberMasterHandler(RoomMemberHandler): ]: raise SynapseError(400, "User %s in room %s" % (user_id, room_id)) - if membership: - await self.store.forget(user_id, room_id) + # In normal case this call is only required if `membership` is not `None`. + # But: After the last member had left the room, the background update + # `_background_remove_left_rooms` is deleting rows related to this room from + # the table `current_state_events` and `get_current_state_events` is `None`. + await self.store.forget(user_id, room_id) def get_users_which_can_issue_invite(auth_events: StateMap[EventBase]) -> List[str]: diff --git a/tests/handlers/test_room_member.py b/tests/handlers/test_room_member.py index 1d13ed1e88..6bbfd5dc84 100644 --- a/tests/handlers/test_room_member.py +++ b/tests/handlers/test_room_member.py @@ -6,7 +6,7 @@ import synapse.rest.admin import synapse.rest.client.login import synapse.rest.client.room from synapse.api.constants import EventTypes, Membership -from synapse.api.errors import LimitExceededError +from synapse.api.errors import LimitExceededError, SynapseError from synapse.crypto.event_signing import add_hashes_and_signatures from synapse.events import FrozenEventV3 from synapse.federation.federation_client import SendJoinResult @@ -17,7 +17,11 @@ from synapse.util import Clock from tests.replication._base import BaseMultiWorkerStreamTestCase from tests.server import make_request from tests.test_utils import make_awaitable -from tests.unittest import FederatingHomeserverTestCase, override_config +from tests.unittest import ( + FederatingHomeserverTestCase, + HomeserverTestCase, + override_config, +) class TestJoinsLimitedByPerRoomRateLimiter(FederatingHomeserverTestCase): @@ -287,3 +291,88 @@ class TestReplicatedJoinsLimitedByPerRoomRateLimiter(BaseMultiWorkerStreamTestCa ), LimitExceededError, ) + + +class RoomMemberMasterHandlerTestCase(HomeserverTestCase): + servlets = [ + synapse.rest.admin.register_servlets, + synapse.rest.client.login.register_servlets, + synapse.rest.client.room.register_servlets, + ] + + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.handler = hs.get_room_member_handler() + self.store = hs.get_datastores().main + + # Create two users. + self.alice = self.register_user("alice", "pass") + self.alice_ID = UserID.from_string(self.alice) + self.alice_token = self.login("alice", "pass") + self.bob = self.register_user("bob", "pass") + self.bob_ID = UserID.from_string(self.bob) + self.bob_token = self.login("bob", "pass") + + # Create a room on this homeserver. + self.room_id = self.helper.create_room_as(self.alice, tok=self.alice_token) + + def test_leave_and_forget(self) -> None: + """Tests that forget a room is successfully. The test is performed with two users, + as forgetting by the last user respectively after all users had left the + is a special edge case.""" + self.helper.join(self.room_id, user=self.bob, tok=self.bob_token) + + # alice is not the last room member that leaves and forgets the room + self.helper.leave(self.room_id, user=self.alice, tok=self.alice_token) + self.get_success(self.handler.forget(self.alice_ID, self.room_id)) + self.assertTrue( + self.get_success(self.store.did_forget(self.alice, self.room_id)) + ) + + # the server has not forgotten the room + self.assertFalse( + self.get_success(self.store.is_locally_forgotten_room(self.room_id)) + ) + + def test_leave_and_forget_last_user(self) -> None: + """Tests that forget a room is successfully when the last user has left the room.""" + + # alice is the last room member that leaves and forgets the room + self.helper.leave(self.room_id, user=self.alice, tok=self.alice_token) + self.get_success(self.handler.forget(self.alice_ID, self.room_id)) + self.assertTrue( + self.get_success(self.store.did_forget(self.alice, self.room_id)) + ) + + # the server has forgotten the room + self.assertTrue( + self.get_success(self.store.is_locally_forgotten_room(self.room_id)) + ) + + def test_forget_when_not_left(self) -> None: + """Tests that a user cannot not forgets a room that has not left.""" + self.get_failure(self.handler.forget(self.alice_ID, self.room_id), SynapseError) + + def test_rejoin_forgotten_by_user(self) -> None: + """Test that a user that has forgotten a room can do a re-join. + The room was not forgotten from the local server. + One local user is still member of the room.""" + self.helper.join(self.room_id, user=self.bob, tok=self.bob_token) + + self.helper.leave(self.room_id, user=self.alice, tok=self.alice_token) + self.get_success(self.handler.forget(self.alice_ID, self.room_id)) + self.assertTrue( + self.get_success(self.store.did_forget(self.alice, self.room_id)) + ) + + # the server has not forgotten the room + self.assertFalse( + self.get_success(self.store.is_locally_forgotten_room(self.room_id)) + ) + + self.helper.join(self.room_id, user=self.alice, tok=self.alice_token) + # TODO: A join to a room does not invalidate the forgotten cache + # see https://github.com/matrix-org/synapse/issues/13262 + self.store.did_forget.invalidate_all() + self.assertFalse( + self.get_success(self.store.did_forget(self.alice, self.room_id)) + ) diff --git a/tests/storage/test_roommember.py b/tests/storage/test_roommember.py index ceec690285..8794401823 100644 --- a/tests/storage/test_roommember.py +++ b/tests/storage/test_roommember.py @@ -158,7 +158,7 @@ class RoomMemberStoreTestCase(unittest.HomeserverTestCase): # Check that alice's display name is now None self.assertEqual(row[0]["display_name"], None) - def test_room_is_locally_forgotten(self): + def test_room_is_locally_forgotten(self) -> None: """Test that when the last local user has forgotten a room it is known as forgotten.""" # join two local and one remote user self.room = self.helper.create_room_as(self.u_alice, tok=self.t_alice) @@ -199,7 +199,7 @@ class RoomMemberStoreTestCase(unittest.HomeserverTestCase): self.get_success(self.store.is_locally_forgotten_room(self.room)) ) - def test_join_locally_forgotten_room(self): + def test_join_locally_forgotten_room(self) -> None: """Tests if a user joins a forgotten room the room is not forgotten anymore.""" self.room = self.helper.create_room_as(self.u_alice, tok=self.t_alice) self.assertFalse( -- cgit 1.5.1