fix(error): return 500 for response validation errors#1676
fix(error): return 500 for response validation errors#1676raunak-rpm wants to merge 2 commits intoelysiajs:mainfrom
Conversation
Response validation errors now correctly return HTTP 500 (Internal Server Error) instead of 422 (Unprocessable Entity). This change distinguishes between: - Request validation errors (422): Client sent invalid data (query, body, headers, params, cookie) - client's fault - Response validation errors (500): Server returned data that doesn't match its own schema - server's bug The fix modifies ValidationError class in src/error.ts to: 1. Use dynamic status based on validation type 2. Set status = 500 when type === 'response', otherwise 422 3. Update toResponse() to use this.status This is a breaking change for APIs that relied on 422 for response validation errors, but it's the semantically correct behavior. Closes elysiajs#1480
WalkthroughResponse validation errors now surface as HTTP 500 (Internal Server Error) instead of 422. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (5)
✏️ Tip: You can disable this entire section by setting 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/core/as.test.ts`:
- Around line 107-108: The inline comment in the test assertion incorrectly uses
"rgba(23, 73, 137, 0)" instead of the issue reference "#1480"; update the
comment text above the expect in the test function (the comment that starts
"Response validation errors return 500 (server error) - see ...") to use "#1480"
so it matches other comments.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/error.tstest/core/as.test.tstest/core/dynamic.test.tstest/core/handle-error.test.tstest/core/normalize.test.tstest/extends/error.test.tstest/extends/models.test.tstest/path/group.test.tstest/path/guard.test.tstest/standard-schema/reference.test.tstest/standard-schema/standalone.test.tstest/standard-schema/validate.test.tstest/validator/novalidate.test.tstest/validator/response.test.tstest/validator/standalone.test.tstest/validator/validator.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
test/path/guard.test.ts (2)
src/index.ts (1)
error(3088-3128)test/types/lifecycle/soundness.ts (15)
error(60-62)error(126-128)error(193-195)error(275-277)error(695-697)error(722-724)error(754-756)error(787-789)error(1444-1446)error(1462-1464)error(1512-1514)error(1531-1533)error(1583-1585)error(1602-1604)error(1652-1654)
test/extends/models.test.ts (1)
src/index.ts (1)
error(3088-3128)
test/validator/response.test.ts (2)
test/utils.ts (1)
req(1-2)src/universal/request.ts (1)
body(81-144)
🔇 Additional comments (55)
src/error.ts (3)
287-295: Well-documented status code differentiation.The JSDoc clearly explains the rationale behind the 500/422 split, which is helpful for maintainers and consumers of this API. This aligns with HTTP semantics: 422 for client-sent invalid data vs. 500 for server-side bugs.
492-494: Implementation correctly maps validation type to HTTP status.The conditional
type === 'response' ? 500 : 422accurately implements the PR objective. All other validation types (query, body, headers, params, cookie) will receive 422, while response validation failures receive 500.
559-567:toResponsecorrectly uses the dynamic status.The method now references
this.statusinstead of a hardcoded value, ensuring the response reflects the appropriate HTTP status based on validation type.test/standard-schema/reference.test.ts (2)
124-125: Test correctly updated for response validation status change.The expectation change from 422 to 500 aligns with the PR objective. The inline comment provides good traceability to issue
#1480.
155-156: Multiple response validation test correctly updated.Consistent with the single response test, this correctly expects 500 for response validation failures on discriminated response schemas.
test/path/group.test.ts (1)
141-142: Group response validation test correctly updated.The test appropriately expects 500 for response validation errors within grouped routes. Request validation tests in this file correctly remain at 422.
test/standard-schema/validate.test.ts (2)
103-104: Single response validation test correctly updated.The expectation change to 500 is consistent with the PR objective for response validation errors.
129-130: Multiple response validation test correctly updated.This test verifies that discriminated union response schemas that fail validation return 500, which is the expected behavior for server-side schema mismatches.
test/extends/error.test.ts (1)
91-92: Response validation error test correctly updated.This test verifies that when a route returns data that doesn't match its declared response schema (
t.Null()), it now correctly returns HTTP 500. The subsequent assertion at line 93 confirms the response body is still JSON-formatted, which is appropriate for error responses.test/core/handle-error.test.ts (1)
565-577: LGTM!The test correctly validates that response validation errors now return HTTP 500. The handler returns a string (
'invalid response') while the schema expectst.Number(), which is a server-side bug scenario. The comment clearly references issue#1480for traceability.test/standard-schema/standalone.test.ts (3)
160-163: LGTM!The test correctly expects HTTP 500 for response validation errors when the server returns data not matching the declared response schema (e.g.,
id: undefinedwhenz.number()is expected).
211-214: LGTM!Correctly updated for the multiple response validation scenario. The
/unknownpath triggers a response validation failure, appropriately returning 500.
358-361: LGTM!The plugin merge test correctly expects 500 for response validation errors while maintaining consistency with the broader PR changes.
test/core/normalize.test.ts (3)
72-74: LGTM!The test correctly expects HTTP 500 when response validation fails due to strict mode (
normalize: false). The server returns{ hello: 'world', a: 'b' }but the schema only allows{ hello: t.String() }without additional properties.
121-123: LGTM!Correctly updated for the multiple response strict validation scenario.
176-178: LGTM!Consistent with the other normalize tests - response validation errors now return 500.
test/extends/models.test.ts (2)
317-323: LGTM!The test correctly validates that returning
1(number) when the response schema expects'res'(t.String()) results in HTTP 500. The reference model validation now properly identifies this as a server error.
350-359: LGTM!Correctly updated for the per-status response validation scenario. The handler returns
status(400, 1)but the schema for status 400 expects a string.test/path/guard.test.ts (8)
154-156: LGTM!The guard response validation test correctly expects HTTP 500 when the handler returns
1but the guard schema expectst.String().
172-174: LGTM!Correctly updated for the global guard application scenario.
243-245: LGTM!The global guard test correctly expects
[500, 500, 500]since all three routes (/inner,/plugin,/) return non-numeric values but the global guard expectst.Number().
281-283: LGTM!The mixed result
[500, 200, 500]is correct:/innerfails (returns string, expects number),/pluginpasses (returns boolean, local override expectst.Boolean()),/fails (returns string, expects number).
319-321: LGTM!The result
[500, 200, 200]correctly reflects the scoped override behavior where/pluginand/pass their respective response validations.
351-353: LGTM!The scoped guard test correctly expects
[500, 500, 200]- the scoped guard applies to/innerand/plugin(within plugin scope) but not to/(outside scope).
380-382: LGTM!The local guard test correctly expects
[500, 200, 200]- only/innerhas the response validation applied.
409-411: LGTM!The "only cast guard" test correctly expects
[500, 200]- the scoped guard's response validation applies to/inner(returns string, expects number) but/returns1which matchest.Number().test/core/dynamic.test.ts (4)
532-536: LGTM!The test expectations are correctly updated to expect 500 for response validation errors (
/invalidand/invalid-201routes) while maintaining 200/201 for valid responses. The comment clearly references the issue for future context.
556-560: LGTM!Correct status expectation for response validation error in the clean response scenario.
613-617: LGTM!Correctly expects 500 for response validation errors in afterHandle scenarios while valid responses return their expected status codes.
640-644: LGTM!Consistent with the other response validation error test updates.
test/validator/validator.test.ts (3)
26-28: LGTM!Correctly updated to expect 500 for response validation errors from beforeHandle.
47-49: LGTM!Correctly updated to expect 500 for response validation errors from afterHandle.
72-74: LGTM!Correctly updated for the combined beforeHandle with afterHandle scenario.
test/core/as.test.ts (4)
145-146: LGTM!Correctly expects 500 for routes with response validation errors and 200 for the route with valid response schema.
183-184: LGTM!Test expectations correctly reflect scoped override behavior with response validation.
215-216: LGTM!Correctly expects 500 for scoped response validation errors and 200 for the route outside the scope.
249-250: LGTM!Correctly expects 500 for all routes when scoped twice with response validation errors.
test/validator/standalone.test.ts (8)
32-34: LGTM!Correctly expects 500 for response validation error when the route returns a name that doesn't match the declared response schema.
69-71: LGTM!Correctly updated for merged guard with local schema scenario.
99-101: LGTM!Correctly updated for multiple guards without local schema.
139-141: LGTM!Correctly updated for multiple guards with local schema.
176-178: LGTM!Correctly updated for override guard scenario.
210-212: Good distinction maintained between request and response validation.The comment explicitly notes this is a body validation error (client-sent invalid data) which correctly returns 422, not 500. This helps clarify the semantic difference.
265-267: LGTM!Correctly expects 500 for response validation with merged object schemas.
315-317: Good distinction maintained.Body validation error correctly returns 422 as this is client-sent invalid data.
test/validator/response.test.ts (7)
194-196: LGTM!Correctly updated to expect 500 for strict validation failure when
normalize: false.
295-299: LGTM!Correctly expects 500 for response validation errors when the response doesn't match the status-specific schema (200 expects String, 201 expects Number).
427-429: LGTM!Correctly expects 500 for the
/validate-errorroute returning an invalid response.
570-602: Excellent test coverage for the new behavior.The new test suite clearly documents the expected behavior and verifies both the status code (500) and the error structure (
type: 'validation',on: 'response'). This is valuable for regression testing and API contract verification.
604-648: LGTM!Good coverage for edge cases: status-specific schemas, missing required fields, and wrong types all correctly return 500.
650-726: Comprehensive validation of request vs response distinction.These tests ensure that request validation errors (body, query, params, headers) continue to return 422 as expected. This is critical for maintaining backward compatibility and correct semantic status codes.
728-759: Excellent test for demonstrating the distinction in a single route.This test clearly shows that the same endpoint can produce either 422 (invalid request) or 500 (invalid response) depending on the validation failure type. This is the most effective way to document the intended behavior.
test/validator/novalidate.test.ts (3)
190-209: LGTM!The test correctly validates that response validation still occurs for status codes without
NoValidate. The updated expectation of 500 (instead of 422) properly reflects the PR's fix: response validation errors are server-side bugs and should return HTTP 500.
241-254: LGTM!This baseline test confirms that without
NoValidate, response validation is enforced. The updated 500 status expectation correctly reflects the fix for issue#1480.
256-270: LGTM!The test verifies that strict object schema validation catches missing required properties in responses. The 500 status expectation aligns with the PR's fix for response validation errors.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
2 participants
Summary
Fixes #1480
Response validation errors now correctly return HTTP 500 (Internal Server Error) instead of HTTP 422 (Unprocessable Entity).
Problem
Previously, all validation errors returned HTTP 422, regardless of whether they were:
When the server returns data that doesn't match its own declared schema, that's a server bug, not a client error. Returning 422 incorrectly blames the client for a server-side issue.
Solution
Modified
ValidationErrorclass insrc/error.tsto use dynamic status codes based on validation type:Semantic Distinction
responsequery,body,params,headers,cookieChanges
src/error.ts: UpdatedValidationErrorclassstatusfrom static422to dynamic based ontypetoResponse()to usethis.statusTests: Added 11 new tests + updated 44 existing tests
Breaking Change
A response that doesn't match the server's own schema is clearly an "unexpected condition" (500), not a problem with the client's request (422).
Test Results
Example
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.