feat(album_artist): server-side schema for album + artist + track_artist (phase 4.d.0.1) - #34
Merged
Merged
Conversation
…ist (phase 4.d.0.1)
First PR of the 4.d.0 sprint chain — adds the entity tables the
web client needs for proper album / artist views with drill-down
on `/profiles/{p}/libraries/{l}`. Schema-only; apply pipeline
upsert lands in 4.d.0.2, REST endpoints in 4.d.0.4, web views
in 4.d.0.5.
Tables:
- `artist (id, library_id, name, picture_hash, …)` UNIQUE
(library_id, name).
- `album (id, library_id, canonical_title, album_artist_id,
year, cover_hash, is_compilation, …)` UNIQUE NULLS NOT
DISTINCT (library_id, canonical_title, album_artist_id) —
PG15+ syntax (target PG17) so two compilation rows with NULL
album_artist_id collapse to one.
- `track_artist (track_id, artist_id, position)` PK (track_id,
artist_id) with reverse-direction index (artist_id, track_id)
for the artist drill-down.
- `track.album_id BIGINT REFERENCES album(id) ON DELETE SET
NULL` + index (album_id, disc_number, track_number).
Per-library scope (matches `track.library_id`): the same album
in two libraries = two rows; dropping a library reclaims its
album + artist + track_artist rows in one cascade. Cascade
asymmetry is deliberate — join rows (`track_artist`) cascade
both ways; entity rows (`album`, `track`) use SET NULL on their
FK targets so a stray artist scrub doesn't lose the album row,
and a stray album scrub doesn't lose the audio file row.
13 schema-invariant tests cover:
- Album natural key (NULLS NOT DISTINCT collapse, same title
under different artists, compilation + attributed coexistence).
- Per-library artist name uniqueness (same name in two libs OK).
- Cascade chain (library → album / artist / track / track_artist,
track → track_artist, album → track.album_id SET NULL, artist
→ album.album_artist_id SET NULL).
- track_artist PK + position ordering.
- CHECK constraints (empty name, empty title, negative position).
CR pre-push findings applied:
- C1 (blocker): migration timestamp renamed `20260608000000` →
`20260608120000` to avoid collision with the already-merged
`20260608000000_artwork_repair_backoff.sql` — sqlx::migrate!
keys on the integer prefix, two files with the same prefix
break apply.
- H1: documented in the migration header that the apply pipeline
must order multi-artist rows by `(position ASC, artist_id ASC)`
so read sites stay deterministic on tied positions.
- M1: added `album_library_updated_idx` and
`artist_library_updated_idx` for the future
`GET /libraries/{l}/{albums,artists}` sort. Same shape as
`library_profile_updated_idx` / `track_library_added_idx` — every
entity table in this repo ships its list-query index alongside
the table.
- M2: cited the sqlx 0.9 SMALLINT → i64 narrowing rejection in
the year-column comment (same reason `track.rating` is BIGINT).
- M4: one-line note in the test header explaining the heavyweight
spawn_authenticated + REST mint choice (vs direct SQL on
`profile` + `library`).
- L5: lifted `1_700_000_000_000` to a `const FIXED_NOW_MS`.
Skipped:
- L1 (per-library vs per-profile scope): per-library is the right
call — confirmed by CR.
- L2 (NULLS NOT DISTINCT PG version): non-concern at the sqlx
layer; PG14- fails loudly at apply.
- L3 (FK asymmetry): correct as-is; documented.
- L4 (position=0 primary-artist invariant): apply-pipeline
contract, not schema. Pinned in 4.d.0.2 instead.
`cargo check --all-targets` clean.
Signed-off-by: InstaZDLL <github.105mh@8shield.net>
|
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 (3)
📝 WalkthroughWalkthroughCette PR introduit le schéma SQL Phase 4.d.0.1 pour ChangesSchéma de métadonnées musicales
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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}
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 `@migrations/20260608120000_album_artist.sql`:
- Around line 73-75: Les FK actuelles autorisent des liaisons
inter-bibliothèques : ajoutez dans la migration des contraintes qui forcent
l’égalité de library_id pour les relations concernées (par ex. remplacer ou
compléter album.album_artist_id REFERENCES artist(id) par une contrainte
composite liant (album.album_artist_id, album.library_id) à (artist.id,
artist.library_id) ou implémenter un trigger/constraint CHECK équivalent ;
faites de même pour track_artist (track_id, artist_id) → track(id,
track.library_id)/artist(id, artist.library_id) et pour track.album_id →
album(id, album.library_id)). Ensuite ajoutez des tests d’intégration négatifs
qui tentent d’insérer des relations cross-library (album→artist, track→album,
track_artist cross-library) et qui doivent échouer.
In `@tests/album_artist.rs`:
- Around line 270-423: Add negative "cross-library" tests in
tests/album_artist.rs that assert schema prevents linking entities across
libraries: create two libraries (via mint_library), then attempt (1) to set
album.album_artist_id to an artist from the other library, (2) to set
track.album_id to an album from the other library, and (3) to insert a
track_artist row pairing a track and artist from different libraries; for each
case use the existing helpers (insert_artist, insert_album, insert_track) and
perform the offending SQL (UPDATE or INSERT) expecting a database error
(constraint violation) rather than success, using async test functions named
e.g. album_artist_cross_library_constraint,
track_cross_library_album_constraint, and track_artist_cross_library_constraint
to lock the invariant against regressions.
🪄 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: 8e3e635c-1341-4561-a76f-5a1f8e5c78e8
📒 Files selected for processing (3)
CLAUDE.mdmigrations/20260608120000_album_artist.sqltests/album_artist.rs
…argo fmt
CR finding (real): the original migration's single-column FKs let
`album.album_artist_id`, `track.album_id`, and `track_artist`
pairs link entities across libraries — the per-library cascade
chain assumes intra-library locality, so a stray cross-library
link would leak rows on a library delete.
Fix: composite FKs that carry `library_id` in BOTH columns, with
the parent table's `UNIQUE (id, library_id)` as the FK target. A
try to set `album.album_artist_id` to an artist in a different
library can't satisfy the composite FK; same for `track.album_id`
and `track_artist (track_id, artist_id)`.
Schema changes:
- `track` (existing) gets `UNIQUE (id, library_id)` so dependent
tables can use it as a composite FK target.
- `artist` and `album` each get `UNIQUE (id, library_id)` for the
same reason.
- `album.album_artist_id` → composite FK with `ON DELETE SET NULL
(album_artist_id)` (PG15+ column-level form) so the album's
library_id stays intact when its artist is scrubbed.
- `track.album_id` → composite FK with `ON DELETE SET NULL
(album_id)`.
- `track_artist` gains a `library_id BIGINT NOT NULL` column with
composite FKs to BOTH `track(id, library_id)` and
`artist(id, library_id)` — the shared `library_id` column means
the only way to insert a row is for track AND artist to live in
the referenced library. The apply pipeline (4.d.0.2) derives
this value from `track.library_id` at upsert.
Test surface:
- 3 new cross-library tests:
- `album_artist_cross_library_constraint` — album in lib_a
linking to artist in lib_b is rejected.
- `track_cross_library_album_constraint` — track in lib_b
linking to album in lib_a is rejected.
- `track_artist_cross_library_constraint` — pairing a track in
lib_a with an artist in lib_b fails BOTH possible
`library_id` values (whichever we pass, one composite FK
rejects the row).
- New `insert_track_artist` / `try_insert_track_artist` helpers
(track_artist now carries library_id, so all 5 existing
callsites had to update — helper saves the repetition).
- New `try_insert_track` variant for the cross-library track
test (the existing `insert_track` panics on error).
CI lint:
- `cargo fmt --all` applied — addresses the rustfmt CI failure
on the original PR.
CLAUDE.md updated to document the cross-library guard pattern
for future entity additions.
`cargo check --all-targets` + `cargo fmt --all --check` both
clean locally.
Signed-off-by: InstaZDLL <github.105mh@8shield.net>
Owner
Author
|
@coderabbitai CI fixes + cross-library guards pushed (commit 6902056). Findings appliqués :
CLAUDE.md updated to document the cross-library guard pattern for future entity additions.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch the diff of commit 6902056
gh api repos/InstaZDLL/waveflow-server/commits/6902056 --jq '.files[] | "\(.filename)\n---\n\(.patch)\n==="'Length of output: 31756
Migration ✅
Tests ✅
Rien à redire, l'implémentation est solide. Je mets à jour la mémoire avec le pattern. 🐇 ✨ [approve] ✏️ Learnings added
🧠 Learnings used |
12 tasks
Merged
6 tasks
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
First PR of the 4.d.0 sprint chain — adds the entity tables the web client needs for proper album / artist views with drill-down on
/profiles/{p}/libraries/{l}. Schema-only; apply pipeline upsert lands in 4.d.0.2, REST endpoints in 4.d.0.4, web views in 4.d.0.5.Tables
artist (id, library_id, name, picture_hash, created_at, updated_at)UNIQUE(library_id, name).album (id, library_id, canonical_title, album_artist_id, year, cover_hash, is_compilation, …)UNIQUE NULLS NOT DISTINCT(library_id, canonical_title, album_artist_id)— PG15+ syntax (we target PG17) so two compilation rows with NULLalbum_artist_idcollapse to one.track_artist (track_id, artist_id, position)PK(track_id, artist_id)with reverse-direction index for the artist drill-down.ALTER TABLE track ADD COLUMN album_id BIGINT REFERENCES album(id) ON DELETE SET NULL+ index(album_id, disc_number, track_number).Design notes
track.library_id): same album in two libraries = two rows; dropping a library reclaims its album + artist + track_artist rows in one cascade.track_artist) cascade both ways; entity rows (album,track) useSET NULLon their FK targets so a stray artist scrub doesn't lose the album row, and a stray album scrub doesn't lose the audio file row.IS NULLmatch for compilations.CR pre-push findings applied
20260608000000→20260608120000to avoid collision with the already-merged20260608000000_artwork_repair_backoff.sql—sqlx::migrate!keys on the integer prefix, two files with the same prefix break apply.(position ASC, artist_id ASC)so read sites stay deterministic on tied positions.album_library_updated_idxandartist_library_updated_idxfor the futureGET /libraries/{l}/{albums,artists}sort. Same shape aslibrary_profile_updated_idx/track_library_added_idx.yearcolumn comment.spawn_authenticated + REST mintchoice (vs direct SQL onprofile+library).1_700_000_000_000to aconst FIXED_NOW_MS.Skipped: L1 (per-library scope confirmed correct), L2 (PG version non-concern at sqlx layer), L3 (FK asymmetry documented), L4 (apply-pipeline contract, pinned in 4.d.0.2).
Test plan
13 schema-invariant tests in
tests/album_artist.rs:cargo check --all-targetsclean locally. Tests run on CI Postgres.Summary by CodeRabbit
New Features
Documentation
Tests