feat(validation): add API to suppress validation #301
Conversation
|
Claude finished @ymc9's task —— View job Pull Request Review: Add API to Suppress ValidationI've completed a thorough review of this PR that adds the ability to suppress input validation in ZenStack. Here's my comprehensive feedback: Review Checklist:
✅ Strengths1. Excellent API Design
2. Clean Implementation
3. Comprehensive Test Coverage
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds an API to suppress input validation by introducing a new $setInputValidation() method and a validateInput client option. This allows users to bypass validation rules expressed through attributes like @email, @regex, and @@validate.
- Added
$setInputValidation(enable: boolean)method to the client contract and implementation - Introduced
validateInputoption toClientOptions(defaults totrue) - Modified
InputValidatorto conditionally apply validation rules based on thevalidateInputsetting
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime/src/client/options.ts | Added validateInput boolean option to ClientOptions |
| packages/runtime/src/client/contract.ts | Added $setInputValidation method signature to the client contract |
| packages/runtime/src/client/client-impl.ts | Implemented $setInputValidation method to create a new client instance with updated validation setting |
| packages/runtime/src/client/crud/validator/index.ts | Modified validator to conditionally apply extra validations based on client options |
| tests/e2e/orm/validation/custom-validation.test.ts | Added test verifying validation can be disabled and original client remains unaffected |
| tests/regression/test/v2-migrated/issue-2025.test.ts | Added regression test for issue 2025 using validation suppression |
| tests/regression/test/v2-migrated/issue-2019.test.ts | Added regression test for issue 2019 with policy client |
| tests/regression/test/v2-migrated/issue-2007.test.ts | Added skipped regression test for issue 2007 with field-level policy |
| tests/regression/test/v2-migrated/issue-2000.test.ts | Added skipped regression test for issue 2000 with validation rules |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
WalkthroughAdds a client-level input-validation toggle ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1){packages,samples,tests}/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)packages/runtime/src/client/crud/validator/index.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (7)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/runtime/src/client/crud/validator/index.ts (1)
258-298: Critical: Cache invalidation issue with validation toggle.The
schemaCacheis keyed by{model, operation, options}(line 201-205) but doesn't include thevalidateInputsetting. When a user toggles validation using$setInputValidation(), the cache may return stale schemas that were generated with different validation settings.Consider one of these solutions:
Solution 1 (Recommended): Include
extraValidationsEnabledin the cache key:private validate<T, Options = undefined>( model: GetModels<Schema>, operation: string, options: Options, getSchema: GetSchemaFunc<Schema, Options>, args: unknown, ) { const cacheKey = stableStringify({ model, operation, options, + validateInput: this.extraValidationsEnabled, });Solution 2: Clear the cache when validation is toggled (requires tracking in ClientImpl):
$setInputValidation(enable: boolean) { const newOptions: ClientOptions<Schema> = { ...this.options, validateInput: enable, }; const newClient = new ClientImpl<Schema>(this.schema, newOptions, this); // InputValidator will be recreated in createClientProxy, getting a fresh cache return newClient; }Note: Solution 2 is already implicitly implemented since
createClientProxycreates a newInputValidatorinstance for each client, so each client with different validation settings will have its own cache. However, this could still be an issue if the schema cache grows too large over time when toggling validation repeatedly.
🧹 Nitpick comments (1)
tests/regression/test/v2-migrated/issue-2019.test.ts (1)
4-85: LGTM! Test correctly validates tenant propagation through delegated models.The regression test is well-structured and accurately verifies that
tenantIdfrom the auth context propagates to nestedPostUserLikesrecords created viacreateManywithin a delegated model hierarchy. The test flow is clear: setup tenant/user, authenticate, create post with nested likes, and assert tenant propagation.Optional enhancement for robustness:
Consider testing with multiple nested likes to ensure consistent tenant propagation across all items:
createMany: { data: [ { userId: user.id, }, + { + userId: user.id, + }, ], },Then update the assertion:
- expect(result.likes[0].tenantId).toBe(tenant.id); + result.likes.forEach(like => { + expect(like.tenantId).toBe(tenant.id); + });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/runtime/src/client/client-impl.ts(2 hunks)packages/runtime/src/client/contract.ts(1 hunks)packages/runtime/src/client/crud/validator/index.ts(3 hunks)packages/runtime/src/client/options.ts(1 hunks)tests/e2e/orm/validation/custom-validation.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-2000.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-2007.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-2019.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-2025.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/runtime/src/client/contract.tspackages/runtime/src/client/options.tstests/e2e/orm/validation/custom-validation.test.tstests/regression/test/v2-migrated/issue-2019.test.tstests/regression/test/v2-migrated/issue-2007.test.tstests/regression/test/v2-migrated/issue-2025.test.tspackages/runtime/src/client/client-impl.tstests/regression/test/v2-migrated/issue-2000.test.tspackages/runtime/src/client/crud/validator/index.ts
tests/e2e/**
📄 CodeRabbit inference engine (CLAUDE.md)
End-to-end tests must live under
tests/e2e/
Files:
tests/e2e/orm/validation/custom-validation.test.ts
🧬 Code graph analysis (7)
tests/e2e/orm/validation/custom-validation.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-2019.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-2007.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-2025.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
packages/runtime/src/client/client-impl.ts (3)
packages/runtime/src/client/options.ts (1)
ClientOptions(45-96)packages/runtime/src/client/crud/validator/index.ts (1)
InputValidator(54-1470)packages/runtime/src/client/contract.ts (1)
ClientContract(50-165)
tests/regression/test/v2-migrated/issue-2000.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
packages/runtime/src/client/crud/validator/index.ts (3)
packages/runtime/src/client/contract.ts (1)
ClientContract(50-165)packages/runtime/src/client/crud/validator/utils.ts (4)
addStringValidation(24-86)addNumberValidation(88-114)addBigIntValidation(116-143)addDecimalValidation(145-203)packages/runtime/src/client/crud/operations/base.ts (1)
options(94-96)
🪛 Gitleaks (8.28.0)
tests/regression/test/v2-migrated/issue-2000.test.ts
[high] 61-61: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (5)
packages/runtime/src/client/client-impl.ts (2)
291-297: LGTM! Consistent implementation pattern.The
$setInputValidationmethod follows the same immutable pattern as other client modification methods like$setAuth,$use, and$unuse, correctly creating a new client instance with updated options.
336-336: LGTM! Correct refactor for validator construction.The change from passing
client.$schemato passing the full client (cast toClientContract<Schema>) correctly aligns with the updatedInputValidatorconstructor signature, enabling the validator to access client options.packages/runtime/src/client/crud/validator/index.ts (2)
57-65: LGTM! Clean refactor with proper encapsulation.The constructor refactor to accept
ClientContract<Schema>and the addition of private getters forschemaandextraValidationsEnabledprovide clean encapsulation. The default behavior (validation enabled whenvalidateInputis not explicitly set tofalse) is correct.
54-65: Verify cache isolation across validation toggles.Each
InputValidatorinstance has its ownschemaCache(line 55), and$setInputValidation()creates a new client instance with a new validator. This means the cache is isolated per validation setting, which is correct.However, please verify this behavior with a test that:
- Creates a client with validation enabled
- Performs operations (populating the cache)
- Toggles validation off with
$setInputValidation(false)- Verifies operations behave differently (confirming a fresh cache)
tests/regression/test/v2-migrated/issue-2025.test.ts (1)
4-38: The custom matchers.toBeRejectedByValidation()and.toResolveTruthy()are both properly defined inpackages/testtools/src/vitest-ext.tsand exported throughpackages/testtools/src/types.d.ts. They are actively used throughout the test suite, including correctly in the test under review. No issues identified.
|
Claude finished @ymc9's task —— View job Pull Request Review: Add API to Suppress Validation
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/runtime/src/client/crud/validator/index.ts (1)
263-293: Conditional validation logic is well-implemented.The conditional application of validation based on
extraValidationsEnabledis correct and consistently applied across all primitive types.The Decimal handling (lines 281-293) could be refactored for clarity:
-.with('Decimal', () => { - const options: [z.ZodSchema, z.ZodSchema, ...z.ZodSchema[]] = [ - z.number(), - z.instanceof(Decimal), - z.string(), - ]; - if (this.extraValidationsEnabled) { - for (let i = 0; i < options.length; i++) { - options[i] = addDecimalValidation(options[i]!, attributes); - } - } - return z.union(options); -}) +.with('Decimal', () => { + const baseOptions = [z.number(), z.instanceof(Decimal), z.string()]; + const options = this.extraValidationsEnabled + ? baseOptions.map(opt => addDecimalValidation(opt, attributes)) + : baseOptions; + return z.union(options as [z.ZodSchema, z.ZodSchema, ...z.ZodSchema[]]); +})
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/runtime/src/client/crud/validator/index.ts(5 hunks)tests/e2e/orm/validation/custom-validation.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/orm/validation/custom-validation.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/runtime/src/client/crud/validator/index.ts
🧬 Code graph analysis (1)
packages/runtime/src/client/crud/validator/index.ts (3)
packages/runtime/src/client/contract.ts (1)
ClientContract(50-165)packages/runtime/src/client/crud/validator/utils.ts (5)
addStringValidation(24-86)addNumberValidation(88-114)addBigIntValidation(116-143)addDecimalValidation(145-203)addCustomValidation(205-226)packages/runtime/src/client/crud/operations/base.ts (1)
options(94-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: claude-review
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (3)
packages/runtime/src/client/crud/validator/index.ts (3)
57-65: LGTM! Well-designed encapsulation.The constructor and getter changes provide good encapsulation by accessing the schema and validation settings through the client. The default behavior (validation enabled when
validateInput !== false) is sensible and aligns with the PR objective of adding an API to suppress validation.
939-944: LGTM! Conditional validation for create and update schemas.The conditional application of
addCustomValidationto both unchecked and checked schema variants is correct and consistent. The@@validateattributes are properly applied only whenextraValidationsEnabledis true, aligning with the overall design.Also applies to: 1223-1228
201-209: Schema caching is correct; each client has its own InputValidator instance.The verification confirms that
new InputValidatoris instantiated increateClientProxy(packages/runtime/src/client/client-impl.ts:336), creating a separate instance per client. Since eachInputValidatorinstance is isolated to one client and maintains its ownschemaCache, the cache is safe even thoughextraValidationsEnabledis not included in the cache key. The validation settings are tied to the client and thus to the specificInputValidatorinstance.
|
Claude finished @ymc9's task —— View job Pull Request Review: Add API to Suppress Validation
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/runtime/src/client/crud/validator/index.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/runtime/src/client/crud/validator/index.ts
🧬 Code graph analysis (1)
packages/runtime/src/client/crud/validator/index.ts (2)
packages/runtime/src/client/contract.ts (1)
ClientContract(50-165)packages/runtime/src/client/crud/validator/utils.ts (5)
addStringValidation(24-86)addNumberValidation(88-114)addBigIntValidation(116-143)addDecimalValidation(145-203)addCustomValidation(205-226)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: claude-review
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
🔇 Additional comments (5)
packages/runtime/src/client/crud/validator/index.ts (5)
59-65: LGTM!The getters are well-designed:
schemaprovides clean encapsulation of client schema accessextraValidationsEnabledcorrectly treatsundefinedas enabled (default behavior)
263-279: LGTM!The conditional validation logic for
String,Int,Float, andBigInttypes is correctly implemented. These types don't require transformation, so conditionally applying only the validation attributes is appropriate.
935-940: LGTM!The conditional application of
addCustomValidationto both unchecked and checked variants is correct and consistent.
1219-1224: LGTM!The update schema changes mirror the create schema pattern correctly, applying
addCustomValidationconditionally to both variants.
57-57: All InputValidator instantiations updated. Verified the sole instantiation in client-impl.ts passesclient as unknown as ClientContract<Schema>and no other instantiations exist.
|
Claude finished @ymc9's task —— View job Pull Request Review: Add API to Suppress Validation
|
Summary by CodeRabbit