🔧 fix: respect toResponse() method on Error classes#1543
🔧 fix: respect toResponse() method on Error classes#1543SaltyAom merged 5 commits intoelysiajs:mainfrom
Conversation
Fixes toResponse() being ignored when custom error classes extend Error, and status code being overridden when errors with toResponse() are thrown. The errorToResponse() functions now check for toResponse() before creating default Error responses, and the error handler extracts status from the Response returned by toResponse() before passing it through mapResponse()
WalkthroughAdds an early error-to-response path: if a thrown/returned value exposes toResponse(), the code invokes (and awaits) it, maps the result via afterResponse/mapResponse with a computed target set (headers/status/redirect), and short-circuits prior onError/fallback logic; preserves fallback when absent or when toResponse() throws. Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Route Handler
participant Compose as composeErrorHandler
participant Check as toResponse Check
participant ToResp as error.toResponse()
participant After as afterResponse()
participant Map as mapResponse()
participant OnErr as onError Hooks
participant Fallback as Fallback Response
Handler->>Compose: returns or throws a value/error
Compose->>Check: does value.toResponse() exist?
alt has toResponse()
Check->>ToResp: invoke (sync or await)
ToResp-->>Compose: resolved raw (value or Response) or throws
alt resolved raw produced
Compose->>After: run afterResponse() (if any)
After->>Map: mapResponse(raw, targetSet)
Map-->>Handler: return mapped Response (short-circuit)
else toResponse() threw
Compose->>OnErr: fall back to onError/fallback flow
OnErr->>Fallback: build fallback response
Fallback-->>Handler: return fallback Response
end
else no toResponse()
Compose->>OnErr: run onError hooks
OnErr->>Fallback: build JSON fallback response
Fallback-->>Handler: return fallback Response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/adapter/bun/handler.ts(2 hunks)src/adapter/web-standard/handler.ts(2 hunks)src/compose.ts(1 hunks)test/core/handle-error.test.ts(1 hunks)
…) decide the final status and resolve Async toResponse() still collapsing to 500
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/adapter/bun/handler.ts(2 hunks)src/adapter/web-standard/handler.ts(2 hunks)src/compose.ts(1 hunks)test/core/handle-error.test.ts(1 hunks)
src/compose.ts
Outdated
| `if(typeof error?.toResponse==='function'&&error.constructor.name!=="ValidationError"&&error.constructor.name!=="TransformDecodeError"){` + | ||
| `const raw=error.toResponse()\n` + | ||
| `const apply=(resolved)=>{` + | ||
| `if(resolved instanceof Response)set.status=resolved.status\n` + | ||
| afterResponse() + | ||
| `return context.response=context.responseValue=mapResponse(${saveResponse}resolved,set${adapter.mapResponseContext})\n` + | ||
| `}\n` + | ||
| `if(typeof raw?.then==='function')return raw.then(apply)\n` + | ||
| `return apply(raw)\n` + | ||
| `}\n` |
There was a problem hiding this comment.
Keep mapResponse hooks in the toResponse fast path
The new early-return path short-circuits before the onMapResponse loop a few lines below, so any global app.mapResponse(...) hooks (security headers, telemetry, etc.) are now skipped whenever an error provides toResponse(). That’s a regression from the previous flow, where every error response still went through those hooks. Please restructure this branch so the mapped value still runs through the existing onMapResponse pipeline (including async handlers) before returning. Without that, those hooks silently stop running for the very cases this PR targets.
🤖 Prompt for AI Agents
In src/compose.ts around lines 2605 to 2614, the early-return branch that
handles error.toResponse() currently bypasses the shared
onMapResponse/mapResponse pipeline; instead of returning immediately from the
toResponse fast path, resolve raw (await if it's a promise), apply the same
status-setting and afterResponse steps, then pass the resolved response into the
existing mapResponse/onMapResponse pipeline (ensuring async handlers are
awaited) and only return after that pipeline completes and assigns
context.response/context.responseValue; preserve the existing set.status
assignment and afterResponse invocation while reusing the same
mapResponse/context adapter call so global app.mapResponse hooks run for error
responses too.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/compose.ts(4 hunks)src/dynamic-handle.ts(2 hunks)
🔇 Additional comments (3)
src/dynamic-handle.ts (1)
698-721: LGTM! Correctly implements precedence flow.The conditional logic properly implements the design where
toResponse()takes precedence over error hooks. Whencontext.responseis set bytoResponse(), error hooks are skipped and the response flows directly throughmapResponse()hooks.src/compose.ts (2)
2532-2533: Good practice: Clear comment explaining async requirement.The comment clearly documents why the error handler must always be async, which helps future maintainers understand the architectural decision.
2614-2614: LGTM! Conditional flow correctly implements toResponse() precedence.The modified conditions properly short-circuit error handling when
toResponse()has already produced a response, ensuring that the response still flows throughmapResponse()hooks at the end.Also applies to: 2668-2668
| fnLiteral += | ||
| `if(typeof error?.toResponse==='function'&&error.constructor.name!=="ValidationError"&&error.constructor.name!=="TransformDecodeError"){` + | ||
| `let raw=error.toResponse()\n` + | ||
| `if(typeof raw?.then==='function')raw=await raw\n` + | ||
| `if(raw instanceof Response)set.status=raw.status\n` + | ||
| `context.response=context.responseValue=raw\n` + | ||
| `}\n` |
There was a problem hiding this comment.
Use instanceof checks and wrap toResponse() in try-catch.
The constructor name string comparison is fragile and will break with minification or inheritance. Additionally, since this is in the error handler itself, if toResponse() throws an error, it could cause an infinite loop or make error recovery impossible.
Apply this diff to use instanceof checks and add error handling:
- if(typeof error?.toResponse==='function'&&error.constructor.name!=="ValidationError"&&error.constructor.name!=="TransformDecodeError"){
- let raw=error.toResponse()
- if(typeof raw?.then==='function')raw=await raw
- if(raw instanceof Response)set.status=raw.status
- context.response=context.responseValue=raw
- }
+ if(typeof error?.toResponse==='function'&&!(error instanceof ValidationError)&&!(error instanceof TransformDecodeError)){
+ try{
+ let raw=error.toResponse()
+ if(typeof raw?.then==='function')raw=await raw
+ if(raw instanceof Response)set.status=raw.status
+ context.response=context.responseValue=raw
+ }catch(toResponseError){
+ // If toResponse() fails, fall back to standard error handling
+ context.response=undefined
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/compose.ts around lines 2598 to 2604, the error handler uses fragile
constructor.name string checks and calls error.toResponse() without protection;
replace the string checks with proper instanceof checks against ValidationError
and TransformDecodeError (import or reference the actual classes in scope), and
wrap the call to toResponse() in a try-catch: call toResponse() (await if it
returns a promise) inside try, set set.status if result is a Response and assign
context.response/context.responseValue to the result; in the catch, avoid
rethrowing into the same handler—assign a safe fallback response (or leave
context untouched) and log or record the secondary error so error recovery
doesn't loop.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/dynamic-handle.ts (1)
687-701: Well done addressing the past review feedback.The toResponse() handling now properly uses
instanceofchecks and includes try-catch error handling. The async promise handling and status extraction logic are correct.Minor suggestion: Consider adding a brief comment in the catch block (lines 697-700) explaining that the empty catch is intentional—when toResponse() fails, the handler falls back to standard error processing by leaving context.response unset.
src/compose.ts (1)
2600-2609: The toResponse() implementation correctly addresses previous review concerns.The code now uses
instanceofchecks (line 2601), properly handles async promises (line 2604), extracts Response status (line 2605), and includes try-catch error handling. The exclusion of ValidationError and TransformDecodeError ensures they go through standard validation error paths.Optional: As with src/dynamic-handle.ts, consider adding a brief comment explaining the intentionally empty catch block (lines 2607-2608)—it allows graceful fallback to standard error handling when toResponse() fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/compose.ts(7 hunks)src/dynamic-handle.ts(2 hunks)test/core/handle-error.test.ts(1 hunks)
🔇 Additional comments (5)
src/dynamic-handle.ts (1)
703-726: LGTM! The toResponse() fast-path correctly prioritizes custom error responses.The conditional flow ensures that:
- When toResponse() succeeds, onError hooks are bypassed (line 703)
- toResponse() results still go through mapResponse hooks (lines 716-723), preserving any response transformation plugins
- Headers and status from toResponse() are properly preserved
This design aligns well with the PR objectives.
src/compose.ts (3)
3-3: Correct addition of TransformDecodeError to the error handling surface.The import (line 3) and injection into the generated error handler (lines 2523-2524, 2722-2723) properly expose ValidationError and TransformDecodeError for runtime instanceof checks, excluding them from custom toResponse() handling as intended.
Also applies to: 2523-2524, 2722-2723
2534-2535: Correctly made error handler async.The comment clearly explains the rationale, and this change is necessary to support async toResponse() methods demonstrated in the test suite (lines 436-517 in test/core/handle-error.test.ts).
2619-2619: Excellent error handling flow that preserves mapResponse hooks for toResponse() results.The conditional logic at line 2619 ensures toResponse() results bypass onError hooks while still flowing through mapResponse hooks (lines 2686-2704). This addresses the past review concern about keeping mapResponse in the fast path, ensuring global response transformation plugins (security headers, telemetry, etc.) still run for custom error responses.
The validation error handling (lines 2665-2676) correctly executes unconditionally since toResponse() explicitly excludes ValidationError and TransformDecodeError.
Also applies to: 2665-2676, 2686-2704
test/core/handle-error.test.ts (1)
341-551: Excellent comprehensive test coverage for toResponse() functionality.The test suite thoroughly validates:
- Core scenarios (lines 341-407): Error and non-Error objects with toResponse(), both returned and thrown
- Header propagation (lines 409-434): Custom headers including Content-Type are preserved from toResponse() Response objects
- Async support (lines 436-517): Promise-returning toResponse() methods work correctly with simulated delays, for both Error and non-Error objects
- Error handling (lines 519-551): When toResponse() itself throws (sync or async), the handler correctly falls back to standard error handling with the original error message and 500 status
The tests validate that the implementation properly:
- Respects custom status codes (418, 419) from toResponse()
- Propagates response headers through the pipeline
- Gracefully degrades when toResponse() fails
Great work on the edge case coverage!
2 participants
This resolves both #1539 and #1540
Problem
We had two related bugs with how Elysia handles the
toResponse()method on error objects:Bug 1 (#1540): When a custom error class extends
Errorand implementstoResponse(), returning it from a handler would ignore thetoResponse()method entirely and create a default error response with status 500.Bug 2 (#1539): When throwing an error (Error or non-Error) with
toResponse(), the method would be called but the status code from the returned Response would be replaced with 500.Example that was broken:
According to the documentation, this should work, but the implementation had some gaps.
Root Cause
For returned errors: The
errorToResponse()function in both adapters was creating default Error responses without checking if the error has atoResponse()method first.For thrown errors: The error handler in
compose.tswas callingtoResponse(), but the status had already been set to 500 earlier in the catch block. When the Response was passed tomapResponse(), it would use the pre-set status instead of extracting it from the Response object.Solution
1. Updated errorToResponse() in both adapters
Added a check for
toResponse()before falling back to default Error response generation. IftoResponse()exists, we call it and pass the result throughmapResponse()to handle it consistently with other response types.Files changed:
src/adapter/web-standard/handler.tssrc/adapter/bun/handler.ts2. Updated error handler in compose.ts
Added an early check for
toResponse()that runs before onError hooks. This check:toResponse()mapResponse()File changed:
src/compose.tsDesign Decisions
toResponse() takes precedence over onError hooks
We decided that if an error implements
toResponse(), it should be used even if there are onError hooks registered. This makes sense because the error class is explicitly defining how it should be represented in the response. If you want onError to override this, you can check for the error type and return your own Response from the hook.Validation errors are exempt
ValidationError and TransformDecodeError are excluded from the toResponse() check to maintain existing behavior for validation errors, which have their own specialized handling.
Headers and status are both preserved
The Response returned by
toResponse()is passed throughmapResponse(), which means both custom headers and status codes are preserved. This gives full control to the error class over its HTTP response.Testing
Added comprehensive test coverage:
Verified both original bug scenarios now work correctly with the exact code from the bug reports.
Breaking Changes
None. This fixes behavior to match the documented API, so existing code that wasn't relying on the buggy behavior will continue to work as expected.
Summary by CodeRabbit
New Features
Bug Fixes
Tests