Expose Content-Length for CORS headers to send through hub#400
Merged
Expose Content-Length for CORS headers to send through hub#400
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/server/src/controllers/versions.rs (1)
650-655: Assert presence, not exact equality, forAccess-Control-Expose-Headers.
crates/server/src/helpers.rsis append-based, so these exact-match assertions will start failing as soon as another header is exposed. The same matcher is duplicated in the sibling controller tests; checking thatContent-Lengthis present will be more stable.🧪 Suggested assertion pattern
- assert_eq!( - resp.headers() - .get(header::ACCESS_CONTROL_EXPOSE_HEADERS) - .unwrap(), - header::CONTENT_LENGTH.as_str() - ); + let exposes_content_length = resp + .headers() + .get_all(header::ACCESS_CONTROL_EXPOSE_HEADERS) + .into_iter() + .any(|value| { + value + .to_str() + .unwrap() + .split(',') + .any(|name| name.trim().eq_ignore_ascii_case(header::CONTENT_LENGTH.as_str())) + }); + assert!(exposes_content_length);Also applies to: 706-711
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/server/src/controllers/versions.rs` around lines 650 - 655, The test currently asserts exact equality of resp.headers().get(header::ACCESS_CONTROL_EXPOSE_HEADERS).unwrap() to header::CONTENT_LENGTH.as_str(), but helpers append exposed headers so the assertion should instead check that the Access-Control-Expose-Headers header contains Content-Length; update the assertion in the versions controller test (and the duplicate in the sibling controller tests) to convert the header value to a string (via to_str()/as_str()) and assert it contains header::CONTENT_LENGTH.as_str() (or split on commas and assert any segment equals Content-Length) rather than asserting exact equality.crates/lib/src/util/fs.rs (1)
1595-1606: Collapse the cache-hit metadata lookups into one storage round-trip.On S3 this branch is now
HEAD(derived_version_exists) +HEAD(get_version_derived_size) +GET(get_version_derived_stream) for every cached resize. That adds latency/cost on the hot path and widens the TOCTOU window between the advertised length and the body fetch. Consider replacing the boolean existence check with a metadata lookup that returnsOption<u64>, or having the store return(stream, content_length)together.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/lib/src/util/fs.rs` around lines 1595 - 1606, Replace the three-step existence/size/stream round-trip (version_store.derived_version_exists, get_version_derived_size, get_version_derived_stream) with a single metadata/combined retrieval API on the store (e.g., add or use a method like get_version_derived_metadata(img_hash, derived_filename) -> Option<u64> or get_version_derived(img_hash, derived_filename) -> Option<(Stream, u64)>), then change the resize-cache branch in the code that currently calls derived_version_exists, get_version_derived_size and get_version_derived_stream to call that single method and early-return when it yields Some(metadata_or_pair); keep error handling the same for store errors and handle the None case as a cache miss. This reduces S3 HEAD/GET calls and eliminates the TOCTOU window between advertised length and body fetch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/lib/src/storage/version_store.rs`:
- Around line 205-214: The doc comment for get_version_derived_size incorrectly
describes a stream-returning API; update the documentation on the
get_version_derived_size method to state it returns the size in bytes (u64) of
the derived file (e.g., resized image, video thumbnail) for the given orig_hash
and derived_filename, mention any error cases (OxenError) and the semantics
(e.g., size on disk or content length), and remove references to streaming so
implementors and rustdoc reflect the actual return type.
---
Nitpick comments:
In `@crates/lib/src/util/fs.rs`:
- Around line 1595-1606: Replace the three-step existence/size/stream round-trip
(version_store.derived_version_exists, get_version_derived_size,
get_version_derived_stream) with a single metadata/combined retrieval API on the
store (e.g., add or use a method like get_version_derived_metadata(img_hash,
derived_filename) -> Option<u64> or get_version_derived(img_hash,
derived_filename) -> Option<(Stream, u64)>), then change the resize-cache branch
in the code that currently calls derived_version_exists,
get_version_derived_size and get_version_derived_stream to call that single
method and early-return when it yields Some(metadata_or_pair); keep error
handling the same for store errors and handle the None case as a cache miss.
This reduces S3 HEAD/GET calls and eliminates the TOCTOU window between
advertised length and body fetch.
In `@crates/server/src/controllers/versions.rs`:
- Around line 650-655: The test currently asserts exact equality of
resp.headers().get(header::ACCESS_CONTROL_EXPOSE_HEADERS).unwrap() to
header::CONTENT_LENGTH.as_str(), but helpers append exposed headers so the
assertion should instead check that the Access-Control-Expose-Headers header
contains Content-Length; update the assertion in the versions controller test
(and the duplicate in the sibling controller tests) to convert the header value
to a string (via to_str()/as_str()) and assert it contains
header::CONTENT_LENGTH.as_str() (or split on commas and assert any segment
equals Content-Length) rather than asserting exact equality.
🪄 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: CHILL
Plan: Pro
Run ID: e361c75b-79b1-4e7e-9b21-af1f740547db
📒 Files selected for processing (8)
crates/lib/src/storage/local.rscrates/lib/src/storage/s3.rscrates/lib/src/storage/version_store.rscrates/lib/src/util/fs.rscrates/server/src/controllers/file.rscrates/server/src/controllers/versions.rscrates/server/src/controllers/workspaces/files.rscrates/server/src/helpers.rs
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.
No description provided.