Skip to content

Commit 01bad8d

Browse files
committed
Remove the tokens table
This table was always one-to-one with the emails table, and we should have always been updating it when the email was updated. This moves all of that logic into the database, and merges the two tables. The only reason that these tables were ever separate as far as I can tell was to represent when the confirmation email hadn't been sent. However, we can just do that with a nullable column. However, having the `token` column leads to some awkward `unwrap`s in the code where we return it, since we know that updating the `email` always generates a new token. For that reason I have made the `token` column `NOT NULL`, but left the timestamp nullable. Rows in the `emails` table that did not previously have an associated `token` row will have a `NULL` value for this column.
1 parent fba2ade commit 01bad8d

File tree

7 files changed

+134
-196
lines changed

7 files changed

+134
-196
lines changed
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
DROP TRIGGER trigger_emails_reconfirm ON emails;
2+
DROP TRIGGER trigger_emails_set_token_generated_at ON emails;
3+
DROP FUNCTION reconfirm_email_on_email_change();
4+
DROP FUNCTION emails_set_token_generated_at();
5+
6+
CREATE TABLE tokens (
7+
id SERIAL PRIMARY KEY,
8+
email_id INTEGER NOT NULL UNIQUE REFERENCES emails (id),
9+
token TEXT NOT NULL,
10+
created_at TIMESTAMP NOT NULL DEFAULT NOW()
11+
);
12+
13+
INSERT INTO tokens (email_id, token, created_at)
14+
SELECT id, token, token_generated_at FROM emails WHERE token_generated_at IS NOT NULL;
15+
16+
ALTER TABLE emails DROP COLUMN token;
17+
ALTER TABLE emails DROP COLUMN token_generated_at;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
ALTER TABLE emails ADD COLUMN token TEXT NOT NULL DEFAULT random_string(26);
2+
ALTER TABLE emails ADD COLUMN token_generated_at TIMESTAMP;
3+
UPDATE emails SET token = tokens.token, token_generated_at = tokens.created_at
4+
FROM tokens WHERE tokens.email_id = emails.id;
5+
DROP TABLE tokens;
6+
7+
CREATE FUNCTION emails_set_token_generated_at() RETURNS trigger AS $$
8+
BEGIN
9+
NEW.token_generated_at := CURRENT_TIMESTAMP;
10+
RETURN NEW;
11+
END
12+
$$ LANGUAGE plpgsql;
13+
14+
CREATE FUNCTION reconfirm_email_on_email_change() RETURNS trigger AS $$
15+
BEGIN
16+
IF NEW.email IS DISTINCT FROM OLD.email THEN
17+
NEW.token := random_string(26);
18+
NEW.verified := false;
19+
END IF;
20+
RETURN NEW;
21+
END
22+
$$ LANGUAGE plpgsql;
23+
24+
CREATE TRIGGER trigger_emails_set_token_generated_at BEFORE
25+
INSERT OR UPDATE OF token ON emails
26+
FOR EACH ROW EXECUTE PROCEDURE emails_set_token_generated_at();
27+
28+
CREATE TRIGGER trigger_emails_reconfirm BEFORE UPDATE
29+
ON emails
30+
FOR EACH ROW EXECUTE PROCEDURE reconfirm_email_on_email_change();
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
ALTER TABLE emails DROP CONSTRAINT fk_emails_user_id;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
ALTER TABLE emails ADD CONSTRAINT fk_emails_user_id FOREIGN KEY (user_id) REFERENCES users (id);

src/schema.rs

+22-42
Original file line numberDiff line numberDiff line change
@@ -437,6 +437,18 @@ table! {
437437
///
438438
/// (Automatically generated by Diesel.)
439439
verified -> Bool,
440+
/// The `token` column of the `emails` table.
441+
///
442+
/// Its SQL type is `Text`.
443+
///
444+
/// (Automatically generated by Diesel.)
445+
token -> Text,
446+
/// The `token_generated_at` column of the `emails` table.
447+
///
448+
/// Its SQL type is `Nullable<Timestamp>`.
449+
///
450+
/// (Automatically generated by Diesel.)
451+
token_generated_at -> Nullable<Timestamp>,
440452
}
441453
}
442454

@@ -558,38 +570,6 @@ table! {
558570
}
559571
}
560572

561-
table! {
562-
/// Representation of the `tokens` table.
563-
///
564-
/// (Automatically generated by Diesel.)
565-
tokens (id) {
566-
/// The `id` column of the `tokens` table.
567-
///
568-
/// Its SQL type is `Int4`.
569-
///
570-
/// (Automatically generated by Diesel.)
571-
id -> Int4,
572-
/// The `email_id` column of the `tokens` table.
573-
///
574-
/// Its SQL type is `Int4`.
575-
///
576-
/// (Automatically generated by Diesel.)
577-
email_id -> Int4,
578-
/// The `token` column of the `tokens` table.
579-
///
580-
/// Its SQL type is `Varchar`.
581-
///
582-
/// (Automatically generated by Diesel.)
583-
token -> Varchar,
584-
/// The `created_at` column of the `tokens` table.
585-
///
586-
/// Its SQL type is `Timestamp`.
587-
///
588-
/// (Automatically generated by Diesel.)
589-
created_at -> Timestamp,
590-
}
591-
}
592-
593573
table! {
594574
/// Representation of the `users` table.
595575
///
@@ -784,22 +764,22 @@ table! {
784764
}
785765
}
786766

787-
joinable!(follows -> users (user_id));
788-
joinable!(version_authors -> users (user_id));
789-
joinable!(crates_keywords -> keywords (keyword_id));
790-
joinable!(crates_categories -> categories (category_id));
791767
joinable!(api_tokens -> users (user_id));
792768
joinable!(crate_downloads -> crates (crate_id));
769+
joinable!(crate_owner_invitations -> crates (crate_id));
793770
joinable!(crate_owners -> crates (crate_id));
771+
joinable!(crate_owners -> teams (owner_id));
772+
joinable!(crate_owners -> users (owner_id));
773+
joinable!(crates_categories -> categories (category_id));
794774
joinable!(crates_categories -> crates (crate_id));
795775
joinable!(crates_keywords -> crates (crate_id));
776+
joinable!(crates_keywords -> keywords (keyword_id));
796777
joinable!(dependencies -> crates (crate_id));
797-
joinable!(follows -> crates (crate_id));
798-
joinable!(versions -> crates (crate_id));
799778
joinable!(dependencies -> versions (version_id));
779+
joinable!(emails -> users (user_id));
780+
joinable!(follows -> crates (crate_id));
781+
joinable!(follows -> users (user_id));
782+
joinable!(version_authors -> users (user_id));
800783
joinable!(version_authors -> versions (version_id));
801784
joinable!(version_downloads -> versions (version_id));
802-
joinable!(crate_owners -> teams (owner_id));
803-
joinable!(crate_owners -> users (owner_id));
804-
joinable!(crate_owner_invitations -> crates (crate_id));
805-
joinable!(tokens -> emails (email_id));
785+
joinable!(versions -> crates (crate_id));

src/tests/user.rs

+21-33
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,7 @@ use conduit::{Handler, Method};
44

55
use cargo_registry::token::ApiToken;
66
use cargo_registry::krate::EncodableCrate;
7-
use cargo_registry::user::{Email, EncodablePrivateUser, EncodablePublicUser, NewEmail, NewUser,
8-
Token, User};
7+
use cargo_registry::user::{Email, EncodablePrivateUser, EncodablePublicUser, NewUser, User};
98
use cargo_registry::version::EncodableVersion;
109

1110
use diesel::prelude::*;
@@ -658,7 +657,7 @@ fn test_insert_into_email_table_with_email_change() {
658657
*/
659658
#[test]
660659
fn test_confirm_user_email() {
661-
use cargo_registry::schema::{emails, tokens};
660+
use cargo_registry::schema::emails;
662661

663662
#[derive(Deserialize)]
664663
struct R {
@@ -675,7 +674,7 @@ fn test_confirm_user_email() {
675674
let user = {
676675
let conn = app.diesel_database.get().unwrap();
677676
let user = NewUser {
678-
email: Some("potato@example.com"),
677+
email: Some("potato2@example.com"),
679678
..::new_user("potato")
680679
};
681680

@@ -686,15 +685,10 @@ fn test_confirm_user_email() {
686685

687686
let email_token = {
688687
let conn = app.diesel_database.get().unwrap();
689-
let email_info = emails::table
690-
.filter(emails::user_id.eq(user.id))
691-
.first::<Email>(&*conn)
692-
.unwrap();
693-
let token_info = tokens::table
694-
.filter(tokens::email_id.eq(email_info.id))
695-
.first::<Token>(&*conn)
696-
.unwrap();
697-
token_info.token
688+
Email::belonging_to(&user)
689+
.select(emails::token)
690+
.first::<String>(&*conn)
691+
.unwrap()
698692
};
699693

700694
let mut response = ok_resp!(
@@ -707,7 +701,7 @@ fn test_confirm_user_email() {
707701

708702
let mut response = ok_resp!(middle.call(req.with_path("/api/v1/me").with_method(Method::Get),));
709703
let r = ::json::<R>(&mut response);
710-
assert_eq!(r.user.email.unwrap(), "potato@example.com");
704+
assert_eq!(r.user.email.unwrap(), "potato2@example.com");
711705
assert_eq!(r.user.login, "potato");
712706
assert!(r.user.email_verified);
713707
assert!(r.user.email_verification_sent);
@@ -719,8 +713,9 @@ fn test_confirm_user_email() {
719713
*/
720714
#[test]
721715
fn test_existing_user_email() {
722-
use cargo_registry::schema::{emails, users};
723-
use diesel::insert;
716+
use cargo_registry::schema::emails;
717+
use chrono::NaiveDateTime;
718+
use diesel::update;
724719

725720
#[derive(Deserialize)]
726721
struct R {
@@ -731,24 +726,17 @@ fn test_existing_user_email() {
731726
let mut req = ::req(app.clone(), Method::Get, "/me");
732727
{
733728
let conn = app.diesel_database.get().unwrap();
734-
let user = ::new_user("potahto");
735-
736-
// Deliberately not using User::create_or_update since that
737-
// will try to send a verification email; we want to simulate
738-
// a user who already had an email before we added verification.
739-
let user = insert(&user)
740-
.into(users::table)
741-
.get_result::<User>(&*conn)
742-
.unwrap();
743-
744-
let email = NewEmail {
745-
user_id: user.id,
746-
747-
verified: false,
729+
let new_user = NewUser {
730+
email: Some("[email protected]"),
731+
..::new_user("potahto")
748732
};
749-
750-
insert(&email).into(emails::table).execute(&*conn).unwrap();
751-
733+
let user = new_user.create_or_update(&conn).unwrap();
734+
update(Email::belonging_to(&user))
735+
// Users created before we added verification will have
736+
// `NULL` in the `token_generated_at` column.
737+
.set(emails::token_generated_at.eq(None::<NaiveDateTime>))
738+
.execute(&*conn)
739+
.unwrap();
752740
::sign_in_as(&mut req, &user);
753741
}
754742

0 commit comments

Comments
 (0)