fix(schema): correct hasElysiaMeta recursive logic to prevent short-c…#1787
fix(schema): correct hasElysiaMeta recursive logic to prevent short-c…#1787NongTham wants to merge 2 commits intoelysiajs:mainfrom
Conversation
WalkthroughReworked traversal logic in schema utilities: hasAdditionalProperties and hasElysiaMeta now iterate all object properties before returning, removing per-property early returns. Standardized some catch blocks and error-field references; added tests for hasElysiaMeta covering order and deep nesting. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/schema.ts (2)
231-242:⚠️ Potential issue | 🟠 MajorMirror this traversal fix in
hasAdditionalProperties().
hasElysiaMeta()is fixed here, but Line 128 inhasAdditionalProperties()still returns from inside the first property iteration. A schema whose first property is plain and whose second property carries nestedadditionalPropertieswill still be misclassified, so the same short-circuit bug remains reachable throughvalidator.hasAdditionalProperties.🛠 Proposed fix
for (const key of Object.keys(properties)) { const property = properties[key] if (property.type === 'object') { if (hasAdditionalProperties(property)) return true } else if (property.anyOf) { for (let i = 0; i < property.anyOf.length; i++) if (hasAdditionalProperties(property.anyOf[i])) return true } - - return property.additionalProperties + if (property.additionalProperties) return true } return false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schema.ts` around lines 231 - 242, The traversal bug fixed in hasElysiaMeta must be mirrored in hasAdditionalProperties: do not return from inside the first property iteration — instead, when schema.type === 'object' and schema.properties exists, iterate every property (using the same pattern as hasElysiaMeta), check each property's nested schema for additionalProperties (and recurse into hasAdditionalProperties as needed), and only return true if any property ultimately indicates additionalProperties; otherwise fall through to return schema.additionalProperties === true (or existing final check). Update hasAdditionalProperties to use the same non-short-circuit loop and recursive checks as hasElysiaMeta to correctly detect nested additionalProperties across all properties.
778-877:⚠️ Potential issue | 🔴 Critical
safeParse()in the else branch throwsReferenceErrorinstead of returning error response.The
safeParse()method in the else branch (lines ~879-950) referencescompiled.Errors(v)in its catch block, butcompiledis aletvariable declared after the entire if-else block. When the catch block executes beforecompiledis initialized, it throwsReferenceError: Cannot access 'compiled' before initializationinstead of returning{ success: false, ... }.🛠 Fix
validator.safeParse = (v) => { try { return { success: true, data: validator.Decode(validator.Clean?.(v) ?? v), error: null } } catch (error) { - const errors = [...compiled.Errors(v)].map(mapValueError) + const errors = [...validator.Errors(v)].map(mapValueError) return { success: false, data: null, error: errors[0]?.summary, errors } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schema.ts` around lines 778 - 877, The safeParse() catch block references compiled.Errors(v) which can trigger a ReferenceError because the compiled variable is declared later; update safeParse to use the already-defined validator.Errors(v) (or another initialized symbol like Errors or validator) instead of compiled, or move the compiled declaration so it is initialized before safeParse is defined; specifically change safeParse's catch to compute errors from validator.Errors(v) and map via mapValueError to return the error shape without relying on compiled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/schema.ts`:
- Around line 231-242: The traversal bug fixed in hasElysiaMeta must be mirrored
in hasAdditionalProperties: do not return from inside the first property
iteration — instead, when schema.type === 'object' and schema.properties exists,
iterate every property (using the same pattern as hasElysiaMeta), check each
property's nested schema for additionalProperties (and recurse into
hasAdditionalProperties as needed), and only return true if any property
ultimately indicates additionalProperties; otherwise fall through to return
schema.additionalProperties === true (or existing final check). Update
hasAdditionalProperties to use the same non-short-circuit loop and recursive
checks as hasElysiaMeta to correctly detect nested additionalProperties across
all properties.
- Around line 778-877: The safeParse() catch block references compiled.Errors(v)
which can trigger a ReferenceError because the compiled variable is declared
later; update safeParse to use the already-defined validator.Errors(v) (or
another initialized symbol like Errors or validator) instead of compiled, or
move the compiled declaration so it is initialized before safeParse is defined;
specifically change safeParse's catch to compute errors from validator.Errors(v)
and map via mapValueError to return the error shape without relying on compiled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 881e15e9-e7bd-4aa3-b9da-e26be9deb3f9
📒 Files selected for processing (2)
src/schema.tstest/schema/schema-utils.test.ts
…eError in src/schema.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/schema.ts (1)
226-233: Loop fix is correct, but the final return is redundant.The traversal fix at line 229 properly ensures all properties are checked recursively. However, line 232
return schema.elysiaMeta === metais redundant sinceschema.elysiaMeta === metawas already checked at line 200 - if it were true, the function would have returned there. At line 232, this will always evaluate tofalse.This isn't a bug, but could be simplified for clarity:
♻️ Optional: Simplify the return statement
for (const key of Object.keys(properties)) { const property = properties[key] if (hasElysiaMeta(meta, property)) return true } - return schema.elysiaMeta === meta + return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schema.ts` around lines 226 - 233, The final return in hasElysiaMeta is redundant because schema.elysiaMeta was already checked earlier; remove the trailing "return schema.elysiaMeta === meta" (or replace it with "return false") so the function returns true only when found during the earlier checks/recursive loop and otherwise returns false at the end; update the hasElysiaMeta function to end with an explicit false return if needed and ensure no other logic depends on that redundant comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/schema.ts`:
- Around line 226-233: The final return in hasElysiaMeta is redundant because
schema.elysiaMeta was already checked earlier; remove the trailing "return
schema.elysiaMeta === meta" (or replace it with "return false") so the function
returns true only when found during the earlier checks/recursive loop and
otherwise returns false at the end; update the hasElysiaMeta function to end
with an explicit false return if needed and ensure no other logic depends on
that redundant comparison.
1 participant
This PR fixes a bug in
hasElysiaMetaintroduced in #1649 where the recursive search loop was unintentionally short-circuiting on the first property checked.Changes:
returnstatement outside theforloop inhasElysiaMetato allow deep traversal.schema-utils.test.tsto ensurehasElysiaMetacorrectly finds nested metadata in Object and Union schemas without breaking.(Note: The
t.Unionbehavior originally reported in #1783 dropping subsequent schema properties is determined to be expected TypeBox matching behavior under Elysia's root-only additionalProperties normalization. This PR solely addresses the loop defect preventing meta tagging.)Summary by CodeRabbit
Bug Fixes
Tests
Chores