refactor: Static File Handler Overhaul #127
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughRewrote static file serving to use NewContext handlers with secure root resolution, per-path LRU file-info caching, ETag/Last-Modified/Cache-Control support, full Range (single & multipart) handling, MIME resolution via resolver, removed directory-listing, adjusted Body/MimeType defaults, and added routing context accessors and related tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant H as StaticHandler
participant Cache as FileInfoCache
participant FS as FileSystem
C->>H: GET/HEAD /path
H->>H: normalize & resolve -> resolvedPath
alt outside root or not a file
H-->>C: 404 Not Found
else
H->>Cache: lookup resolvedPath
alt cache miss
H->>FS: stat & read header bytes
FS-->>H: metadata (size,mtime,header bytes)
H->>Cache: store _FileInfo (etag,mime,stat)
end
H->>H: check If-None-Match / If-Modified-Since
alt not modified
H-->>C: 304 Not Modified
else
H->>H: parse Range / If-Range
alt single range
H->>FS: stream bytes [start..end]
FS-->>H: byte stream
H-->>C: 206 Partial Content + Content-Range
else multiple ranges
loop per range
H->>FS: stream segment
FS-->>H: bytes
end
H-->>C: 206 multipart/byteranges (boundary)
else no range
H->>FS: stream full file
FS-->>H: bytes
H-->>C: 200 OK
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
nielsenko
changed the title
nielsenko
force-pushed
the
static-files
branch
5 times, most recently
from
813c988 to
aff9012
Compare
nielsenko
force-pushed
the
static-files
branch
3 times, most recently
from
4a5e7e9 to
95156a1
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #127 +/- ##
==========================================
+ Coverage 83.40% 84.20% +0.79%
==========================================
Files 84 83 -1
Lines 2676 2716 +40
Branches 1479 1480 +1
==========================================
+ Hits 2232 2287 +55
+ Misses 444 429 -15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nielsenko
force-pushed
the
static-files
branch
from
95156a1 to
306860d
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/static/basic_file_test.dart (1)
153-171: Correct the If-Modified-Since > Last-Modified scenario setup.This test duplicates the “equal” case; it should set If-Modified-Since to after the file’s last modified time to truly validate the “after” branch.
- final headers = - Headers.build((final mh) => mh.ifModifiedSince = modified); + final headers = Headers.build( + (final mh) => mh.ifModifiedSince = modified.add(const Duration(seconds: 1)), + );lib/src/middleware/routing_middleware.dart (1)
31-37: Fix late field initialization bug in constructorArgumentError.checkNotNull(_toHandler, ...) reads the late field before it’s set, causing a LateInitializationError when T is not a Handler and toHandler is null. Throw explicitly instead of reading the late field.
_RoutingMiddlewareBuilder( this._router, { final Handler Function(T)? toHandler, }) { if (toHandler != null) { _toHandler = toHandler; } else if (_isSubtype<T, Handler>()) { _toHandler = (final x) => x as Handler; - } - ArgumentError.checkNotNull(_toHandler, 'toHandler'); + } else { + throw ArgumentError( + 'toHandler must be provided when T is not a Handler.', + ); + } }
🧹 Nitpick comments (23)
lib/src/body/types/mime_type.dart (1)
71-72: Broaden text detection beyond onlytext/*(JSON, XML, JS, form-encoded are text-like).Relying solely on
primaryType == 'text'will treatapplication/json,application/xml,application/javascript,image/svg+xml(via+xml), andapplication/*+json|+xmlas non-text. IfisTextinforms charset/encoding decisions elsewhere (as the PR summary suggests), this may yield suboptimal defaults and headers for these widely used textual types.Suggested improvement: expand
isTextto include common text-like application subtypes and structured syntax suffixes.- bool get isText => primaryType == 'text'; + bool get isText { + if (primaryType == 'text') return true; + if (primaryType == 'application') { + final st = subType.toLowerCase(); + // Common text-like app types and structured syntax suffixes. + return st == 'json' || + st == 'xml' || + st == 'javascript' || + st == 'x-www-form-urlencoded' || + st.endsWith('+json') || + st.endsWith('+xml'); + } + return false; + }test/static/range_edge_cases_test.dart (2)
112-114: Fix test descriptions to match expected 206 status.Both descriptions say “200 OK” but the assertions correctly expect
HttpStatus.partialContent(206). Update the titles to avoid confusion in test output.- 'then a 200 OK status is returned with correct content', () async { + 'then a 206 Partial Content status is returned with correct content', () async {Apply the same change to both tests (“last byte” and “first byte”).
Also applies to: 134-136
155-176: Consider asserting Accept-Ranges for partial responses.To fully validate Range semantics, add an assertion that
Accept-Rangesis present and set tobytesfor 206 responses. This catches regressions where the header is omitted.Example (adjust to your headers API):
expect(response.headers.value('accept-ranges'), 'bytes'); // or, if typed: expect(response.headers.acceptRanges, 'bytes');Also applies to: 178-199, 202-224, 226-247
test/static/symbolic_link_test.dart (3)
5-5: Remove now-unused internal import.
package:relic/src/io/static/static_handler.dartappears unused after commenting out the “serveFilesOutsidePath” tests and relying on the public API. Drop it to avoid analyzer warnings.-import 'package:relic/src/io/static/static_handler.dart';
49-50: Replace temporary skips with tracked context or conditional platform skips.
skip: 'TMP'is opaque and easy to forget. Either:
- tie the skip message to a tracking issue/PR (“Skipped pending symlink semantics stabilization – see PR #127”), or
- convert to
onPlatformif it’s platform-specific like the Windows case above.Example:
- skip: 'TMP', + skip: 'Temporarily disabled pending symlink resolution semantics; see PR #127.',Also applies to: 84-86
95-165: Remove dead tests for deprecatedserveFilesOutsidePath.Given the option was removed in this PR, keeping the whole group commented adds noise. Safer to delete to avoid confusion.
-/* - group('Given access outside of root is enabled', () { - ... - }); - */test/middleware/routing_middleware_test.dart (1)
77-85: Update test description to match new semantics (no exception; returns empty map).The test name still claims a StateError is thrown, but the assertion now expects an empty map via ctx.pathParameters. Align the description with behavior.
- 'Then the next handler is called and pathParameters access throws StateError', + 'Then the next handler is called and pathParameters is empty',test/static/basic_file_test.dart (1)
71-79: Fix mismatch between test name and exercised path (encoded vs. unencoded space).The test description promises an “unencoded space” path, but the request uses %20. Exercise an actual unencoded space to validate routing and URL handling.
- final response = await makeRequest(handler, '/files/with%20space.txt'); + final response = await makeRequest(handler, '/files/with space.txt');If makeRequest requires a pre-encoded URL, consider adjusting the description instead to avoid false signaling.
test/static/unsupported_methods_test.dart (1)
18-106: Good coverage for 405 + Allow; consider DRYing repeated assertions.The four tests are nearly identical except for the method. Extract a tiny helper to reduce duplication and ease future additions (e.g., PATCH).
// Add near the top of the group: Future<void> expectMethodNotAllowedFor(Handler handler, String path, RequestMethod method) async { final response = await makeRequest(handler, path, method: method); expect(response.statusCode, HttpStatus.methodNotAllowed); expect(response.body.contentLength, 0); expect(await response.readAsString(), isEmpty); final allow = response.headers.allow; expect(allow, isNotNull); expect(allow, containsAll({RequestMethod.get, RequestMethod.head})); expect(allow!.length, 2); } // Then replace body of each test with: await expectMethodNotAllowedFor(handler, '/test_file.txt', RequestMethod.post);test/static/not_found_test.dart (1)
97-123: Guard the symlink test for platforms without symlink support (Windows).Creating symlinks may fail on Windows or restricted environments. Mark the test as skipped on Windows to avoid flakiness.
- test( + test( 'Given a symbolic link pointing outside the served directory, ' 'when the request is made, ' 'then a 404 Not Found status is returned', () async { @@ - }); + }, skip: Platform.isWindows);test/static/if_modified_since_test.dart (1)
18-35: Add coverage for If-None-Match vs If-Modified-Since precedenceHTTP requires If-None-Match to take precedence over If-Modified-Since. Add a test where both headers are present and ETag matches, expecting 304 regardless of the IMS date.
I can draft a companion test that sets both headers and asserts 304. Want me to add it?
test/static/if_none_match_test.dart (2)
18-35: Add wildcard If-None-Match (“*”) caseWhen the resource exists and If-None-Match is "*", the server should return 304 Not Modified (for safe methods). Add a test to lock in this behavior.
You can add something like:
test('If-None-Match: * yields 304 when resource exists', () async { final headers = Headers.build( (mh) => mh.ifNoneMatch = const IfNoneMatchHeader.any(), ); final response = await makeRequest(handler, '/test_file.txt', headers: headers); expect(response.statusCode, HttpStatus.notModified); expect(response.body.contentLength, 0); expect(await response.readAsString(), isEmpty); });
37-53: Consider explicit strong/weak ETag coverageETag generation here is presumably strong. It’s useful to add one test asserting behavior with a weak ETag in If-None-Match (e.g., W/"...") to ensure comparison semantics are correct.
Happy to draft a weak-ETag test tailored to the current header types if desired.
test/static/if_range_test.dart (2)
69-71: Assert Content-Range metadata on 206 responsesYou already verify status and payload. Adding checks for Content-Range strengthens correctness (start, end, and total size), and guards regressions in range calculations.
If the response exposes headers.contentRange, I can add precise assertions for bytes 0-4/16.
74-91: Add coverage for multi-range + If-Range behaviorMulti-part ranges should behave identically wrt If-Range: return 206 multipart when the validator matches, otherwise 200 with full body. One small test here will close that gap.
test/static/content_type_test.dart (1)
29-76: Optional: Add SVG and WASM casesTwo practical additions: image/svg+xml and application/wasm. These help catch common asset-type regressions.
I can append tests for /icon.svg and /module.wasm with appropriate expected MIME types.
lib/src/middleware/routing_middleware.dart (2)
42-49: Decode path once; current approach is fine, but consider NormalizedPath.from decoded stringYou decode with Uri.decodeFull before lookup, which is good. If NormalizedPath offers a canonicalizer from raw path, using it consistently here could simplify future changes. Not required.
50-52: Avoid redundantawaiton returnNo try/catch here; returning the Future directly avoids a micro-async hop.
- return await handler(ctx); + return handler(ctx);test/static/range_test.dart (3)
6-6: Remove commented-out import statement.The commented-out import for
pathpackage should be removed since it's not being used.-//import 'package:path/path.dart' as p;
133-152: Consider simplifying the boundary extraction logic.The
_extractBoundaryfunction could be made more concise using regular expressions or simpler string manipulation.-String? _extractBoundary(final String contentType) { - // Split the Content-Type header by ';' to get individual parameters - final parts = contentType.split(';'); - - // Find the part that starts with 'boundary=' - final boundaryPart = parts.firstWhere( - (final part) => part.trim().startsWith('boundary='), - orElse: () => '', - ); - - if (boundaryPart.isEmpty) { - return null; - } - - // Extract the boundary string itself and remove any surrounding quotes - final boundary = - boundaryPart.trim().substring('boundary='.length).replaceAll('"', ''); - - return boundary; -} +String? _extractBoundary(final String contentType) { + final match = RegExp(r'boundary=([^;]+)').firstMatch(contentType); + return match?.group(1)?.replaceAll('"', '').trim(); +}
10-130: Consider adding edge case tests for range requests.While the basic range request scenarios are covered, consider adding tests for:
- Invalid ranges (e.g., start > end)
- Out-of-bounds ranges (e.g., start beyond file size)
- Overlapping ranges in multipart requests
- Empty range list
Would you like me to generate additional test cases for these edge scenarios to improve test coverage?
lib/src/io/static/static_handler.dart (2)
17-28: Consider making the LRU cache size configurable.The file info cache is hard-coded to 10,000 entries. For applications serving many files or with memory constraints, it would be beneficial to make this configurable.
Consider adding a static setter or configuration parameter:
class StaticHandlerConfig { static int cacheSize = 10000; } final _fileInfoCache = LruCache<String, _FileInfo>(StaticHandlerConfig.cacheSize);
273-277: The assert statement on Line 275 may not execute in production.Assert statements are typically removed in production builds. If this is meant to catch an invalid state in production, consider throwing an exception instead.
- // Unreachable, ifRange parser would throw on first access - assert(false, 'Invalid If-Range header'); - return false; + // This should be unreachable as ifRange parser would throw on first access + throw StateError('Invalid If-Range header: missing both ETag and Last-Modified');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (22)
benchmark/benchmark.dart(1 hunks)lib/src/body/body.dart(2 hunks)lib/src/body/types/mime_type.dart(1 hunks)lib/src/io/static/directory_listing.dart(0 hunks)lib/src/io/static/static_handler.dart(1 hunks)lib/src/middleware/routing_middleware.dart(4 hunks)pubspec.yaml(2 hunks)test/middleware/routing_middleware_test.dart(1 hunks)test/static/basic_file_test.dart(2 hunks)test/static/cache_control_test.dart(1 hunks)test/static/content_type_test.dart(1 hunks)test/static/create_file_handler_test.dart(0 hunks)test/static/default_document_test.dart(0 hunks)test/static/directory_listing_test.dart(0 hunks)test/static/if_modified_since_test.dart(1 hunks)test/static/if_none_match_test.dart(1 hunks)test/static/if_range_test.dart(1 hunks)test/static/not_found_test.dart(1 hunks)test/static/range_edge_cases_test.dart(1 hunks)test/static/range_test.dart(1 hunks)test/static/symbolic_link_test.dart(4 hunks)test/static/unsupported_methods_test.dart(1 hunks)
💤 Files with no reviewable changes (4)
- test/static/directory_listing_test.dart
- test/static/default_document_test.dart
- test/static/create_file_handler_test.dart
- lib/src/io/static/directory_listing.dart
🔇 Additional comments (15)
test/static/range_edge_cases_test.dart (3)
21-63: Good negative coverage for malformed Range headers.The three malformed-input cases look solid and will help prevent silent acceptances of invalid headers. Nice.
65-108: Edge-case coverage is on point (start beyond EOF, suffix > file, end < start).These assertions align with RFC 7233/9110 expectations. The suffix-range test validating a 206 with length-capped body is especially valuable.
250-266: Nice handling for empty range-list.Explicitly verifying that an empty range list yields 200 with full content (and no Content-Range) is a useful guard against edge-case regressions.
pubspec.yaml (1)
13-13: Dependencies align with the new functionality.
- Adding
crypto: ^3.0.0makes sense for ETag (SHA-1) generation.- Updating
cli_toolsand addingconfigtrack the new benchmark import.No issues from me. If you haven’t already, run
dart pub outdatedlocally to confirm no surprising constraints withlintsandmimeupper-bounds.Also applies to: 25-26
benchmark/benchmark.dart (1)
9-9: Import ofconfigis correct and matches the enum-based option API.The types
Configuration,OptionDefinition, andConfigOptionBaseresolve cleanly withconfig ^0.7.x. Looks good.test/static/basic_file_test.dart (2)
227-229: Migration away from useHeaderBytesForContentType looks correct.Dropping the deprecated flag in favor of default behavior is consistent with the new API that performs MIME detection via extensions and magic numbers.
Please confirm that createStaticHandler now enables magic-number detection by default for extensionless files. If it’s behind a feature flag or resolver-only path, prefer making the test explicit by injecting a resolver (like below) to avoid regressions when defaults change.
- final handler = createStaticHandler( - d.sandbox, /* useHeaderBytesForContentType: true */ - ); + final handler = createStaticHandler( + d.sandbox, + // Ensure header-bytes (magic number) detection remains explicit in tests + mimeResolver: mime.MimeTypeResolver()..addMagicNumber([0x89, 0x50, 0x4E, 0x47], 'image/png'), + );
251-253: LGTM: switched to mimeResolver API.Using mimeResolver with an explicit magic-number definition for WebP is clear and robust.
test/static/cache_control_test.dart (1)
18-70: LGTM: Cache-Control configuration is exercised well.Tests cover explicit directives, default absence, and no-store. This gives confidence in the header serialization and optional behavior.
lib/src/middleware/routing_middleware.dart (1)
61-69: Nice: safe defaults for new gettersReturning {} / NormalizedPath.empty / decoded current path when no route matched makes the extension ergonomics solid.
lib/src/body/body.dart (2)
69-81: The change to encoding behavior is well-designed.The automatic UTF-8 encoding for text MIME types while leaving binary data unencoded is a sensible default that aligns with HTTP standards and typical use cases.
84-95: Consistent encoding logic between fromDataStream and fromData.The encoding resolution logic is consistent across both factory methods, which maintains API predictability.
lib/src/io/static/static_handler.dart (4)
66-74: Good security implementation for symbolic link validation.The check ensures that symbolic links cannot escape the root directory, preventing path traversal attacks.
182-186: Consider chunked hashing for large files.The current ETag generation reads the entire file into memory. For very large files, this could cause memory issues.
The current implementation using
sha1.bind(file.openRead())is actually stream-based and efficient. No change needed.
333-370: Excellent implementation of multipart range responses.The multipart/byteranges implementation correctly handles multiple ranges with proper boundaries, headers, and content sections according to HTTP specifications. The streaming approach with
StreamControlleris efficient for handling large files.
246-249: All If-Range scenarios are covered by existing testsThe codebase includes comprehensive tests for both ETag-based and Last-Modified-based If-Range validation in header parsing and static file handling. No further action is required.
nielsenko
force-pushed
the
static-files
branch
from
a0c8a96 to
734326c
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/src/io/static/static_handler.dart (1)
373-393: Existing concern resolved: Range calculation is correct.The range boundary calculation correctly handles both regular ranges and suffix-length requests as per HTTP specifications. The clamping ensures bounds stay within file size limits.
🧹 Nitpick comments (3)
lib/src/io/static/static_handler.dart (3)
17-28: Consider making the LRU cache size configurable.The hardcoded cache size of 10,000 entries might not be optimal for all deployments. Consider making this configurable to allow users to tune memory usage based on their specific needs.
Add a configuration parameter:
+/// Default LRU cache size for file information. +const int _defaultFileInfoCacheSize = 10000; + /// Cached file information including MIME type, file stats, and ETag. class _FileInfo { final MimeType? mimeType; final FileStat stat; final String etag; const _FileInfo(this.mimeType, this.stat, this.etag); } /// LRU cache for file information to avoid repeated file system operations. -final _fileInfoCache = LruCache<String, _FileInfo>(10000); +LruCache<String, _FileInfo> _fileInfoCache = LruCache<String, _FileInfo>(_defaultFileInfoCacheSize); + +/// Sets the maximum number of file info entries to cache. +/// This should be called before creating any static handlers. +void setFileInfoCacheSize(int size) { + _fileInfoCache = LruCache<String, _FileInfo>(size); +}
183-186: Consider chunked hashing for very large files.The current ETag generation reads the entire file into memory to compute the SHA-1 hash. For very large files, this could cause memory issues. While the streaming API is used, the entire hash computation still requires processing the full file.
For production environments with large files, consider implementing a more efficient ETag strategy:
/// Generates an ETag for the file using SHA-1 hash. Future<String> _generateETag(final File file) async { - final hash = await sha1.bind(file.openRead()).last; - return hash.toString(); + final stat = file.statSync(); + // For large files, use size and modification time for ETag + if (stat.size > 100 * 1024 * 1024) { // 100MB threshold + return '${stat.size}-${stat.modified.millisecondsSinceEpoch}'; + } + // For smaller files, use SHA-1 hash + final hash = await sha1.bind(file.openRead()).last; + return hash.toString(); }
59-74: Add tests for valid symlinks pointing inside the rootWe have comprehensive tests for “symlink pointing outside the served directory” in
test/static/not_found_test.dart(around lines 97–119), but no coverage for the case where a symlink points inside the root directory. To fully exercise this security check and prevent regressions:• Add a test under the same
group('Given symbolic links (if supported)', …)that
– Creates a symlink inside the sandbox pointing to a file also insided.sandbox
– Issues a request for that symlink path
– Asserts a successful (200-OK) or expected file response• (Optional) Extend coverage with nested symlink scenarios and directory symlinks
This will ensure that safe, in-root symlinks continue to work while still blocking external ones.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
lib/src/body/types/mime_type.dart(1 hunks)lib/src/io/static/static_handler.dart(1 hunks)test/middleware/routing_middleware_test.dart(1 hunks)test/static/basic_file_test.dart(3 hunks)test/static/cache_control_test.dart(1 hunks)test/static/content_type_test.dart(1 hunks)test/static/create_file_handler_test.dart(0 hunks)test/static/default_document_test.dart(0 hunks)test/static/if_modified_since_test.dart(1 hunks)test/static/if_none_match_test.dart(1 hunks)test/static/if_range_test.dart(1 hunks)test/static/not_found_test.dart(1 hunks)test/static/range_edge_cases_test.dart(1 hunks)test/static/range_test.dart(1 hunks)test/static/symbolic_link_test.dart(4 hunks)test/static/unsupported_methods_test.dart(1 hunks)
💤 Files with no reviewable changes (2)
- test/static/create_file_handler_test.dart
- test/static/default_document_test.dart
🚧 Files skipped from review as they are similar to previous changes (11)
- test/static/range_test.dart
- test/static/unsupported_methods_test.dart
- test/static/content_type_test.dart
- test/static/if_modified_since_test.dart
- test/middleware/routing_middleware_test.dart
- test/static/if_none_match_test.dart
- test/static/range_edge_cases_test.dart
- test/static/cache_control_test.dart
- test/static/if_range_test.dart
- test/static/not_found_test.dart
- test/static/symbolic_link_test.dart
🔇 Additional comments (8)
lib/src/body/types/mime_type.dart (2)
71-84: LGTM! Well-designed text classification with comprehensive coverage.The
isTextgetter correctly identifies text MIME types using primary type checking and handles common text-like application subtypes including JSON, XML, and structured syntax suffixes (+json, +xml). The implementation aligns well with the encoding determination logic in the static file handler.
92-99: LGTM! Proper equality and hash code implementation.The equality operator and hash code are correctly implemented for value equality based on primary and sub types. This enables proper MIME type comparisons and caching in collections like the LRU cache used in the static file handler.
lib/src/io/static/static_handler.dart (3)
113-140: LGTM! Well-structured file serving with proper HTTP semantics.The
_serveFilefunction provides excellent separation of concerns: method validation, caching, conditional requests, range requests, and full file serving. The flow is clear and follows HTTP specifications correctly.
243-258: LGTM! Comprehensive range request handling.The range request handler properly validates If-Range headers and elegantly handles single vs. multiple ranges using pattern matching. The fallback to full file serving for invalid ranges is correct per HTTP specifications.
406-410: Null MIME type handling resolved.The previous issue with the non-null assertion operator on
fileInfo.mimeType!has been fixed. The code now properly handles null MIME types with a fallback toMimeType.octetStream.test/static/basic_file_test.dart (3)
71-79: LGTM! Test validates unencoded space handling.The test change correctly validates that the handler can process request paths with unencoded spaces, which is important for URL handling flexibility.
218-225: LGTM! Test aligns with new default MIME type behavior.The test correctly expects
application/octet-streamfor unknown file extensions, which aligns with the new secure default behavior in the static handler.
229-231: LGTM! Test parameter updates align with API changes.The tests have been properly updated to use the new
mimeResolverparameter instead of the removedcontentTypeResolveranduseHeaderBytesForContentTypeparameters. The commented-out parameters serve as helpful documentation of the API change.Also applies to: 253-254
|
@coderabbitai Add an issue to make LRU cache size configurable. Will revisit this later |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll create an issue to track making the LRU cache size configurable for the static file handler. @nielsenko I've created an issue to track making the LRU cache size configurable for the static file handler. The issue includes context from the current PR implementation and your request, and has been assigned to you for future consideration. 🏁 Script executed: Length of output: 1312 @nielsenko Perfect! I've successfully created issue #131 to track making the LRU cache size configurable for the static file handler. Issue created: #131 The issue includes all the relevant context from PR #127, references your comment, and outlines the enhancement request with clear acceptance criteria. It's been assigned to you for future implementation when you're ready to revisit this improvement. |
nielsenko
force-pushed
the
static-files
branch
2 times, most recently
from
81be31a to
f0e442f
Compare
…rving static files Allow user to specify Cache-Control header to use Ensure you can always read matchedPath, remainingPath, and pathParameters on RequestContext, even if no routing setup. Remaining will be the normalized version of the full path of the request. The others will be empty Handle url decoding in routing Upgrade cli_tools dep Set correct mimetype, if possible Allow single files to be served as well Add tests for static_handler
nielsenko
force-pushed
the
static-files
branch
from
f0e442f to
c3db64c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/static/create_file_handler_test.dart (1)
132-146: Duplicate test case detected.Lines 132-146 and 148-162 appear to be testing the same scenario - both check that a range request with start > end returns a 416 status.
🧹 Nitpick comments (3)
lib/src/io/static/static_handler.dart (3)
27-27: Consider making the LRU cache size configurable.The hardcoded cache size of 10,000 entries may not be optimal for all use cases. For applications serving many files or with memory constraints, this could be problematic.
Since an issue (#131) has already been created to track this, would you like me to provide an implementation that makes the cache size configurable through a parameter or environment variable?
183-186: Consider performance implications of computing SHA-1 on every cache miss.Computing SHA-1 hash of entire files on cache misses could cause performance issues for large files. Consider using file metadata (size + modified time) for ETag generation instead, which is a common practice.
For better performance with large files, consider generating ETags from file metadata:
-/// Generates an ETag for the file using SHA-1 hash. -Future<String> _generateETag(final File file) async { - final hash = await sha1.bind(file.openRead()).last; - return hash.toString(); +/// Generates an ETag for the file using metadata. +Future<String> _generateETag(final File file) async { + final stat = file.statSync(); + // Use size and modified time for ETag generation + final data = '${stat.size}-${stat.modified.millisecondsSinceEpoch}'; + final hash = sha1.convert(utf8.encode(data)); + return hash.toString(); }
121-123: Consider explicit handling of unsupported methods.While the code correctly returns 405 for unsupported methods, consider adding support for OPTIONS method to enable CORS preflight requests, which is commonly needed for static file servers.
Consider extending method support for OPTIONS:
/// Checks if the HTTP method is allowed for file serving. bool _isMethodAllowed(final RequestMethod method) { - return method == RequestMethod.get || method == RequestMethod.head; + return method == RequestMethod.get || + method == RequestMethod.head || + method == RequestMethod.options; }And handle OPTIONS in the main handler:
// Validate HTTP method final method = ctx.request.method; - if (!_isMethodAllowed(method)) return _methodNotAllowedResponse(ctx); + if (method == RequestMethod.options) { + return ctx.withResponse(Response.ok( + headers: Headers.build((final mh) => mh.allow = [ + RequestMethod.get, + RequestMethod.head, + RequestMethod.options, + ]), + )); + } + if (!_isMethodAllowed(method)) return _methodNotAllowedResponse(ctx);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (22)
benchmark/benchmark.dart(1 hunks)lib/src/body/body.dart(2 hunks)lib/src/body/types/mime_type.dart(1 hunks)lib/src/io/static/directory_listing.dart(0 hunks)lib/src/io/static/static_handler.dart(1 hunks)lib/src/middleware/routing_middleware.dart(4 hunks)pubspec.yaml(2 hunks)test/middleware/routing_middleware_test.dart(1 hunks)test/static/basic_file_test.dart(3 hunks)test/static/cache_control_test.dart(1 hunks)test/static/content_type_test.dart(1 hunks)test/static/create_file_handler_test.dart(7 hunks)test/static/default_document_test.dart(5 hunks)test/static/directory_listing_test.dart(0 hunks)test/static/if_modified_since_test.dart(1 hunks)test/static/if_none_match_test.dart(1 hunks)test/static/if_range_test.dart(1 hunks)test/static/not_found_test.dart(1 hunks)test/static/range_edge_cases_test.dart(1 hunks)test/static/range_test.dart(1 hunks)test/static/symbolic_link_test.dart(1 hunks)test/static/unsupported_methods_test.dart(1 hunks)
💤 Files with no reviewable changes (2)
- test/static/directory_listing_test.dart
- lib/src/io/static/directory_listing.dart
🚧 Files skipped from review as they are similar to previous changes (16)
- pubspec.yaml
- benchmark/benchmark.dart
- test/static/if_none_match_test.dart
- test/static/if_range_test.dart
- lib/src/body/types/mime_type.dart
- lib/src/body/body.dart
- test/static/range_test.dart
- test/middleware/routing_middleware_test.dart
- test/static/unsupported_methods_test.dart
- test/static/not_found_test.dart
- test/static/range_edge_cases_test.dart
- lib/src/middleware/routing_middleware.dart
- test/static/default_document_test.dart
- test/static/basic_file_test.dart
- test/static/if_modified_since_test.dart
- test/static/symbolic_link_test.dart
🔇 Additional comments (5)
lib/src/io/static/static_handler.dart (2)
68-74: Security validation looks good!The symbolic link validation correctly prevents path traversal attacks by ensuring resolved paths stay within the root directory. The implementation properly handles both regular files and symbolic links.
218-235: Conditional header handling is well-implemented.The implementation correctly handles both If-None-Match (with wildcard support) and If-Modified-Since headers according to HTTP specifications.
test/static/content_type_test.dart (2)
36-50: Comprehensive MIME type test coverage.Good coverage of common file types including images, web assets, and WebAssembly. The parameterized test structure makes it easy to add more MIME types as needed.
95-119: Charset handling tests are thorough.The tests correctly verify that text files include UTF-8 charset while binary files don't, which aligns with HTTP best practices.
test/static/cache_control_test.dart (1)
18-42: Well-structured Cache-Control tests.The tests properly verify that Cache-Control headers are set correctly with various directives including max-age, public/private cache, and must-revalidate.
SandPod
left a comment
SandPod
left a comment
There was a problem hiding this comment.
Big PR -> a lot of questions.
Overall, it looks good. This will be a solid addition. Would be nice to know what of this is not supported by shelf today.
I had a couple of questions that I wanted to discuss. Some things might be better discussed during a call.
There are also a couple of test fix suggestions.
We do not support linking outside root, which can be enabled in shelf with |
nielsenko
force-pushed
the
static-files
branch
from
5294e44 to
1c00ea1
Compare
nielsenko
force-pushed
the
static-files
branch
from
2077e6d to
83f12f7
Compare
nielsenko
force-pushed
the
static-files
branch
from
eb543ec to
cae909a
Compare
nielsenko
changed the title
2 participants
Description
Static File Handler Overhaul
This PR reimplements the static file serving functionality to provide better HTTP compliance, improved performance, and enhanced security.
Key Improvements
HTTP Compliance & Standards
If-None-Match,If-Modified-Since)Last-Modified,Accept-Ranges, andCache-ControlheadersPerformance Enhancements
Security & Reliability
API Changes
Simplified Configuration
serveFilesOutsidePath,defaultDocument,listDirectories,useHeaderBytesForContentTypedefaultResponse,mimeResolver,cacheControlcreateFileHandler()function for serving single filesSmart Content-Type Handling
nullfor binary files,utf8only for text filestext/plaintoapplication/octet-streamMimeType.isTextgetter for better content type logicEnhanced Routing Context
matchedPathandremainingPathproperties to routing middlewareTesting
Dependencies
cryptopackage for ETag generationThis overhaul brings Relic's static file serving up to production standards with proper HTTP semantics, making it suitable for serving static assets in web applications.
Related Issues
Pre-Launch Checklist
Please ensure that your PR meets the following requirements before submitting:
///), ensuring consistency with existing project documentation.Breaking Changes
createStaticHandler()parameters simplified - some options no longer availablecreateFileHandler()API simplifiedSummary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores
Tests