feat(validator): support unknownKeys strip mode in object validation#66
Open
robelest wants to merge 1 commit intoget-convex:mainfrom
Open
feat(validator): support unknownKeys strip mode in object validation#66robelest wants to merge 1 commit intoget-convex:mainfrom
robelest wants to merge 1 commit intoget-convex:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds support for stripping unknown object keys during validation via a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Validator
participant Database
Client->>Server: call function with object arg (may contain unknown keys)
Server->>Validator: validate args (object validator, unknownKeys: "strip" or union)
alt A non-strip union member validates
Validator-->>Server: success (no deletions applied)
else Only strip-object member validates
Validator->>Validator: collect unknown keys, defer deletions
Validator-->>Server: success (with candidate value)
Server->>Validator: apply deferred deletions (strip unknown keys)
end
Server->>Database: perform operation / use parsed args
Database-->>Server: result
Server-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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 `@index.ts`:
- Around line 1073-1082: The loop that deletes unexpected keys (for (const k of
Object.keys(value)) { ... delete value[k]; }) mutates the input object in-place
when validator.unknownKeys === "strip", which can corrupt the shared value when
this object validator is tried as one branch of a "union" validator; change the
code to avoid in-place mutation by operating on a shallow copy of value (e.g.,
const stripped = { ...value } and delete keys from stripped) or defer performing
deletions until after all field validations succeed so the original value passed
into the "union" branch remains unmodified; locate the object-validation branch
that reads validator.unknownKeys and the union handling logic to implement the
shallow-copy or deferred-deletion approach.
robelest
force-pushed
the
feat/unknownKeys-strip-support
branch
2 times, most recently
from
ea58c43 to
569b647
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
convex/argumentsValidation.test.ts (1)
10-59: LGTM — test helpers are well-structured with proper cleanup.
patchExportArgscorrectly usesbindto capture the original and thetry/finallypattern in every caller ensures restoration. One minor note: therestore()callback assigns an own property (the bound original) rather than deleting the own property to re-expose the prototype chain. This is benign for these tests but not a perfect structural restore — considerdelete functionExport.exportArgsas the restore action if the original was a prototype method:🔧 Optional: cleaner restore that preserves prototype chain
function patchExportArgs( functionExport: { exportArgs: () => string }, patch: (exported: any) => void, ): () => void { + const hadOwnProp = Object.prototype.hasOwnProperty.call(functionExport, "exportArgs"); const originalExportArgs = functionExport.exportArgs.bind(functionExport); functionExport.exportArgs = () => { const exported = JSON.parse(originalExportArgs()); patch(exported); return JSON.stringify(exported); }; return () => { - functionExport.exportArgs = originalExportArgs; + if (hadOwnProp) { + functionExport.exportArgs = originalExportArgs; + } else { + delete (functionExport as any).exportArgs; + } }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/argumentsValidation.test.ts` around lines 10 - 59, The restore callback returned by patchExportArgs currently reassigns the bound originalExportArgs as an own property which hides any prototype method; change the restore logic in patchExportArgs to delete functionExport.exportArgs instead of assigning originalExportArgs so the original prototype chain is preserved (ensure this change is applied where patchExportArgs is used by forceStripUnknownKeys and forceStripUnknownKeysOnUnionObjectField).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/argumentsValidation.ts`:
- Around line 69-88: The calls to v.object(..., { unknownKeys: "strip" }) are
using an unsupported second parameter; update queryWithStripObjectArg,
mutationWithStripObjectArg, and actionWithStripObjectArg to remove the {
unknownKeys: "strip" } argument (i.e., call v.object({ a: v.number() }) ), or if
you need lenient validation replace the schema with v.any() or
v.record(v.string(), v.any()) as appropriate so the handler receives the
expected shape without type errors.
In `@index.ts`:
- Around line 1031-1065: The current tryValidate in the "union" case allows
nested strip-object validations to mutate the original value prematurely; change
tryValidate to operate on a deep clone when the union member is (or contains) an
object with unknownKeys === "strip": detect if member.type === "object" &&
member.unknownKeys === "strip" or if member's schema contains nested
strip-objects, then call validateValidator on structuredClone(value) instead of
value; if that validation succeeds, copy the validated/stripped result back into
the original (e.g., replace the object's properties or assign the clone),
otherwise discard the clone so failed branch attempts do not mutate the
original. Ensure you update the tryValidate function and keep using
validateValidator, validator.value, and the unknownKeys === "strip" checks as
the identifying points.
---
Nitpick comments:
In `@convex/argumentsValidation.test.ts`:
- Around line 10-59: The restore callback returned by patchExportArgs currently
reassigns the bound originalExportArgs as an own property which hides any
prototype method; change the restore logic in patchExportArgs to delete
functionExport.exportArgs instead of assigning originalExportArgs so the
original prototype chain is preserved (ensure this change is applied where
patchExportArgs is used by forceStripUnknownKeys and
forceStripUnknownKeysOnUnionObjectField).
robelest
force-pushed
the
feat/unknownKeys-strip-support
branch
from
569b647 to
ed552c4
Compare
Author
|
Addressed the latest CodeRabbit feedback in commit ed552c4:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
index.ts (1)
968-984:target.push(...source)can overflow the call stack for large arrays.Spreading a large array into
pushpasses every element as a separate function argument, which exhausts the stack limit for arrays beyond ~65 k elements (engine-dependent). Replace with a loop or in-place fill:♻️ Safer array replacement
if (Array.isArray(target) && Array.isArray(source)) { target.length = 0; - target.push(...source); + for (const item of source) { + target.push(item); + } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 968 - 984, The array copy in copyValidatedValueIntoTarget uses target.push(...source) which can overflow the call stack for very large source arrays; replace the spread push with a safe in-place copy (e.g., clear target and either push in chunks or use a for loop to push each element, or assign by index) so large arrays are handled without creating a huge argument list; update the block handling Array.isArray(target) && Array.isArray(source) to iterate or chunk-push instead of using the spread operator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@index.ts`:
- Around line 968-984: The array copy in copyValidatedValueIntoTarget uses
target.push(...source) which can overflow the call stack for very large source
arrays; replace the spread push with a safe in-place copy (e.g., clear target
and either push in chunks or use a for loop to push each element, or assign by
index) so large arrays are handled without creating a huge argument list; update
the block handling Array.isArray(target) && Array.isArray(source) to iterate or
chunk-push instead of using the spread operator.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
validateValidatorto supportunknownKeys: "strip"on object validators.Changes
Production (
index.ts)unknownKeys?: "strict" | "strip"toValidatorJSONobject validators.Tests
convex/argumentsValidation.tsconvex/argumentsValidation.test.tsconvex/schemaValidation.test.tsContext
This supports the
v.object({ ... }, { unknownKeys: "strip" })feature from get-convex/convex-backend#348, aligned with corresponding helpers changes.Note: tests monkey-patch serialized validator JSON to include
unknownKeys: "strip"because the currently publishedconvexpackage does not emit this property yet.Summary by CodeRabbit
New Features
Tests