fix: handle standard schema validators in ValidationError.all #1483
Conversation
Fixes elysiajs#1482 Previously, accessing `error.all` with Zod or other standard schema validators would throw a TypeError. This occurred because the `all` getter only handled TypeBox validators. This PR handles for other standard schema validators.
WalkthroughUpdates ValidationError.all to handle standard schema validators by mapping their issues to MapValueError[], while preserving legacy paths for TypeBox/Value validators. Adds tests demonstrating Zod validator compatibility and structured error extraction via onError. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Server
participant Validator as Schema Validator
participant Handler as Route Handler
participant ErrorAll as ValidationError.all
Client->>Server: HTTP request
Server->>Validator: Validate input (standard or legacy)
alt Validation fails
Validator-->>Server: throw ValidationError
Server->>ErrorAll: Access error.all
rect rgba(200, 230, 255, 0.3)
note right of ErrorAll: If standard schema<br/>map issues -> MapValueError[]
end
alt Non-standard/TypeBox
note right of ErrorAll: Use validator.Errors / Value.Errors
end
Server-->>Client: 422 with errors array
else Validation succeeds
Server->>Handler: Execute route logic
Handler-->>Client: 2xx response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/error.ts (1)
434-465: Verify path format consistency across validator types.The path format differs between TypeBox and standard validators:
- TypeBox validators (lines 461-464): Return errors with
pathin format"/field"(leading slash, from ValueError)- Standard validators (line 454): Return errors with
pathin format"field"(no leading slash, joined from array)This inconsistency could break existing code. For example, the existing test at lines 58-86 does
i.path.slice(1), which expects a leading character. This would fail for standard validators where the path is already"field"rather than"/field".Consider standardizing the format by prepending
"/"to the standard validator paths to match TypeBox format, or documenting this breaking change.Apply this diff to standardize the path format:
// Map standard schema issues to the expected format return issues?.map((issue: any) => ({ summary: issue.message, - path: issue.path?.join('.') || 'root', + path: issue.path?.length ? '/' + issue.path.join('/') : '', message: issue.message, value: this.value })) || []Note: This change makes the path format
"/field1/field2"to match TypeBox's ValueError.path format before it's processed by mapValueError consumers.
🧹 Nitpick comments (1)
src/error.ts (1)
452-452: Improve type safety.The
issue: anytype lacks type safety. Consider using proper typing for standard schema issues to catch potential errors at compile time.If Zod types are available, define an interface for standard validator issues:
interface StandardSchemaIssue { message: string path?: (string | number)[] // Add other common properties }Then update line 452:
- return issues?.map((issue: any) => ({ + return issues?.map((issue: StandardSchemaIssue) => ({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/error.ts(1 hunks)test/lifecycle/error.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/lifecycle/error.test.ts (1)
src/error.ts (1)
ValidationError(257-542)
🔇 Additional comments (2)
test/lifecycle/error.test.ts (2)
12-12: LGTM! Clean import.The Zod import is appropriately added to support the new test cases.
455-530: Tests verify Zod validator compatibility.The two new test cases successfully demonstrate that:
ValidationError.allno longer throws TypeError with Zod validators (resolving issue #1482)- Error objects include expected properties (
path,message,summary)- Proper HTTP 422 status is returned for validation failures
However, these tests don't verify path format consistency with TypeBox validators. Consider adding a test that compares path formats between TypeBox and Zod errors to catch the inconsistency flagged in src/error.ts.
Consider adding a test to verify path format consistency:
it('ValidationError.all returns consistent path format across validators', async () => { // Test with TypeBox const typeboxApp = new Elysia() .onError(({ error }) => { if (error instanceof ValidationError) { return { errors: error.all } } }) .post('/typebox', ({ body }) => body, { body: t.Object({ name: t.String() }) }) // Test with Zod const zodApp = new Elysia() .onError(({ error }) => { if (error instanceof ValidationError) { return { errors: error.all } } }) .post('/zod', ({ body }) => body, { body: z.object({ name: z.string() }) }) const [typeboxRes, zodRes] = await Promise.all([ typeboxApp.handle(post('/typebox', {})), zodApp.handle(post('/zod', {})) ]) const [typeboxData, zodData] = await Promise.all([ typeboxRes.json(), zodRes.json() ]) // Verify both use same path format (either "/field" or "field") const typeboxPath = (typeboxData as any).errors[0].path const zodPath = (zodData as any).errors[0].path expect(typeboxPath.startsWith('/')).toBe(zodPath.startsWith('/')) })
| : // @ts-ignore | ||
| this.validator.schema)['~standard'] | ||
|
|
||
| const issues = standard.validate(this.value).issues |
There was a problem hiding this comment.
Re-validation on every access degrades performance.
Line 449 calls standard.validate(this.value) every time the all getter is accessed. This re-validates the entire value on each call, which is expensive. The validation already occurred in the constructor (line 292), and the issues could be cached.
Consider caching the validation issues in the constructor and reusing them in the getter to avoid redundant validation:
valueError?: ValueError
expected?: unknown
customError?: string
+ private cachedIssues?: any[]Then in the constructor's standard validator branch (after line 292):
const _errors = errors ?? standard.validate(value).issues
+ this.cachedIssues = _errorsAnd in the getter:
- const issues = standard.validate(this.value).issues
+ const issues = this.cachedIssues ?? standard.validate(this.value).issues📝 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.
| const issues = standard.validate(this.value).issues | |
| const issues = this.cachedIssues ?? standard.validate(this.value).issues |
🤖 Prompt for AI Agents
In src/error.ts around line 449 (and the constructor around line 292), the
getter currently calls standard.validate(this.value) on every access which
re-runs expensive validation; instead compute and cache the validation result
once in the constructor (when the standard validator branch runs) into a private
field (e.g. this._validationIssues) and have the all getter return that cached
value; if the value can ever be mutated, add logic to invalidate/update the
cache on mutation, otherwise simply reuse the cached issues to avoid repeated
validation.
| return issues?.map((issue: any) => ({ | ||
| summary: issue.message, | ||
| path: issue.path?.join('.') || 'root', | ||
| message: issue.message, | ||
| value: this.value |
There was a problem hiding this comment.
Value assignment is misleading.
Line 456 assigns this.value (the entire input object) to each error's value property. However, TypeBox's ValueError includes the field-specific value, not the entire input. This inconsistency could mislead consumers who expect the actual field value.
For standard validators, either:
- Extract the field-specific value by traversing
this.valueusingissue.path, or - Omit the
valueproperty if field-specific extraction is not feasible
Consider extracting field-specific values:
// Map standard schema issues to the expected format
return issues?.map((issue: any) => ({
summary: issue.message,
path: issue.path?.join('.') || 'root',
message: issue.message,
- value: this.value
+ value: issue.path?.reduce((obj: any, key: string) => obj?.[key], this.value) ?? this.value
})) || []This traverses the path to extract the specific field's value.
📝 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 issues?.map((issue: any) => ({ | |
| summary: issue.message, | |
| path: issue.path?.join('.') || 'root', | |
| message: issue.message, | |
| value: this.value | |
| // Map standard schema issues to the expected format | |
| return issues?.map((issue: any) => ({ | |
| summary: issue.message, | |
| path: issue.path?.join('.') || 'root', | |
| message: issue.message, | |
| value: issue.path?.reduce((obj: any, key: string) => obj?.[key], this.value) ?? this.value | |
| })) || [] |
🤖 Prompt for AI Agents
In src/error.ts around lines 452 to 456, the current code sets each error's
value to this.value (the entire input) which is misleading; change it to derive
the field-specific value by traversing this.value using issue.path (e.g., if
issue.path is empty use this.value/root, otherwise walk the path segments safely
and return undefined if any segment missing) and assign that extracted value to
the error.value property; if implementing traversal is not feasible, remove the
value property instead.
1 participant
Fixes #1482
Previously, accessing
error.allwith Zod or other standard schema validators would throw a TypeError. This occurred because theallgetter only handled TypeBox validators. This PR handles for other standard schema validators.Summary by CodeRabbit
New Features
Tests