feat(body): Support mime-type inference #206
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 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. 📝 WalkthroughWalkthroughImplements MIME-type inference in Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant B as Body.fromString
participant I as _tryInferTextMimeTypeFrom
participant R as Result(Body)
C->>B: fromString(text, encoding=?, mimeType=null)
alt mimeType provided
B->>R: Construct Body with provided mimeType/encoding
else infer from text
B->>I: Inspect prefix/whitespace
I-->>B: Inferred MIME (json/xml/html/plain)
B->>R: Construct Body with inferred MIME and encoding
end
sequenceDiagram
participant C as Caller
participant D as Body.fromData
participant MR as MimeTypeResolver
participant R as Result(Body)
C->>D: fromData(bytes, encoding=?, mimeType=null)
alt mimeType provided
D->>R: Construct Body with provided mimeType/encoding
else infer from bytes
D->>MR: lookupMimeType(bytes)
MR-->>D: MIME or null
D->>R: Construct Body with resolved MIME or octet-stream
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #206 +/- ##
==========================================
+ Coverage 91.85% 91.90% +0.04%
==========================================
Files 85 85
Lines 3156 3175 +19
Branches 1631 1639 +8
==========================================
+ Hits 2899 2918 +19
Misses 257 257 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@CodeRabbit 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: 0
🧹 Nitpick comments (3)
test/body/body_infer_mime_type_test.dart (1)
9-52: Consider adding a test for JSON array inference.The tests comprehensively cover JSON object inference (starting with
{), but there's no test verifying JSON array inference (starting with[). While the implementation inbody.darthandles this case, adding a test would ensure this path is validated.Example test to add:
test( 'Given JSON array content without explicit mimeType, ' 'when Body.fromString is called, ' 'then it infers application/json', () { const jsonArrayContent = '[{"key": "value"}, {"key2": "value2"}]'; final body = Body.fromString(jsonArrayContent); expect(body.bodyType?.mimeType, MimeType.json); });lib/src/body/body.dart (2)
78-111: Minor: Consider case-insensitive HTML tag detection.The inference logic is well-structured and avoids unnecessary allocations. However, HTML tag detection is case-sensitive for the
<htmltag (line 106) while it handlesDOCTYPEcase variations. Although uppercase HTML tags are rare in modern code,<HTML>is technically valid HTML.If desired, you could make the HTML tag check case-insensitive:
- if (prefix.startsWith('<!DOCTYPE html') || - prefix.startsWith('<!doctype html') || - prefix.startsWith('<html')) { + final lowerPrefix = prefix.toLowerCase(); + if (lowerPrefix.startsWith('<!doctype html') || + lowerPrefix.startsWith('<html')) { return MimeType.html; }Note: This adds a
toLowerCase()allocation, so weigh the trade-off.
134-157: Consider defensive error handling for MIME type parsing.The MIME type resolution logic is well-structured, but there's a potential risk: if
_resolver.lookupreturns an invalid MIME string (unlikely but possible),MimeType.parsewill throw aFormatExceptionand crash the factory.While the
mimepackage should only return valid MIME strings or null, adding defensive error handling would improve robustness:if (mimeType == null) { final mimeString = _resolver.lookup('', headerBytes: body); - mimeType = mimeString == null ? null : MimeType.parse(mimeString); + if (mimeString != null) { + try { + mimeType = MimeType.parse(mimeString); + } on FormatException { + // Invalid MIME string from resolver, ignore and fall back + mimeType = null; + } + } }Alternatively, if you're confident the
mimepackage always returns valid strings, the current code is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/src/body/body.dart(4 hunks)lib/src/body/types/body_type.dart(0 hunks)lib/src/body/types/mime_type.dart(3 hunks)test/body/body_infer_mime_type_test.dart(1 hunks)
💤 Files with no reviewable changes (1)
- lib/src/body/types/body_type.dart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-22T07:34:15.403Z
Learnt from: CR
PR: serverpod/relic#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-22T07:34:15.403Z
Learning: Applies to {lib,example}/**/*.dart : Use Uint8List for request/response bodies; do not use List<int>
Applied to files:
lib/src/body/body.dart
🔇 Additional comments (7)
lib/src/body/types/mime_type.dart (3)
48-48: LGTM! Clear documentation.The added doc comment appropriately documents the constructor's purpose.
61-62: LGTM! Defensive input sanitization.Adding
.trim()correctly handles whitespace in MIME type strings, improving robustness against malformed input.
71-71: LGTM! Clear documentation.The doc comment accurately describes the getter's behavior.
test/body/body_infer_mime_type_test.dart (1)
87-157: LGTM! Comprehensive binary MIME type inference tests.The tests correctly use standard file format signatures (PNG, JPEG, GIF, PDF) and properly verify inference, fallback behavior, explicit overrides, and empty data handling.
lib/src/body/body.dart (3)
3-6: LGTM! Necessary imports for inference logic.The
dart:mathimport supports themin()function in the prefix extraction logic, andpackage:mime/mime.dartprovides theMimeTypeResolverfor binary MIME type detection.
57-76: LGTM! Well-documented optional inference.The factory correctly makes
mimeTypeoptional and provides clear documentation about the inference behavior and performance implications. The fallback toMimeType.plainTextis a sensible default.
113-117: LGTM! Sufficient whitespace detection.The helper correctly identifies common whitespace characters (space, tab, newline, carriage return), which is sufficient for typical HTTP body content.
nielsenko
force-pushed
the
mimetype-infer
branch
from
54c99f6 to
bbd013b
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds automatic MIME type inference to the Relic web server's Body class. Instead of defaulting to text/plain and application/octet-stream, the Body constructors now analyze the content to detect appropriate MIME types like JSON, HTML, XML, and various binary formats.
Key changes:
- Automatic MIME type detection for string and binary content when not explicitly provided
- Removal of predefined static body type constants in favor of dynamic creation
- Enhanced MIME type parsing with input sanitization
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/src/body/body.dart | Added MIME inference logic for text and binary content with fallback defaults |
| lib/src/body/types/body_type.dart | Removed static body type presets, requiring explicit creation |
| lib/src/body/types/mime_type.dart | Enhanced MIME type parsing with trimming and added documentation |
| test/body/body_infer_mime_type_test.dart | Comprehensive test suite covering MIME inference scenarios |
…Body.fromData Tests cover: - HTML text data detection - XML text data detection - JSON text data detection - Unknown text data fallback to text/plain - PNG binary data detection - JPEG binary data detection - GIF binary data detection - PDF binary data detection - Unknown binary data fallback to application/octet-stream - Explicit mimeType override - Empty binary data handling
nielsenko
force-pushed
the
mimetype-infer
branch
from
bbd013b to
e767a1c
Compare
SandPod
left a comment
SandPod
left a comment
There was a problem hiding this comment.
LGTM with one comment to remove redundant comments.
2 participants
Description
This PR will allow relic to infer the mime-type of a body on construction, if not explicitly set. In particular html passed to
Body.fromStringwon't be given mime-typetext/plain, but other types are inferred as well. See tests for examples.Related Issues
Pre-Launch Checklist
Please ensure that your PR meets the following requirements before submitting:
///), ensuring consistency with existing project documentation.Breaking Changes
Additional Notes
It is debatable if this is a breaking change. The interface doesn't change, and if you specify
mimeTypethere is no change, but with this PR we try to infer themimeTypeinstead of just defaultingtext/plainandapplication/octet-stream.Summary by CodeRabbit
New Features
Refactor
Documentation
Tests