feat(share): mint + revoke by canonical id (phase 1.g.1b)#27
Merged
Conversation
Closes the API loop for desktop sharing. Without this, the
desktop would have to first push a sync_op, wait for the apply
pipeline to materialise the playlist row, then look up its
server BIGSERIAL id, then call the classic mint endpoint with
that id — three round-trips for what is conceptually one
operation.
Two new JWT-authed routes:
- POST /api/v1/share/playlists/by-canonical/{profile_canonical_id}/{playlist_canonical_id}
- DELETE /api/v1/share/playlists/by-canonical/{profile_canonical_id}/{playlist_canonical_id}
Same semantics as the classic mint / revoke, same idempotent
COALESCE(share_token, $candidate) under the hood, same no-
existence-leak 404 shape. The tenant chain becomes (user_id,
profile.canonical_id, playlist.canonical_id) — the playlist
must already have been materialised by the apply pipeline, so a
desktop user can only mint for playlists their own canonical
ids point at.
Mirrored helpers in db::share — mint_or_get_token_by_canonical
and revoke_token_by_canonical — keep raw SQL inside the db
layer per the project's no-SQL-in-handlers rule.
Tests (tests/share.rs): 4 new integration tests covering the
end-to-end flow (sync_op push -> apply materialises -> mint by
canonical -> public GET resolves), idempotency, revoke closing
the public URL, and 404 for foreign / unknown canonical ids.
Shared materialise_playlist_via_sync helper mirrors what the
desktop drain task will do once the desktop side of Phase 1.g.0
ships.
Local: cargo fmt + cargo clippy --all-targets --all-features --
-D warnings clean; cargo test --test share = 11/11.
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 (1)
📝 WalkthroughWalkthroughLe PR ajoute POST/DELETE pour mint/revoke de tokens de partage via identifiants canoniques, implémente deux fonctions DB race-free correspondantes, et ajoute des tests e2e vérifiant idempotence, remint après revoke et isolation multi-tenant. ChangesPartage par identifiants canoniques
Sequence DiagramsequenceDiagram
participant User as Client JWT-authed
participant MintHandler as POST /api/v1/share/playlists/by-canonical/{...}
participant MintDB as mint_or_get_token_by_canonical
participant RevokeHandler as DELETE /api/v1/share/playlists/by-canonical/{...}
participant RevokeDB as revoke_token_by_canonical
participant DB as PostgreSQL
participant PublicGet as GET /api/v1/share/playlists/{token}
User->>MintHandler: POST (profile_canonical_id, playlist_canonical_id)
MintHandler->>MintDB: call(user_id, canonical_ids)
MintDB->>DB: COALESCE + ownership check
DB-->>MintDB: Option<String>
MintDB-->>MintHandler: Ok(Some(token))
MintHandler-->>User: 200 { token }
User->>PublicGet: GET token
PublicGet->>DB: resolve playlist by token
DB-->>PublicGet: playlist details
PublicGet-->>User: 200 PublicPlaylistResponse
User->>RevokeHandler: DELETE (profile_canonical_id, playlist_canonical_id)
RevokeHandler->>RevokeDB: call(user_id, canonical_ids)
RevokeDB->>DB: UPDATE share_token=NULL
DB-->>RevokeDB: rows_affected > 0
RevokeDB-->>RevokeHandler: Ok(true)
RevokeHandler-->>User: 204 No Content
User->>PublicGet: GET token (revoked)
PublicGet->>DB: lookup (token cleared)
DB-->>PublicGet: not found
PublicGet-->>User: 404
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 `@tests/share.rs`:
- Around line 307-309: Add a missing test that verifies revoke + remint produces
a new token for the by-canonical surface: create a new async sqlx::test named
by_canonical_revoke_then_remint_returns_fresh_token (migrator =
"waveflow_server::db::MIGRATOR") in tests/share.rs that mirrors
revoke_then_remint_returns_a_fresh_token but uses the by-canonical endpoints
(the POST/DELETE to
/api/v1/share/playlists/by-canonical/{PROF_CANON}/{pl_canon}); use
spawn_authenticated, materialise_playlist_via_sync, mint the initial token,
DELETE to revoke, re-POST to remint, extract tokens from JSON responses and
assert_ne!(token1, token2) to ensure mint_or_get_token_by_canonical’s
COALESCE+NULL path produces a fresh token after revocation.
- Around line 56-90: The helper materialise_playlist_via_sync currently uses
fixed device_id ("share-by-canonical-test") and lamport_ts = 1 which can violate
the UNIQUE (user_id, device_id, lamport_ts) constraint if called multiple times;
update materialise_playlist_via_sync to produce unique sync identifiers per
call—either accept a lamport_ts (or device_id) parameter or generate a
colliding-safe value inside the function (e.g., increment lamport_ts, use a
random/UUID device_id, or derive a timestamp-based lamport) and include that
generated value in the JSON payload so repeated calls do not collide.
🪄 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: f4cf3b10-bddf-4bf8-b1b2-cf151f530a35
📒 Files selected for processing (3)
src/api/share.rssrc/db.rstests/share.rs
…e helper Two findings from CodeRabbit on PR #27: The materialise_playlist_via_sync helper was using a fixed device_id ("share-by-canonical-test") and lamport_ts = 1, which would 409 the second call inside the same test via the (user_id, device_id, lamport_ts) UNIQUE. No current test hits this, but the helper is a footgun for any future scenario that needs more than one playlist. Swapped device_id to a fresh UUID per call — every materialisation now plays the role of a distinct device, so the lamport scope is per-call and lamport_ts = 1 stays valid. Added by_canonical_revoke_then_remint_returns_fresh_token to mirror the classic-surface coverage. Pins the COALESCE-on-NULL path: after a revoke clears share_token, the next mint must generate a new candidate rather than re-using the prior value. Same shape as revoke_then_remint_returns_a_fresh_token, just against the by-canonical endpoints. cargo fmt + cargo clippy --all-targets --all-features -- -D warnings clean; cargo test --test share = 12/12. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
Owner
Author
|
@coderabbitai both findings addressed in 10df360. 1. Missing revoke + remint coverage — added 2. Collision-risk in Local: cargo fmt + clippy strict + 12/12 share tests. |
|
Tip For best results, initiate chat on the files or code changes.
12/12 ✅ — bon pour la suite. (◕‿◕✿)🐇 |
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
Closes the API loop for desktop sharing. Without this, the desktop would have to first push a sync_op, wait for the apply pipeline (Phase 1.g.0, PR #26) to materialise the playlist row, then look up its server BIGSERIAL id, then call the classic mint endpoint with that id — three round-trips for what is conceptually one operation.
What lands
Two new JWT-authed routes parallel to the classic mint / revoke:
POST /api/v1/share/playlists/by-canonical/{profile_canonical_id}/{playlist_canonical_id}DELETE /api/v1/share/playlists/by-canonical/{profile_canonical_id}/{playlist_canonical_id}Same idempotent `COALESCE(share_token, $candidate)` SQL pattern under the hood, same no-existence-leak 404 shape. The tenant chain becomes `(user_id, profile.canonical_id, playlist.canonical_id)` — the playlist must already have been materialised by the apply pipeline, so a desktop user can only mint for playlists their own canonical ids point at.
New helpers in `db::share` keep raw SQL out of the handlers per the project's convention.
Tests
4 new integration tests in `tests/share.rs`:
Shared `materialise_playlist_via_sync` helper mirrors what the desktop drain task will do once Phase 1.g.0-desktop ships.
Test plan
Summary by CodeRabbit
Nouvelles fonctionnalités
Tests