feat(validators): respect unknownKeys strip mode in validate()#913
feat(validators): respect unknownKeys strip mode in validate()#913robelest wants to merge 2 commits intoget-convex:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds and tests "strip" mode for object validators (via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: |
|
What happens if I pass this object: {
"a": 1,
"b": 2,
"c": 3,
}to this validator: v.union(
v.object({
a: v.number(),
}, { unknownFields: "strip" }),
v.object({
b: v.number(),
c: v.optional(v.number()),
}, { unknownFields: "strip" }),
v.object({
a: v.union(v.string(), v.number()),
b: v.optional(v.number()),
c: v.any(),
}, { unknownFields: "strip" }),
)? As a user I might expect to get { a: 1, b: 2, c: 3 } since it was possible, even though the first two technically matched.
What have you thought about so far @robelest ? |
|
FYI you need to put the PR template CLA disclaimer in the PR description to pass that test |
|
@ianmacartney what do you think of something like this for the 3 prs |
|
High level seems reasonable - though not sure how much time /overhead will be spent here. |
robelest
force-pushed
the
feat/unknownKeys-strip-support
branch
from
281167b to
5d3dc13
Compare
![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 (3)
packages/convex-helpers/validators.test.ts (1)
56-60: Monkey-patchingunknownKeysis acceptable as a temporary workaround.The comment in the PR notes mentions this will be removed once the SDK natively serializes
unknownKeys. Consider adding a// TODO: remove once convex SDK exposes unknownKeyscomment to make the temporary nature explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/convex-helpers/validators.test.ts` around lines 56 - 60, Add an inline TODO comment clarifying that the monkey-patch is temporary: update the helper function withStripUnknownKeys (which sets validator.unknownKeys = "strip") to include a comment like "// TODO: remove once convex SDK exposes unknownKeys" so future maintainers know this workaround should be removed when the SDK natively supports unknownKeys.packages/convex-helpers/validators.ts (1)
856-862: Type predicate narrows toVObjectbut doesn't encode the strip constraint.The return type
validator is VObject<any, any, any>is accurate for the runtime check but doesn't distinguish strip-mode objects at the type level. This is fine for internal use today, but consider adding a brief doc comment noting theunknownKeysproperty isn't yet in the publicVObjecttype.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/convex-helpers/validators.ts` around lines 856 - 862, isStripObjectValidator currently narrows the runtime validator to VObject but also checks the internal unknownKeys === "strip" flag which isn't represented in the public VObject type; add a brief doc comment above the isStripObjectValidator function explaining that this predicate ensures both kind === "object" and unknownKeys === "strip" at runtime and that the unknownKeys property is an internal implementation detail not exposed on the public VObject type, so callers should not rely on the type system to reflect the strip-mode constraint.packages/convex-helpers/server/validators.test.ts (1)
345-348: DuplicatewithStripUnknownKeyshelper across test files.This helper is identical to the one in
validators.test.ts(line 57). Consider extracting it to a shared test utility (e.g., atest-utils.tsfile) to avoid duplication — especially since both will need to be removed when the SDK adds nativeunknownKeyssupport.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/convex-helpers/server/validators.test.ts` around lines 345 - 348, Duplicate helper withStripUnknownKeys (which sets (validator as any).unknownKeys = "strip" for a v.object validator) should be extracted into a shared test utility; create a test-utils.ts that exports function withStripUnknownKeys(validator: ReturnType<typeof v.object>) and move the implementation there, then replace the local definitions in validators.test.ts and the other test file with an import from that shared module and remove the duplicate definitions; ensure the exported signature matches usages and update imports in tests to reference the new withStripUnknownKeys helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/convex-helpers/validators.ts`:
- Around line 914-931: The current two-pass union logic uses
firstMatchingUnionMember(validator.members, ..., {allowUnknownFields: false,
includeStripObjects: false}) which causes parse() to throw for unions of plain
v.object members with extra fields; change the strict-pass call so it uses
allowUnknownFields: true (i.e., call firstMatchingUnionMember(...,
{allowUnknownFields: true, includeStripObjects: false})) so non-strip object
members still match when inputs contain extra fields, then keep the second
strip-pass (includeStripObjects: true) to handle stripable members and call
stripUnknownFields on the matched member as before.
---
Nitpick comments:
In `@packages/convex-helpers/server/validators.test.ts`:
- Around line 345-348: Duplicate helper withStripUnknownKeys (which sets
(validator as any).unknownKeys = "strip" for a v.object validator) should be
extracted into a shared test utility; create a test-utils.ts that exports
function withStripUnknownKeys(validator: ReturnType<typeof v.object>) and move
the implementation there, then replace the local definitions in
validators.test.ts and the other test file with an import from that shared
module and remove the duplicate definitions; ensure the exported signature
matches usages and update imports in tests to reference the new
withStripUnknownKeys helper.
In `@packages/convex-helpers/validators.test.ts`:
- Around line 56-60: Add an inline TODO comment clarifying that the monkey-patch
is temporary: update the helper function withStripUnknownKeys (which sets
validator.unknownKeys = "strip") to include a comment like "// TODO: remove once
convex SDK exposes unknownKeys" so future maintainers know this workaround
should be removed when the SDK natively supports unknownKeys.
In `@packages/convex-helpers/validators.ts`:
- Around line 856-862: isStripObjectValidator currently narrows the runtime
validator to VObject but also checks the internal unknownKeys === "strip" flag
which isn't represented in the public VObject type; add a brief doc comment
above the isStripObjectValidator function explaining that this predicate ensures
both kind === "object" and unknownKeys === "strip" at runtime and that the
unknownKeys property is an internal implementation detail not exposed on the
public VObject type, so callers should not rely on the type system to reflect
the strip-mode constraint.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/convex-helpers/validators.ts (3)
758-761: Add an inline comment explaining why strip objects bypass the extra-fields check unconditionally.This two-part condition encodes an important invariant: strip-mode objects ALWAYS accept unknown fields during
validate(), even whenallowUnknownFields: false. This is precisely whyfirstMatchingUnionMembermust explicitly skip strip objects in the first pass — relying onvalidate()to reject them would not work. Without a comment, a future reader may be tempted to simplify the condition or invert theunknownKeyscheck, breaking the strict-pass logic.📝 Suggested clarification comment
+ // Note: strip objects intentionally bypass the unknown-fields check + // regardless of allowUnknownFields. This invariant lets + // firstMatchingUnionMember explicitly skip them in the strict first + // pass rather than relying on validate() to reject them. if ( !opts?.allowUnknownFields && (validator as any).unknownKeys !== "strip" ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/convex-helpers/validators.ts` around lines 758 - 761, Add an inline comment next to the condition that checks opts?.allowUnknownFields and (validator as any).unknownKeys !== "strip" explaining that strip-mode validators intentionally accept unknown fields during validate() even when allowUnknownFields is false, and that this behavior is why firstMatchingUnionMember must skip strip objects in its first pass rather than relying on validate() to reject them; mention the involved symbols: unknownKeys, allowUnknownFields, validate(), and firstMatchingUnionMember so future readers won't invert or simplify the check.
920-941: Missing test: mixed-mode union (strip + non-strip members) where member order determines the stripping result.In the second pass, the first member in declaration order wins. For a union like
v.union(stripMember({a}), strictMember({a, b}))with value{a:"x", b:1, extra:"field"}:
- First pass:
stripMemberis skipped;strictMemberfails (extrais an unknown field) →undefined.- Second pass:
stripMemberappears first, matches withallowUnknownFields: true→ result is{a: "x"}(bis silently dropped).Reversing the order (
strictMemberfirst) preservesb. The tests at lines 759–770 document this for strip-only unions, but the mixed case is absent. A developer mixing strip and non-strip members in the same union may be surprised that a leading strip member causes the subsequent non-strip member — which would capture more fields — to be bypassed.Would you like me to draft a test that documents this ordering-sensitive behavior? For example:
test("mixed union: declaration order determines strip result when extra fields present", () => { // strip member listed first → wins in second pass, drops 'b' const stripFirst = v.union( withStripUnknownKeys(v.object({ a: v.string() })), v.object({ a: v.string(), b: v.number() }), ); expect(parse(stripFirst, { a: "x", b: 1, extra: "!" })).toEqual({ a: "x" }); // non-strip member listed first → wins in second pass, keeps 'b' const strictFirst = v.union( v.object({ a: v.string(), b: v.number() }), withStripUnknownKeys(v.object({ a: v.string() })), ); expect(parse(strictFirst, { a: "x", b: 1, extra: "!" })).toEqual({ a: "x", b: 1 }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/convex-helpers/validators.ts` around lines 920 - 941, Add a unit test that documents the ordering-sensitive behavior of union members when mixing strip and strict object validators: construct two unions (using v.union with withStripUnknownKeys(v.object(...)) and v.object(...)), one with the strip-member first and one with the strict-member first, then parse a value containing both expected fields and an extra field and assert the first preserves only the stripped fields while the second preserves all strict-member fields; reference the union behavior exercised by firstMatchingUnionMember and stripUnknownFields so the test explicitly demonstrates how declaration order affects which member wins during the second (permissive) pass.
870-886:includeStripObjectssemantics are inverted from what the name implies — consider renaming.When
includeStripObjects: true, the skip conditionisStripObjectValidator(member) && !opts.includeStripObjectsevaluates tofalsefor every member (both strip and non-strip), so all members are iterated. The name implies "only include strip objects," but the flag really controls whether strip objects are excluded. Callers reading the two call sites instripUnknownFieldshave to trace the logic to understand whatincludeStripObjects: truedoes.♻️ Suggested rename
function firstMatchingUnionMember( members: Validator<any, any, any>[], value: unknown, - opts: { allowUnknownFields: boolean; includeStripObjects: boolean }, + opts: { allowUnknownFields: boolean; excludeStripObjects: boolean }, ): Validator<any, any, any> | undefined { for (const member of members) { - if (isStripObjectValidator(member) && !opts.includeStripObjects) { + if (isStripObjectValidator(member) && opts.excludeStripObjects) { continue; }And update call sites in
stripUnknownFields:- const strictMember = firstMatchingUnionMember(validator.members, value, { - allowUnknownFields: false, - includeStripObjects: false, - }); + const strictMember = firstMatchingUnionMember(validator.members, value, { + allowUnknownFields: false, + excludeStripObjects: true, + }); ... - const permissiveMember = firstMatchingUnionMember(validator.members, value, { - allowUnknownFields: true, - includeStripObjects: true, - }); + const permissiveMember = firstMatchingUnionMember(validator.members, value, { + allowUnknownFields: true, + excludeStripObjects: false, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/convex-helpers/validators.ts` around lines 870 - 886, The flag includeStripObjects in firstMatchingUnionMember is named/used incorrectly (semantics are inverted); rename it to excludeStripObjects and invert the condition: change the parameter to opts: { allowUnknownFields: boolean; excludeStripObjects: boolean }, and update the loop guard to if (isStripObjectValidator(member) && opts.excludeStripObjects) continue; then update the two call sites in stripUnknownFields to pass the inverted boolean (i.e., replace includeStripObjects: X with excludeStripObjects: !X) so the meaning matches the name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/convex-helpers/validators.ts`:
- Around line 920-941: Remove the duplicate review comment and keep the union
branch logic as-is; add a short inline comment above the
firstMatchingUnionMember/second pass in the case "union" block clarifying that
the second pass (includeStripObjects: true) causes allowUnknownFields to be
effectively true for all members (so strict-only unions are handled), and run
the existing tests (including "parse strips unknown fields from unions") to
confirm no regression; references: case "union", firstMatchingUnionMember,
stripUnknownFields.
---
Nitpick comments:
In `@packages/convex-helpers/validators.ts`:
- Around line 758-761: Add an inline comment next to the condition that checks
opts?.allowUnknownFields and (validator as any).unknownKeys !== "strip"
explaining that strip-mode validators intentionally accept unknown fields during
validate() even when allowUnknownFields is false, and that this behavior is why
firstMatchingUnionMember must skip strip objects in its first pass rather than
relying on validate() to reject them; mention the involved symbols: unknownKeys,
allowUnknownFields, validate(), and firstMatchingUnionMember so future readers
won't invert or simplify the check.
- Around line 920-941: Add a unit test that documents the ordering-sensitive
behavior of union members when mixing strip and strict object validators:
construct two unions (using v.union with withStripUnknownKeys(v.object(...)) and
v.object(...)), one with the strip-member first and one with the strict-member
first, then parse a value containing both expected fields and an extra field and
assert the first preserves only the stripped fields while the second preserves
all strict-member fields; reference the union behavior exercised by
firstMatchingUnionMember and stripUnknownFields so the test explicitly
demonstrates how declaration order affects which member wins during the second
(permissive) pass.
- Around line 870-886: The flag includeStripObjects in firstMatchingUnionMember
is named/used incorrectly (semantics are inverted); rename it to
excludeStripObjects and invert the condition: change the parameter to opts: {
allowUnknownFields: boolean; excludeStripObjects: boolean }, and update the loop
guard to if (isStripObjectValidator(member) && opts.excludeStripObjects)
continue; then update the two call sites in stripUnknownFields to pass the
inverted boolean (i.e., replace includeStripObjects: X with excludeStripObjects:
!X) so the meaning matches the name.
2 participants
Summary
validate()now respectsunknownKeys: "strip"onv.object()validators.parse()union handling now follows strict-first semantics for consistency across Convex repos and with Zod-style union ordering expectations.What changed
packages/convex-helpers/validators.tsvalidate()object unknown-field checks now skip rejection when(validator as any).unknownKeys === "strip".stripUnknownFields()union selection now uses two passes:allowUnknownFields: falseallowUnknownFields: trueparse()union stripping.packages/convex-helpers/validators.test.tsparse().packages/convex-helpers/server/validators.test.tsNotes
convexnpm package (^1.31.0) does not yet emitunknownKeysin serialized validator JSON, so tests use monkey-patch helpers.partial(),addFieldsToValidator(), andvRequired()currently loseunknownKeyswhen reconstructing viav.object(fields); out of scope here.Related
get-convex/convex-backendPR nice error on nested triggers #348get-convex/convex-testPR Move Hono intoconvex-helpersnpm package #66By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Summary by CodeRabbit
New Features
Tests