🔧 fix: t.Record validation failure in response schemas #1634
Conversation
Skip normalization for schemas with patternProperties to prevent Clean from removing valid Record properties
WalkthroughAdded an exported Changes
Sequence Diagram(s)sequenceDiagram
rect `#F6FAFF`
participant Client
participant Handler as "Request/Response Handler"
participant SchemaUtil as "schema.ts: hasPatternProperties"
participant Validator as "getSchemaValidator"
participant Mirror as "exactMirror"
participant Cleaner as "cleaner"
end
Client->>Handler: send request / expect response
Handler->>SchemaUtil: inspect schema (hasPatternProperties?)
SchemaUtil-->>Handler: true / false
alt schema has patternProperties
Handler->>Validator: request validator (guarded)
Validator->>Cleaner: choose cleaner (avoid exactMirror)
Cleaner-->>Validator: cleaned/validated payload
else no patternProperties
Handler->>Validator: request validator
Validator->>Mirror: create exactMirror for normalization
Mirror-->>Validator: normalized/mirrored payload
end
Validator->>Handler: validation result (ok / errors)
Handler->>Client: return response or validation error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
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
🧹 Nitpick comments (2)
src/schema.ts (1)
802-802: Consider simplifying the condition.The condition checks both
schema.additionalProperties === falseand!hasPatternProperties(schema). However, Line 941 (compiled mode) only checks!hasPatternProperties(schema)without theadditionalPropertiescondition.For consistency and to ensure all schemas with
patternPropertiesskip the Clean function, consider:🔎 Suggested simplification
- if (normalize && schema.additionalProperties === false && !hasPatternProperties(schema)) { + if (normalize && !hasPatternProperties(schema)) {This ensures the Clean function is skipped for all schemas containing
patternProperties, regardless of theadditionalPropertiessetting.test/validator/response.test.ts (1)
567-567: Update placeholder issue number.The comment references
issues/XXXXwhich appears to be a placeholder. Based on the PR objectives, this should reference issue #1631.🔎 Suggested fix
- // https://github.com/elysiajs/elysia/issues/XXXX + // https://github.com/elysiajs/elysia/issues/1631
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/schema.tstest/validator/response.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
test/validator/response.test.ts (1)
test/utils.ts (1)
req(1-2)
🔇 Additional comments (5)
src/schema.ts (4)
720-721: LGTM: Correct guard for patternProperties.The check correctly prevents setting
additionalProperties = falsewhen the schema containspatternProperties, which would conflict with the Record type's dynamic keys.
727-728: LGTM: Consistent patternProperties guards.These checks correctly prevent forcing
additionalPropertieswhenpatternPropertiesexist in the schema, maintaining compatibility with Record types.Also applies to: 738-738
941-961: LGTM: Clean function gating in compiled mode.The condition correctly skips Clean function creation when
patternPropertiesexist, resolving the validation bug for Record schemas.
83-114: Fix correctly addresses exact-mirror's limitation with patternProperties.The implementation properly prevents
exact-mirror'sCleanfunction from incorrectly removing Record properties by skipping normalization whenpatternPropertiesare detected. ThehasPatternPropertiesfunction correctly identifies schemas using pattern-based dynamic keys through recursive traversal, including composition operators likeanyOf,allOf, and nested object/array types.test/validator/response.test.ts (1)
568-706: LGTM: Comprehensive regression test coverage.The six new tests thoroughly cover Record validation scenarios:
- Original bug with nested Record in arrays and objects
- Simple and complex Record structures
- Negative case (invalid data rejection)
- Nested Records
- Backward compatibility for regular Objects with
additionalProperties=falseThis provides excellent coverage for the bug fix and ensures no regressions.
- Fix schema.not handling (single schema, not array) - Simplify Clean condition for consistency - Replace 'as any' with @ts-expect-error in tests - Remove unnecessary comments
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/schema.ts (1)
130-130: Fix:schema.notis a single schema, not an array.Line 130 incorrectly treats
schema.notas an array by calling.some()on it, which will cause a runtime error. According to JSON Schema specification and TypeBox,notaccepts a single schema object, not an array.This is the same bug that was fixed in
hasPatternPropertiesat line 100. The correct pattern (used at line 329 inhasRefand line 377 inhasTransform) is to call the predicate function directly.🔎 Proposed fix
- if (schema.not) return schema.not.some(hasAdditionalProperties) + if (schema.not) return hasAdditionalProperties(schema.not)Based on past review comments and the fix applied to
hasPatternProperties.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/schema.tstest/validator/response.test.ts
🧰 Additional context used
🧬 Code graph analysis (2)
test/validator/response.test.ts (1)
test/utils.ts (1)
req(1-2)
src/schema.ts (1)
src/type-system/index.ts (1)
TypeCheck(685-685)
🔇 Additional comments (5)
test/validator/response.test.ts (2)
566-616: LGTM! Excellent regression test for the original bug.This test directly validates the fix for the issue where
t.Recordschemas nested in objects/arrays were causing 422 validation errors. The test covers both list items and nested objects with Record fields, which matches the reported bug scenario.
618-706: LGTM! Comprehensive test coverage for Record validation.The test suite thoroughly covers:
- Simple Record validation (618-630)
- Records with complex object values (632-652)
- Validation failure scenarios (654-666)
- Deeply nested Records (668-688)
- Backward compatibility ensuring regular Objects remain strictly validated (690-706)
The use of
@ts-expect-errorat lines 657 and 693 is appropriate for testing intentional validation failures and aligns with the PR's code quality improvements.src/schema.ts (3)
83-114: LGTM! Well-implemented recursive pattern detection.The
hasPatternPropertiesfunction correctly:
- Recursively traverses the schema tree (Import, unions, objects, arrays)
- Line 100 correctly handles
schema.notas a single schema (not an array), which addresses the bug mentioned in past review comments- Properly unwraps TypeCheck wrapper schemas
720-720: LGTM! Correct guards to prevent conflict betweenpatternPropertiesandadditionalProperties.The checks at lines 720, 727-728, and 738 correctly prevent setting
additionalProperties: falsewhenpatternPropertiesis present in the schema. This is essential because:
patternPropertiesallows dynamic keys matching a pattern- Setting
additionalProperties: falsewould conflict with this behavior- These guards are placed at all key locations where
additionalPropertiesis being setAlso applies to: 727-738
802-818: LGTM! Core fix for Record validation - correctly skipsCleanwhenpatternPropertiespresent.These guards (lines 802 and 941-961) implement the core solution described in the PR objectives:
- Detect schemas containing
patternPropertiesusinghasPatternProperties()- Skip creating/applying the
Cleanfunction fromexact-mirrorfor those schemas- This prevents the bug where Record properties (which use
patternProperties) were being stripped during response normalizationThe fix is applied in both:
- Dynamic validation mode (line 802)
- Compiled validation mode (lines 941-961)
This ensures backward compatibility: regular
t.Objectschemas still getCleannormalization, while schemas withpatternPropertiespreserve their dynamic keys.Also applies to: 941-961
This is marked outside of the scope and the change has not been introduced in this PR, so this will not be getting addressed |
|
just for information, i have routes that use t.Record without this issue so I'm not sure if this is really the issue. i also don't think that disabling object cleaning when there's a record anywhere in the object is a correct fix. it should clean the object, just not the record. |
|
Hmm, yeah, now that I think about it this does seem kinda too broad. Also, this might actually be a similar issue to what LASkuma was facing after all. Would you be able to check if this draft PR elysiajs/exact-mirror#26 fixes your issue? It seems to return the correct thing on my end @zoriya: > curl http://localhost:3003/
{"list":[{"toto":{"bar":1},"foo":{"link":"first"}}],"one":{"toto":{"bar":0},"foo":{"link":"second"}}}% Thanks for the feedback, nice catch! |
- Update createMirror calls to use TAnySchema instead of TSchema - Add regression test for Record with nested objects - Ensures compatibility with exact-mirror's stricter type signature
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/schema.ts (1)
96-100:someOfis not a valid JSON Schema keyword—this check is dead code.JSON Schema only defines
anyOf,oneOf,allOf, andnot. ThesomeOfcheck on line 98 will never execute sinceschema.someOfis alwaysundefined. While harmless, consider removing it for clarity.🔎 Proposed fix
if (schema.anyOf) return schema.anyOf.some(hasPatternProperties) if (schema.oneOf) return schema.oneOf.some(hasPatternProperties) - if (schema.someOf) return schema.someOf.some(hasPatternProperties) if (schema.allOf) return schema.allOf.some(hasPatternProperties) if (schema.not) return hasPatternProperties(schema.not)Note: The same issue exists in
hasAdditionalProperties(line 128) andhasElysiaMeta(line 236-238), but those are outside this PR's scope.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/schema.tstest/validator/response.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/validator/response.test.ts
🔇 Additional comments (5)
src/schema.ts (5)
83-95: Well-structured recursive helper.The
hasPatternPropertiesfunction correctly traverses the schema tree, handles TypeCheck wrappers, Import schemas, and composition keywords. The fix forschema.not(treating it as a single schema rather than an array) is correctly applied here.Also applies to: 101-114
720-721: Correct guards to preserve Record schema semantics.These checks prevent
additionalProperties: falsefrom being forced onto schemas containingpatternProperties, which is essential for TypeBoxRecordschemas to work correctly with dynamic keys.Also applies to: 727-729, 738-738
802-818: Correct guard for dynamic mode.Skipping
Cleanassignment whenhasPatternPropertiesreturns true prevents the exact-mirror/Value.Clean from stripping valid Record keys. The fallback tocreateCleaneron exactMirror failure is preserved.
941-961: Correct guard for compiled mode.Mirrors the dynamic mode fix, ensuring
Cleanis not created for schemas containingpatternProperties. The different normalize modes (true,'exactMirror','typebox') are all properly guarded.
543-547: Type cast for exact-mirror compatibility.The
TAnySchemacast aligns with the exact-mirror API expectations. Same pattern correctly applied at lines 805 and 944.
|
my bad, I accidentally click merge but I think it should not |
3 participants
Fix t.Record validation failure in response schemas
this resolves #1631
Problem
Response validation fails when using
t.Recordtypes, returning 422 validation errors even with valid data.Example:
Root Cause
The bug is in
exact-mirrorlibrary -optionalsInArraycleanup state was bleeding across sibling structures, causing Record properties to be incorrectly removed.Solution
Upstream Fix (exact-mirror)
Fixed in elysiajs/exact-mirror#26 by clearing
optionalsInArraystate after use, preventing pollution across sibling arrays/records.Compatibility Changes (this PR)
src/schema.ts:
createMirrorcalls to useTAnySchematype casts instead ofTSchema(lines 543, 805, 944)test/validator/response.test.ts: