feat(artwork): shared object-store cache for cover art (phase 1.h.1)#28
Merged
Conversation
|
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)
📝 WalkthroughWalkthroughCette PR ajoute un cache d’artwork indexé par BLAKE3 avec stockage local (object_store), table Postgres metadata_artwork, endpoints POST (authentifié) et GET (public) et tests d’intégration vérifiant validation, idempotence et erreurs. ChangesArtwork Cache & Storage
Sequence Diagram(s)sequenceDiagram
participant Client
participant JWT_Middleware
participant UploadHandler as upload_artwork
participant GetHandler as get_artwork
participant DB as Database
participant Storage as ArtworkStorage
rect rgba(200, 220, 255, 0.5)
Note over Client,Storage: POST /api/v1/artwork (Authentifié)
Client->>JWT_Middleware: Bearer token + octets
JWT_Middleware->>UploadHandler: request
UploadHandler->>UploadHandler: validate Content-Type and size
UploadHandler->>UploadHandler: compute BLAKE3(body) -> hash
UploadHandler->>DB: fetch_meta(hash)
alt meta exists
DB-->>UploadHandler: Some(ArtworkMeta)
UploadHandler-->>Client: 200 UploadResponse (short-circuit)
else meta absent
UploadHandler->>Storage: put(artwork/<hash>, bytes)
Storage-->>UploadHandler: OK
UploadHandler->>DB: insert_if_absent(hash, mime, size)
DB-->>UploadHandler: OK
UploadHandler-->>Client: 200 UploadResponse
end
end
rect rgba(200, 255, 200, 0.5)
Note over Client,Storage: GET /api/v1/artwork/{hash} (Public)
Client->>GetHandler: GET /api/v1/artwork/{hash}
GetHandler->>GetHandler: validate hash format
GetHandler->>DB: fetch_meta(hash)
alt found
DB-->>GetHandler: Some(mime)
GetHandler->>Storage: get(artwork/<hash>)
alt retrieved
Storage-->>GetHandler: Bytes
GetHandler-->>Client: 200 + Cache-Control:public,max-age=31536000,immutable + ETag
else not found
Storage-->>GetHandler: StorageError::NotFound
GetHandler-->>Client: 500 Internal Error
end
else not found
GetHandler-->>Client: 404 Not Found
end
end
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 |
Lays the foundation of the server-side artwork pipeline: a BLAKE3-
keyed object_store wrapper, a `metadata_artwork` table that mirrors
the desktop's on-disk cache, and two endpoints that round-trip raw
image bytes.
POST /api/v1/artwork is JWT-authed, accepts image/jpeg | image/png |
image/webp up to 4 MiB, hashes server-side and skips the storage
write when the metadata row already exists — idempotent re-uploads
of the same content are free. GET /api/v1/artwork/{hash} is public
(the 64-hex hash is the credential, same shape as the share token),
short-circuits on a 64-char lowercase-hex validator before touching
object_store keys, and serves the recorded Content-Type with
Cache-Control: immutable + ETag = "<hash>".
object_store's LocalFileSystem backend ships here; the S3 backend
lands in 1.h.2 with no caller change (same trait, same put/get/exists).
Image resize (3 sizes) + the async pipeline follow in 1.h.3 + 1.i.1.
Tests cover round-trip, idempotency, 503-when-disabled, MIME +
empty-body rejections, bearer requirement on POST, hash validation
on GET, and the charset-parameter-tolerant Content-Type parse.
Signed-off-by: InstaZDLL <github.105mh@8shield.net>
InstaZDLL
force-pushed
the
feat/1-h-1-artwork-storage
branch
from
5bdeb81 to
a8eb339
Compare
![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 `@tests/artwork.rs`:
- Around line 1-16: The coverage matrix mentions rejecting oversize uploads but
no test covers it; add a test named upload_rejects_oversized_body that
constructs a request body with length > MAX_UPLOAD_BYTES, posts to the artwork
upload endpoint (same helper/path used by other tests in tests/artwork.rs), and
asserts the response status is 413 PAYLOAD_TOO_LARGE (and optionally checks the
error body); use the same Content-Type and auth setup as existing upload tests
so it fits into the test harness and references the MAX_UPLOAD_BYTES constant to
generate the oversized payload.
🪄 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: 849339b6-ddcf-4a0f-8372-230c42957d4c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock,!*.lock
📒 Files selected for processing (13)
.env.exampleCLAUDE.mdCargo.tomlmigrations/20260606000000_metadata_artwork.sqlsrc/api/artwork.rssrc/api/mod.rssrc/config.rssrc/db.rssrc/lib.rssrc/main.rssrc/storage.rstests/artwork.rstests/support.rs
CodeRabbit noted the coverage matrix promised a 413 test but no test asserted it. Add `upload_rejects_oversized_body` (4 MiB + 1 byte post → 413) and widen the auth router's `DefaultBodyLimit` to `MAX_UPLOAD_BYTES + 1024` so the handler's `body.len() > MAX_UPLOAD_BYTES` check is what observes the overflow — axum's default 2 MiB limit would otherwise pre-empt our error variant and let the test pass for the wrong reason. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
Owner
Author
|
@coderabbitai bien vu — j'avais oublié de tester le cap 4 MiB. Ajouté Subtilité : axum 0.8 ship un Push: f91fb3f. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check the relevant changes in the commit f91fb3f
git show f91fb3f --stat
echo "---"
git show f91fb3f -- src/api/artwork.rs | head -80
echo "---"
git show f91fb3f -- tests/artwork.rs | head -60Length of output: 5196
Tout est bon. ✓ (ﾉ◕ヮ◕)ﾉ*:･ﾟ✧ 🧠 Learnings used |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/artwork.rs (1)
1-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIncohérence entre la matrice de couverture et les tests implémentés.
La matrice de couverture (ligne 12) annonce « 400 / 413 boundary cases (wrong MIME, empty body, oversize) », mais aucun test ne valide le rejet d'un corps dépassant 4 MiB (413 PAYLOAD_TOO_LARGE). Les tests couvrent MIME invalide (ligne 165) et corps vide (ligne 181), mais pas le cas
body.len() > MAX_UPLOAD_BYTES.Soit ajouter un test
upload_rejects_oversized_body, soit retirer « oversize » de la matrice.🤖 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 `@tests/artwork.rs` around lines 1 - 16, The coverage matrix claims an "oversize" case but no test implements it; add a test named upload_rejects_oversized_body that posts a payload with length > MAX_UPLOAD_BYTES to the POST /api/v1/artwork endpoint and asserts a 413 PAYLOAD_TOO_LARGE response (and does not create a storage row), using the same test harness/helpers as the other upload tests and referencing the MAX_UPLOAD_BYTES constant to construct the oversize body; alternatively, if you prefer not to add a test, remove the word "oversize" from the coverage matrix line to keep doc and tests consistent.
🤖 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.
Duplicate comments:
In `@tests/artwork.rs`:
- Around line 1-16: The coverage matrix claims an "oversize" case but no test
implements it; add a test named upload_rejects_oversized_body that posts a
payload with length > MAX_UPLOAD_BYTES to the POST /api/v1/artwork endpoint and
asserts a 413 PAYLOAD_TOO_LARGE response (and does not create a storage row),
using the same test harness/helpers as the other upload tests and referencing
the MAX_UPLOAD_BYTES constant to construct the oversize body; alternatively, if
you prefer not to add a test, remove the word "oversize" from the coverage
matrix line to keep doc and tests consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 10daa358-c60a-497c-b607-49c3c8d73b31
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock,!*.lock
📒 Files selected for processing (13)
.env.exampleCLAUDE.mdCargo.tomlmigrations/20260606000000_metadata_artwork.sqlsrc/api/artwork.rssrc/api/mod.rssrc/config.rssrc/db.rssrc/lib.rssrc/main.rssrc/storage.rstests/artwork.rstests/support.rs
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 (1)
src/api/artwork.rs (1)
185-199: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueIncohérence mineure dans la réponse idempotente.
Quand le contenu existe déjà (ligne 185), la réponse utilise
byte_sizeetmimede la requête courante au lieu des valeurs stockées (_existing). En pratique c'est identique (même hash = mêmes bytes = même taille), mais récupérer_existing.byte_sizeet_existing.mimeserait plus cohérent et documenterait l'état réel en base.♻️ Suggestion optionnelle
- if let Some(_existing) = crate::db::artwork::fetch_meta(&state.db, &hash) + if let Some(existing) = crate::db::artwork::fetch_meta(&state.db, &hash) .await .map_err(ArtworkError::Db)? { return Ok(( StatusCode::OK, Json(UploadResponse { hash: hash.clone(), - byte_size, - mime, + byte_size: existing.byte_size, + mime: existing.mime, url: format!("/api/v1/artwork/{hash}"), }), ) .into_response()); }🤖 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/api/artwork.rs` around lines 185 - 199, The idempotent upload branch returns the request's byte_size and mime instead of the stored values; update the code that handles the Some(_existing) case (the result of crate::db::artwork::fetch_meta) to read and return _existing.byte_size and _existing.mime in the UploadResponse (keeping hash and url as-is) so the response reflects the database state; ensure you still map_err(ArtworkError::Db)? and return the same StatusCode::OK Json(...) response shape.
🤖 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/api/artwork.rs`:
- Around line 185-199: The idempotent upload branch returns the request's
byte_size and mime instead of the stored values; update the code that handles
the Some(_existing) case (the result of crate::db::artwork::fetch_meta) to read
and return _existing.byte_size and _existing.mime in the UploadResponse (keeping
hash and url as-is) so the response reflects the database state; ensure you
still map_err(ArtworkError::Db)? and return the same StatusCode::OK Json(...)
response shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0de72ed6-91aa-4f15-a815-167acdb8369a
📒 Files selected for processing (2)
src/api/artwork.rstests/artwork.rs
CodeRabbit flagged that the idempotent branch was echoing the request's `byte_size` + `mime` instead of the stored row's values. Today they're identical (BLAKE3 collision-resistance), but `UploadResponse::mime` already documents the contract "what the server stored" — so a future canonicalisation (e.g. mapping `image/jpg` → `image/jpeg`) would silently break that promise. Read through `existing` so the documented behaviour holds unconditionally. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
Owner
Author
|
@coderabbitai bien vu — le DTO documente déjà Push: fe18d4d. |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
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
Sprint 1 / Phase 1.h.1 — foundation of the server-side artwork pipeline. Lays the storage abstraction + the
metadata_artworktable + the two CRUD endpoints, all behind theWAVEFLOW_ARTWORK_LOCAL_DIRopt-in flag.POST /api/v1/artwork(JWT-authed): raw image bytes,Content-Type ∈ {image/jpeg, image/png, image/webp}, 4 MiB cap. Hashes server-side with BLAKE3, skips the storage write when the row already exists — idempotent. Returns{ hash, byte_size, mime, url }.GET /api/v1/artwork/{hash}(public — the 64-hex hash is the credential, same shape as the share token): serves bytes with the recordedContent-Type,Cache-Control: public, max-age=31536000, immutable,ETag: "<hash>". Boundary validator rejects anything that isn't 64 lowercase hex chars before touchingobject_storekeys.Storage layer: Apache's
object_storecrate with the LocalFileSystem backend in this PR. S3 (object_storefeatureaws) lands in 1.h.2 with no caller change — sameObjectStoretrait, sameput/get/exists. Image resize (3 sizes) + the async pipeline follow in 1.h.3 + 1.i.1.Both routes 503 when storage is unconfigured — same opt-in shape as streaming.
Surface added
migrations/20260606000000_metadata_artwork.sql— Postgres table, BLAKE3 hex PK, MIME + byte_size + created_at. CHECK constraints mirror the application-layer validators so a SQL-only client can't pollute the table.src/storage.rs—ArtworkStorage(Arc-cloneable),ArtworkConfig::from_env,StorageError,is_well_shaped_hash.src/api/artwork.rs— both handlers + the localArtworkError→IntoResponsemapping.src/db.rs::artwork—insert_if_absent(race-safe viaON CONFLICT DO NOTHING) +fetch_meta.tests/artwork.rs— 9 integration tests covering round-trip, idempotency, 503-when-disabled, MIME + empty-body rejections, bearer requirement on POST, hash validation on GET, charset-parameter-tolerantContent-Typeparsing.Test plan
cargo fmt --all --checkcargo clippy --all-targets --all-features -- -D warningscargo test --lib(24/24 unit tests pass locally, including 6 new instorage::tests)DATABASE_URL=postgres://... cargo test --test artwork(integration suite — needs a reachable Postgres)cargo testend-to-end (every existing test file got anartwork: Nonefield on itsAppState/Configliterals; no behaviour change for non-artwork tests)Follow-up tracked
object_store'sawsfeature. MinIO-compat via custom endpoint.apalisPostgres-backed job queue, first job = artwork pipeline async.Part of the post-1.g sprint plan validated 2026-06-05.
Summary by CodeRabbit
Nouvelles Fonctionnalités
Configuration
Données
Validation
Documentation
Tests