Skip to content

Conversation

@ymc9
Copy link
Member

@ymc9 ymc9 commented Dec 29, 2025

Summary by CodeRabbit

Release Notes

  • New Features

    • Added validation for UUID (versions 4 and 7), CUID (versions 1 and 2), and Nanoid (positive length) functions.
    • Enhanced default value generation with automatic version/length selection for UUID, CUID, and Nanoid.
  • Tests

    • Introduced comprehensive test suites validating UUID, CUID, and Nanoid function defaults and constraints.

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

Copilot AI review requested due to automatic review settings December 29, 2025 15:06
@claude
Copy link

claude bot commented Dec 29, 2025

Claude finished @ymc9's task —— View job


PR Review: feat(orm): support where enum

I'll provide a comprehensive review of this PR. Here's my analysis plan:

Todo List

  • Read repository guidelines and context
  • Analyze language package changes (validator and tests)
  • Review ORM package changes (client operations)
  • Examine test coverage in e2e tests
  • Review package.json dependency changes
  • Provide comprehensive feedback on code quality, bugs, performance, security, and tests

Analysis Complete - Providing Review Feedback

Based on my analysis of the code changes, here's my comprehensive review:

Overall Assessment

This PR appears to be mislabeled. The title suggests "support where enum" but the actual implementation is focused on ID generator improvements with enhanced validation and format string support for uuid(), cuid(), nanoid(), and ulid() functions.

⚠️ Critical Issues

1. Misleading PR Title & Missing Description

  • Title mentions "where enum" but no enum-related where clause functionality is implemented
  • No PR description provided to explain the actual changes
  • This makes it difficult to understand the intended purpose and scope

2. Inconsistent Commit History

  • The branch name fix/id-gen suggests this is a bug fix, but changes appear to be feature additions
  • Need clarity on whether this is a fix or a feature

Code Quality Assessment

Strengths

Function Invocation Validator (packages/language/src/validators/function-invocation-validator.ts):

  • Well-structured validation logic with proper error messages
  • Good use of decorator pattern for function-specific validators
  • Comprehensive validation for ID generator functions with proper version checking:
    • uuid(): Only versions 4 and 7 allowed
    • cuid(): Only versions 1 and 2 allowed
    • nanoid(): Positive length validation
  • Format string validation with proper regex to ensure unescaped %s is present

ORM Base Operations (packages/orm/src/client/crud/operations/base.ts):

  • Clean generator evaluation logic in evalGenerator() method (lines 862-898)
  • Proper format string handling with escape sequence support
  • Well-organized ID generation with appropriate library calls

Test Coverage:

  • Comprehensive test suite in function-invocation.test.ts covering:
    • Valid ID function calls without format strings
    • Valid format strings with proper %s placeholders
    • Invalid format strings missing %s
    • Version validation for uuid and cuid
    • Length validation for nanoid
  • E2E tests validate actual ID generation and format

⚠️ Areas for Improvement

Code Organization:

  1. Mixed Concerns: The validator file has both validation logic and hardcoded validation rules. Consider extracting rules to a configuration object for better maintainability.

Error Handling:

  1. Generic Error Messages: Some validation errors could be more specific. For example, line 184 in validator: "argument is not assignable to parameter" could specify expected vs actual types.

Performance Considerations:

  1. Regex Compilation: In _checkRegex() method (line 285), the RegExp is created for each validation. Consider caching compiled regex patterns.
  2. Generator Evaluation: The evalGenerator() method uses pattern matching which is performant, but the format string replacement (line 907) runs on every ID generation.

Security Considerations:

  1. Format String Safety: The regex validation for format strings is appropriate and prevents injection issues
  2. ID Collision Risk: No validation for uniqueness conflicts when custom format strings are used
  3. Input Validation: Good input sanitization in the validator

Test Coverage Analysis

Comprehensive Coverage

  • Unit Tests: 152 new lines in function-invocation.test.ts with good edge case coverage
  • E2E Tests: Real-world validation of ID generation
  • Validation Tests: Both positive and negative test cases

⚠️ Missing Coverage

  1. Error Recovery: No tests for malformed AST or invalid function references
  2. Concurrent Generation: No tests for ID generation under concurrent load
  3. Format String Edge Cases: Limited testing of complex escape sequences

Dependencies & Package Changes

Appropriate Additions

  • Added cuid@^3.0.0 and updated @paralleldrive/cuid2@^2.2.2
  • Added ulid@^3.0.0 and uuid@^11.0.5
  • All additions are for legitimate ID generation functionality

⚠️ Considerations

  1. Dependency Versions: Using latest versions is good, but ensure compatibility across the ecosystem
  2. Bundle Size: Multiple ID generation libraries may increase bundle size

Recommendations

🔧 Required Changes

  1. Fix PR Title & Description: Update to accurately reflect ID generator improvements
  2. Add PR Description: Explain the motivation, changes, and testing approach
  3. Branch Naming: Consider whether this should be a feature branch rather than a fix branch

🚀 Suggested Improvements

  1. Error Messages: Make validation errors more descriptive with expected vs actual type information
  2. Performance: Consider caching regex compilation in validation logic
  3. Documentation: Add JSDoc comments to new validation methods
  4. Testing: Add performance tests for ID generation under load

🧹 Nice to Have

  1. Extract Constants: Move validation rules to a configuration object
  2. Type Safety: Add stronger typing for generator function parameters
  3. Metrics: Consider adding metrics for ID generation performance

Verdict

While the implementation quality is good with solid validation logic and comprehensive tests, the misleading PR title and missing description are critical issues that need immediate attention. The actual functionality appears to be a valuable enhancement to ZenStack's ID generation capabilities.

Recommendation: Request PR title/description updates before merge, as the technical implementation is sound but the communication about changes is inadequate.

@coderabbitai
Copy link

coderabbitai bot commented Dec 29, 2025

📝 Walkthrough

Walkthrough

This PR introduces argument-level validation for UUID, CUID, and Nanoid function invocations, enforcing version constraints (UUID v4/v7, CUID v1/v2) and positive lengths for Nanoid, along with corresponding runtime generation logic that selects appropriate implementation based on detected versions, and updates dependencies to include CUID v3.

Changes

Cohort / File(s) Summary
Validator Implementation
packages/language/src/validators/function-invocation-validator.ts
Added three private validation methods: _checkUuid (enforces versions 4 or 7), _checkCuid (enforces versions 1 or 2), _checkNanoid (enforces positive length). Minor formatting alignment in findIndex lambda syntax.
Validator Tests
packages/language/test/function-invocation.test.ts
Refactored error-assertion calls to two-argument format (schema, error message). Added new test suites validating UUID version acceptance (4, 7), CUID version acceptance (1, 2), and Nanoid length validation (positive values).
Runtime Generation Logic
packages/orm/src/client/crud/operations/base.ts
Introduces firstArgVal capture for default function arguments. Updates UUID/CUID/Nanoid generation to select implementations based on detected versions/lengths: CUID chooses between cuid1() and cuid2(), UUID selects v7 or v4, Nanoid uses detected length.
Dependencies
packages/orm/package.json, tests/e2e/package.json
Added cuid ^3.0.0 dependency to both files. Reordered peer dependencies and added @types/sql.js dev dependency in ORM package.
E2E Tests
tests/e2e/orm/client-api/default-values.test.ts
Refactored to use createTestClient(schema) helper. Updated imports to use getUuidVersion, isCuidV1, isCuidV2. Added schema fields for uuid4 and cuid1 with corresponding version-specific validation assertions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

Poem

🐰 With uuid, cuid, and nanoid IDs so bright,
We validate versions—four, seven, one, two just right,
Nanoid lengths must be positive, no zero or less,
Default values now checked for correctness! ✨
The rabbit hops proudly through each validation test!

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 change: adding version argument support to the cuid() generator, which is demonstrated across validator updates, handler logic, tests, and dependencies.
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 fix/id-gen

📜 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 6917a8c and 652a1b0.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (6)
  • packages/language/src/validators/function-invocation-validator.ts
  • packages/language/test/function-invocation.test.ts
  • packages/orm/package.json
  • packages/orm/src/client/crud/operations/base.ts
  • tests/e2e/orm/client-api/default-values.test.ts
  • tests/e2e/package.json
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

E2E tests should validate real-world schema compatibility with established projects

Files:

  • tests/e2e/orm/client-api/default-values.test.ts
🧠 Learnings (6)
📚 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 tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects

Applied to files:

  • packages/language/test/function-invocation.test.ts
  • tests/e2e/orm/client-api/default-values.test.ts
  • tests/e2e/package.json
📚 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 tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests

Applied to files:

  • packages/language/test/function-invocation.test.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/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests

Applied to files:

  • tests/e2e/orm/client-api/default-values.test.ts
  • tests/e2e/package.json
  • packages/orm/package.json
📚 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:

  • tests/e2e/package.json
  • packages/orm/package.json
📚 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/package.json
📚 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 **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`

Applied to files:

  • packages/orm/package.json
🧬 Code graph analysis (2)
packages/orm/src/client/crud/operations/base.ts (1)
packages/schema/src/expression-utils.ts (1)
  • ExpressionUtils (19-123)
packages/language/src/validators/function-invocation-validator.ts (3)
packages/language/src/generated/ast.ts (2)
  • InvocationExpr (533-538)
  • InvocationExpr (540-540)
packages/language/src/utils.ts (1)
  • getLiteral (272-286)
packages/sdk/src/ts-schema-generator.ts (1)
  • getLiteral (1084-1097)
⏰ 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). (4)
  • GitHub Check: Agent
  • GitHub Check: claude-review
  • GitHub Check: build-test (22.x, postgresql)
  • GitHub Check: build-test (22.x, sqlite)
🔇 Additional comments (15)
packages/orm/src/client/crud/operations/base.ts (2)

1-3: LGTM!

The import changes cleanly introduce support for both CUID v1 and v2 by aliasing createId as cuid2 and importing the v1 implementation separately. This maintains clarity and aligns with the version-based generation logic introduced below.


861-899: The validator enforcement for version constraints is properly in place. The validators correctly restrict UUID to versions 4 and 7, CUID to versions 1 and 2, and NANOID to positive lengths before reaching runtime. The runtime logic in evalGenerator correctly assumes these constraints have been validated at schema parse time and handles the defined versions appropriately.

packages/language/src/validators/function-invocation-validator.ts (4)

91-91: Minor formatting consistency improvement.

The change from param => to (param) => improves consistency with the codebase's arrow function parameter style.


195-207: UUID version validation correctly enforces v4 and v7.

The validator properly restricts UUID generation to versions 4 and 7 only, coordinating with the runtime generation logic in packages/orm/src/client/crud/operations/base.ts (lines 873-877).


209-221: CUID version validation correctly enforces v1 and v2.

The validator properly restricts CUID generation to versions 1 and 2, coordinating with the runtime generation logic in packages/orm/src/client/crud/operations/base.ts (lines 868-872).


223-235: Nanoid length validation correctly enforces positive values.

The validator properly requires positive lengths for Nanoid generation, coordinating with the runtime generation logic in packages/orm/src/client/crud/operations/base.ts (lines 878-882). Note that it checks length <= 0 rather than length < 1 to handle both zero and negative values.

packages/language/test/function-invocation.test.ts (4)

179-276: Test refactoring improves readability.

The conversion from single-string arguments to separate schema and error message parameters enhances test clarity and maintainability.


278-318: Comprehensive UUID version validation tests.

The test suite correctly validates:

  • Positive cases: UUID v4 and v7 are accepted
  • Negative case: UUID v1 is rejected with appropriate error message

Tests align with the validator implementation in packages/language/src/validators/function-invocation-validator.ts (lines 195-207).


320-360: Comprehensive CUID version validation tests.

The test suite correctly validates:

  • Positive cases: CUID v1 and v2 are accepted
  • Negative case: CUID v0 is rejected with appropriate error message

Tests align with the validator implementation in packages/language/src/validators/function-invocation-validator.ts (lines 209-221).


362-416: Comprehensive Nanoid length validation tests.

The test suite correctly validates:

  • Positive cases: lengths 1 and 21 are accepted
  • Negative cases: lengths 0 and -1 are rejected with appropriate error message

Tests align with the validator implementation in packages/language/src/validators/function-invocation-validator.ts (lines 223-235).

tests/e2e/orm/client-api/default-values.test.ts (3)

1-6: Improved imports for comprehensive validation.

The updated imports correctly distinguish between CUID v1 and v2 validators (isCuidV1 and isCuidV2) and add UUID version checking (getUuidVersion), enabling precise assertions on generated default values.


8-23: Comprehensive schema covers all default value generators.

The string-based schema thoroughly tests all supported default value generators with version/length variants:

  • UUID: default, explicit v4, and v7
  • CUID: default (v1), explicit v1, and v2
  • Nanoid: default and custom length (8)
  • Plus ulid, DateTime, and Boolean defaults

This aligns with the validation and generation logic introduced across the PR.

Based on learnings, E2E tests should validate real-world schema compatibility with established projects.


27-40: Thorough validation of all generated default values.

The test correctly validates each default value generator:

  • Line 30: UUID default uses v4 (validates version explicitly)
  • Line 31: uuid4 explicitly uses v4
  • Line 32: uuid7 uses v7
  • Lines 33-35: CUID variants validated with appropriate version checkers
  • Lines 36-39: Nanoid, ulid, and DateTime defaults validated

Assertions use proper validation functions ensuring generated values meet expected formats and versions.

tests/e2e/package.json (1)

28-28: cuid v3.0.0 is compatible with the v1 API.

The package exports both the expected default function for CUID v1 generation (imported as cuid1 in packages/orm/src/client/crud/operations/base.ts) and the named isCuid validator (imported in tests/e2e/orm/client-api/default-values.test.ts), confirming the dependency is correctly configured.

packages/orm/package.json (1)

87-88: Dependency versions are correctly specified and verified.

The cuid v3.0.0 package provides the v1 API used in base.ts (imported as cuid1 and called directly as cuid1()), and the @zenstackhq/schema workspace version is compatible with ORM package usage. Both dependencies match runtime requirements.


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.

@ymc9 ymc9 changed the title feat(orm): support where enum fix(orm): cuid() generator should respect version argument Dec 29, 2025
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 support for versioned ID generators in the ORM's default value functions. The changes enable developers to specify versions for uuid() (v4 or v7) and cuid() (v1 or v2) functions, with defaults of v4 for uuid and v1 for cuid when no version is specified.

Key changes:

  • Added cuid v1 support via the cuid package (v3.0.0) alongside existing cuid2 support
  • Implemented validation for uuid versions (4 or 7), cuid versions (1 or 2), and nanoid lengths (positive numbers)
  • Updated tests to verify correct version handling for all ID generator functions

Reviewed changes

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

Show a summary per file
File Description
tests/e2e/package.json Added cuid package dependency for cuid v1 support
tests/e2e/orm/client-api/default-values.test.ts Refactored tests to verify version-specific ID generation for uuid, cuid, and nanoid
pnpm-lock.yaml Updated lockfile with cuid package and its metadata
packages/orm/src/client/crud/operations/base.ts Implemented version branching for cuid (v1/v2) and uuid (v4/v7) generators
packages/orm/package.json Added cuid package as a dependency
packages/language/test/function-invocation.test.ts Added comprehensive validation tests for uuid, cuid, and nanoid parameter constraints
packages/language/src/validators/function-invocation-validator.ts Implemented validators to enforce version constraints for uuid, cuid, and nanoid functions
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

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

@ymc9 ymc9 merged commit be330ff into dev Dec 29, 2025
11 checks passed
@ymc9 ymc9 deleted the fix/id-gen branch December 29, 2025 15:18
@sanny-io sanny-io mentioned this pull request Dec 30, 2025
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.

3 participants