Skip to content

Conversation

@ymc9
Copy link
Member

@ymc9 ymc9 commented Nov 27, 2025

Summary by CodeRabbit

  • New Features

    • Added $setOptions to reconfigure client instances with new options.
    • Added allowQueryTimeOmitOverride option to control query-time field-omit overrides.
    • Made computedFields configuration optional in client setup.
  • Tests

    • Added end-to-end tests for query-time omit override validation and related flows.
    • Added tests consolidating input-validation toggles via cloned option objects.

✏️ Tip: You can customize this high-level summary in your review settings.

Copilot AI review requested due to automatic review settings November 27, 2025 02:51
@coderabbitai
Copy link

coderabbitai bot commented Nov 27, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds a new Client.$setOptions(...) API, a new client option allowQueryTimeOmitOverride, updates the omit validator to consult client options, and adds/adjusts end-to-end tests for omit and input-validation flows.

Changes

Cohort / File(s) Summary
Client API (interface + impl)
packages/orm/src/client/contract.ts, packages/orm/src/client/client-impl.ts
Added $setOptions<Options extends ClientOptions<Schema>>(options: Options): ClientContract<Schema, Options> to the ClientContract interface and ClientImpl implementation (preserves parent reference).
Client options type
packages/orm/src/client/options.ts
Introduced optional allowQueryTimeOmitOverride?: boolean; adjusted computedFields conditional to be optional under HasComputedFields<Schema>.
Query validator
packages/orm/src/client/crud/validator/index.ts
Added a private options getter exposing this.client.$options; changed makeOmitSchema to restrict omit patterns (boolean vs literal true) based on allowQueryTimeOmitOverride.
End-to-end tests
tests/e2e/orm/client-api/omit.test.ts, tests/e2e/orm/validation/custom-validation.test.ts
Added test asserting behavior when allowQueryTimeOmitOverride is disabled; refactored validation tests to use a cloned client options object (dbNoValidation) instead of per-call toggles.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Pay special attention to:
    • packages/orm/src/client/options.ts: verify the conditional type change for computedFields does not break type inference or introduce unintended optionality.
    • packages/orm/src/client/crud/validator/index.ts: ensure the boolean vs literal(true) distinction is applied correctly and covers all omit schema branches.
    • Tests: confirm e2e coverage matches intended runtime validation and that the new $setOptions usage is exercised where expected.

Possibly related PRs

Poem

🐇 I nibbled on options and stitched them anew,

A client reborn with a clearer view,
Omit rules obeyed when the flag says no,
Options swapped gently — hop, off we go!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main feature: adding a new option to disallow query-time omit overrides, which is the primary change across all modified files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/rpc-api-omit

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ade2cfe and e54dfdf.

📒 Files selected for processing (1)
  • packages/orm/src/client/options.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible

Applied to files:

  • packages/orm/src/client/options.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization

Applied to files:

  • packages/orm/src/client/options.ts
⏰ 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 (2)
packages/orm/src/client/options.ts (2)

87-92: LGTM!

The new allowQueryTimeOmitOverride option is well-documented with clear JSDoc explaining the default behavior and the validation error that occurs when disabled. The grammar issue from the previous review has been addressed.


93-100: LGTM!

The conditional type structure is correctly formatted and consistent with the HasProcedures conditional below. The type semantics correctly require computedFields when the schema has computed fields.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude
Copy link

claude bot commented Nov 27, 2025

Claude finished @ymc9's task —— View job


Code Review for feat/rpc-api-omit

I've completed my analysis of the pull request. This change adds a new setting allowQueryTimeOmitOverride to control whether query-time omit overrides are allowed.

Todo List

  • Read and analyze all modified files
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage
  • Provide comprehensive feedback

Review Summary

✅ Positive Aspects

  1. Clear Implementation: The feature is well-implemented with a clean separation between schema-level and validation-level changes.

  2. Comprehensive Testing: The test coverage is excellent, covering both positive and negative cases for the new setting.

  3. Backward Compatibility: Defaults to true to maintain existing behavior, ensuring no breaking changes.

  4. Type Safety: Good integration with the existing type system and validation framework.

🔍 Code Quality & Best Practices

Good practices observed:

  • Proper JSDoc documentation in packages/orm/src/client/options.ts:88-92
  • Consistent naming convention (allowQueryTimeOmitOverride)
  • Logical placement in the options hierarchy
  • Clean validation logic in packages/orm/src/client/crud/validator/index.ts:790-796

Minor suggestions:

  • The condition this.options.allowQueryTimeOmitOverride !== false is used consistently, which is correct for defaulting to true

🐛 Potential Issues

No significant bugs found, but some considerations:

  1. Documentation: While the JSDoc is clear, it would be helpful to have examples in the documentation showing both true and false behaviors.

  2. Error Messages: When validation fails, users get a generic validation error. Consider if a more specific error message would be helpful for this particular case.

⚡ Performance Considerations

Minimal performance impact:

  • The validation check is a simple boolean comparison
  • Schema caching remains effective
  • No additional overhead in the hot path

🔒 Security Concerns

No security issues identified:

  • The feature actually improves security by allowing stricter control over field omission
  • No potential for injection or bypass attacks
  • Proper validation occurs at the input validation layer

🧪 Test Coverage

Excellent test coverage:

  • Tests both enabled and disabled states of the setting
  • Covers various query types (create, findFirst, etc.)
  • Tests the interaction with existing omit functionality
  • Verifies that omit: { field: true } still works when override is disabled

Test file changes:

  • tests/e2e/orm/client-api/omit.test.ts:144-161 - New test case covering the feature
  • tests/e2e/orm/validation/custom-validation.test.ts - Minor adjustment for validation setup

📋 Specific Code Review Notes

packages/orm/src/client/options.ts:88-92

  • Good documentation explaining the behavior when set to false
  • Proper default value handling

packages/orm/src/client/crud/validator/index.ts:790-796

  • Clean conditional logic
  • Proper use of Zod validation types (z.boolean() vs z.literal(true))

tests/e2e/orm/client-api/omit.test.ts:144-161

  • Comprehensive test covering the main use case
  • Good use of toBeRejectedByValidation() matcher
  • Tests both rejection and acceptance cases

🎯 Recommendations

  1. Documentation Enhancement: Consider adding usage examples to the main documentation showing when and why to use this setting.

  2. Error Message Improvement: Consider providing a more specific error message when omit override is attempted but not allowed.

  3. Type Definitions: The implementation correctly handles the TypeScript types, maintaining type safety.

Overall Assessment

This is a well-implemented feature that adds useful functionality for controlling field omission behavior. The code quality is high, test coverage is comprehensive, and the implementation follows established patterns in the codebase. The feature enhances security and control without introducing breaking changes.

Recommendation: ✅ Approve with minor suggestions above


Copilot finished reviewing on behalf of ymc9 November 27, 2025 02:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a new client option allowQueryTimeOmitOverride to control whether fields marked with @omit can be overridden at query time. When set to false, attempting to set an omitted field to false in a query's omit clause will trigger a validation error. The PR also introduces a new $setOptions method and deprecates the existing $setInputValidation method.

  • Adds allowQueryTimeOmitOverride option (defaults to true for backward compatibility)
  • Introduces $setOptions method for setting arbitrary client options
  • Deprecates $setInputValidation in favor of $setOptions

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/orm/src/client/options.ts Added allowQueryTimeOmitOverride option definition with documentation
packages/orm/src/client/contract.ts Added $setOptions method signature and deprecated $setInputValidation
packages/orm/src/client/client-impl.ts Implemented $setOptions method
packages/orm/src/client/crud/validator/index.ts Updated omit schema validation to enforce allowQueryTimeOmitOverride setting
tests/e2e/orm/client-api/omit.test.ts Added test coverage for the new allowQueryTimeOmitOverride setting
tests/e2e/orm/validation/custom-validation.test.ts Migrated from deprecated $setInputValidation to $setOptions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@claude
Copy link

claude bot commented Nov 27, 2025

Claude finished @ymc9's task —— View job


Code Review for feat/rpc-api-omit

I've completed my analysis of the pull request. This change adds a new setting allowQueryTimeOmitOverride to control whether query-time omit overrides are allowed.

Todo List

  • Read and analyze all modified files
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage
  • Provide comprehensive feedback

Review Summary

✅ Positive Aspects

  1. Clear Implementation: The feature is well-implemented with a clean separation between schema-level and validation-level changes.

  2. Comprehensive Testing: The test coverage is excellent, covering both positive and negative cases for the new setting.

  3. Backward Compatibility: Defaults to true to maintain existing behavior, ensuring no breaking changes.

  4. Type Safety: Good integration with the existing type system and validation framework.

🔍 Code Quality & Best Practices

Good practices observed:

  • Proper JSDoc documentation in packages/orm/src/client/options.ts:88-92
  • Consistent naming convention (allowQueryTimeOmitOverride)
  • Logical placement in the options hierarchy
  • Clean validation logic in packages/orm/src/client/crud/validator/index.ts:790-796

Minor suggestions:

  • The condition this.options.allowQueryTimeOmitOverride !== false is used consistently, which is correct for defaulting to true

🐛 Potential Issues

No significant bugs found, but some considerations:

  1. Documentation: While the JSDoc is clear, it would be helpful to have examples in the documentation showing both true and false behaviors.

  2. Error Messages: When validation fails, users get a generic validation error. Consider if a more specific error message would be helpful for this particular case.

⚡ Performance Considerations

Minimal performance impact:

  • The validation check is a simple boolean comparison
  • Schema caching remains effective
  • No additional overhead in the hot path

🔒 Security Concerns

No security issues identified:

  • The feature actually improves security by allowing stricter control over field omission
  • No potential for injection or bypass attacks
  • Proper validation occurs at the input validation layer

🧪 Test Coverage

Excellent test coverage:

  • Tests both enabled and disabled states of the setting
  • Covers various query types (create, findFirst, etc.)
  • Tests the interaction with existing omit functionality
  • Verifies that omit: { field: true } still works when override is disabled

Test file changes:

  • tests/e2e/orm/client-api/omit.test.ts:144-161 - New test case covering the feature
  • tests/e2e/orm/validation/custom-validation.test.ts - Minor adjustment for validation setup

📋 Specific Code Review Notes

packages/orm/src/client/options.ts:88-92

  • Good documentation explaining the behavior when set to false
  • Proper default value handling

packages/orm/src/client/crud/validator/index.ts:790-796

  • Clean conditional logic
  • Proper use of Zod validation types (z.boolean() vs z.literal(true))

tests/e2e/orm/client-api/omit.test.ts:144-161

  • Comprehensive test covering the main use case
  • Good use of toBeRejectedByValidation() matcher
  • Tests both rejection and acceptance cases

🎯 Recommendations

  1. Documentation Enhancement: Consider adding usage examples to the main documentation showing when and why to use this setting.

  2. Error Message Improvement: Consider providing a more specific error message when omit override is attempted but not allowed.

  3. Type Definitions: The implementation correctly handles the TypeScript types, maintaining type safety.

Overall Assessment

This is a well-implemented feature that adds useful functionality for controlling field omission behavior. The code quality is high, test coverage is comprehensive, and the implementation follows established patterns in the codebase. The feature enhances security and control without introducing breaking changes.

Recommendation: ✅ Approve with minor suggestions above


@ymc9 ymc9 merged commit fbcc6e4 into dev Nov 27, 2025
5 checks passed
@ymc9 ymc9 deleted the feat/rpc-api-omit branch November 27, 2025 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants