From 2afea56e56017f02de98e67316b4457568ad5b26 Mon Sep 17 00:00:00 2001 From: Angelo Kontaxis Date: Fri, 9 Jan 2026 20:06:16 +0000 Subject: [PATCH] feat: add id field to role (#470) --- .../database/fixtures/server_with_roles.json | 2 + .../admin_migrations/ops/mongodb/scripts.rs | 46 ++++++++++++++++++- .../core/database/src/models/servers/model.rs | 43 ++++++++++------- .../core/database/src/models/servers/ops.rs | 2 +- .../src/models/servers/ops/mongodb.rs | 4 +- .../src/models/servers/ops/reference.rs | 4 +- crates/core/database/src/util/bridge/v0.rs | 4 ++ crates/core/models/src/v0/servers.rs | 4 ++ .../fixtures/server_with_many_roles.json | 4 ++ .../delta/src/routes/channels/message_send.rs | 33 +++++-------- .../delta/src/routes/servers/roles_create.rs | 11 +---- .../delta/src/routes/servers/roles_delete.rs | 2 +- crates/delta/src/routes/servers/roles_edit.rs | 1 - crates/delta/src/util/test.rs | 33 +++++++------ 14 files changed, 124 insertions(+), 69 deletions(-) diff --git a/crates/core/database/fixtures/server_with_roles.json b/crates/core/database/fixtures/server_with_roles.json index 0a76489d..21ea5656 100644 --- a/crates/core/database/fixtures/server_with_roles.json +++ b/crates/core/database/fixtures/server_with_roles.json @@ -47,6 +47,7 @@ ], "roles": { "__ID:5__": { + "_id": "__ID:5__", "name": "Moderator", "permissions": { "a": 545270208, @@ -55,6 +56,7 @@ "rank": 1 }, "__ID:6__": { + "_id": "__ID:6__", "name": "Owner", "permissions": { "a": 0, diff --git a/crates/core/database/src/models/admin_migrations/ops/mongodb/scripts.rs b/crates/core/database/src/models/admin_migrations/ops/mongodb/scripts.rs index 472f665e..174a81a6 100644 --- a/crates/core/database/src/models/admin_migrations/ops/mongodb/scripts.rs +++ b/crates/core/database/src/models/admin_migrations/ops/mongodb/scripts.rs @@ -25,7 +25,7 @@ struct MigrationInfo { revision: i32, } -pub const LATEST_REVISION: i32 = 49; // MUST BE +1 to last migration +pub const LATEST_REVISION: i32 = 50; // MUST BE +1 to last migration pub async fn migrate_database(db: &MongoDb) { let migrations = db.col::("migrations"); @@ -1246,7 +1246,7 @@ pub async fn run_migrations(db: &MongoDb, revision: i32) -> i32 { .expect("Failed to update voice channels"); }; - if revision <= 48 { + if revision <= 48 { info!("Running migration [revision 48 / 22-10-2025]: Add Video + Listen to default permissions"); db.col::("servers") @@ -1264,6 +1264,48 @@ pub async fn run_migrations(db: &MongoDb, revision: i32) -> i32 { .expect("Failed to update default_permissions"); }; + if revision <= 49 { + info!("Running migration [revision 49 / 12-12-2025]: Add _id key to roles"); + + #[derive(Serialize, Deserialize, Clone)] + struct Server { + #[serde(rename = "_id")] + pub id: String, + #[serde(default = "HashMap::::new")] + pub roles: HashMap, + } + + let mut servers = db + .db() + .collection::("servers") + .find(doc! { + "roles": { + "$exists": true, + "$ne": {} + } + }) + .await + .unwrap() + .map(|res| res.expect("Failed to decode Server { id, roles }")); + + while let Some(server) = servers.next().await { + let mut doc = doc! {}; + + for id in server.roles.keys() { + doc.insert( + format!("roles.{id}._id"), + id, + ); + } + + db.db() + .collection::("servers") + .update_one(doc! { "_id": &server.id }, doc! { "$set": doc }) + .await + .unwrap(); + } + }; + // Reminder to update LATEST_REVISION when adding new migrations. LATEST_REVISION.max(revision) } diff --git a/crates/core/database/src/models/servers/model.rs b/crates/core/database/src/models/servers/model.rs index eea8f392..183dd3d2 100644 --- a/crates/core/database/src/models/servers/model.rs +++ b/crates/core/database/src/models/servers/model.rs @@ -68,6 +68,9 @@ auto_derived_partial!( auto_derived_partial!( /// Role pub struct Role { + /// Unique Id + #[serde(rename = "_id")] + pub id: String, /// Role name pub name: String, /// Permissions available to this role @@ -246,7 +249,6 @@ impl Server { role.update( db, &self.id, - role_id, PartialRole { permissions: Some(permissions), ..Default::default() @@ -297,6 +299,7 @@ impl Role { /// Into optional struct pub fn into_optional(self) -> PartialRole { PartialRole { + id: Some(self.id), name: Some(self.name), permissions: Some(self.permissions), colour: self.colour, @@ -306,20 +309,29 @@ impl Role { } /// Create a role - pub async fn create(&self, db: &Database, server_id: &str) -> Result { - let role_id = Ulid::new().to_string(); - db.insert_role(server_id, &role_id, self).await?; + pub async fn create(db: &Database, server: &Server, name: String) -> Result { + let role = Role { + id: Ulid::new().to_string(), + name, + // Rank of the new role should be below the lowest role + rank: server.roles.len() as i64, + colour: None, + hoist: false, + permissions: Default::default(), + }; + + db.insert_role(&server.id, &role).await?; EventV1::ServerRoleUpdate { - id: server_id.to_string(), - role_id: role_id.to_string(), - data: self.clone().into_optional().into(), + id: server.id.clone(), + role_id: role.id.clone(), + data: role.clone().into_optional().into(), clear: vec![], } - .p(server_id.to_string()) + .p(server.id.clone()) .await; - Ok(role_id) + Ok(role) } /// Update server data @@ -327,7 +339,6 @@ impl Role { &mut self, db: &Database, server_id: &str, - role_id: &str, partial: PartialRole, remove: Vec, ) -> Result<()> { @@ -337,14 +348,14 @@ impl Role { self.apply_options(partial.clone()); - db.update_role(server_id, role_id, &partial, remove.clone()) + db.update_role(server_id, &self.id, &partial, remove.clone()) .await?; EventV1::ServerRoleUpdate { id: server_id.to_string(), - role_id: role_id.to_string(), + role_id: self.id.clone(), data: partial.into(), - clear: vec![], + clear: remove.into_iter().map(Into::into).collect(), } .p(server_id.to_string()) .await; @@ -360,15 +371,15 @@ impl Role { } /// Delete a role - pub async fn delete(self, db: &Database, server_id: &str, role_id: &str) -> Result<()> { + pub async fn delete(self, db: &Database, server_id: &str) -> Result<()> { EventV1::ServerRoleDelete { id: server_id.to_string(), - role_id: role_id.to_string(), + role_id: self.id.clone(), } .p(server_id.to_string()) .await; - db.delete_role(server_id, role_id).await + db.delete_role(server_id, &self.id).await } } diff --git a/crates/core/database/src/models/servers/ops.rs b/crates/core/database/src/models/servers/ops.rs index ee07ba8f..c7ae6228 100644 --- a/crates/core/database/src/models/servers/ops.rs +++ b/crates/core/database/src/models/servers/ops.rs @@ -29,7 +29,7 @@ pub trait AbstractServers: Sync + Send { async fn delete_server(&self, id: &str) -> Result<()>; /// Insert a new role into server object - async fn insert_role(&self, server_id: &str, role_id: &str, role: &Role) -> Result<()>; + async fn insert_role(&self, server_id: &str, role: &Role) -> Result<()>; /// Update an existing role on a server async fn update_role( diff --git a/crates/core/database/src/models/servers/ops/mongodb.rs b/crates/core/database/src/models/servers/ops/mongodb.rs index 958963c9..ecdff53b 100644 --- a/crates/core/database/src/models/servers/ops/mongodb.rs +++ b/crates/core/database/src/models/servers/ops/mongodb.rs @@ -69,7 +69,7 @@ impl AbstractServers for MongoDb { } /// Insert a new role into server object - async fn insert_role(&self, server_id: &str, role_id: &str, role: &Role) -> Result<()> { + async fn insert_role(&self, server_id: &str, role: &Role) -> Result<()> { self.col::(COL) .update_one( doc! { @@ -77,7 +77,7 @@ impl AbstractServers for MongoDb { }, doc! { "$set": { - "roles.".to_owned() + role_id: to_document(role) + "roles.".to_owned() + &role.id: to_document(role) .map_err(|_| create_database_error!("to_document", "role"))? } }, diff --git a/crates/core/database/src/models/servers/ops/reference.rs b/crates/core/database/src/models/servers/ops/reference.rs index 18ee1b38..572311bd 100644 --- a/crates/core/database/src/models/servers/ops/reference.rs +++ b/crates/core/database/src/models/servers/ops/reference.rs @@ -72,10 +72,10 @@ impl AbstractServers for ReferenceDb { } /// Insert a new role into server object - async fn insert_role(&self, server_id: &str, role_id: &str, role: &Role) -> Result<()> { + async fn insert_role(&self, server_id: &str, role: &Role) -> Result<()> { let mut servers = self.servers.lock().await; if let Some(server) = servers.get_mut(server_id) { - server.roles.insert(role_id.to_string(), role.clone()); + server.roles.insert(role.id.clone(), role.clone()); Ok(()) } else { Err(create_error!(NotFound)) diff --git a/crates/core/database/src/util/bridge/v0.rs b/crates/core/database/src/util/bridge/v0.rs index 915b75ce..de63db90 100644 --- a/crates/core/database/src/util/bridge/v0.rs +++ b/crates/core/database/src/util/bridge/v0.rs @@ -896,6 +896,7 @@ impl From for crate::SystemMessageChannels { impl From for Role { fn from(value: crate::Role) -> Self { Role { + id: value.id, name: value.name, permissions: value.permissions, colour: value.colour, @@ -908,6 +909,7 @@ impl From for Role { impl From for crate::Role { fn from(value: Role) -> crate::Role { crate::Role { + id: value.id, name: value.name, permissions: value.permissions, colour: value.colour, @@ -920,6 +922,7 @@ impl From for crate::Role { impl From for PartialRole { fn from(value: crate::PartialRole) -> Self { PartialRole { + id: value.id, name: value.name, permissions: value.permissions, colour: value.colour, @@ -932,6 +935,7 @@ impl From for PartialRole { impl From for crate::PartialRole { fn from(value: PartialRole) -> crate::PartialRole { crate::PartialRole { + id: value.id, name: value.name, permissions: value.permissions, colour: value.colour, diff --git a/crates/core/models/src/v0/servers.rs b/crates/core/models/src/v0/servers.rs index c3ed20a8..0d330334 100644 --- a/crates/core/models/src/v0/servers.rs +++ b/crates/core/models/src/v0/servers.rs @@ -85,6 +85,9 @@ auto_derived_partial!( auto_derived_partial!( /// Role pub struct Role { + /// Unique Id + #[cfg_attr(feature = "serde", serde(rename = "_id"))] + pub id: String, /// Role name pub name: String, /// Permissions available to this role @@ -181,6 +184,7 @@ auto_derived!( } /// Response after creating new role + // TODO: remove this in favor of just Role pub struct NewRoleResponse { /// Id of the role pub id: String, diff --git a/crates/delta/fixtures/server_with_many_roles.json b/crates/delta/fixtures/server_with_many_roles.json index 49cfe276..c32529a5 100644 --- a/crates/delta/fixtures/server_with_many_roles.json +++ b/crates/delta/fixtures/server_with_many_roles.json @@ -47,6 +47,7 @@ ], "roles": { "__ID:5__": { + "_id": "__ID:5__", "name": "Moderator", "permissions": { "a": 545270216, @@ -55,6 +56,7 @@ "rank": 1 }, "__ID:6__": { + "_id": "__ID:6__", "name": "Owner", "permissions": { "a": 0, @@ -63,6 +65,7 @@ "rank": 0 }, "__ID:7__": { + "_id": "__ID:7__", "name": "Lower Rank 1", "permissions": { "a": 0, @@ -71,6 +74,7 @@ "rank": 2 }, "__ID:8__": { + "_id": "__ID:8__", "name": "Lower Rank 2", "permissions": { "a": 0, diff --git a/crates/delta/src/routes/channels/message_send.rs b/crates/delta/src/routes/channels/message_send.rs index e0dac276..d0479fa1 100644 --- a/crates/delta/src/routes/channels/message_send.rs +++ b/crates/delta/src/routes/channels/message_send.rs @@ -151,29 +151,20 @@ mod test { name: "Hidden Channel".to_string(), description: None, nsfw: Some(false), - voice: None + voice: None, }, true, ) .await .expect("Failed to make new channel"); - let role = Role { - name: "Show Hidden Channel".to_string(), - permissions: OverrideField { a: 0, d: 0 }, - colour: None, - hoist: false, - rank: 5, - }; - - let role_id = role - .create(&harness.db, &server.id) + let role = Role::create(&harness.db, &server, "Show Hidden Channel".to_string()) .await .expect("Failed to create the role"); let mut overrides = HashMap::new(); overrides.insert( - role_id.clone(), + role.id.clone(), OverrideField { a: (ChannelPermission::ViewChannel) as i64, d: 0, @@ -281,7 +272,7 @@ mod test { "Mention failed to be scrubbed when the user cannot see the channel" ); - let second_member_roles = vec![role_id.clone()]; + let second_member_roles = vec![role.id.clone()]; let partial = PartialMember { id: None, joined_at: None, @@ -290,7 +281,7 @@ mod test { timeout: None, roles: Some(second_member_roles), can_publish: None, - can_receive: None + can_receive: None, }; second_member .update(&harness.db, partial, vec![]) @@ -496,7 +487,7 @@ mod test { let (_, _, other_user) = harness.new_user().await; let (server, _) = harness.new_server(&user).await; let channel = harness.new_channel(&server).await; - let (role_id, _role) = harness + let role = harness .new_role( &server, 1, @@ -518,7 +509,7 @@ mod test { Some(&harness.amqp), channel.clone(), v0::DataMessageSend { - content: Some(format!("Mentioning @everyone and role <%{}>", &role_id)), + content: Some(format!("Mentioning @everyone and role <%{}>", &role.id)), nonce: None, attachments: None, replies: None, @@ -563,7 +554,7 @@ mod test { Some(&harness.amqp), channel.clone(), v0::DataMessageSend { - content: Some(format!("Mentioning `@everyone` and role `<%{}>`", &role_id)), + content: Some(format!("Mentioning `@everyone` and role `<%{}>`", &role.id)), nonce: None, attachments: None, replies: None, @@ -605,7 +596,7 @@ mod test { "Role mentions detected when inside codeblock" ); - other_member.roles.push(role_id.clone()); + other_member.roles.push(role.id.clone()); harness .db .update_member( @@ -615,10 +606,10 @@ mod test { id: None, joined_at: None, nickname: None, - roles: Some(vec![role_id.clone()]), + roles: Some(vec![role.id.clone()]), timeout: None, can_publish: None, - can_receive: None + can_receive: None, }, vec![], ) @@ -632,7 +623,7 @@ mod test { Some(&harness.amqp), channel.clone(), v0::DataMessageSend { - content: Some(format!("Mentioning @everyone and role <%{}>", &role_id)), + content: Some(format!("Mentioning @everyone and role <%{}>", &role.id)), nonce: None, attachments: None, replies: None, diff --git a/crates/delta/src/routes/servers/roles_create.rs b/crates/delta/src/routes/servers/roles_create.rs index e68d5ea6..20a3c94b 100644 --- a/crates/delta/src/routes/servers/roles_create.rs +++ b/crates/delta/src/routes/servers/roles_create.rs @@ -40,17 +40,10 @@ pub async fn create( })); }; - let role = Role { - name: data.name, - // Rank of the new role should be below the lowest role - rank: server.roles.len() as i64, - colour: None, - hoist: false, - permissions: Default::default(), - }; + let role = Role::create(db, &server, data.name).await?; Ok(Json(v0::NewRoleResponse { - id: role.create(db, &server.id).await?, + id: role.id.clone(), role: role.into(), })) } diff --git a/crates/delta/src/routes/servers/roles_delete.rs b/crates/delta/src/routes/servers/roles_delete.rs index 77df2501..232ae92e 100644 --- a/crates/delta/src/routes/servers/roles_delete.rs +++ b/crates/delta/src/routes/servers/roles_delete.rs @@ -37,7 +37,7 @@ pub async fn delete( return Err(create_error!(NotElevated)); } - role.delete(db, &server.id, &role_id).await?; + role.delete(db, &server.id).await?; for channel_id in &server.channels { let channel = Reference::from_unchecked(channel_id).as_channel(db).await?; diff --git a/crates/delta/src/routes/servers/roles_edit.rs b/crates/delta/src/routes/servers/roles_edit.rs index 639589af..0a122c1f 100644 --- a/crates/delta/src/routes/servers/roles_edit.rs +++ b/crates/delta/src/routes/servers/roles_edit.rs @@ -61,7 +61,6 @@ pub async fn edit( role.update( db, &server.id, - &role_id, partial, remove.into_iter().map(Into::into).collect(), ) diff --git a/crates/delta/src/util/test.rs b/crates/delta/src/util/test.rs index b90a1a5c..a6251509 100644 --- a/crates/delta/src/util/test.rs +++ b/crates/delta/src/util/test.rs @@ -6,7 +6,7 @@ use futures::StreamExt; use rand::Rng; use redis_kiss::redis::aio::PubSub; use revolt_database::{ - events::client::EventV1, Channel, Database, Member, Message, Server, User, AMQP, + events::client::EventV1, Channel, Database, Member, Message, PartialRole, Server, User, AMQP, }; use revolt_database::{util::idempotency::IdempotencyKey, Role}; use revolt_models::v0; @@ -139,21 +139,26 @@ impl TestHarness { server: &Server, rank: i64, overrides: Option, - ) -> (String, Role) { - let role = Role { - name: TestHarness::rand_string(), - permissions: overrides.unwrap_or(OverrideField { a: 0, d: 0 }), - rank, - colour: None, - hoist: false, - }; - - let id = role - .create(&self.db, &server.id) + ) -> Role { + let mut role = Role::create(&self.db, &server, TestHarness::rand_string()) .await .expect("Failed to create test role"); - (id, role) + if let Some(overrides) = overrides { + role.update( + &self.db, + &server.id, + PartialRole { + permissions: Some(overrides), + ..Default::default() + }, + Vec::new(), + ) + .await + .expect("Failed to set test role overrides"); + }; + + role } pub async fn new_channel(&self, server: &Server) -> Channel { @@ -165,7 +170,7 @@ impl TestHarness { name: "Test Channel".to_string(), description: None, nsfw: Some(false), - voice: None + voice: None, }, true, )