Fix bugs in moving channels that could cause channels to be stranded or moved unexpectedly

Made channel linking not query in a loop

co-authored-by: Max <max@zed.dev>

crates/collab/src/db/queries/channels.rs

@@ -828,68 +828,53 @@ impl Database {
     ) -> Result<ChannelGraph> {
         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
+            DELETE FROM channel_paths
-                WHERE
+            WHERE
-                    id_path LIKE '%' || $1 || '/' || $2 || '%'
+                id_path LIKE '%/' || $1 || '/' || $2 || '/%'
-            "#;
+            RETURNING id_path, channel_id
-        let channel_paths_stmt = Statement::from_sql_and_values(
+        "#;
-            self.pool.get_database_backend(),
+
-            sql,
+        let paths = channel_path::Entity::find()
-            [from.to_proto().into(), channel.to_proto().into()],
+            .from_raw_sql(Statement::from_sql_and_values(
-        );
+                self.pool.get_database_backend(),
-        tx.execute(channel_paths_stmt).await?;
+                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#"
+        if is_stranded {
-            INSERT INTO channel_paths
+            let root_paths: Vec<_> = paths
-            (id_path, channel_id)
+                .iter()
-            SELECT
+                .map(|path| {
-                '/' || $1 || '/', $2
+                    let start_offset = path.id_path.find(&format!("/{}/", channel)).unwrap();
-            WHERE NOT EXISTS
+                    channel_path::ActiveModel {
-                (SELECT *
+                        channel_id: ActiveValue::Set(path.channel_id),
-                 FROM channel_paths
+                        id_path: ActiveValue::Set(path.id_path[start_offset..].to_string()),
-                 WHERE channel_id = $2)
+                    }
-            "#;
+                })
-
+                .collect();
-        let channel_paths_stmt = Statement::from_sql_and_values(
+            channel_path::Entity::insert_many(root_paths)
-            self.pool.get_database_backend(),
+                .exec(&*tx)
-            sql,
+                .await?;
-            [channel.to_proto().into(), channel.to_proto().into()],
+        }
-        );
-        tx.execute(channel_paths_stmt).await?;
 
         Ok(())
     }
@@ -978,6 +971,13 @@ impl Database {
         from: ChannelId,
         to: ChannelId,
     ) -> Result<ChannelGraph> {
+        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?;

crates/collab/src/db/tests/channel_tests.rs

@@ -791,6 +791,64 @@ async fn test_db_channel_moving(db: &Arc<Database>) {
     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<Database>) {
+    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<ChannelId>)]) {
     let mut actual_map: HashMap<ChannelId, HashSet<ChannelId>> = HashMap::default();

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?;
 

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::<Vec<_>>()
     });
-    pretty_assertions::assert_eq!(dbg!(actual), expected_channels);
+    pretty_assertions::assert_eq!(actual, expected_channels);
 }