refactor(orm): move validateInput logic into InputValidator#2480
Conversation
…ed option Instead of conditionally passing undefined for inputValidator when validateInput is false, the InputValidator now accepts an `enabled` option that controls whether Zod-based validation runs. When disabled, validate methods return the input as-is without parsing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job Pull Request ReviewTodo List:
Overall AssessmentThis is a well-designed refactoring that improves the separation of concerns in the validation system. The changes move validation control logic into the ✅ Strengths1. Clean Architecture Improvement
2. Consistent API Design
3. Comprehensive Test CoverageThe test additions are excellent:
4. Code Quality
🔍 Technical Review
|
📝 WalkthroughWalkthroughAdds runtime control to input validation by letting InputValidator accept an options object with an enabled flag; clients and clones now initialize validators with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
📝 Coding Plan
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.
🧹 Nitpick comments (1)
packages/orm/src/client/zod/factory.ts (1)
111-111: Consider simplifying now-dead validation branches.With
extraValidationsEnabledfixed totrue, the ternary branches using it are now unreachable on thefalseside. Collapsing those branches will reduce complexity and make intent clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/zod/factory.ts` at line 111, The field extraValidationsEnabled is hardcoded true so any ternary or conditional branches that select the false path are dead; locate usages of extraValidationsEnabled (e.g., in the Zod factory class methods and any expressions like extraValidationsEnabled ? A : B) and collapse them to the true branch (remove the ?: or if/else false branches), simplify logic accordingly, and delete any now-unused helper code guarded only by the false branch to reduce complexity and clarify intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/orm/src/client/zod/factory.ts`:
- Line 111: The field extraValidationsEnabled is hardcoded true so any ternary
or conditional branches that select the false path are dead; locate usages of
extraValidationsEnabled (e.g., in the Zod factory class methods and any
expressions like extraValidationsEnabled ? A : B) and collapse them to the true
branch (remove the ?: or if/else false branches), simplify logic accordingly,
and delete any now-unused helper code guarded only by the false branch to reduce
complexity and clarify intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03c6cafe-8ae9-44e9-866c-e30e9dfec0c6
📒 Files selected for processing (6)
packages/orm/src/client/client-impl.tspackages/orm/src/client/contract.tspackages/orm/src/client/crud/validator/validator.tspackages/orm/src/client/options.tspackages/orm/src/client/zod/factory.tstests/e2e/orm/validation/custom-validation.test.ts
|
@claude check why postgres tests are failing |
|
Claude finished @ymc9's task —— View job I'll analyze the postgres test failures for you. Todo List
Root Cause AnalysisThe postgres test failure is in the new The Problem
Why it only fails on PostgreSQL
Fix RecommendationThe test needs to avoid ID collision. Here are two solutions: Option 1: Use different explicit IDs// Line 243: Use a high ID that won't conflict
await expect(dbDisabled.item.create({ data: { id: 1000, url: 'not-a-url' } })).toResolveTruthy();
// Line 250: Use a different high ID
await expect(dbReEnabled.item.create({ data: { id: 1001, url: 'https://example.com' } })).toResolveTruthy();Option 2: Don't specify IDs (recommended)// Line 243: Let auto-increment handle it
await expect(dbDisabled.item.create({ data: { url: 'not-a-url' } })).toResolveTruthy();
// Line 250: Let auto-increment handle it
await expect(dbReEnabled.item.create({ data: { url: 'https://example.com' } })).toResolveTruthy();Option 2 is preferred because it tests the validation behavior without introducing PostgreSQL-specific ID collision issues and better matches real-world usage patterns. Impact Assessment
Would you like me to implement the fix? |
Yes, fix with option 2 @claude |
|
Claude finished @ymc9's task —— View job Fixed PostgreSQL test failure ✅ Todo List
Root CauseThe test failure occurred because PostgreSQL's auto-increment doesn't automatically skip manually inserted IDs, causing a unique constraint violation when trying to create a second item. Fix Applied
Result
The PostgreSQL tests should now pass! 🎯 |
… constraint violation Co-authored-by: Yiming Cao <ymc9@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/orm/validation/custom-validation.test.ts (1)
149-153: Prefer avoiding explicit IDs in these inserts to prevent sequence-related test brittleness.Given the recent PostgreSQL ID-collision issue, these explicit IDs can reintroduce sequence-coupling risks if test flow changes later. Let autoincrement assign IDs and reuse returned IDs for follow-up operations.
Suggested diff
- await expect( - dbNoValidation.user.create({ - data: { - id: 1, - email: 'xyz', - }, - }), - ).toResolveTruthy(); + const createdUser = await dbNoValidation.user.create({ + data: { + email: 'xyz', + }, + }); + expect(createdUser).toBeTruthy(); await expect( dbNoValidation.user.update({ - where: { id: 1 }, + where: { id: createdUser.id }, data: { email: 'a@b.com', }, }), ).toResolveTruthy(); - await expect(db.post.create({ data: { id: 1, title: 'ab' } })).toResolveTruthy(); + await expect(db.post.create({ data: { title: 'ab' } })).toResolveTruthy();Also applies to: 158-159, 265-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/validation/custom-validation.test.ts` around lines 149 - 153, The test inserts use explicit id values which can cause sequence-related brittleness; update calls to dbNoValidation.user.create to omit the id field so the DB autoincrements, capture the returned record (e.g., const user = await dbNoValidation.user.create(...)) and reuse user.id for subsequent operations and assertions; apply the same change to the other dbNoValidation.user.create occurrences in this test to avoid ID collisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/orm/validation/custom-validation.test.ts`:
- Around line 149-153: The test inserts use explicit id values which can cause
sequence-related brittleness; update calls to dbNoValidation.user.create to omit
the id field so the DB autoincrements, capture the returned record (e.g., const
user = await dbNoValidation.user.create(...)) and reuse user.id for subsequent
operations and assertions; apply the same change to the other
dbNoValidation.user.create occurrences in this test to avoid ID collisions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c0f0795-19ce-48c4-acd3-3db529974760
📒 Files selected for processing (1)
tests/e2e/orm/validation/custom-validation.test.ts
1 participant
Summary
enabledoption toInputValidatorconstructor to control whether Zod-based validation runsvalidateInput: false, the validator skips parsing and returns input as-is instead of receivingundefinedas the validatorZodSchemaFactoryto always include extra validations (controlled at theInputValidatorlevel now)validateInputJSDoc to clarify it controls all query args validation, not just attribute-based validationTest plan
allows disabling validationtest passes (renamed todisabling validation makes validation attributes ineffective)disabling validation skips structural validation for all CRUD operations— verifies malformed args bypass Zod validation when disabled$setInputValidation toggles validation— verifies enable/disable/re-enable flowdisabling validation at client creation time— verifiesvalidateInput: falseworks from initial client options🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests