MSC3818: Copy room type on upgrade #3818
Merged
Conversation
This file contains 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
Signed-off-by: Aminda Suomalainen <[email protected]>
Signed-off-by: Aminda Suomalainen <[email protected]>
Mikaela
changed the title
Signed-off-by: Aminda Suomalainen <[email protected]>
Mikaela
force-pushed
the
copy-room-type-on-upgrade
branch
from
9c29084
to
ffb4274
Compare
…and what to change Signed-off-by: Aminda Suomalainen <[email protected]>
Signed-off-by: Aminda Suomalainen <[email protected]> Co-authored-by: Andy Balaam <[email protected]>
clokep
reviewed
May 18, 2022
turt2live
added
proposal
Signed-off-by: Aminda Suomalainen <[email protected]> Co-authored-by: Patrick Cloke <[email protected]>
Signed-off-by: Aminda Suomalainen <[email protected]>
Signed-off-by: Aminda Suomalainen <[email protected]>
FSG-Cat
reviewed
May 18, 2022
FSG-Cat
reviewed
May 18, 2022
turt2live
removed
the
needs-implementation
FSG-Cat
reviewed
May 18, 2022
Mikaela
commented
May 19, 2022
KitsuneRal
approved these changes
May 22, 2022
squahtx
pushed a commit
to matrix-org/synapse
that referenced
this pull request
May 24, 2022
Synapse 1.60.0rc1 (2022-05-24) ============================== This release of Synapse adds a unique index to the `state_group_edges` table, in order to prevent accidentally introducing duplicate information (for example, because a database backup was restored multiple times). If your Synapse database already has duplicate rows in this table, this could fail with an error and require manual remediation. Additionally, the signature of the `check_event_for_spam` module callback has changed. The previous signature has been deprecated and remains working for now. Module authors should update their modules to use the new signature where possible. See [the upgrade notes](https://github.com/matrix-org/synapse/blob/develop/docs/upgrade.md#upgrading-to-v1600) for more details. Features -------- - Measure the time taken in spam-checking callbacks and expose those measurements as metrics. ([\#12513](#12513)) - Add a `default_power_level_content_override` config option to set default room power levels per room preset. ([\#12618](#12618)) - Add support for [MSC3787: Allowing knocks to restricted rooms](matrix-org/matrix-spec-proposals#3787). ([\#12623](#12623)) - Send `USER_IP` commands on a different Redis channel, in order to reduce traffic to workers that do not process these commands. ([\#12672](#12672), [\#12809](#12809)) - Synapse will now reload [cache config](https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html#caching) when it receives a [SIGHUP](https://en.wikipedia.org/wiki/SIGHUP) signal. ([\#12673](#12673)) - Add a config options to allow for auto-tuning of caches. ([\#12701](#12701)) - Update [MSC2716](matrix-org/matrix-spec-proposals#2716) implementation to process marker events from the current state to avoid markers being lost in timeline gaps for federated servers which would cause the imported history to be undiscovered. ([\#12718](#12718)) - Add a `drop_federated_event` callback to `SpamChecker` to disregard inbound federated events before they take up much processing power, in an emergency. ([\#12744](#12744)) - Implement [MSC3818: Copy room type on upgrade](matrix-org/matrix-spec-proposals#3818). ([\#12786](#12786), [\#12792](#12792)) - Update to the `check_event_for_spam` module callback. Deprecate the current callback signature, replace it with a new signature that is both less ambiguous (replacing booleans with explicit allow/block) and more powerful (ability to return explicit error codes). ([\#12808](#12808)) Bugfixes -------- - Fix a bug introduced in Synapse 1.7.0 that would prevent events from being sent to clients if there's a retention policy in the room when the support for retention policies is disabled. ([\#12611](#12611)) - Fix a bug introduced in Synapse 1.57.0 where `/messages` would throw a 500 error when querying for a non-existent room. ([\#12683](#12683)) - Add a unique index to `state_group_edges` to prevent duplicates being accidentally introduced and the consequential impact to performance. ([\#12687](#12687)) - Fix a long-standing bug where an empty room would be created when a user with an insufficient power level tried to upgrade a room. ([\#12696](#12696)) - Fix a bug introduced in Synapse 1.30.0 where empty rooms could be automatically created if a monthly active users limit is set. ([\#12713](#12713)) - Fix push to dismiss notifications when read on another client. Contributed by @SpiritCroc @ Beeper. ([\#12721](#12721)) - Fix poor database performance when reading the cache invalidation stream for large servers with lots of workers. ([\#12747](#12747)) - Delete events from the `federation_inbound_events_staging` table when a room is purged through the admin API. ([\#12770](#12770)) - Give a meaningful error message when a client tries to create a room with an invalid alias localpart. ([\#12779](#12779)) - Fix a bug introduced in 1.43.0 where a file (`providers.json`) was never closed. Contributed by @arkamar. ([\#12794](#12794)) - Fix a long-standing bug where finished log contexts would be re-started when failing to contact remote homeservers. ([\#12803](#12803)) - Fix a bug, introduced in Synapse 1.21.0, that led to media thumbnails being unusable before the index has been added in the background. ([\#12823](#12823)) Updates to the Docker image --------------------------- - Fix the docker file after a dependency update. ([\#12853](#12853)) Improved Documentation ---------------------- - Fix a typo in the Media Admin API documentation. ([\#12715](#12715)) - Update the OpenID Connect example for Keycloak to be compatible with newer versions of Keycloak. Contributed by @nhh. ([\#12727](#12727)) - Fix typo in server listener documentation. ([\#12742](#12742)) - Link to the configuration manual from the welcome page of the documentation. ([\#12748](#12748)) - Fix typo in `run_background_tasks_on` option name in configuration manual documentation. ([\#12749](#12749)) - Add information regarding the `rc_invites` ratelimiting option to the configuration docs. ([\#12759](#12759)) - Add documentation for cancellation of request processing. ([\#12761](#12761)) - Recommend using docker to run tests against postgres. ([\#12765](#12765)) - Add missing user directory endpoint from the generic worker documentation. Contributed by @olmari. ([\#12773](#12773)) - Add additional info to documentation of config option `cache_autotuning`. ([\#12776](#12776)) - Update configuration manual documentation to document size-related suffixes. ([\#12777](#12777)) - Fix invalid YAML syntax in the example documentation for the `url_preview_accept_language` config option. ([\#12785](#12785)) Deprecations and Removals ------------------------- - Require a body in POST requests to `/rooms/{roomId}/receipt/{receiptType}/{eventId}`, as required by the [Matrix specification](https://spec.matrix.org/v1.2/client-server-api/#post_matrixclientv3roomsroomidreceiptreceipttypeeventid). This breaks compatibility with Element Android 1.2.0 and earlier: users of those clients will be unable to send read receipts. ([\#12709](#12709)) Internal Changes ---------------- - Improve event caching mechanism to avoid having multiple copies of an event in memory at a time. ([\#10533](#10533)) - Preparation for faster-room-join work: return subsets of room state which we already have, immediately. ([\#12498](#12498)) - Add `@cancellable` decorator, for use on endpoint methods that can be cancelled when clients disconnect. ([\#12586](#12586), [\#12588](#12588), [\#12630](#12630), [\#12694](#12694), [\#12698](#12698), [\#12699](#12699), [\#12700](#12700), [\#12705](#12705)) - Enable cancellation of `GET /rooms/$room_id/members`, `GET /rooms/$room_id/state` and `GET /rooms/$room_id/state/$event_type/*` requests. ([\#12708](#12708)) - Improve documentation of the `synapse.push` module. ([\#12676](#12676)) - Refactor functions to on `PushRuleEvaluatorForEvent`. ([\#12677](#12677)) - Preparation for database schema simplifications: stop writing to `event_reference_hashes`. ([\#12679](#12679)) - Remove code which updates unused database column `application_services_state.last_txn`. ([\#12680](#12680)) - Refactor `EventContext` class. ([\#12689](#12689)) - Remove an unneeded class in the push code. ([\#12691](#12691)) - Consolidate parsing of relation information from events. ([\#12693](#12693)) - Convert namespace class `Codes` into a string enum. ([\#12703](#12703)) - Optimize private read receipt filtering. ([\#12711](#12711)) - Drop the logging level of status messages for the URL preview cache expiry job from INFO to DEBUG. ([\#12720](#12720)) - Downgrade some OIDC errors to warnings in the logs, to reduce the noise of Sentry reports. ([\#12723](#12723)) - Update configs used by Complement to allow more invites/3PID validations during tests. ([\#12731](#12731)) - Fix a long-standing bug where the user directory background process would fail to make forward progress if a user included a null codepoint in their display name or avatar. ([\#12762](#12762)) - Tweak the mypy plugin so that `@cached` can accept `on_invalidate=None`. ([\#12769](#12769)) - Move methods that call `add_push_rule` to the `PushRuleStore` class. ([\#12772](#12772)) - Make handling of federation Authorization header (more) compliant with RFC7230. ([\#12774](#12774)) - Refactor `resolve_state_groups_for_events` to not pull out full state when no state resolution happens. ([\#12775](#12775)) - Do not keep going if there are 5 back-to-back background update failures. ([\#12781](#12781)) - Fix federation when using the demo scripts. ([\#12783](#12783)) - The `hash_password` script now fails when it is called without specifying a config file. Contributed by @jae1911. ([\#12789](#12789)) - Improve and fix type hints. ([\#12567](#12567), [\#12477](#12477), [\#12717](#12717), [\#12753](#12753), [\#12695](#12695), [\#12734](#12734), [\#12716](#12716), [\#12726](#12726), [\#12790](#12790), [\#12833](#12833)) - Update EventContext `get_current_event_ids` and `get_prev_event_ids` to accept state filters and update calls where possible. ([\#12791](#12791)) - Remove Caddy from the Synapse workers image used in Complement. ([\#12818](#12818)) - Add Complement's shared registration secret to the Complement worker image. This fixes tests that depend on it. ([\#12819](#12819)) - Support registering Application Services when running with workers under Complement. ([\#12826](#12826)) - Disable 'faster room join' Complement tests when testing against Synapse with workers. ([\#12842](#12842))
anoadragon453
approved these changes
Jul 19, 2022
There was a problem hiding this comment.
LGTM for the most part. Note that whether this field is copied over is an implementation detail today, and Synapse will already copy it over. That being said, as the type can be quite crucial to the functionality of the room, mandating it to be copied over makes sense in a lot of ways.
I think this is ready for a review from a wider audience, so I'm going to push it forward.
@mscbot fcp merge
Comment on lines
+3
to
+4
| Unless the room upgrade API specifies that room type must be copied over, clients cannot rely on | ||
| rooms staying the same type leading to trouble. |
There was a problem hiding this comment.
Note that clients still cannot rely on the homeserver copying over the type field unless the homeserver advertises support for the spec version which this MSC makes it into. It may be worth mentioning that under the Unstable Prefix header (even though this MSC doesn't specify any unstable prefixes) or perhaps adding a Client Considerations header.
Clients may consider waiting until the supported spec version is advertised before showing a button, or perhaps prompt users with an "Are you sure?". Users who are aware of whether their homeserver has implemented experimental support for the feature can carry on ahead, whilst the casual user would be warned against potentially breaking the room.
Comment on lines
+9
to
+14
| This MSC proposes that the room upgade API MUST copy the [room type](https://spec.matrix.org/v1.2/client-server-api/#types) | ||
| over to the new room. Otherwise clients cannot trust that to happen and [Spaces](https://spec.matrix.org/v1.2/client-server-api/#spaces) | ||
| or [MSC3588](https://github.com/matrix-org/matrix-spec-proposals/pull/3588) Story rooms may incorrectly become | ||
| normal rooms breaking user-experience. | ||
|
|
||
| The Spec currently specfies this in [section 11.32.3. server behaviour](https://spec.matrix.org/v1.2/client-server-api/#server-behaviour-16): |
There was a problem hiding this comment.
Suggested change
| This MSC proposes that the room upgade API MUST copy the [room type](https://spec.matrix.org/v1.2/client-server-api/#types) | |
| over to the new room. Otherwise clients cannot trust that to happen and [Spaces](https://spec.matrix.org/v1.2/client-server-api/#spaces) | |
| or [MSC3588](https://github.com/matrix-org/matrix-spec-proposals/pull/3588) Story rooms may incorrectly become | |
| normal rooms breaking user-experience. | |
| The Spec currently specfies this in [section 11.32.3. server behaviour](https://spec.matrix.org/v1.2/client-server-api/#server-behaviour-16): | |
| This MSC proposes that the room upgade API MUST copy the [room type](https://spec.matrix.org/v1.3/client-server-api/#types) | |
| over to the new room. Otherwise clients cannot trust that to happen and [Spaces](https://spec.matrix.org/v1.3/client-server-api/#spaces) | |
| or [MSC3588](https://github.com/matrix-org/matrix-spec-proposals/pull/3588) Story rooms may incorrectly become | |
| normal rooms breaking user-experience. | |
| The Spec currently specfies this in [section 11.32.3. server behaviour](https://spec.matrix.org/v1.3/client-server-api/#server-behaviour-16): |
Just so we have the latest state available. The anchor points continue to work on the v1.3 versions of each link.
| > `type` field set to what it was in the previous room (if it was set), and the applicable `room_version`. | ||
|
|
||
|
|
||
| ## Potential issues |
There was a problem hiding this comment.
Another potential issue is that this will prevent a room from ever being upgraded from one type to another. For instance, if you had a room on com.example.my_feature.v1, it could not be upgraded to a com.example.my_feature.v2-type room.
Arguably though, this could be worked around by manually upgrading the room (create a new room, copy state over, send a tombstone in the old room). And I would consider the above an edge case.
|
Team member @anoadragon453 has proposed to merge this. The next step is review by the rest of the tagged people: Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for information about what commands tagged team members can give me. |
mscbot
added
disposition-merge
proposed-final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
mscbot
added
final-comment-period
|
The final comment period, with a disposition to merge, as per the review above, is now complete. |
mscbot
added
finished-final-comment-period
and removed
disposition-merge
final-comment-period
turt2live
added
spec-pr-missing
turt2live
added a commit
to matrix-org/matrix-spec
that referenced
this pull request
Aug 2, 2022
|
Spec PR: matrix-org/matrix-spec#1198 |
turt2live
added
spec-pr-in-review
turt2live
added a commit
to matrix-org/matrix-spec
that referenced
this pull request
Aug 3, 2022
|
Merged 🎉 |
turt2live
added
merged
54 tasks
14 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This or equivalent MSC is blocking element-hq/element-meta#1961
and matrix-org/synapse#11896.Rendered
Resolves: matrix-org/matrix-spec#911
Signed-off-by: Aminda Suomalainen
Implementations: matrix-org/synapse#12786
FCP proposal: #3818 (comment)