Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates request.type to return a trimmed, lowercase media type, bringing it into alignment with RFC 9110 and the normalization performed by content-type/type-is, with regression tests for whitespace and mixed-case headers. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2019 +/- ##
=======================================
Coverage 99.90% 99.90%
=======================================
Files 9 9
Lines 2144 2146 +2
=======================================
+ Hits 2142 2144 +2
Misses 2 2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This is a good fix that I think can be deferred for v4. |
|
Thanks! Agreed. A header like |
|
@ybai08 we're good, I think/hope. I am hoping to do a patch release soon. Great work! It's much appreciated. |
ctx.request.typecuts theContent-Typevalue at the first;and returns what comes before it unchanged. This gives the wrong result for two valid headers:request.typeon masterapplication/json ; charset=utf-8'application/json '(trailing space)'application/json'Application/JSON; charset=utf-8'Application/JSON''application/json'RFC 9110 section 8.3.1 allows optional whitespace before the
;that starts the parameters (media-type = type "/" subtype parameters, withparameters = *( OWS ";" OWS parameter )). It also says type and subtype are case-insensitive. So code likectx.request.type === 'application/json'fails today for headers that are perfectly valid.ctx.is('json')already matches both, becausetype-isnormalizes the value, sorequest.typecurrently disagrees withctx.is().The getter now trims the media type and lowercases it. That matches what
content-type/type-isreturn for the same header. Headers that are already lowercase with no space before;(the common case) give the same result as before.Tests
__tests__/request/type.test.js(whitespace before parameters, mixed case). Both fail on master (actual: 'application/json 'andactual: 'Application/JSON') and pass with this change.npm test: 468 tests, 468 pass (Node v26.10.0, macOS).npx standard: clean.The lowercasing could be seen as a behavior change for anyone comparing against a mixed-case literal. If you'd rather only trim, I can drop that part.
Checklist
Summary by Sourcery
Normalize Content-Type media types returned by
request.typefor standards-compliant, case-insensitive matching.Bug Fixes:
request.typeby trimming optional whitespace and lowercasing media types so valid Content-Type headers are interpreted consistently.Tests:
Summary by CodeRabbit