fix head request handler again #1524
Conversation
WalkthroughRefactored HEAD request handling in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as composeGeneralHandler
participant Promise as Promise Chain
participant Response as Response Builder
Client->>Handler: HEAD Request
Handler->>Promise: Promise.resolve(ht[...].composed(c))
Promise->>Promise: .then(_ht => getResponseLength(_ht))
Promise->>Response: Set content-length header
Response->>Response: Create Response(null, {status, statusText, headers})
Response-->>Client: Return HEAD Response with Content-Length
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| `return Promise.resolve(ht[${methods.GET ?? methods.ALL}].composed(c)).then(_ht=>getResponseLength(_ht).then((length)=>{` + | ||
| `_ht.headers.set('content-length', length)\n` + | ||
| `return new Response(null,{status:_ht.status,statusText:_ht.statusText,headers:_ht.headers})\n` + | ||
| `})` + | ||
| `}\n` | ||
| `}))\n` |
There was a problem hiding this comment.
Add null check before calling getResponseLength() to prevent potential runtime errors.
The static route HEAD handling wraps the handler result in Promise.resolve() and immediately chains to getResponseLength(_ht) without checking if _ht is null/undefined. In contrast, the dynamic route HEAD handling (line 2298) includes an if(_res) guard before proceeding. If composed(c) returns null/undefined, this will pass null to getResponseLength(), likely causing a runtime error.
Additionally, the inconsistency between static and dynamic route handling patterns makes the codebase harder to maintain.
Consider applying this pattern to match the dynamic route handling:
switchMap +=
`case 'HEAD':` +
- `return Promise.resolve(ht[${methods.GET ?? methods.ALL}].composed(c)).then(_ht=>getResponseLength(_ht).then((length)=>{` +
+ `return Promise.resolve(ht[${methods.GET ?? methods.ALL}].composed(c)).then(_ht=>{` +
+ `if(!_ht)return\n` +
+ `return getResponseLength(_ht).then((length)=>{` +
`_ht.headers.set('content-length', length)\n` +
`return new Response(null,{status:_ht.status,statusText:_ht.statusText,headers:_ht.headers})\n` +
- `}))\n`
+ `})})\n`Alternatively, if handlers are guaranteed to never return null/undefined, document this assumption clearly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `return Promise.resolve(ht[${methods.GET ?? methods.ALL}].composed(c)).then(_ht=>getResponseLength(_ht).then((length)=>{` + | |
| `_ht.headers.set('content-length', length)\n` + | |
| `return new Response(null,{status:_ht.status,statusText:_ht.statusText,headers:_ht.headers})\n` + | |
| `})` + | |
| `}\n` | |
| `}))\n` | |
| `return Promise.resolve(ht[${methods.GET ?? methods.ALL}].composed(c)).then(_ht=>{` + | |
| `if(!_ht)return\n` + | |
| `return getResponseLength(_ht).then((length)=>{` + | |
| `_ht.headers.set('content-length', length)\n` + | |
| `return new Response(null,{status:_ht.status,statusText:_ht.statusText,headers:_ht.headers})\n` + | |
| `})})\n` |
🤖 Prompt for AI Agents
In src/compose.ts around lines 2384–2387, the static HEAD handler calls
getResponseLength(_ht) without checking whether _ht (the result of composed(c))
is null/undefined; add a truthiness guard like the dynamic route does: if _ht is
truthy, await getResponseLength(_ht), set the content-length header and return
the Response constructed from _ht; if _ht is falsy, skip getResponseLength and
immediately return an appropriate empty Response (mirroring the dynamic route
behavior) so no null is passed into getResponseLength and static and dynamic
handling patterns are consistent.
|
I'm in need of this fix @SaltyAom any idea of when we get a new release? maybe it's time to figure out canary builds. |
3 participants
fix #1520 again
Summary by CodeRabbit