feat(core): rename_for_user returns the updated row - #184
Merged
Conversation
Follow-up on CodeRabbit's atomicity finding against waveflow-server PR #6: the server handler did `rename_for_user` (bool) → `get_for_user` (Option<Profile>), which can flip to a misleading 404 if a concurrent DELETE commits between the two calls. Change the signature to `Option<Profile>` and run the SQL as `UPDATE … RETURNING id, user_id, name, color_id, avatar_hash, data_dir, created_at, last_used_at` so the updated row comes back in one round-trip. The caller no longer needs the post- rename SELECT — eliminating the race window entirely. `insert_for_user` and `touch_last_used_for_user` have the same two-statement shape today; they're left as-is since neither has an active consumer racing the read-back. Worth revisiting when they grow one.
📝 WalkthroughWalkthroughLa PR met à jour le contrat et l'implémentation de la méthode ChangesSignature et implémentation de rename_for_user
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This was referenced May 30, 2026
InstaZDLL
added a commit
that referenced
this pull request
May 30, 2026
) Same shape as the profile work in PRs #181 / #183 / #184: server-only inherent methods that scope every query to both `profile_id` (the resource's owning profile) and `user_id` (the request's authenticated user). The single-tenant `LibraryRepository` trait stays untouched on the desktop side, and `PostgresLibraryRepository` deliberately does NOT implement it — a careless `Box<dyn LibraryRepository>` over the Postgres backend would otherwise let user A walk user B's libraries. Methods (5): - `list_for_profile(profile_id, user_id)` — MRU-first, empty list when the user doesn't own the profile (no tenancy leak, no auth pre-check round-trip) - `get_for_profile(id, profile_id, user_id)` — single row, `None` blurs missing / foreign-profile / foreign-user - `insert_for_profile(draft, profile_id, user_id)` — `INSERT ... SELECT FROM profile WHERE id = $1 AND user_id = $7`, returns the inserted row via `RETURNING *` so the caller skips a follow-up SELECT (same race elimination as PR #184's rename_for_user) - `update_for_profile(id, patch, now_ms, profile_id, user_id)` — COALESCE partial update, `UPDATE ... RETURNING *` for the same reason - `delete_for_profile(id, profile_id, user_id)` — `EXISTS` clause on profile validates ownership without a separate join Every SQL statement encodes the (profile_id, user_id) ownership pair in its WHERE clause so the storage layer is the single point of enforcement — no convention-by-comment, no handler-discipline gap. Domain: - `Library` gains `profile_id: i64` with `#[sqlx(default)]`, mirroring `Profile.user_id`. Desktop SELECTs that omit the column (no `profile_id` on the per-profile SQLite `library` table) still round-trip cleanly via the default. The lone desktop call site (`commands/library::create_library`) now sets `profile_id: 0` explicitly to match. Counts (`track_count`, `album_count`, `artist_count`, `genre_count`, `folder_count`) are stubbed at `0::bigint` in every SELECT for this phase; they become real aggregates as tracks / albums / playlists land in 1.b.5b+, without changing the wire shape. Schema lives in `waveflow-server/migrations/` (next PR): `library.profile_id BIGINT NOT NULL REFERENCES profile(id) ON DELETE CASCADE` + the usual indices. Zero behaviour change on the desktop. Validated: workspace check + clippy + 111 tests pass.
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
1 participant
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.
Summary
Follow-up on CodeRabbit's atomicity finding against waveflow-server PR #6. The server's PATCH handler did `rename_for_user` (bool) → `get_for_user` (Option), which can flip to a misleading 404 if a concurrent DELETE commits between the two calls.
Change
`PostgresProfileRepository::rename_for_user` now returns `Option` and runs the SQL as `UPDATE … RETURNING …`. The updated row comes back in one round-trip; the caller no longer needs a post-rename SELECT, which eliminates the race window entirely.
`insert_for_user` and `touch_last_used_for_user` have the same two-statement shape today; left as-is since neither has an active consumer racing the read-back. Worth revisiting if they grow one.
Test plan
Summary by CodeRabbit
Notes de version