Change default room version to 12 (MSC4289, MSC4291) - #20155
Closed
barodeur wants to merge 23 commits into
Closed
Conversation
Implements the Matrix v1.16 default room version.
Collaborator
Author
|
Here is a little breakdown of the failing tests: 1. Unit tests: 52 failures, all pre-v12 assumptions in the tests themselves
2. Sliding sync list tests: a latent test-harness bug that v12 makes deterministicThis one is a bit funny. A few The cause is in the harness: Nobody noticed because under v11 the leaked room ID is random and never matches a room a later test creates. Under v12 the room ID is the hash of the create event, and under the fake clock the first room 3. Complement: one test that can't work with a v12 defaultAll three Complement jobs fail on exactly one test, The test creates its room at the server's default version, then forges its intentionally-bad event (a float in the content) with v5 event-building rules, since no v6+ builder will let a float through. The v5 builder validates |
The test harness caches parsed HomeServerConfig objects and hands each test a copy. deepcopy_config recursed into Config sub-objects but assigned every other attribute by reference, so mutable containers such as rooms_to_exclude_from_sync were shared between the cached template and every homeserver built from it. A test that mutates such a value in place (test_exclude_rooms_from_sync appends a room ID) leaked it into all later tests in the process. This went unnoticed because the leaked room IDs were random and never matched a room in a later test. With room version 12 as the default, room IDs are derived from the create event, which is identical from test to test under the fake clock, so the leaked ID matched the first room of every subsequent test and broke the sliding sync list tests.
test_auto_create_auto_join_room_preset_invalid_permissions checks that registration survives a failing auto-join invite by demoting the inviter's power level. In the auto-join flow the inviter is auto_join_user_id, which is also the user the room is created as, so the test necessarily demotes the room creator - which room version 12 forbids (MSC4289: creators have infinite power level and must not appear in power_levels users). The scenario is only possible in older room versions; the code path under test is room-version agnostic.
…vents_into_room The test only checks that a state event sent through the module API round-trips unchanged; the power levels content is a payload. Give the power level to a second user instead of the room creator, who must not appear in power_levels users in room version 12 (MSC4289).
test_not_enough_power stages the "no local user with enough power" case by having the creator demote themselves, which room version 12 forbids (MSC4289: creators have infinite power and cannot appear in power_levels users); pin it to room version 11, where it still walks the original code path. In room version 12 the creator is the only implicit admin, so the equivalent failure is a room whose creator has left. Add test_v12_room_creator_left for that branch of the servlet, asserting its distinct error message.
…uired_state_me The power levels event only exists so that user1 can send a custom state event; the creator (user2) doesn't need an explicit entry, and must not have one in room version 12 (MSC4289).
test_redaction_flag_ignored_for_user_if_banner_lacks_redaction_power and its kicker counterpart want a user who can ban/kick but cannot redact. Use a dedicated moderator at power level 75 for that instead of the room creator: creators are always allowed to redact, so the tests now exercise the power level comparison they were written for.
The fixture's power levels event only needs to grant the second user the ability to send state events; the creator doesn't need an entry.
…levels The power levels event in test_sent_event_end_up_in_room_state is only a trigger for the module callback; the creator, who sends it, doesn't need an explicit entry in users.
Rooms are now created with a tombstone power level of 150 rather than 100 (MSC4289, so that admins cannot upgrade a room out from under its creators). Grant the upgrading user 150 in test_power_levels and test_power_levels_user_default, and assert along the way that 100 is no longer sufficient.
The test checks that stringy power levels are converted to integers when a room is upgraded. Room creators are stripped from the power levels users map on upgrade to the current default room version, so put the stringy value on the other user instead.
The test cripples the room creator's power level so that nobody in the room can send a dummy event, which requires the creator to be an ordinary user; that is only the case in room versions before 12 (MSC4289). Add a small helper so the test can pick the room version without duplicating the room creation from prepare().
…left-room tests The override in test_server_left_room_after_us and test_we_cause_server_left_room only needs to let user1 send a state event; the creator (user2) doesn't need an entry.
…ation_event The create event is implicitly part of every event's auth chain, and listing it explicitly is rejected.
…sion 11 and 12 The test relies on two users being tied at power level 100 so that state resolution orders their power levels events by timestamp. Use a dedicated room creator so that neither of them is the creator (whose power level is infinite in room version 12), and only list the create event in auth_events for room versions that require it.
…ion_fails_in_public_room The test passed the public room's forward extremities as both prev_event_ids and auth_event_ids to create_event - for the private room's event too. Neither is a valid auth list, and the call invite check doesn't depend on them. Drop them so the public room call fails for the intended reason (call invites are not allowed in public rooms) rather than on the malformed auth list.
…PresenceJoinTestCase _add_new_user forges a remote join; the create event is implicitly part of the auth chain in the current default room version and must not be listed explicitly.
The fake remote server in MSC4293RedactOnBanKickTestCase hard-coded room version 11 when joining (?ver=), signing and parsing events, and listed the create event explicitly in the hand-built auth_events. Use the version of the room that was actually created, and leave the create event to be implied by the auth chain.
The error paths in `_get_events_from_db` (event in an unknown room, unknown room version, wrong event format, corrupted event) read the room ID from the raw event JSON. Create events in room version 12 and later have no `room_id` field, so for those events the paths raised `KeyError` instead of skipping the event or reporting the problem. This only matters when one of those paths is taken, which doesn't happen in normal operation: the main case is rolling back to a Synapse release that doesn't know a room version after having joined rooms of that version, where sync is meant to keep working and exclude those rooms. Take the room ID from the `events` table row instead, which is always populated.
…default room version The remote room DAG was built as room version 10 with a domain-style room ID and a creator field on the create event, while some of its events were built with the server's default room version. Build the whole DAG with the default room version instead: the room ID is derived from the create event, and the create event is implicit in each event's auth chain.
…ith the default room version The hand-built create event carried a domain-style room ID and a creator field. Build it for the default room version instead: the room ID is derived from the create event, which is implicit in the join event's auth chain.
The test exercises the deprecated __getitem__ on events; create events don't carry a room_id key, so look up sender instead.
Collaborator
Author
|
Closing this PR, it duplicates #20130 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #19414 (Matrix v1.16 support).
Change the default room version for newly created rooms from 11 to 12.
Room version 12 was introduced in Matrix v1.16 and implements MSC4289 (explicit room creators with infinite power level) and MSC4291 (room IDs derived from the create event hash). The spec states that servers SHOULD use room version 12 as the default when creating new rooms. Synapse has supported v12 for a while but still defaults to v11.
Changes TBD:
DEFAULT_ROOM_VERSIONto"12", update thedefault_room_versionconfig schema and regenerate the config documentation.Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.