feat(playlists): owner-facing /tracks read endpoint (phase 1.j.c) - #33
Conversation
The web client lists playlist tracks via the new
`GET /api/v1/profiles/{profile_id}/playlists/{id}/tracks`. Same
tenant chain as `get_playlist` (playlist -> profile -> user), 404
blurs missing/foreign rows.
The owner read does NOT filter NULL snapshots — pre-1.j.b desktops
emitted ops without them, and the rows still belong to the
playlist; only the public share preview applies the snapshot
filter. Ordering is `(position ASC, track_id ASC)` so the
tiebreaker stays deterministic (the PK guarantees uniqueness).
`fetch_for_owner` is intentionally two queries (ownership check
+ tracks fetch); a single CTE would conflate "not owned" with
"owned but empty" and we need the 404-vs-[] distinction at the
HTTP boundary. The race window between the two queries is benign
because `playlist_track.playlist_id` is `ON DELETE CASCADE`.
Tests use a direct SQL seed helper rather than the apply pipeline
so the assertion surface stays on the owner endpoint's contract.
Covers: empty -> [], position+track_id ordering with NULL
snapshot pass-through and `added_at` round-trip, foreign-tenant
404 proxy attack on both profile ids, unknown playlist 404,
missing-auth 401. Tiebreaker is exercised via two rows sharing
the same position so a future drop of `track_id ASC` from the
SQL would fail the test.
OpenAPI guard test extended so a future refactor that drops
`#[utoipa::path]` from the new handler trips before the docs
go stale.
Signed-off-by: InstaZDLL <github.105mh@8shield.net>
📝 WalkthroughWalkthroughCe PR ajoute un endpoint ChangesPlaylist track listing feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as list_playlist_tracks
participant DBFn as fetch_for_owner
participant Postgres
Client->>API: GET /api/v1/profiles/{profile_id}/playlists/{id}/tracks
API->>DBFn: fetch_for_owner(id, profile_id, user_id)
DBFn->>Postgres: verify ownership chain
Postgres-->>DBFn: ownership result
alt Not owner or playlist missing
DBFn-->>API: None
API-->>Client: 404 playlist not found
else Owner verified
DBFn->>Postgres: select tracks ordered by position then track_id
Postgres-->>DBFn: rows
DBFn-->>API: Some rows
API-->>Client: 200 track list
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
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 `@tests/playlists.rs`:
- Around line 637-913: Update the tests to return anyhow::Result<()> and replace
all unwrap()/expect() chains with ? so failures produce rich errors: change the
signatures of list_tracks_empty_playlist_returns_empty_array,
list_tracks_returns_position_order_with_snapshots,
list_tracks_foreign_tenant_returns_404, list_tracks_unknown_playlist_is_404, and
list_tracks_without_auth_is_401 to return anyhow::Result<()>, replace
occurrences like .send().await.unwrap(), .error_for_status().unwrap(),
.json().await.unwrap(), and any other unwrap()/expect() in those functions with
the ? operator (propagating errors), and add a final Ok(()) at the end of each
test; keep existing attributes (e.g. #[sqlx::test]) and adjust imports to
include anyhow if not already present.
🪄 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: 8b22cc65-c432-4809-aa86-0f2141ecf969
📒 Files selected for processing (5)
CLAUDE.mdsrc/api/playlists.rssrc/db.rstests/openapi.rstests/playlists.rs
Summary
GET /api/v1/profiles/{profile_id}/playlists/{id}/tracksso the web client can list a playlist's tracks (Sprint 4.c.4 — was blocked because the server didn't expose the join table on the read side).get_playlist. 404 blurs "no such playlist" / "wrong profile" / "wrong user" — no existence leak.Design notes
fetch_for_owneris two queries by design: a single CTE conflates "not owned" with "owned but empty" in the result set, and we need the 404-vs-[]distinction at the HTTP boundary. The race window is benign becauseplaylist_track.playlist_idisON DELETE CASCADE— documented in the helper's doc-comment so it doesn't rot.(position ASC, track_id ASC)— the PK guarantees uniqueness so the tiebreaker is deterministic. Tests exercise it with two rows that share a position.seed_playlist_tracktest helper does a direct SQL insert rather than going through the apply pipeline, so the assertion surface stays on the owner endpoint's contract.Test plan
200 [], not404.(position ASC, track_id ASC)order withadded_atand snapshot fields passed through verbatim — including NULL snapshots for pre-1.j.b rows.404; owner's own GET still succeeds (proves the 404 wasn't because the row was missing).404.401(JWT middleware fires before the handler).tests/openapi.rsguard).CC @InstaZDLL.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests