Skip to content

Gh login cant be unique #1109

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 9, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions migrations/2017-09-26-200549_users_lowercase_index/down.sql
Original file line number Diff line number Diff line change
@@ -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
12 changes: 9 additions & 3 deletions migrations/2017-09-26-200549_users_lowercase_index/up.sql
Original file line number Diff line number Diff line change
@@ -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;
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
-- This file intentionally left blank; see the corresponding up.sql
Original file line number Diff line number Diff line change
@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still want this index to be on the lowercase name though, don't we? Should we have another migration that is along the lines of:

DROP INDEX index_users_gh_login;
CREATE INDEX lower_gh_login ON users (lower(gh_login));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, good call!

ALTER TABLE users DROP CONSTRAINT IF EXISTS unique_gh_login;
DROP INDEX IF EXISTS lower_gh_login;
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
DROP INDEX IF EXISTS lower_gh_login;
CREATE INDEX index_users_gh_login ON users (gh_login);
2 changes: 2 additions & 0 deletions migrations/2017-10-09-135625_add_lower_gh_login_index/up.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
DROP INDEX IF EXISTS index_users_gh_login;
CREATE INDEX lower_gh_login ON users (lower(gh_login));
34 changes: 30 additions & 4 deletions src/tests/user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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("[email protected]"), None, None, "bar")
.create_or_update(&conn)
NewUser::new(
1,
"foobar",
Some("[email protected]"),
Some("I was first then deleted my github account"),
None,
"bar"
).create_or_update(&conn)
);
t!(
NewUser::new(
2,
"FOOBAR",
Some("[email protected]"),
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]
Expand Down
3 changes: 2 additions & 1 deletion src/user/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -456,12 +456,13 @@ pub fn me(req: &mut Request) -> CargoResult<Response> {

/// Handles the `GET /users/:user_id` route.
pub fn show(req: &mut Request) -> CargoResult<Response> {
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::<User>(&*conn)?;

#[derive(Serialize)]
Expand Down