engineering: minor improvements to HttpFile #456
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to improve the HttpFile implementation by replacing hard-coded string literals with library-provided constants for HTTP headers and status codes.
Changes:
- Introduced a new
HttpRangeRequeststruct to handle HTTP range header generation - Replaced hard-coded HTTP header names with constants from
reqwest::header - Replaced numeric HTTP status code comparisons with
StatusCodeconstants - Improved error handling in Docker credential authentication flow
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| crates/trident/src/io_utils/mod.rs | Added private http_range module |
| crates/trident/src/io_utils/http_range.rs | New module containing HttpRangeRequest struct for generating HTTP range headers |
| crates/trident/src/io_utils/http_file.rs | Refactored to use header/status constants, improved auth error handling, uses new HttpRangeRequest |
| /// A struct representing a file range in an HTTP request. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub(super) struct HttpRangeRequest { | ||
| pub start: Option<u64>, | ||
| pub end: Option<u64>, | ||
| } |
There was a problem hiding this comment.
The documentation for this struct should clarify the semantics of the start and end fields. Specifically, it should state whether end represents an inclusive byte position (e.g., end=99 means bytes 0-99) or an exclusive position (e.g., end=100 means bytes 0-99). Based on the usage in section_reader which calculates end = section_offset + size - 1, it appears end is intended to be an inclusive byte position, but this should be explicitly documented to prevent bugs.
| Some(match (self.start, self.end) { | ||
| (Some(start), Some(end)) => format!("bytes={}-{}", start, end - 1), | ||
| (Some(start), None) => format!("bytes={}-", start), | ||
| (None, Some(end)) => format!("bytes=-{}", end), |
There was a problem hiding this comment.
This implementation changes the semantics from the old code. The old code generated bytes=0-{end} which means "get bytes from position 0 to position end (inclusive)". The new code generates bytes=-{end} which means "get the last end bytes of the file". These are different HTTP range semantics. While this case doesn't appear to be used in the current codebase, the semantic change could cause bugs if this method is used with start=None, end=Some(value) in the future. The old behavior should be preserved or the semantics should be clearly documented.
| /// when requesting the whole file because both start and end are None. | ||
| pub fn to_header_value(self) -> Option<String> { | ||
| Some(match (self.start, self.end) { | ||
| (Some(start), Some(end)) => format!("bytes={}-{}", start, end - 1), |
There was a problem hiding this comment.
The end - 1 calculation is incorrect here. The end parameter passed to this method already represents an inclusive byte position (as calculated in section_reader at line 365: let end = section_offset + size - 1). Subtracting 1 again will result in requesting one fewer byte than intended. For example, if requesting bytes 0-99, this will generate "bytes=0-98" instead of "bytes=0-99", resulting in only 99 bytes being read instead of 100.
| let range = HttpRangeRequest::new(Some(0), Some(100)); | ||
| assert_eq!(range.to_header_value(), Some("bytes=0-99".to_string())); |
There was a problem hiding this comment.
This test assertion is incorrect. If the end parameter is 100 and represents an inclusive byte position, the header should be "bytes=0-100", not "bytes=0-99". The test is currently passing because the implementation has a bug (subtracting 1), but both the test and implementation are wrong. The test should either pass Some(99) as end to get "bytes=0-99", or expect "bytes=0-100" if passing Some(100).
| .with_body(data) | ||
| .with_header("content-length", &data.len().to_string()) | ||
| .with_header("content-type", "text/plain") | ||
| .with_header(CONTENT_RANGE, &data.len().to_string()) |
There was a problem hiding this comment.
The header constant should be CONTENT_LENGTH, not CONTENT_RANGE. This test is mocking a response header that specifies the content length of the response body, not a content range. The old code used "content-length" (lowercase) which is correct. CONTENT_RANGE is used in responses to range requests to indicate what range was returned (e.g., "bytes 0-99/1000"), not the total content length.
| .with_header(CONTENT_RANGE, &data.len().to_string()) | |
| .with_header(CONTENT_LENGTH, &data.len().to_string()) |
|
Superseded by #458 |
🔍 Description
Minor improvements, mostly use library-provided constants instead fo hard-coded values.