From e7a490bc39afbd2c1d20eb36a94a026b6bce1b94 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Fri, 6 Oct 2017 19:44:27 -0400 Subject: [PATCH 1/4] Remove unique gh_login index migration because it can't run on prod --- .../2017-09-26-200549_users_lowercase_index/down.sql | 4 +--- .../2017-09-26-200549_users_lowercase_index/up.sql | 12 +++++++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/migrations/2017-09-26-200549_users_lowercase_index/down.sql b/migrations/2017-09-26-200549_users_lowercase_index/down.sql index 9d0bfbb293b..04f210b326f 100644 --- a/migrations/2017-09-26-200549_users_lowercase_index/down.sql +++ b/migrations/2017-09-26-200549_users_lowercase_index/down.sql @@ -1,3 +1 @@ -DROP INDEX lower_gh_login; -ALTER TABLE users DROP CONSTRAINT unique_gh_login; -CREATE INDEX index_users_gh_login ON users (gh_login); +-- This migration intentionally left blank; see corresponding up.sql diff --git a/migrations/2017-09-26-200549_users_lowercase_index/up.sql b/migrations/2017-09-26-200549_users_lowercase_index/up.sql index 83199c98380..1d779444ddb 100644 --- a/migrations/2017-09-26-200549_users_lowercase_index/up.sql +++ b/migrations/2017-09-26-200549_users_lowercase_index/up.sql @@ -1,3 +1,9 @@ -DROP INDEX IF EXISTS index_users_gh_login; -ALTER TABLE users ADD CONSTRAINT unique_gh_login UNIQUE(gh_login); -CREATE UNIQUE INDEX lower_gh_login ON users (lower(gh_login)); +-- This migration was merged into the master branch but could not be deployed to production +-- because production gh_login isn't actually unique. Later, this migration was commented out +-- so that it will be a no-op on production, and a new migration was added to correct the database +-- of anyone who ran this migration locally. +-- +-- DROP INDEX IF EXISTS index_users_gh_login; +-- ALTER TABLE users ADD CONSTRAINT unique_gh_login UNIQUE(gh_login); +-- CREATE UNIQUE INDEX lower_gh_login ON users (lower(gh_login)); +SELECT 1; \ No newline at end of file From 10d88b1f669f646746da2cf8f5077c8f5d4fb917 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Fri, 6 Oct 2017 19:51:59 -0400 Subject: [PATCH 2/4] Create migrations to fix anyone's local db if they ran 2017-09-26-200549 This should have no effect on production. --- .../down.sql | 1 + .../2017-10-06-234455_fix_local_dbs_unique_gh_login/up.sql | 7 +++++++ 2 files changed, 8 insertions(+) create mode 100644 migrations/2017-10-06-234455_fix_local_dbs_unique_gh_login/down.sql create mode 100644 migrations/2017-10-06-234455_fix_local_dbs_unique_gh_login/up.sql diff --git a/migrations/2017-10-06-234455_fix_local_dbs_unique_gh_login/down.sql b/migrations/2017-10-06-234455_fix_local_dbs_unique_gh_login/down.sql new file mode 100644 index 00000000000..e218a66df6d --- /dev/null +++ b/migrations/2017-10-06-234455_fix_local_dbs_unique_gh_login/down.sql @@ -0,0 +1 @@ +-- This file intentionally left blank; see the corresponding up.sql \ No newline at end of file diff --git a/migrations/2017-10-06-234455_fix_local_dbs_unique_gh_login/up.sql b/migrations/2017-10-06-234455_fix_local_dbs_unique_gh_login/up.sql new file mode 100644 index 00000000000..3e92ddbf169 --- /dev/null +++ b/migrations/2017-10-06-234455_fix_local_dbs_unique_gh_login/up.sql @@ -0,0 +1,7 @@ +-- This migration should have no effect on production. +-- Its sole purpose is to fix the local database of anyone who ran 2017-09-26-200549 locally-- +-- that migration can't be run on production because lower(gh_login) isn't unique on production. + +CREATE INDEX IF NOT EXISTS index_users_gh_login ON users (gh_login); +ALTER TABLE users DROP CONSTRAINT IF EXISTS unique_gh_login; +DROP INDEX IF EXISTS lower_gh_login; \ No newline at end of file From 40ca32d736649e7b4c032be06b1ca73ad40b08d7 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Fri, 6 Oct 2017 20:03:40 -0400 Subject: [PATCH 3/4] When there are users with same gh_login, choose the latest id This is the latest user that crates.io knows about with this username. --- src/tests/user.rs | 34 ++++++++++++++++++++++++++++++---- src/user/mod.rs | 3 ++- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/src/tests/user.rs b/src/tests/user.rs index e1d88d9ed7b..0a8902d9dea 100644 --- a/src/tests/user.rs +++ b/src/tests/user.rs @@ -81,20 +81,46 @@ fn show() { } #[test] -fn show_case_insensitive() { +fn show_latest_user_case_insensitively() { let (_b, app, middle) = ::app(); { let conn = t!(app.diesel_database.get()); + // Please do not delete or modify the setup of this test in order to get it to pass. + // This setup mimics how GitHub works. If someone abandons a GitHub account, the username is + // available for anyone to take. We need to support having multiple user accounts + // with the same gh_login in crates.io. `gh_id` is stable across renames, so that field + // should be used for uniquely identifying GitHub accounts whenever possible. For the + // crates.io/user/:username pages, the best we can do is show the last crates.io account + // created with that username. t!( - NewUser::new(1, "foobar", Some("foo@bar.com"), None, None, "bar") - .create_or_update(&conn) + NewUser::new( + 1, + "foobar", + Some("foo@bar.com"), + Some("I was first then deleted my github account"), + None, + "bar" + ).create_or_update(&conn) + ); + t!( + NewUser::new( + 2, + "FOOBAR", + Some("later-foo@bar.com"), + Some("I was second, I took the foobar username on github"), + None, + "bar" + ).create_or_update(&conn) ); } let mut req = ::req(app.clone(), Method::Get, "api/v1/users/fOObAr"); let mut response = ok_resp!(middle.call(&mut req)); let json: UserShowPublicResponse = ::json(&mut response); - assert_eq!("foobar", json.user.login); + assert_eq!( + "I was second, I took the foobar username on github", + json.user.name.unwrap() + ); } #[test] diff --git a/src/user/mod.rs b/src/user/mod.rs index 68a4193af58..cce723f49c2 100644 --- a/src/user/mod.rs +++ b/src/user/mod.rs @@ -456,12 +456,13 @@ pub fn me(req: &mut Request) -> CargoResult { /// Handles the `GET /users/:user_id` route. pub fn show(req: &mut Request) -> CargoResult { - use self::users::dsl::{gh_login, users}; + use self::users::dsl::{gh_login, id, users}; let name = &req.params()["user_id"].to_lowercase(); let conn = req.db_conn()?; let user = users .filter(::lower(gh_login).eq(name)) + .order(id.desc()) .first::(&*conn)?; #[derive(Serialize)] From e691e7597ddf0eb1b2bb1996a6c5da0b1e5658fe Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Mon, 9 Oct 2017 09:57:39 -0400 Subject: [PATCH 4/4] Add a regular, non-unique index on lower(gh_login) --- migrations/2017-10-09-135625_add_lower_gh_login_index/down.sql | 2 ++ migrations/2017-10-09-135625_add_lower_gh_login_index/up.sql | 2 ++ 2 files changed, 4 insertions(+) create mode 100644 migrations/2017-10-09-135625_add_lower_gh_login_index/down.sql create mode 100644 migrations/2017-10-09-135625_add_lower_gh_login_index/up.sql diff --git a/migrations/2017-10-09-135625_add_lower_gh_login_index/down.sql b/migrations/2017-10-09-135625_add_lower_gh_login_index/down.sql new file mode 100644 index 00000000000..0c11f8c69d5 --- /dev/null +++ b/migrations/2017-10-09-135625_add_lower_gh_login_index/down.sql @@ -0,0 +1,2 @@ +DROP INDEX IF EXISTS lower_gh_login; +CREATE INDEX index_users_gh_login ON users (gh_login); diff --git a/migrations/2017-10-09-135625_add_lower_gh_login_index/up.sql b/migrations/2017-10-09-135625_add_lower_gh_login_index/up.sql new file mode 100644 index 00000000000..bbdf72f6abc --- /dev/null +++ b/migrations/2017-10-09-135625_add_lower_gh_login_index/up.sql @@ -0,0 +1,2 @@ +DROP INDEX IF EXISTS index_users_gh_login; +CREATE INDEX lower_gh_login ON users (lower(gh_login));