#182 Add room upgrade.

Merged
timo merged 3 commits from q-b/conduit-fork:rooms_upgrade into master 3 weeks ago
q-b commented 1 month ago

Note that upgrading to room version 6 won’t work until: 5153279eba

Should fix #93

Comments are welcome and expected :)

- [x] I agree to release my code and all other changes of this PR under the AGPL-3.0 license Note that upgrading to room version 6 won't work until: https://github.com/ruma/ruma/commit/5153279eba5036d1af3bfb0e8b65f434538cd660 Should fix #93 Comments are welcome and expected :)
q-b commented 1 month ago
Poster

In case you’re looking for a way to test:

curl -XPOST -d '{"new_version": "5"}' "http://localhost:8000/_matrix/client/r0/rooms/${MATRIX_ROOM_ID}/upgrade?access_token=${MATRIX_ACCESS_TOKEN}"
In case you're looking for a way to test: ```bash curl -XPOST -d '{"new_version": "5"}' "http://localhost:8000/_matrix/client/r0/rooms/${MATRIX_ROOM_ID}/upgrade?access_token=${MATRIX_ACCESS_TOKEN}" ```
q-b changed title from WIP: impl. room_upgrade to impl. room_upgrade 1 month ago
timo requested changes 1 month ago
timo left a comment

Looking good! You forgot to transfer user settings though (see last line in the server behaviour section in the spec)

src/client_server/room.rs
@@ -346,0 +372,4 @@
// Create a replacement room.
let replacement_room = RoomId::new(db.globals.server_name());

// Send a m.room.tombstone event to the old room to indicate that it is not intended to be used any further.
timo commented 1 month ago

Maybe add a comment like “This will fail if the sender does not have the required permissions”

Maybe add a comment like "This will fail if the sender does not have the required permissions"
q-b marked this conversation as resolved
src/client_server/room.rs
@@ -346,0 +404,4 @@
.deserialize()
.map_err(|_| Error::bad_database("Invalid room event in database."))?
.federate
.into();
timo commented 1 month ago

I don’t think you need .into here. You also don’t need to specify the type federate: bool in line 395

I don't think you need .into here. You also don't need to specify the type `federate: bool` in line 395
q-b marked this conversation as resolved
src/client_server/room.rs
@@ -346,0 +442,4 @@
.content
.clone(),
)
.expect("Database contains invalid PDU.")
timo commented 1 month ago

Replace expect messages like this with “Raw::from_value always works”

Replace expect messages like this with "Raw::from_value always works"
q-b commented 1 month ago

Should I change other expect()? Also, do you want messages to start with capital letters? end with .?

Should I change other `expect()`? Also, do you want messages to start with capital letters? end with `.`?
timo commented 1 month ago

.expect messages are start lowercase (except for names like “Raw”) and don’t end with .

Comments start capitalized and don’t end with .

Doc-comments start capitalized and end with .

I should write a “how to contribute” guide...

.expect messages are start lowercase (except for names like "Raw") and don't end with `.` Comments start capitalized and don't end with `.` Doc-comments start capitalized and end with `.` I should write a "how to contribute" guide...
q-b marked this conversation as resolved
src/client_server/room.rs
@@ -346,0 +446,4 @@
.deserialize()
.map_err(|_| Error::bad_database("Invalid room event in database."))?
.is_direct
.into();
timo commented 1 month ago

I don’t think we need to copy is_direct. It’s set to false after accepting the invite. Instead we should transfer the user data here:

When a user joins the new room, the server should automatically transfer/replicate some of the user’s personalized settings such as notifications, tags, etc.

I don't think we need to copy is_direct. It's set to false after accepting the invite. Instead we should transfer the user data here: > When a user joins the new room, the server should automatically transfer/replicate some of the user's personalized settings such as notifications, tags, etc.
q-b commented 1 month ago

I need to research that one (for my own comprehension)...

I need to research that one (for my own comprehension)...
src/client_server/room.rs
@@ -346,0 +470,4 @@
&db.account_data,
)?;

// Recommanded transferable state events list from the specs.
timo commented 1 month ago

s/Recommanded/Recommended/

s/Recommanded/Recommended/
q-b marked this conversation as resolved
src/client_server/room.rs
@@ -346,0 +487,4 @@
for event_type in transferable_state_events {
let event_content = match db.rooms.room_state_get(&body.room_id, &event_type, "")? {
Some(v) => v.content.clone(),
None => continue, // Skipping missing events.
timo commented 1 month ago

This is a good way to do this, small nitpick: Can you not end comments with a “.” ? This goes for all comments except doc-comments (those starting with ///)

This is a good way to do this, small nitpick: Can you not end comments with a "." ? This goes for all comments except doc-comments (those starting with `///`)
q-b marked this conversation as resolved
src/client_server/room.rs
@@ -346,0 +545,4 @@
},
&db.globals,
&db.account_data,
)?;
timo commented 1 month ago

We shouldn’t ? here as this might fail. The spec also says “If possible”, so either check the permissions first or (easier) remove the ? and instead start the statement with let _ = , so the result is consumed (and doesn’t generate a warning). Also add a comment that explains this

We shouldn't `?` here as this might fail. The spec also says "If possible", so either check the permissions first or (easier) remove the ? and instead start the statement with `let _ = `, so the result is consumed (and doesn't generate a warning). Also add a comment that explains this
q-b commented 1 month ago

I went with ok() instead, does that work for you?

I went with `ok()` instead, does that work for you?
q-b marked this conversation as resolved
q-b changed title from impl. room_upgrade to WIP: impl. room_upgrade 1 month ago
q-b changed title from WIP: impl. room_upgrade to Add room upgrade. 1 month ago
q-b requested review from timo 1 month ago
timo requested changes 1 month ago
timo left a comment

Your implementation of moving over the account data is wrong. You copy some specific accoutn data events, but we need to copy all of them.

You can call account_data.changes_since(room_id, user_id, 0) to get all account data events and then insert all of those events using account_data.update

src/client_server/room.rs
@@ -346,0 +493,4 @@
// Moves any local aliases to the new room
for alias in db.rooms.room_aliases(&body.room_id) {
db.rooms.set_alias(
&alias.expect("database contains an invalid alias"),
timo commented 1 month ago

Use db.rooms.room_aliases(&body.room_id).filter_map(|r| r.ok()) above instead of .expect here

Use `db.rooms.room_aliases(&body.room_id).filter_map(|r| r.ok())` above instead of .expect here
q-b marked this conversation as resolved
src/client_server/room.rs
@@ -346,0 +515,4 @@
// Setting events_default and invite to the greater of 50 and users_default + 1
power_levels_event_content.events_default = 100.into();
power_levels_event_content.invite = 100.into();
power_levels_event_content.users_default += 1.into();
timo commented 1 month ago

Setting events_default and invite to the greater of 50 and users_default + 1

This means you take 50 and users_default+1, find the greater number and set events_default and invite to that

> Setting events_default and invite to the greater of 50 and users_default + 1 This means you take `50` and `users_default+1`, find the greater number and set events_default and invite to that
q-b commented 1 month ago

I totally misread that one... The third time it clicked, thank you for pointing it out!

I totally misread that one... The third time it *clicked*, thank you for pointing it out!
q-b marked this conversation as resolved
timo approved these changes 3 weeks ago
timo merged commit b5488f86b5 into master 3 weeks ago
q-b deleted branch rooms_upgrade 3 weeks ago

Reviewers

timo approved these changes 3 weeks ago
The pull request has been merged as b5488f86b5.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.