refactor: Fix several bugs in upgrade endpoint, update MSC4168 #1807
No reviewers
Labels
No labels
Blocked
Bug
Changelog
Added
Changelog
Missing
Changelog
None
Cherry-picking
Database
Dependencies
Dependencies/Renovate
Difficulty
Easy
Difficulty
Hard
Difficulty
Medium
Documentation
Enhancement
Good first issue
Help wanted
Inherited
Matrix/Administration
Matrix/Appservices
Matrix/Auth
Matrix/Client
Matrix/Core
Matrix/E2EE
Matrix/Federation
Matrix/Hydra
Matrix/MSC
Matrix/Media
Matrix/T&S
Merge
Merge/Manual
Merge/Squash
Meta
Meta/CI
Meta/Packaging
Priority
Blocking
Priority
High
Priority
Low
Security
Status
Confirmed
Status
Duplicate
Status
Invalid
Status
Needs Investigation
Support
Wont fix
old/ci/cd
old/rust
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
continuwuation/continuwuity!1807
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "nex/fix/room-upgrades"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This pull request changes major parts of the
POST /_matrix/client/v3/rooms/{roomId}/upgradeendpoint handler, which fixes several performance issues and bugs related to upgrading, and also updates the MSC4168: Updatem.space.*state on room upgrade implementation to fully implement the upstream proposal (the previous implementation was buggy and partial).Fixes #1806, fixes #1805
Pull request checklist:
mainbranch, and the branch is named something other thanmain.myself, if applicable. This includes ensuring code compiles.
senderin older room versions7c43a6336dde353d08da@ -46,0 +49,4 @@/// the new room ID.////// See: https://github.com/matrix-org/matrix-spec-proposals/pull/4168async fn msc4168_update_parent_spaces(I don't think these need the MSC prefix in the function name
idk i'd like some indication that the functionality is for an unstable proposal. realistically I'd like us to be able to actually do feature gating too but that's out of scope. Would something like
unstable_update_parent_spacessuffice?shrug, I think that having the MSC link in the doc comment is fine. saves us from having to rename the functions when the MSC becomes stable (or more likely forgetting to do that)
@ -69,0 +270,4 @@if !services.config.allow_unstable_room_versions&& UNSTABLE_ROOM_VERSIONS.contains(&body.new_version){return Err(Error::BadRequest(this could be combined with the conditional ahead instead of returning the same error message twice
I tried that but it looked ugly and hard to follow, I'd rather be explicit
@ -85,3 +293,1 @@// Note that this does internally call the policy server with a fake room ID,// which may not be good?let tombstone_test_result = services// Check tombstone permission by attempting to create (but not send) the event.why can't we check power levels?
forward extremities might mean the tombstone would soft fail (well more accurately we'd refuse to create the event anyway), so it's more efficient to just create and discard an event. Also technically has a micro advantage in that it'll warm up the cache for the room about to be tombstoned before it actually gets tombstoned
@ -230,0 +377,4 @@// NOTE: Using a hardcoded room ID for the temporary mutex means only one room// can be created at a time. This is actually beneficial, as it reduces the// risk of concurrent in-flight collisions.services.rooms.state.mutex.lock("!new-room").awaitthis needs to be refactored at some point in the future so we don't need to have a lock to create a room
I've got a branch I'm doing that in but it's a rather large refactor so this will make do for the time being
@ -46,0 +149,4 @@// In rooms that reference the old room via m.space.parent events...// NOTE: Doing that would be expensive. We'll instead fetch rooms which the// space claims are children.We would only need to do this for rooms which the upgrading user is joined to and has permission to send the state event to. There are a few other optimisations regarding the rules of what is a valid parent reference too. This is probably a later enhancement, though.
7e13d2fd115f88abf341