diff --git a/crates/collab/src/db/queries/channels.rs b/crates/collab/src/db/queries/channels.rs index 2d65139a98..16a0891d16 100644 --- a/crates/collab/src/db/queries/channels.rs +++ b/crates/collab/src/db/queries/channels.rs @@ -828,68 +828,53 @@ impl Database { ) -> Result { self.check_user_is_channel_admin(to, user, &*tx).await?; - let to_ancestors = self.get_channel_ancestors(to, &*tx).await?; + let paths = channel_path::Entity::find() + .filter(channel_path::Column::IdPath.like(&format!("%/{}/%", channel))) + .all(tx) + .await?; + + let mut new_path_suffixes = HashSet::default(); + for path in paths { + if let Some(start_offset) = path.id_path.find(&format!("/{}/", channel)) { + new_path_suffixes.insert(( + path.channel_id, + path.id_path[(start_offset + 1)..].to_string(), + )); + } + } + + let paths_to_new_parent = channel_path::Entity::find() + .filter(channel_path::Column::ChannelId.eq(to)) + .all(tx) + .await?; + + let mut new_paths = Vec::new(); + for path in paths_to_new_parent { + if path.id_path.contains(&format!("/{}/", channel)) { + Err(anyhow!("cycle"))?; + } + + new_paths.extend(new_path_suffixes.iter().map(|(channel_id, path_suffix)| { + channel_path::ActiveModel { + channel_id: ActiveValue::Set(*channel_id), + id_path: ActiveValue::Set(format!("{}{}", &path.id_path, path_suffix)), + } + })); + } + + channel_path::Entity::insert_many(new_paths) + .exec(&*tx) + .await?; + + // remove any root edges for the channel we just linked + { + channel_path::Entity::delete_many() + .filter(channel_path::Column::IdPath.like(&format!("/{}/%", channel))) + .exec(&*tx) + .await?; + } + let mut channel_descendants = self.get_channel_descendants([channel], &*tx).await?; - for ancestor in to_ancestors { - if channel_descendants.contains_key(&ancestor) { - return Err(anyhow!("Cannot create a channel cycle").into()); - } - } - - // Now insert all of the new paths - let sql = r#" - INSERT INTO channel_paths - (id_path, channel_id) - SELECT - id_path || $1 || '/', $2 - FROM - channel_paths - WHERE - channel_id = $3 - ON CONFLICT (id_path) DO NOTHING; - "#; - let channel_paths_stmt = Statement::from_sql_and_values( - self.pool.get_database_backend(), - sql, - [ - channel.to_proto().into(), - channel.to_proto().into(), - to.to_proto().into(), - ], - ); - tx.execute(channel_paths_stmt).await?; - for (descdenant_id, descendant_parent_ids) in - channel_descendants.iter().filter(|(id, _)| id != &&channel) - { - for descendant_parent_id in descendant_parent_ids.iter() { - let channel_paths_stmt = Statement::from_sql_and_values( - self.pool.get_database_backend(), - sql, - [ - descdenant_id.to_proto().into(), - descdenant_id.to_proto().into(), - descendant_parent_id.to_proto().into(), - ], - ); - tx.execute(channel_paths_stmt).await?; - } - } - - // If we're linking a channel, remove any root edges for the channel - { - let sql = r#" - DELETE FROM channel_paths - WHERE - id_path = '/' || $1 || '/' - "#; - let channel_paths_stmt = Statement::from_sql_and_values( - self.pool.get_database_backend(), - sql, - [channel.to_proto().into()], - ); - tx.execute(channel_paths_stmt).await?; - } - if let Some(channel) = channel_descendants.get_mut(&channel) { // Remove the other parents channel.clear(); @@ -936,35 +921,43 @@ impl Database { self.check_user_is_channel_admin(from, user, &*tx).await?; let sql = r#" - DELETE FROM channel_paths - WHERE - id_path LIKE '%' || $1 || '/' || $2 || '%' - "#; - let channel_paths_stmt = Statement::from_sql_and_values( - self.pool.get_database_backend(), - sql, - [from.to_proto().into(), channel.to_proto().into()], - ); - tx.execute(channel_paths_stmt).await?; + DELETE FROM channel_paths + WHERE + id_path LIKE '%/' || $1 || '/' || $2 || '/%' + RETURNING id_path, channel_id + "#; + + let paths = channel_path::Entity::find() + .from_raw_sql(Statement::from_sql_and_values( + self.pool.get_database_backend(), + sql, + [from.to_proto().into(), channel.to_proto().into()], + )) + .all(&*tx) + .await?; + + let is_stranded = channel_path::Entity::find() + .filter(channel_path::Column::ChannelId.eq(channel)) + .count(&*tx) + .await? + == 0; // Make sure that there is always at least one path to the channel - let sql = r#" - INSERT INTO channel_paths - (id_path, channel_id) - SELECT - '/' || $1 || '/', $2 - WHERE NOT EXISTS - (SELECT * - FROM channel_paths - WHERE channel_id = $2) - "#; - - let channel_paths_stmt = Statement::from_sql_and_values( - self.pool.get_database_backend(), - sql, - [channel.to_proto().into(), channel.to_proto().into()], - ); - tx.execute(channel_paths_stmt).await?; + if is_stranded { + let root_paths: Vec<_> = paths + .iter() + .map(|path| { + let start_offset = path.id_path.find(&format!("/{}/", channel)).unwrap(); + channel_path::ActiveModel { + channel_id: ActiveValue::Set(path.channel_id), + id_path: ActiveValue::Set(path.id_path[start_offset..].to_string()), + } + }) + .collect(); + channel_path::Entity::insert_many(root_paths) + .exec(&*tx) + .await?; + } Ok(()) } @@ -978,6 +971,13 @@ impl Database { from: ChannelId, to: ChannelId, ) -> Result { + if from == to { + return Ok(ChannelGraph { + channels: vec![], + edges: vec![], + }); + } + self.transaction(|tx| async move { self.check_user_is_channel_admin(channel, user, &*tx) .await?; diff --git a/crates/collab/src/db/tests/channel_tests.rs b/crates/collab/src/db/tests/channel_tests.rs index 8a03014cf9..429852d128 100644 --- a/crates/collab/src/db/tests/channel_tests.rs +++ b/crates/collab/src/db/tests/channel_tests.rs @@ -791,6 +791,64 @@ async fn test_db_channel_moving(db: &Arc) { assert!(result.channels.is_empty()) } +test_both_dbs!( + test_db_channel_moving_bugs, + test_db_channel_moving_bugs_postgres, + test_db_channel_moving_bugs_sqlite +); + +async fn test_db_channel_moving_bugs(db: &Arc) { + let user_id = db + .create_user( + "user1@example.com", + false, + NewUserParams { + github_login: "user1".into(), + github_user_id: 5, + invite_count: 0, + }, + ) + .await + .unwrap() + .user_id; + + let zed_id = db.create_root_channel("zed", "1", user_id).await.unwrap(); + + let projects_id = db + .create_channel("projects", Some(zed_id), "2", user_id) + .await + .unwrap(); + + let livestreaming_id = db + .create_channel("livestreaming", Some(projects_id), "3", user_id) + .await + .unwrap(); + + // Dag is: zed - projects - livestreaming + + // Move to same parent should be a no-op + assert!(db + .move_channel(user_id, projects_id, zed_id, zed_id) + .await + .unwrap() + .is_empty()); + + // Stranding a channel should retain it's sub channels + db.unlink_channel(user_id, projects_id, zed_id) + .await + .unwrap(); + + let result = db.get_channels_for_user(user_id).await.unwrap(); + assert_dag( + result.channels, + &[ + (zed_id, None), + (projects_id, None), + (livestreaming_id, Some(projects_id)), + ], + ); +} + #[track_caller] fn assert_dag(actual: ChannelGraph, expected: &[(ChannelId, Option)]) { let mut actual_map: HashMap> = HashMap::default(); diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index 87b7720235..fa9cc5ef1e 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -2474,6 +2474,11 @@ async fn move_channel( .move_channel(session.user_id, channel_id, from_parent, to) .await?; + if channels_to_send.is_empty() { + response.send(Ack {})?; + return Ok(()); + } + let members_from = db.get_channel_members(from_parent).await?; let members_to = db.get_channel_members(to).await?; diff --git a/crates/collab/src/tests/channel_tests.rs b/crates/collab/src/tests/channel_tests.rs index 6b300282a1..6bdcee6af3 100644 --- a/crates/collab/src/tests/channel_tests.rs +++ b/crates/collab/src/tests/channel_tests.rs @@ -145,8 +145,6 @@ async fn test_core_channels( ], ); - println!("STARTING CREATE CHANNEL C"); - let channel_c_id = client_a .channel_store() .update(cx_a, |channel_store, cx| { @@ -1028,10 +1026,6 @@ async fn test_channel_moving( // - ep assert_channels_list_shape(client_c.channel_store(), cx_c, &[(channel_ep_id, 0)]); - println!("*******************************************"); - println!("********** STARTING LINK CHANNEL **********"); - println!("*******************************************"); - dbg!(client_b.user_id()); client_b .channel_store() .update(cx_b, |channel_store, cx| { @@ -1199,5 +1193,5 @@ fn assert_channels_list_shape( .map(|(depth, channel)| (channel.id, depth)) .collect::>() }); - pretty_assertions::assert_eq!(dbg!(actual), expected_channels); + pretty_assertions::assert_eq!(actual, expected_channels); }