feat(torii-artifacts): refetch image if the image uri changed #3053
Conversation
|
Ohayo sensei! Here’s the detailed summary of your pull request: WalkthroughThe pull request enhances image handling in the artifacts module by introducing a new asynchronous function, Changes
Sequence Diagram(s)sequenceDiagram
participant S as serve_static_file
participant CH as check_image_hash
participant FP as fetch_and_process_image
participant FS as File System
participant DB as Database
S ->> CH: Validate image hash
alt Hash file missing or hash mismatch
CH -->> S: Return false
S ->> FP: Fetch & process image
FP ->> FS: Save image & write image.hash
FP -->> S: Return updated image
else Valid hash
CH -->> S: Return true
S -->> S: Serve cached image
end
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/server/src/artifacts.rs (2)
213-213: Ohayo sensei! Thorough error message.
Adding context for missing"image"field in metadata helps quickly identify invalid or corrupted token metadata.
341-344: Ohayo sensei! Great step persisting the new image URI hash.
Storing the hash ensures subsequent requests can detect if the stored artifact is outdated. If you foresee large URIs or sensitive info, consider hashing or encrypting the URI before writing to disk for privacy.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/server/src/artifacts.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: docs
- GitHub Check: clippy
- GitHub Check: build
🔇 Additional comments (5)
crates/torii/server/src/artifacts.rs (5)
60-72: Ohayo sensei! Great fallback approach for refetching on error.
If there's an error checking the image hash or the directory doesn’t exist, defaulting totrueforshould_fetchensures freshness of the artifact. This logic is straightforward and helps avoid stale images.
74-74: Ohayo sensei! Consider concurrency implications.
If multiple tasks hitshould_fetchsimultaneously, they could both attempt a refetch at the same time. Although this might be acceptable, you might want a lock or more granular concurrency control if collisions become an issue.
162-194: Ohayo sensei! Excellent implementation of hash-check logic.
This function cleanly determines whether the image URI has changed by comparing the stored hash against the current URI in the database. The fallback toOk(true)ensures the image is fetched if there's no hash or an error reading the file. This is a robust approach to ensure artifact consistency.
207-207: Ohayo sensei! Good use of.context()here.
This additional context enhances debuggability by pinpointing the source of database fetching issues. Nice consistency with other lines.
215-215: Ohayo sensei! Solid validation on image field type.
This ensures the image field is indeed a string, preventing obscure runtime type errors further down the pipeline.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/server/src/artifacts.rs (2)
162-194: Consider optimizing metadata handling, sensei!The function works well but could be improved in a few areas:
- Consider caching parsed metadata to avoid repeated JSON parsing
- Add specific error messages for common failure cases
Here's a suggested optimization:
async fn check_image_hash( token_image_dir: &Utf8PathBuf, token_id: &str, pool: &Pool<Sqlite>, ) -> Result<bool> { let hash_file = token_image_dir.join("image.hash"); // Get current image URI from metadata let query = sqlx::query_as::<_, (String,)>(&format!( "SELECT metadata FROM {TOKENS_TABLE} WHERE id = ?" )) .bind(token_id) .fetch_one(pool) .await - .context("Failed to fetch metadata from database")?; + .context(format!("Failed to fetch metadata for token {}", token_id))?; let metadata: serde_json::Value = serde_json::from_str(&query.0).context("Failed to parse metadata")?; let current_uri = metadata .get("image") - .context("Image URL not found in metadata")? + .context(format!("Image URL not found in metadata for token {}", token_id))? .as_str() - .context("Image field not a string")?; + .context(format!("Image field not a string for token {}", token_id))?;
341-344: Consider atomic write for hash file, sensei!The hash file storage is good, but consider using atomic write operations to prevent partial writes in case of system failures.
Here's a suggested approach using a temporary file:
- // Before returning, store the image URI hash - let hash_file = dir_path.join("image.hash"); - fs::write(&hash_file, &image_uri).await.context("Failed to write hash file")?; + // Store the image URI hash atomically + let hash_file = dir_path.join("image.hash"); + let temp_hash_file = dir_path.join(".image.hash.tmp"); + fs::write(&temp_hash_file, &image_uri).await.context("Failed to write temporary hash file")?; + fs::rename(&temp_hash_file, &hash_file).await.context("Failed to rename hash file")?;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/server/src/artifacts.rs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: docs
- GitHub Check: build
- GitHub Check: clippy
🔇 Additional comments (1)
crates/torii/server/src/artifacts.rs (1)
60-74: Ohayo! Nice optimization for image refetching, sensei!The new logic efficiently determines if an image needs to be refetched by checking the image hash. The error handling gracefully falls back to fetching when hash verification fails.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3053 +/- ##
==========================================
- Coverage 57.65% 57.62% -0.04%
==========================================
Files 439 439
Lines 59437 59470 +33
==========================================
Hits 34267 34267
- Misses 25170 25203 +33 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
Bug Fixes