refactor: Clean up migration logic #2310
No reviewers
Labels
No labels
Abandoned
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
bob the builder
No milestone
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
continuwuation/continuwuity!2310
Loading…
Reference in a new issue
No description provided.
Delete branch "ginger/migration-cleanup"
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 cleans up the extremely ugly and footgun-prone database migration logic to avoid future silly mistakes.
Pull request checklist:
mainbranch, and the branch is named something other thanmain.myself, if applicable. This includes ensuring code compiles.
@ -44,0 +150,4 @@services.server.runtime().spawn(async move {unembed_unsigned_info(metadata, timeline).await.expect("unembed_unsigned_info failed! your database is broken, sorry >.>",This error message is unhelpful and also, unless expect() panics with the context of the result, masks the actual error message
expectprints the error payload, yes. like you said yourself, the only way forunembed_unsigned_infoto fail is if you have a PDU with no raw ID, in which case your DB is irreparably hosedactually
unembed_unsigned_infocan only fail by panicking so I can just remove this entirely@ -44,0 +126,4 @@migration!(split_userid_password),// This is backwards compatible because old versions will recreate the keyspace if it's// missing.migration!("drop_roomsynctoken_shortstatehash" => obliterate_roomsynctoken_shortstatehash_with_extreme_prejudice),nit: no const? kinda inconsistent
no const is necessary, this is the only place where the migration marker is spelled out in the file
@ -44,0 +153,4 @@"unembed_unsigned_info failed! your database is broken, sorry >.>",);db["global"].insert(b"unembed_unsigned_info", []);});Is there a reason to move the marker out of the function itself?
it's more consistent with the other migration functions
i guess
unembed_unsigned_infoinfallible