feat(core): tenant-scoped methods on PostgresProfileRepository#183
Merged
Conversation
Phase 1.b.4a — give `waveflow-server` the multi-tenant API surface it needs for the upcoming `/api/v1/profiles` CRUD endpoints, without forcing the desktop's single-tenant `SqliteProfileRepository` to adopt a `user_id` concept it would never use. Strategy: - `Profile` gains a `user_id: i64` field gated with `#[sqlx(default)]`. Single-tenant SELECTs (SQLite, plus any Postgres admin probe that omits the column) get `user_id = 0` for free; multi-tenant SELECTs carry the real owner. Desktop's `commands::profile::create_profile` builds the struct directly, so it now sets `user_id: 0` explicitly — same sentinel `#[sqlx(default)]` would hand back. - `PostgresProfileRepository` grows a parallel set of **inherent** `*_for_user` methods (not on the `ProfileRepository` trait) that scope every query to a `user_id`: `list_for_user`, `get_for_user`, `insert_for_user`, `rename_for_user`, `touch_last_used_for_user`, `set_data_dir_for_user`, `delete_guarded_for_user`. The server will call these directly; the trait stays untouched so the desktop's `SqliteProfileRepository` doesn't need a sham `user_id` parameter. - `delete_guarded_for_user` mirrors the lock pattern of the single-tenant version: `LOCK TABLE profile IN SHARE ROW EXCLUSIVE MODE` inside a transaction, so two concurrent deletes from the same user can't both observe `COUNT > 1` and empty out the user's profile set. The COUNT subquery is scoped to the user, so user A's deletion is never blocked by user B's surviving rows. - The trait SELECT statements on `PostgresProfileRepository` now include `user_id` so the returned `Profile` carries the real owner for admin/debug callers; the trait `insert` keeps its prior shape (it'll fail at runtime once the schema migration in 1.b.4b adds the NOT NULL `user_id` column — a deliberate footgun documented in the inline comment, since server handlers must call `insert_for_user`). Zero behaviour change on the desktop: workspace check + clippy + 111 tests pass.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughLe changement ajoute ChangesSupport multi-locataire des profils avec tenant-scoping
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
InstaZDLL
added
scope: backend
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/crates/core/src/repository/postgres/profile.rs`:
- Around line 216-226: PostgresProfileRepository currently implements the
single-tenant ProfileRepository exposing non-scoped methods (list_all, get,
insert, etc.) which can be called without user filtering; change the API so
tenant-aware operations and admin operations are separate: introduce a new trait
(e.g., TenantProfileRepository or ProfileRepositoryForUser) that declares the
*_for_user methods and implement that on PostgresProfileRepository, and move the
unscoped admin ops into a distinct trait/impl (e.g., AdminProfileRepository or
PostgresAdminProfileRepository) used only by admin tooling; update callers to
use the tenant trait for normal server flows and the admin trait for
migrations/tests, and ensure insert/get/list_all no longer exist on the
tenant-facing trait (or require user_id) so accidental tenant-isolation loss is
prevented.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 37030bbe-0d8e-430d-969b-15084cbdf5bf
📒 Files selected for processing (3)
src-tauri/crates/app/src/commands/profile.rssrc-tauri/crates/core/src/domain/profile.rssrc-tauri/crates/core/src/repository/postgres/profile.rs
CodeRabbit flagged that leaving the single-tenant ProfileRepository
trait implemented on PostgresProfileRepository made the multi-tenant
isolation depend on a comment ("don't call list_all on a Postgres
repo"). A `Box<dyn ProfileRepository>` pointing at the Postgres impl,
or a future caller threading the repository through trait-object code,
would bypass `WHERE user_id = $1` without any compile-time signal.
Drop the trait impl entirely. Server handlers consume the inherent
`*_for_user` methods directly, so the trait was unused on the
Postgres side anyway. If a future admin tool needs cross-tenant
reads, it can get a dedicated `PostgresAdminProfileRepository`
newtype rather than re-open this footgun.
Net effect:
- Compile-time enforcement of the tenant-scoped API on Postgres
- The broken trait `insert` (no user_id to satisfy NOT NULL FK) is
gone — it would have runtime-failed against the 1.b.4b schema
- SqliteProfileRepository (single-tenant by design) is untouched
- Zero behaviour change: 111 workspace tests still pass
Refs PR #183 / CodeRabbit review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src-tauri/crates/core/src/repository/postgres/profile.rs (2)
207-220:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
set_data_dir_for_userne signale pas si la mise à jour a eu lieu.Contrairement à
rename_for_userqui retournebool, cette méthode retourne()même sirows_affected() == 0. Si le profil n'existe pas ou n'appartient pas à l'utilisateur, l'appel réussit silencieusement, ce qui peut masquer des bugs côté appelant.🛡️ Proposition : retourner bool ou lever une erreur
pub async fn set_data_dir_for_user( &self, id: i64, data_dir: &str, user_id: i64, - ) -> CoreResult<()> { - sqlx::query("UPDATE profile SET data_dir = $1 WHERE id = $2 AND user_id = $3") + ) -> CoreResult<bool> { + let result = sqlx::query("UPDATE profile SET data_dir = $1 WHERE id = $2 AND user_id = $3") .bind(data_dir) .bind(id) .bind(user_id) .execute(&self.pool) .await?; - Ok(()) + Ok(result.rows_affected() > 0) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/crates/core/src/repository/postgres/profile.rs` around lines 207 - 220, La méthode set_data_dir_for_user ne signale pas si la mise à jour a réellement touché une ligne (contrairement à rename_for_user qui retourne bool); modifiez set_data_dir_for_user pour vérifier le résultat de .execute(&self.pool).await?.rows_affected() et soit retourner bool (true si >0, false sinon) soit mapper 0 en une erreur explicite; ajustez la signature de set_data_dir_for_user et les appels consommateurs en conséquence; conservez le même SQL et les mêmes bind(data_dir), bind(id), bind(user_id) mais utilisez rows_affected() pour décider du retour ou lever une erreur.
165-167: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy lift
LOCK TABLEverrouille toute la table, pas juste les lignes du tenant.En multi-tenant, une suppression par l'utilisateur A bloque toutes les écritures de B, C, D… sur
profile. Avec Postgres, unSELECT id FROM profile WHERE id = $1 AND user_id = $2 FOR UPDATEsuivi duSELECT COUNT(*) … FOR UPDATE(ou CTE avecFOR UPDATE) offrirait un verrouillage au niveau des lignes appartenant àuser_iduniquement, sans bloquer les autres tenants.Si le volume de suppressions simultanées reste faible à court terme, ce n'est pas bloquant, mais à surveiller lors de la montée en charge.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/crates/core/src/repository/postgres/profile.rs` around lines 165 - 167, Replace the table-level LOCK used in the deletion flow (the sqlx::query("LOCK TABLE profile IN SHARE ROW EXCLUSIVE MODE") executed on &mut *tx) with row-level locking: first SELECT the target profile row using "SELECT id FROM profile WHERE id = $1 AND user_id = $2 FOR UPDATE" to acquire a lock only on the tenant’s rows, then perform the COUNT/DELETE with a SELECT ... FOR UPDATE or a CTE that locks the same rows so concurrent tenants are not blocked; update the code paths that use tx in this function to run those two SQL statements inside the same transaction instead of the global LOCK TABLE.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src-tauri/crates/core/src/repository/postgres/profile.rs`:
- Around line 207-220: La méthode set_data_dir_for_user ne signale pas si la
mise à jour a réellement touché une ligne (contrairement à rename_for_user qui
retourne bool); modifiez set_data_dir_for_user pour vérifier le résultat de
.execute(&self.pool).await?.rows_affected() et soit retourner bool (true si >0,
false sinon) soit mapper 0 en une erreur explicite; ajustez la signature de
set_data_dir_for_user et les appels consommateurs en conséquence; conservez le
même SQL et les mêmes bind(data_dir), bind(id), bind(user_id) mais utilisez
rows_affected() pour décider du retour ou lever une erreur.
- Around line 165-167: Replace the table-level LOCK used in the deletion flow
(the sqlx::query("LOCK TABLE profile IN SHARE ROW EXCLUSIVE MODE") executed on
&mut *tx) with row-level locking: first SELECT the target profile row using
"SELECT id FROM profile WHERE id = $1 AND user_id = $2 FOR UPDATE" to acquire a
lock only on the tenant’s rows, then perform the COUNT/DELETE with a SELECT ...
FOR UPDATE or a CTE that locks the same rows so concurrent tenants are not
blocked; update the code paths that use tx in this function to run those two SQL
statements inside the same transaction instead of the global LOCK TABLE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8989c062-1756-43cd-8d2e-6ae6ed24b803
📒 Files selected for processing (1)
src-tauri/crates/core/src/repository/postgres/profile.rs
Two CodeRabbit findings on PR #183: 1. `set_data_dir_for_user` returned `()` even when `rows_affected = 0`, silently masking a wrong `user_id` or a stale `id`. Change the return type to `CoreResult<bool>` matching `rename_for_user` so callers can detect the no-op case — `insert_for_user` + `set_data_dir_for_user` race bugs surface instead of swallowing. 2. `delete_guarded_for_user` held `LOCK TABLE profile IN SHARE ROW EXCLUSIVE MODE`, which serialised every tenant's writes for the duration of any user's delete. Replace with a tenant-scoped `SELECT id FROM profile WHERE user_id = $1 FOR UPDATE` at the top of the transaction: row-level locks on every profile owned by the user. Two concurrent deletes from the same user serialise on those locks (so the COUNT(*) re-check stays consistent with the DELETE), but a delete from user A doesn't block writes from user B. Note: a bare `SELECT ... FOR UPDATE` on the *target* row alone (as the first draft of CR's suggestion proposed) wouldn't be enough — two concurrent deletes targeting different rows of the same user could each lock its own row, both pass the COUNT check, and empty the set. Locking the whole tenant's row set is what closes that window. Validated: - cargo check --workspace --all-targets ✅ - cargo clippy --workspace --all-targets -- -D warnings ✅ - cargo test --workspace ✅ (66 + 45 = 111 tests, no regression)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/crates/core/src/repository/postgres/profile.rs`:
- Around line 154-165: Update the module-level header comment to match the
implementation that acquires FOR UPDATE row locks on every profile row owned by
user_id rather than describing a table-level LOCK TABLE ... SHARE ROW EXCLUSIVE
MODE; specifically mention the strategy of opening a transaction, selecting the
user's profile rows FOR UPDATE up-front so concurrent deletes serialize and the
subsequent COUNT(*) re-check sees the same row set, and remove or replace the
old LOCK TABLE wording so the module doc and the FOR UPDATE logic (the
transaction + SELECT ... FOR UPDATE and COUNT re-check) are consistent.
- Around line 173-175: The SELECT ... FOR UPDATE call using sqlx::query("SELECT
id FROM profile WHERE user_id = $1 FOR UPDATE") should enforce a deterministic
lock order—modify the query in the profile lock path (the sqlx::query(...) that
binds user_id and calls .fetch_all(&mut *tx)) to include an ORDER BY on the
stable primary key (e.g., ORDER BY id) so concurrent transactions acquire locks
in a consistent order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ccd3282e-12e0-4381-83e3-a7ca5adca41b
📒 Files selected for processing (1)
src-tauri/crates/core/src/repository/postgres/profile.rs
Two trailing CodeRabbit findings on PR #183: 1. The module header still described `LOCK TABLE … SHARE ROW EXCLUSIVE` while the implementation moved to `FOR UPDATE` row locks last commit. Rewrite the bullet to match what the code actually does — a tenant-scoped `SELECT … FOR UPDATE` plus the COUNT re-check — so a reader scanning the module doc doesn't get misled. 2. The `SELECT id FROM profile WHERE user_id = $1 FOR UPDATE` had no `ORDER BY`. Without one, the plan-driven lock acquisition order varies across sessions; combined with a concurrent INSERT it could deadlock two transactions in an A→B / B→A loop. Add `ORDER BY id` so every transaction acquires locks in the same stable PK order. Validated with `cargo clippy --workspace --all-targets -- -D warnings` and the 111-test workspace suite.
4 tasks
4 tasks
4 tasks
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
Phase 1.b.4a — give `waveflow-server` the multi-tenant API surface it needs for the upcoming `/api/v1/profiles` CRUD endpoints, without forcing the desktop's single-tenant `SqliteProfileRepository` to adopt a `user_id` concept it would never use.
Strategy
`Profile` struct gains `user_id: i64`
`#[sqlx(default)]` makes the field default to `0` when a SELECT doesn't include it — that's the desktop's behaviour today (no `user_id` column on the SQLite `profile` table) and also any Postgres admin probe that omits the column. Multi-tenant SELECTs carry the real owner.
The desktop's `commands::profile::create_profile` builds the struct directly; it now sets `user_id: 0` explicitly to match the sentinel `sqlx(default)` would hand back.
Tenant-scoped inherent methods (NOT on the trait)
`PostgresProfileRepository` grows a parallel API:
The trait `ProfileRepository` stays unchanged, so `SqliteProfileRepository` doesn't gain a sham `user_id` parameter. The server will call `*_for_user` methods exclusively.
Trait SELECTs updated
`list_all`, `get` on the Postgres impl now include `user_id` in the SELECT so an admin / debug caller sees the real owner instead of the `0` sentinel. The trait `insert` keeps its previous shape — it'll fail at runtime once the schema migration in 1.b.4b adds the NOT NULL `user_id` column. That's a deliberate footgun documented inline, since server handlers must call `insert_for_user`.
Out of scope
Test plan
Summary by CodeRabbit
Notes de version