Allow emails table to contain multiple emails per user
#11642
Conversation
kailan
force-pushed
the
multiple-emails-schema
branch
3 times, most recently
from
f6ebfe0 to
77ebf49
Compare
kailan
force-pushed
the
multiple-emails-schema
branch
from
77ebf49 to
edf516e
Compare
| @@ -538,6 +538,9 @@ diesel::table! { | |||
| /// | |||
| /// (Automatically generated by Diesel.) | |||
| token_generated_at -> Nullable<Timestamptz>, | |||
| /// Whether this email is the primary email address for the user. | |||
| #[sql_name = "is_primary"] | |||
There was a problem hiding this comment.
primary is a reserved keyword in Postgres, so we use is_primary as the column name and then alias it to primary in the Diesel schema to be consistent with verified.
| // Otherwise, insert a new email | ||
| self.insert(conn).await |
There was a problem hiding this comment.
probably not worth the extra complexity fixing it, but worth mentioning: if insert_or_update_primary() is called on a NewEmail with primary: false, then this code path would not add it as a primary email. the only call site of this method is currently setting primary: true, but I'm wondering whether we should at least error!(...) log that case for now.
| @@ -536,13 +536,14 @@ | ||
| /// | ||
| /// Its SQL type is `Nullable<Timestamptz>`. | ||
| /// | ||
| /// (Automatically generated by Diesel.) | ||
| token_generated_at -> Nullable<Timestamptz>, | ||
| /// Whether this email is the primary email address for the user. | ||
| - is_primary -> Bool, | ||
| + #[sql_name = "is_primary"] | ||
| + primary -> Bool, | ||
| } | ||
| } | ||
|
|
||
| diesel::table! { | ||
| /// Representation of the `follows` table. | ||
| /// |
There was a problem hiding this comment.
since the patch file is already a bit hard to maintain, I think it might be easier to just use is_primary directly in our code? 😅
| -- Set `is_primary` to true for existing emails | ||
| UPDATE emails SET is_primary = true; |
There was a problem hiding this comment.
we currently have 72k rows in the emails table. I'm a bit scared about running this as part of the migration, since the running migration will block the API server from restarting.
@eth3lbert any thoughts on this?
I guess we could extract the add column is_primary above to a dedicated migration/PR, deploy that to run the migration, then manually run the update is_primary = true without blocking the API server, and then merge/deploy the rest of this migration 🤔
There was a problem hiding this comment.
I guess we could extract the
add column is_primaryabove to a dedicated migration/PR, deploy that to run the migration, then manually run theupdate is_primary = truewithout blocking the API server, and then merge/deploy the rest of this migration 🤔
Yeah, I second this! I think this would be the most proper way to minimize potential issues caused by the introduced database migration.
| ALTER TABLE emails ADD CONSTRAINT unique_primary_email_per_user | ||
| EXCLUDE USING gist ( | ||
| user_id WITH =, | ||
| (is_primary::int) WITH = |
There was a problem hiding this comment.
is this line needed? if we filter below using WHERE is_primary then I guess the above line would be redundant?
| -- Ensure exactly one primary email per user after any insert or update | ||
| CREATE FUNCTION verify_exactly_one_primary_email() | ||
| RETURNS TRIGGER AS $$ | ||
| DECLARE | ||
| primary_count integer; | ||
| BEGIN | ||
| -- Count primary emails for the affected user | ||
| SELECT COUNT(*) INTO primary_count | ||
| FROM emails | ||
| WHERE user_id = COALESCE(NEW.user_id, OLD.user_id) | ||
| AND is_primary = true; | ||
|
|
||
| IF primary_count != 1 THEN | ||
| RAISE EXCEPTION 'User must have one primary email, found %', primary_count; | ||
| END IF; | ||
|
|
||
| RETURN COALESCE(NEW, OLD); | ||
| END; | ||
| $$ LANGUAGE plpgsql; | ||
|
|
||
| CREATE TRIGGER trigger_verify_exactly_one_primary_email | ||
| AFTER INSERT OR UPDATE ON emails | ||
| FOR EACH ROW | ||
| EXECUTE FUNCTION verify_exactly_one_primary_email(); |
There was a problem hiding this comment.
isn't this redundant with the unique_primary_email_per_user constraint above? 🤔
Turbo87
added
C-enhancement ✨
|
Haven't forgotten about this! Thanks for your review – aiming to address your comments soon. |
This PR is split from #11629 – it contains just the database schema-related changes, with no changes to the API, frontend, or any user-visible behaviour, barring a change to the description of three properties of the
/api/v1/meresponse in the OpenAPI schema.I previously had a note here about it being possible for an email address to exist on more than one user account, but this is the current behaviour in production so I have removed it.
Relates to #11597.