refactor: remove recursion from schema utilities to prevent call stack overflows #4044
Conversation
This avoids potential crashes of the interpretor due to massive amounts of recursion. Signed-off-by: Joshua Rogers <MegaManSec@users.noreply.github.com>
This avoids potential crashes of the interpretor due to massive amounts of recursion. Signed-off-by: Joshua Rogers <MegaManSec@users.noreply.github.com>
This avoids potential crashes of the interpretor due to massive amounts of recursion. Signed-off-by: Joshua Rogers <MegaManSec@users.noreply.github.com>
WalkthroughThis pull request refactors error processing and type-handling functions across both the Deno and source libraries. In the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant ZodError as ZodError
participant Stack as ErrorStack
Caller->>ZodError: processError(rootError)
ZodError->>Stack: Push rootError
loop While Stack not empty
Stack->>ZodError: Pop current error
ZodError->>ZodError: Iterate over error issues
alt Issue is union/return/arguments
ZodError->>Stack: Push related errors
else
ZodError->>ZodError: Update fieldErrors
end
end
ZodError->>Caller: Return processed error result
sequenceDiagram
participant Caller as Caller
participant Types as TypeFunctions
participant Stack as TypeStack
Caller->>Types: Call getDiscriminator(type) / mergeValues(a, b)
Types->>Stack: Initialize stack with input(s)
loop While Stack not empty
Stack->>Types: Pop current element
alt Element needs further processing
Types->>Stack: Push inner element(s)
else
Types->>Types: Process and accumulate result
end
end
Types->>Caller: Return final result
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for guileless-rolypoly-866f8a ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/types.ts (1)
3415-3505:⚠️ Potential issueRisk of infinite looping on cyclical references.
While the iterative approach in
mergeValuesprevents stack overflows from recursion, there is no mechanism for detecting or breaking out of cyclical references. If either input contains objects with circular links, the loop could become infinite when repeatedly merging shared keys, arrays, or nested structures.To mitigate this, track visited pairs of objects or otherwise detect cycles. This will ensure reliable merging without the risk of hanging the process.
🧹 Nitpick comments (3)
deno/lib/ZodError.ts (1)
248-271: Consider removing obsolete commented code
Lines 257-263 are commented out. If no longer needed, removing them would simplify the code and improve readability. Otherwise, clarify under which conditions you might need to reintroduce that logic.- // if (typeof el === "string") { - // curr[el] = curr[el] || { _errors: [] }; - // } else if (typeof el === "number") { - // const errorArray: any = []; - // errorArray._errors = []; - // curr[el] = curr[el] || errorArray; - // }src/ZodError.ts (1)
248-271: Remove or clarify commented mapping code
The commented code in lines 257-263 looks like a remnant from the previous implementation. For maintainability, remove this block if it’s not needed, or document why it must remain.- // if (typeof el === "string") { - // curr[el] = curr[el] || { _errors: [] }; - // } else if (typeof el === "number") { - // const errorArray: any = []; - // errorArray._errors = []; - // curr[el] = curr[el] || errorArray; - // }deno/lib/types.ts (1)
3419-3501: Validate partial merges for objects with unequal key sets.The iterative strategy merges two structures only when their types align (objects with objects, arrays with arrays), handling shared keys recursively. This works well for symmetrical objects. However, spread merging objects (
const newObj = { ...a, ...b }) silently includes extra properties from the second object that aren't in the first. This is acceptable unless you require strict structural equality. If that’s the case, you might want to explicitly flag differences in object keys.Additionally, consider adding a mechanism to short-circuit early if either structure is extremely large or deeply nested, as the stack-based merging can become costly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
deno/lib/ZodError.ts(1 hunks)deno/lib/types.ts(2 hunks)src/ZodError.ts(1 hunks)src/types.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
src/ZodError.ts (1)
deno/lib/ZodError.ts (1) (1)
ZodError(200-332)
deno/lib/ZodError.ts (1)
src/ZodError.ts (1) (1)
ZodError(200-332)
src/types.ts (3)
deno/lib/types.ts (5) (5)
ZodTypeAny(50-50)ZodLazy(4266-4291)ZodEffects(4656-4821)ZodEffects(4823-4823)ZodCatch(4981-5055)deno/lib/helpers/util.ts (3) (3)
getParsedType(166-220)ZodParsedType(141-162)ZodParsedType(164-164)src/helpers/util.ts (3) (3)
getParsedType(166-220)ZodParsedType(141-162)ZodParsedType(164-164)
🔇 Additional comments (8)
deno/lib/ZodError.ts (3)
230-233: Rename parameter to improve clarity
The new parameter namerootErrorclarifies the purpose more effectively compared to a genericerror. Initializing thestackwith[rootError]is a good setup for the following iterative approach.
234-245: Check for potential infinite loops in error stacks
The newwhile (stack.length > 0)iteration is clear and should avoid deep recursion. However, if there's a cyclical reference inunionErrors,returnTypeError, orargumentsError, you could risk an infinite loop. Consider adding a mechanism (e.g. a visited set) to guard against cycles if it’s possible in practice.Could you confirm whether cyclical references across union errors can occur in real scenarios?
246-247: Appending errors with no path
Storing top-level issues infieldErrors._errorsensures clarity and consistency. This straightforward approach aligns with existing logic.src/ZodError.ts (3)
230-233: Rename parameter to rootError
Switching fromerrortorootErrormakes the code more understandable. Initializing the stack with a single root error is a clean entry point to the iterative approach.
234-245: Avoid infinite loops with nested or cyclical errors
Thewhile (stack.length > 0)logic properly replaces deep recursion. As a best practice, guard against potential cycles if the library or user code can produce self-referential error graphs.If cyclic error graphs are possible, consider a visited set or similar mechanism to avoid infinite loops.
246-247: Top-level errors appended to _errors
Placing top-level issues infieldErrors._errorsmirrors the preexisting approach and keeps the handling of pathless issues consistent.deno/lib/types.ts (1)
3221-3260: Ensure cyclic references are handled to avoid infinite loops.This iterative approach replaces recursion with a stack-based procedure for extracting potential discriminator values. While it correctly unwinds complex types (lazy schemas, optional/nullable wrappers, etc.), there's no check for cyclic or repeated references. If a schema refers to itself (directly or indirectly), the while loop can run indefinitely. Consider introducing a visited set or similar mechanism to prevent infinite loops when encountering cyclic references.
Do you want me to provide a helper function or script to identify potential self-referential schemas in your codebase?
src/types.ts (1)
3220-3260: Iterative refactor approved with a minor caveat regarding potential loops.This iterative approach in
getDiscriminatoris a clean replacement for recursion and effectively avoids stack overflows. However, be mindful of cyclical or deeply nested schemas that might repeatedly push the same item to the stack. Consider a safeguard (e.g., a visited set) to prevent infinite loops if future changes allow cycles.
This PR rewrites
getDiscriminator,processError, andmergeValuesto use a stack-based iterative approach to deeply nested errors/discriminators, in order to avoid infinite recursion (which could result in a stack overflow, resulting in a crash of the interpreter).Note:
deepPartialifyalso suffers from this issue, but I'm unsure how to approach fixing it.Summary by CodeRabbit
These updates enhance system robustness and performance, ensuring a smoother and more dependable experience for end-users while preserving backward compatibility.