feat: implement Accept header content negotiation for image formats#94
feat: implement Accept header content negotiation for image formats#94
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds Accept header content negotiation for image output formats (WebP, PNG, JPEG). Introduces Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Handler as HTTP Handler\n(src/routes/index.rs)
participant Params as Params\n(src/params.rs)
participant Accept as Negotiation\n(src/accept.rs)
participant Renderer as Renderer/Output
Client->>Handler: GET /... + Accept header
Handler->>Handler: extract Accept header
Handler->>Params: render_options_with_accept(accept_header)
alt explicit ?format provided
Params->>Renderer: use explicit format
else negotiate via Accept
Params->>Accept: negotiate_format(accept_header)
Accept->>Accept: parse Accept, apply q-values
Accept-->>Params: selected OutputFormat or None
alt negotiated
Params->>Renderer: use negotiated format
else no match
Params->>Renderer: default to PNG
end
end
Renderer-->>Client: image response (selected format)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR implements content negotiation for image formats using the Accept HTTP header according to RFC 9110. The implementation adds support for clients to request their preferred image format (WebP, PNG, or JPEG) through the Accept header while maintaining backward compatibility with explicit format query parameters.
Key Changes:
- Adds content negotiation logic using the
headers-acceptcrate to parse and negotiate Accept headers based on quality values - Implements a three-tier priority system: explicit
?format=parameter > Accept header negotiation > PNG default - Introduces comprehensive test coverage for the negotiation logic
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/accept.rs | New module implementing RFC 9110 content negotiation for image formats with quality value parsing |
| src/params.rs | Updates render_options to accept an Accept header parameter and implements priority-based format selection |
| src/routes/index.rs | Extracts Accept header from incoming requests and passes it to the render options builder |
| src/main.rs | Registers the new accept module |
| Cargo.toml | Adds headers-accept and mediatype dependencies for content negotiation |
| Cargo.lock | Lock file updates for new dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let available: Vec<mediatype::MediaType> = [IMAGE_WEBP, IMAGE_PNG, IMAGE_JPEG] | ||
| .iter() | ||
| .filter_map(|s| mediatype::MediaType::parse(s).ok()) | ||
| .collect(); |
There was a problem hiding this comment.
The available formats vector is being created and allocated on every request. Since these media types are constant and the parsing is infallible for valid MIME types, this allocation could be moved to a static or lazy_static to improve performance. Consider using once_cell::sync::Lazy or similar to initialize this once at startup rather than on every request.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/params.rs (1)
113-136: Fix unsafe practice in tests: callvalidate()beforerender_options_with_accept().The Accept header negotiation logic is well-designed. However, production code (src/routes/index.rs line 96-107) correctly calls
validate()beforerender_options_with_accept(), but the test functions in src/params.rs do not. Four test functions callrender_options_with_accept()without callingvalidate()first, violating the documented safety requirement:
test_render_options_defaultstest_render_options_customtest_render_options_accept_header_negotiationtest_render_options_format_overrides_acceptWhile these tests don't panic because they use valid format values, they undermine the safety contract and could mask issues if test defaults change. Update these tests to call
validate()beforerender_options_with_accept().
🧹 Nitpick comments (1)
src/accept.rs (1)
1-41: LGTM with optional optimization suggestion.The implementation correctly follows RFC 9110 content negotiation semantics with proper error handling via
Optionreturns. The logic flow is clean and well-documented.💡 Optional: Consider caching the available media types vector
The
availablemedia types vector is reconstructed on every call. For a minor performance improvement, consider usingLazyLock(stable in Rust 1.80+) to initialize it once:+use std::sync::LazyLock; + +static AVAILABLE_FORMATS: LazyLock<Vec<mediatype::MediaType>> = LazyLock::new(|| { + [IMAGE_WEBP, IMAGE_PNG, IMAGE_JPEG] + .iter() + .filter_map(|s| mediatype::MediaType::parse(s).ok()) + .collect() +}); + pub fn negotiate_format(accept_header: Option<&str>) -> Option<OutputFormat> { let header = accept_header?; let accept = Accept::from_str(header).ok()?; - // Available formats we support - let available: Vec<mediatype::MediaType> = [IMAGE_WEBP, IMAGE_PNG, IMAGE_JPEG] - .iter() - .filter_map(|s| mediatype::MediaType::parse(s).ok()) - .collect(); - - // Negotiate returns the best match based on client preferences (RFC 9110) - let best = accept.negotiate(&available)?; + let best = accept.negotiate(&AVAILABLE_FORMATS)?;This is a micro-optimization and not critical for correctness.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlsrc/accept.rssrc/main.rssrc/params.rssrc/routes/index.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Usecargo fmtto format Rust code
Usecargo clippyto lint Rust code
Files:
src/routes/index.rssrc/accept.rssrc/params.rssrc/main.rs
🧠 Learnings (1)
📚 Learning: 2025-12-16T04:03:37.838Z
Learnt from: CR
Repo: twangodev/ogis PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T04:03:37.838Z
Learning: Applies to src/generator/png.rs : Default image dimensions are 1200x630 pixels
Applied to files:
src/params.rs
🧬 Code graph analysis (3)
src/routes/index.rs (1)
src/telemetry/config.rs (1)
headers(54-60)
src/accept.rs (3)
web/src/lib/stores/playground.svelte.ts (1)
OutputFormat(21-21)src/generator/render.rs (1)
from_str(21-28)src/config.rs (1)
parse(177-179)
src/params.rs (3)
src/accept.rs (1)
negotiate_format(22-41)web/src/lib/stores/playground.svelte.ts (1)
OutputFormat(21-21)src/generator/render.rs (1)
from_str(21-28)
🪛 GitHub Check: Format Check
src/params.rs
[warning] 122-122:
Diff in /home/runner/work/ogis/ogis/src/params.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Agent
- GitHub Check: Code Coverage
- GitHub Check: Code Coverage
🔇 Additional comments (7)
src/main.rs (1)
1-1: LGTM!The module declaration correctly integrates the new Accept header negotiation module into the crate.
src/params.rs (2)
6-6: LGTM!The import correctly references the new
negotiate_formatfunction from theacceptmodule.
296-311: LGTM!The test coverage for Accept header negotiation is comprehensive, verifying both the negotiation behavior and the priority of explicit format parameters over Accept headers.
src/accept.rs (1)
43-106: LGTM!Excellent test coverage including edge cases (nil/empty headers), single format matches, quality precedence scenarios, unsupported formats, and realistic browser-like Accept headers. The tests validate RFC 9110 quality-based negotiation correctly.
src/routes/index.rs (2)
11-11: LGTM!The addition of
HeaderMapto imports and the handler signature change correctly enables Accept header extraction from incoming requests.Also applies to: 65-65
105-107: LGTM!The Accept header extraction and integration with
render_options_with_acceptis clean and idiomatic. The code correctly:
- Extracts the Accept header using
header::ACCEPTconstant- Converts to
Option<&str>with.ok()- Passes it to the negotiation logic
- Maintains the safety requirement by calling
validate()at line 96 beforerender_options_with_acceptCargo.toml (1)
14-14: Dependencies are current and free from known vulnerabilities.Both
headers-accept(0.3.0) andmediatype(0.21.0) are pinned to their latest available versions and have no known security advisories in the RustSec database. No action needed.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/accept.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Usecargo fmtto format Rust code
Usecargo clippyto lint Rust code
Files:
src/accept.rs
🪛 GitHub Check: Format Check
src/accept.rs
[warning] 7-7:
Diff in /home/runner/work/ogis/ogis/src/accept.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Agent
- GitHub Check: Code Coverage
- GitHub Check: Code Coverage
- GitHub Check: build
🔇 Additional comments (3)
src/accept.rs (3)
17-37: LGTM! Clean implementation of content negotiation.The function correctly implements RFC 9110 quality value negotiation:
- Handles all error cases gracefully by returning
None- Uses idiomatic Rust with the
?operator for early returns- Delegates negotiation logic to the
headers-acceptcrate- The linear search through
SUPPORTED_FORMATSis appropriate for 3 elementsThe implementation is clear, well-documented, and follows Rust best practices.
39-102: Excellent test coverage!The test suite comprehensively covers:
- Missing and empty headers
- Individual format matching
- Quality value precedence (RFC 9110 compliance)
- Unsupported format handling
- Real-world browser Accept headers
The browser-like header test (lines 95-101) is particularly valuable for ensuring compatibility with actual client requests.
12-15: LazyLock optimization is excellent and the .unwrap() is safe.The static initialization with
LazyLockproperly ensures MIME types are parsed once at startup rather than on every request. The.unwrap()on line 14 is safe because allOutputFormatvariants are hardcoded to return valid MIME strings:"image/png","image/jpeg", and"image/webp". Since the enum only has these three controlled variants, there is no path for invalid MIME data, and this is covered by test cases.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[test] | ||
| fn test_render_options_accept_header_negotiation() { | ||
| let params = default_params(); | ||
| // Accept header should be used when no format param | ||
| let opts = params.render_options_with_accept(Some("image/webp")); | ||
| assert_eq!(opts.format, OutputFormat::WebP); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_render_options_format_overrides_accept() { | ||
| let mut params = default_params(); | ||
| params.format = Some("jpeg".to_string()); | ||
| // Explicit format should override Accept header | ||
| let opts = params.render_options_with_accept(Some("image/webp")); | ||
| assert_eq!(opts.format, OutputFormat::Jpeg); | ||
| } |
There was a problem hiding this comment.
These tests should call validate() before render_options_with_accept() to properly demonstrate the expected usage pattern documented in the function's docstring (line 120). While the tests currently pass because they use valid formats, they don't reflect the actual usage flow where validation must occur first to prevent panics from the expect() call on line 124.
Consider adding assert!(params.validate(1000).is_ok()); before calling render_options_with_accept() in both test cases to align with the documented contract and actual production usage in the route handler.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/accept.rs (1)
14-16: Consider usingexpect()for clearer panic messages.The
.unwrap()on line 15 will panic ifcontent_type()returns an invalid MIME type. While this should never happen given thatOutputFormat::content_type()is under your control, using.expect()with a descriptive message would make debugging easier if this invariant is ever violated in the future.🔎 Optional improvement
static AVAILABLE_MIMES: LazyLock<[mediatype::MediaType; 3]> = LazyLock::new(|| { - SUPPORTED_FORMATS.map(|f| mediatype::MediaType::parse(f.content_type()).unwrap()) + SUPPORTED_FORMATS.map(|f| { + mediatype::MediaType::parse(f.content_type()) + .expect("OutputFormat::content_type() must return valid MIME types") + }) });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/accept.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Usecargo fmtto format Rust code
Usecargo clippyto lint Rust code
Files:
src/accept.rs
🧬 Code graph analysis (1)
src/accept.rs (2)
web/src/lib/stores/playground.svelte.ts (1)
OutputFormat(21-21)src/generator/render.rs (1)
from_str(21-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Code Coverage
- GitHub Check: Code Coverage
🔇 Additional comments (4)
src/accept.rs (4)
1-7: LGTM: Clean module structure.The module documentation and imports are well-organized. The use of
LazyLockfromstd::syncis appropriate for one-time initialization of the parsed MIME types.
18-38: LGTM: Clean negotiation logic.The negotiation function is well-implemented:
- Proper error handling with early returns via
?operator- Uses
.essence()to correctly strip MIME type parameters before comparison- Linear search through 3 formats is appropriate and efficient
40-103: LGTM: Comprehensive test coverage.The unit tests are well-structured and cover all important scenarios:
- Missing and empty Accept headers
- Individual format matching for all three supported types
- Quality value precedence (both explicit and implicit q=1.0)
- Unsupported formats
- Real-world browser Accept headers
The test coverage ensures the negotiation logic is robust and handles edge cases correctly.
3-3: No action required—headers-accept is already at the latest stable version.The headers-accept crate version 0.3.0 is already the current latest stable version available on crates.io. No security advisories exist for this crate in the RustSec database.
…d improving format negotiation
twangodev
force-pushed
the
feat/header-accept
branch
from
9f6d2de to
3b943b7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn test_no_accept_header() { | ||
| assert_eq!(negotiate_format(None), None); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_empty_accept_header() { | ||
| assert_eq!(negotiate_format(Some("")), None); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_webp_only() { | ||
| assert_eq!( | ||
| negotiate_format(Some("image/webp")), | ||
| Some(OutputFormat::WebP) | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_png_only() { | ||
| assert_eq!(negotiate_format(Some("image/png")), Some(OutputFormat::Png)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_jpeg_only() { | ||
| assert_eq!( | ||
| negotiate_format(Some("image/jpeg")), | ||
| Some(OutputFormat::Jpeg) | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_quality_preference_png_wins() { | ||
| // PNG has higher q value than WebP | ||
| let result = negotiate_format(Some("image/webp;q=0.8, image/png;q=0.9")); | ||
| assert_eq!(result, Some(OutputFormat::Png)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_quality_preference_webp_wins() { | ||
| // WebP has implicit q=1.0, PNG has q=0.9 | ||
| let result = negotiate_format(Some("image/png;q=0.9, image/webp")); | ||
| assert_eq!(result, Some(OutputFormat::WebP)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_unsupported_format_returns_none() { | ||
| assert_eq!(negotiate_format(Some("image/gif")), None); | ||
| assert_eq!(negotiate_format(Some("text/html")), None); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_browser_like_header() { | ||
| // Chrome-like Accept header for images | ||
| let header = "image/avif,image/webp,image/apng,image/svg+xml,image/*;q=0.8,*/*;q=0.5"; | ||
| let result = negotiate_format(Some(header)); | ||
| // Should return WebP since we don't support avif/apng/svg | ||
| assert_eq!(result, Some(OutputFormat::WebP)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding a test case for when Accept header specifies wildcard image/* or */* alone (without specific formats). This would verify that the negotiation properly selects a supported format when only wildcards are provided. For example:
#[test]
fn test_wildcard_image_only() {
assert_eq!(
negotiate_format(Some("image/*")),
Some(OutputFormat::WebP) // or whichever format the library chooses first
);
}
#[test]
fn test_wildcard_all_only() {
assert_eq!(
negotiate_format(Some("*/*")),
Some(OutputFormat::WebP) // or whichever format the library chooses first
);
}This ensures the behavior is well-defined and documented when browsers send generic Accept headers.
|
|
||
| // Get render options and record in span | ||
| let render_options = params.render_options(); | ||
| // Get render options with Accept header negotiation |
There was a problem hiding this comment.
Consider adding a code comment here explaining the Accept header negotiation behavior for future maintainers. For example:
// Extract Accept header for content negotiation
// Supports: image/webp, image/png, image/jpeg with quality values (q)
// Falls back to PNG if no match or header absentThis makes the feature more discoverable when reading the code.
| // Get render options with Accept header negotiation | |
| // Extract Accept header for content negotiation. | |
| // Supports: image/webp, image/png, image/jpeg with quality values (q). | |
| // Falls back to PNG if no match is found or the header is absent. |
|
|
||
| use crate::generator::OutputFormat; | ||
|
|
||
| /// All supported output formats for negotiation. |
There was a problem hiding this comment.
Consider adding a comment explaining why the formats are ordered [WebP, Png, Jpeg]. While the order doesn't directly affect negotiation (quality values take precedence), it may influence the behavior when the headers-accept library has to break ties. A comment like:
/// All supported output formats for negotiation.
/// Order may influence tie-breaking when Accept header has equal quality values.
const SUPPORTED_FORMATS: [OutputFormat; 3] =
[OutputFormat::WebP, OutputFormat::Png, OutputFormat::Jpeg];This makes the ordering decision explicit and documents potential edge case behavior.
| /// All supported output formats for negotiation. | |
| /// All supported output formats for negotiation. | |
| /// Order may influence tie-breaking when Accept header has equal quality values. |
| #[test] | ||
| fn test_render_options_accept_header_negotiation() { | ||
| let params = default_params(); | ||
| // Accept header should be used when no format param | ||
| let opts = params.render_options_with_accept(Some("image/webp")); | ||
| assert_eq!(opts.format, OutputFormat::WebP); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_render_options_format_overrides_accept() { | ||
| let mut params = default_params(); | ||
| params.format = Some("jpeg".to_string()); | ||
| // Explicit format should override Accept header | ||
| let opts = params.render_options_with_accept(Some("image/webp")); | ||
| assert_eq!(opts.format, OutputFormat::Jpeg); | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding a test case for when the Accept header contains an unsupported format but no explicit format parameter is provided. This verifies the fallback behavior. For example:
#[test]
fn test_render_options_unsupported_accept_fallback() {
let params = default_params();
// Should fall back to PNG when Accept header has no supported format
let opts = params.render_options_with_accept(Some("image/gif"));
assert_eq!(opts.format, OutputFormat::Png);
}This ensures the documented behavior (Priority 3: Default to PNG) is tested.
|
|
||
| /// Parsed MIME types for negotiation, initialized once. | ||
| static AVAILABLE_MIMES: LazyLock<[mediatype::MediaType; 3]> = LazyLock::new(|| { | ||
| SUPPORTED_FORMATS.map(|f| mediatype::MediaType::parse(f.content_type()).unwrap()) |
There was a problem hiding this comment.
The .unwrap() call here could be made more explicit about why it's safe. While it will never fail (since the hardcoded MIME types are valid), consider adding a comment explaining this invariant or using .expect() with a descriptive message. This improves code maintainability by making the safety assumption clear to future readers.
| SUPPORTED_FORMATS.map(|f| mediatype::MediaType::parse(f.content_type()).unwrap()) | |
| SUPPORTED_FORMATS.map(|f| { | |
| mediatype::MediaType::parse(f.content_type()) | |
| .expect("OutputFormat::content_type() must return a valid MIME type") | |
| }) |
| OutputFormat::from_str(fmt) | ||
| .expect("format should be validated before calling render_options_with_accept") |
There was a problem hiding this comment.
The .expect() call creates a brittle API contract that relies on runtime validation. While the current code correctly calls validate() before this method, this design makes the API error-prone for future changes. Consider one of these alternatives:
- Make the validation part of this method by returning
Result<RenderOptions, ApiError>and validating the format here if present - Accept
OutputFormatdirectly instead ofOption<String>for the format field (though this would require changes to deserialization) - Use a fallback to the default format instead of panicking:
OutputFormat::from_str(fmt).unwrap_or_default()
Option 3 would be the simplest and maintain backward compatibility while being more defensive.
| OutputFormat::from_str(fmt) | |
| .expect("format should be validated before calling render_options_with_accept") | |
| OutputFormat::from_str(fmt).unwrap_or_default() |
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.