feat: support deleting users who own rooms or have messages#5
Merged
Merged
Conversation
Looking forward to implementing rooms and messages inspired these updates. DELETE FROM users previously failed at the FK for any user with messages or owned rooms. Now: - messages.sender_id is nullable with ON DELETE SET NULL, paired with a sender_username_snapshot column and a CHECK constraint ensuring every message stays attributable - the delete handler snapshots the username onto the user's messages and reassigns owned rooms to the default admin via an inline subquery (no default admin required when the user owns no rooms) - a SELECT ... FOR UPDATE on the target user serializes against concurrent message inserts, closing the race where a new row could slip in between the snapshot and the cascading SET NULL and trip the CHECK I updated the migrations file instead of making a new one because no one is using it but me since it only became public today.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
DELETE FROM userspreviously failed at the foreign-key check for any user with an active footprint in the system:messages.sender_idwasNOT NULLwith the defaultNO ACTION— so deleting a user who'd ever sent a message errored.rooms.owner_idwasNOT NULLwith the defaultNO ACTION— so deleting a user who owned a room errored.In practice this meant user deletion only worked for accounts that had done essentially nothing. Kicking a real participant required manual cleanup first, which isn't a workflow we want admins to live in.
Schema changes
Edited the single existing migration (
20260519000001_init.sql) since the DB is pre-release and only exists in dev environments — no new migration is warranted.messagesnow looks like:sender_idNOT NULLremoved;ON DELETE SET NULLaddedsender_username_snapshot TEXTCHECK (sender_id IS NOT NULL OR sender_username_snapshot IS NOT NULL)rooms.owner_idstaysNOT NULLwithRESTRICTsemantics — reassignment happens explicitly in the delete handler instead of via a cascade, so we don't risk silentlynuking rooms.
Handler changes (
DeleteUserRequest)The handler now runs inside a single transaction. After the existing auth checks (admin-or-self, default-admin guard), the new flow is:
SELECT user_id FROM users WHERE user_id = \$1 FOR UPDATE— takes a row-level lock on the target. Because FK enforcement onmessages.sender_idtakesFOR KEY SHAREon the parent, this serializes against concurrent message inserts and closes the race where a new message could slip in between the snapshot and the cascadingSET NULLand trip theCHECKconstraint.UPDATE messages SET sender_username_snapshot = u.username FROM users u WHERE messages.sender_id = \$1 AND u.user_id = \$1.UPDATE rooms SET owner_id = (SELECT user_id FROM admins WHERE is_default = true) WHERE owner_id = \$1. No-op if the user owns no rooms; rolls back cleanly viaNOT NULLviolation if rooms exist and no default admin does.DELETE FROM users— cascades handle credentials/admins/last_active/memberships;ON DELETE SET NULLnullsmessages.sender_id; the snapshot from step 2satisfies the
CHECK.Design choices worth flagging
Snapshot-on-delete vs. snapshot-on-insert. Two viable schemas: snapshot the username on every message row at insert time (always-populated
sender_username TEXT NOT NULL), or snapshot only at deletion time (nullable column populated by the delete handler). Went with the latter sousersremains the single source of truth whilethe author is live — username renames propagate naturally, and the snapshot only freezes identity at the moment it actually matters.
Inline subquery for room reassignment. An earlier draft fetched the default admin via the existing
get_default_adminhelper and then did the UPDATE. That requireda default admin to exist even when the user owned zero rooms, which broke two tests (
user_deletes_self,self_deleted_user_cannot_authenticate) where no admin isseeded. The subquery form has the right semantics for free: zero rooms → zero rows affected → no default-admin requirement.
Loss of explicit "no default admin" error. With the subquery form, the failure mode for "user owns rooms but no default admin exists" is a generic
NOT NULLviolation on
rooms.owner_idinstead of an explicit branch. Since logging is still a TODO across this handler and the response isFailedeither way, the loss offidelity is invisible today. Worth revisiting once structured logging lands.
Test plan
cargo fmt --all --checkcleancargo clippy --all-targets --locked -- -D warningscleancargo test --locked— all 11 tests intests/delete_user.rspass, including previously-failinguser_deletes_selfandself_deleted_user_cannot_authenticate