From 4ddf2cbb9f8a7ea49226a95bf4a35445ebc8ada3 Mon Sep 17 00:00:00 2001 From: Marshall Bowers Date: Thu, 22 Aug 2024 18:27:22 -0400 Subject: [PATCH] collab: Make `users.github_user_id` required and unique (#16704) This PR makes the `github_user_id` column on the `users` table required and replaces the index with a unique index. I have gone through and ensured that all users have a unique `github_user_id` in the staging and production databases. Release Notes: - N/A --- .../20221109000000_test_schema.sql | 4 +- ..._constraint_on_github_user_id_on_users.sql | 4 + crates/collab/src/api.rs | 2 +- crates/collab/src/db/queries/contributors.rs | 2 +- crates/collab/src/db/queries/users.rs | 107 ++++++++---------- crates/collab/src/db/tables/user.rs | 2 +- crates/collab/src/db/tests/buffer_tests.rs | 2 +- .../collab/src/db/tests/contributor_tests.rs | 4 +- crates/collab/src/db/tests/db_tests.rs | 22 ++-- crates/collab/src/seed.rs | 2 +- .../collab/src/tests/channel_guest_tests.rs | 4 +- crates/collab/src/user_backfiller.rs | 2 +- 12 files changed, 73 insertions(+), 84 deletions(-) create mode 100644 crates/collab/migrations/20240822215737_add_unique_constraint_on_github_user_id_on_users.sql diff --git a/crates/collab/migrations.sqlite/20221109000000_test_schema.sql b/crates/collab/migrations.sqlite/20221109000000_test_schema.sql index 225971ec324e68..4db9774c2357da 100644 --- a/crates/collab/migrations.sqlite/20221109000000_test_schema.sql +++ b/crates/collab/migrations.sqlite/20221109000000_test_schema.sql @@ -9,14 +9,14 @@ CREATE TABLE "users" ( "connected_once" BOOLEAN NOT NULL DEFAULT false, "created_at" TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, "metrics_id" TEXT, - "github_user_id" INTEGER, + "github_user_id" INTEGER NOT NULL, "accepted_tos_at" TIMESTAMP WITHOUT TIME ZONE, "github_user_created_at" TIMESTAMP WITHOUT TIME ZONE ); CREATE UNIQUE INDEX "index_users_github_login" ON "users" ("github_login"); CREATE UNIQUE INDEX "index_invite_code_users" ON "users" ("invite_code"); CREATE INDEX "index_users_on_email_address" ON "users" ("email_address"); -CREATE INDEX "index_users_on_github_user_id" ON "users" ("github_user_id"); +CREATE UNIQUE INDEX "index_users_on_github_user_id" ON "users" ("github_user_id"); CREATE TABLE "access_tokens" ( "id" INTEGER PRIMARY KEY AUTOINCREMENT, diff --git a/crates/collab/migrations/20240822215737_add_unique_constraint_on_github_user_id_on_users.sql b/crates/collab/migrations/20240822215737_add_unique_constraint_on_github_user_id_on_users.sql new file mode 100644 index 00000000000000..3b418f7e2669ad --- /dev/null +++ b/crates/collab/migrations/20240822215737_add_unique_constraint_on_github_user_id_on_users.sql @@ -0,0 +1,4 @@ +alter table users alter column github_user_id set not null; + +drop index index_users_on_github_user_id; +create unique index uix_users_on_github_user_id on users (github_user_id); diff --git a/crates/collab/src/api.rs b/crates/collab/src/api.rs index 1aadcc5cd93a47..84516114270182 100644 --- a/crates/collab/src/api.rs +++ b/crates/collab/src/api.rs @@ -108,7 +108,7 @@ pub async fn validate_api_token(req: Request, next: Next) -> impl IntoR #[derive(Debug, Deserialize)] struct AuthenticatedUserParams { - github_user_id: Option, + github_user_id: i32, github_login: String, github_email: Option, github_user_created_at: Option>, diff --git a/crates/collab/src/db/queries/contributors.rs b/crates/collab/src/db/queries/contributors.rs index 3078de42ed9475..65c8bcd12f7659 100644 --- a/crates/collab/src/db/queries/contributors.rs +++ b/crates/collab/src/db/queries/contributors.rs @@ -63,7 +63,7 @@ impl Database { pub async fn add_contributor( &self, github_login: &str, - github_user_id: Option, + github_user_id: i32, github_email: Option<&str>, github_user_created_at: Option, initial_channel_id: Option, diff --git a/crates/collab/src/db/queries/users.rs b/crates/collab/src/db/queries/users.rs index 2aa5857560d869..54c0dc17d171b3 100644 --- a/crates/collab/src/db/queries/users.rs +++ b/crates/collab/src/db/queries/users.rs @@ -15,7 +15,7 @@ impl Database { let user = user::Entity::insert(user::ActiveModel { email_address: ActiveValue::set(Some(email_address.into())), github_login: ActiveValue::set(params.github_login.clone()), - github_user_id: ActiveValue::set(Some(params.github_user_id)), + github_user_id: ActiveValue::set(params.github_user_id), admin: ActiveValue::set(admin), metrics_id: ActiveValue::set(Uuid::new_v4()), ..Default::default() @@ -99,7 +99,7 @@ impl Database { pub async fn get_or_create_user_by_github_account( &self, github_login: &str, - github_user_id: Option, + github_user_id: i32, github_email: Option<&str>, github_user_created_at: Option, initial_channel_id: Option, @@ -121,70 +121,61 @@ impl Database { pub async fn get_or_create_user_by_github_account_tx( &self, github_login: &str, - github_user_id: Option, + github_user_id: i32, github_email: Option<&str>, github_user_created_at: Option, initial_channel_id: Option, tx: &DatabaseTransaction, ) -> Result { - if let Some(github_user_id) = github_user_id { - if let Some(user_by_github_user_id) = user::Entity::find() - .filter(user::Column::GithubUserId.eq(github_user_id)) - .one(tx) - .await? - { - let mut user_by_github_user_id = user_by_github_user_id.into_active_model(); - user_by_github_user_id.github_login = ActiveValue::set(github_login.into()); - if github_user_created_at.is_some() { - user_by_github_user_id.github_user_created_at = - ActiveValue::set(github_user_created_at); - } - Ok(user_by_github_user_id.update(tx).await?) - } else if let Some(user_by_github_login) = user::Entity::find() - .filter(user::Column::GithubLogin.eq(github_login)) - .one(tx) - .await? - { - let mut user_by_github_login = user_by_github_login.into_active_model(); - user_by_github_login.github_user_id = ActiveValue::set(Some(github_user_id)); - if github_user_created_at.is_some() { - user_by_github_login.github_user_created_at = - ActiveValue::set(github_user_created_at); - } - Ok(user_by_github_login.update(tx).await?) - } else { - let user = user::Entity::insert(user::ActiveModel { - email_address: ActiveValue::set(github_email.map(|email| email.into())), - github_login: ActiveValue::set(github_login.into()), - github_user_id: ActiveValue::set(Some(github_user_id)), - github_user_created_at: ActiveValue::set(github_user_created_at), - admin: ActiveValue::set(false), - invite_count: ActiveValue::set(0), - invite_code: ActiveValue::set(None), - metrics_id: ActiveValue::set(Uuid::new_v4()), - ..Default::default() + if let Some(user_by_github_user_id) = user::Entity::find() + .filter(user::Column::GithubUserId.eq(github_user_id)) + .one(tx) + .await? + { + let mut user_by_github_user_id = user_by_github_user_id.into_active_model(); + user_by_github_user_id.github_login = ActiveValue::set(github_login.into()); + if github_user_created_at.is_some() { + user_by_github_user_id.github_user_created_at = + ActiveValue::set(github_user_created_at); + } + Ok(user_by_github_user_id.update(tx).await?) + } else if let Some(user_by_github_login) = user::Entity::find() + .filter(user::Column::GithubLogin.eq(github_login)) + .one(tx) + .await? + { + let mut user_by_github_login = user_by_github_login.into_active_model(); + user_by_github_login.github_user_id = ActiveValue::set(github_user_id); + if github_user_created_at.is_some() { + user_by_github_login.github_user_created_at = + ActiveValue::set(github_user_created_at); + } + Ok(user_by_github_login.update(tx).await?) + } else { + let user = user::Entity::insert(user::ActiveModel { + email_address: ActiveValue::set(github_email.map(|email| email.into())), + github_login: ActiveValue::set(github_login.into()), + github_user_id: ActiveValue::set(github_user_id), + github_user_created_at: ActiveValue::set(github_user_created_at), + admin: ActiveValue::set(false), + invite_count: ActiveValue::set(0), + invite_code: ActiveValue::set(None), + metrics_id: ActiveValue::set(Uuid::new_v4()), + ..Default::default() + }) + .exec_with_returning(tx) + .await?; + if let Some(channel_id) = initial_channel_id { + channel_member::Entity::insert(channel_member::ActiveModel { + id: ActiveValue::NotSet, + channel_id: ActiveValue::Set(channel_id), + user_id: ActiveValue::Set(user.id), + accepted: ActiveValue::Set(true), + role: ActiveValue::Set(ChannelRole::Guest), }) - .exec_with_returning(tx) + .exec(tx) .await?; - if let Some(channel_id) = initial_channel_id { - channel_member::Entity::insert(channel_member::ActiveModel { - id: ActiveValue::NotSet, - channel_id: ActiveValue::Set(channel_id), - user_id: ActiveValue::Set(user.id), - accepted: ActiveValue::Set(true), - role: ActiveValue::Set(ChannelRole::Guest), - }) - .exec(tx) - .await?; - } - Ok(user) } - } else { - let user = user::Entity::find() - .filter(user::Column::GithubLogin.eq(github_login)) - .one(tx) - .await? - .ok_or_else(|| anyhow!("no such user {}", github_login))?; Ok(user) } } diff --git a/crates/collab/src/db/tables/user.rs b/crates/collab/src/db/tables/user.rs index e65b24133876c2..ff4331331b1f03 100644 --- a/crates/collab/src/db/tables/user.rs +++ b/crates/collab/src/db/tables/user.rs @@ -10,7 +10,7 @@ pub struct Model { #[sea_orm(primary_key)] pub id: UserId, pub github_login: String, - pub github_user_id: Option, + pub github_user_id: i32, pub github_user_created_at: Option, pub email_address: Option, pub admin: bool, diff --git a/crates/collab/src/db/tests/buffer_tests.rs b/crates/collab/src/db/tests/buffer_tests.rs index 708461205e1223..55a8f216c49406 100644 --- a/crates/collab/src/db/tests/buffer_tests.rs +++ b/crates/collab/src/db/tests/buffer_tests.rs @@ -42,7 +42,7 @@ async fn test_channel_buffers(db: &Arc) { false, NewUserParams { github_login: "user_c".into(), - github_user_id: 102, + github_user_id: 103, }, ) .await diff --git a/crates/collab/src/db/tests/contributor_tests.rs b/crates/collab/src/db/tests/contributor_tests.rs index 0d5c0da6e58c22..7add7196aa7006 100644 --- a/crates/collab/src/db/tests/contributor_tests.rs +++ b/crates/collab/src/db/tests/contributor_tests.rs @@ -25,7 +25,7 @@ async fn test_contributors(db: &Arc) { assert_eq!(db.get_contributors().await.unwrap(), Vec::::new()); let user1_created_at = Utc::now(); - db.add_contributor("user1", Some(1), None, Some(user1_created_at), None) + db.add_contributor("user1", 1, None, Some(user1_created_at), None) .await .unwrap(); assert_eq!( @@ -34,7 +34,7 @@ async fn test_contributors(db: &Arc) { ); let user2_created_at = Utc::now(); - db.add_contributor("user2", Some(2), None, Some(user2_created_at), None) + db.add_contributor("user2", 2, None, Some(user2_created_at), None) .await .unwrap(); assert_eq!( diff --git a/crates/collab/src/db/tests/db_tests.rs b/crates/collab/src/db/tests/db_tests.rs index eabc1a9091a783..df27a3e58acea9 100644 --- a/crates/collab/src/db/tests/db_tests.rs +++ b/crates/collab/src/db/tests/db_tests.rs @@ -45,25 +45,25 @@ async fn test_get_users(db: &Arc) { ( user_ids[0], "user1".to_string(), - Some(1), + 1, Some("user1@example.com".to_string()), ), ( user_ids[1], "user2".to_string(), - Some(2), + 2, Some("user2@example.com".to_string()), ), ( user_ids[2], "user3".to_string(), - Some(3), + 3, Some("user3@example.com".to_string()), ), ( user_ids[3], "user4".to_string(), - Some(4), + 4, Some("user4@example.com".to_string()), ) ] @@ -101,23 +101,17 @@ async fn test_get_or_create_user_by_github_account(db: &Arc) { .user_id; let user = db - .get_or_create_user_by_github_account( - "the-new-login2", - Some(102), - None, - Some(Utc::now()), - None, - ) + .get_or_create_user_by_github_account("the-new-login2", 102, None, Some(Utc::now()), None) .await .unwrap(); assert_eq!(user.id, user_id2); assert_eq!(&user.github_login, "the-new-login2"); - assert_eq!(user.github_user_id, Some(102)); + assert_eq!(user.github_user_id, 102); let user = db .get_or_create_user_by_github_account( "login3", - Some(103), + 103, Some("user3@example.com"), Some(Utc::now()), None, @@ -125,7 +119,7 @@ async fn test_get_or_create_user_by_github_account(db: &Arc) { .await .unwrap(); assert_eq!(&user.github_login, "login3"); - assert_eq!(user.github_user_id, Some(103)); + assert_eq!(user.github_user_id, 103); assert_eq!(user.email_address, Some("user3@example.com".into())); } diff --git a/crates/collab/src/seed.rs b/crates/collab/src/seed.rs index 6b56f46d50e461..bb5b78b059557e 100644 --- a/crates/collab/src/seed.rs +++ b/crates/collab/src/seed.rs @@ -127,7 +127,7 @@ pub async fn seed(config: &Config, db: &Database, force: bool) -> anyhow::Result let user = db .get_or_create_user_by_github_account( &github_user.login, - Some(github_user.id), + github_user.id, github_user.email.as_deref(), None, None, diff --git a/crates/collab/src/tests/channel_guest_tests.rs b/crates/collab/src/tests/channel_guest_tests.rs index 3415c974fcd545..b071b583fff791 100644 --- a/crates/collab/src/tests/channel_guest_tests.rs +++ b/crates/collab/src/tests/channel_guest_tests.rs @@ -168,7 +168,7 @@ async fn test_channel_requires_zed_cla(cx_a: &mut TestAppContext, cx_b: &mut Tes server .app_state .db - .get_or_create_user_by_github_account("user_b", Some(100), None, Some(Utc::now()), None) + .get_or_create_user_by_github_account("user_b", 100, None, Some(Utc::now()), None) .await .unwrap(); @@ -266,7 +266,7 @@ async fn test_channel_requires_zed_cla(cx_a: &mut TestAppContext, cx_b: &mut Tes server .app_state .db - .add_contributor("user_b", Some(100), None, Some(Utc::now()), None) + .add_contributor("user_b", 100, None, Some(Utc::now()), None) .await .unwrap(); diff --git a/crates/collab/src/user_backfiller.rs b/crates/collab/src/user_backfiller.rs index 35e02bcd0a8967..83382e4ec66785 100644 --- a/crates/collab/src/user_backfiller.rs +++ b/crates/collab/src/user_backfiller.rs @@ -84,7 +84,7 @@ impl UserBackfiller { self.db .get_or_create_user_by_github_account( &user.github_login, - Some(github_user.id), + github_user.id, user.email_address.as_deref(), Some(github_user.created_at), initial_channel_id,