Skip to content

Conversation

@ymc9
Copy link
Member

@ymc9 ymc9 commented Sep 30, 2025

fixes #274

Summary by CodeRabbit

  • New Features
    • Test project utility accepts optional inline schema content for quicker setup.
  • Bug Fixes
    • Improved handling of auth-based default values to ensure accurate schema generation and prevent unintended defaults.
  • Tests
    • Added a regression test covering auth-derived default values in relations (issue 274).
    • Simplified an existing regression test structure for clarity and maintainability.

Copilot AI review requested due to automatic review settings September 30, 2025 23:36
@coderabbitai
Copy link

coderabbitai bot commented Sep 30, 2025

Warning

Rate limit exceeded

@ymc9 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 0 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between efa1768 and e29c7e1.

📒 Files selected for processing (2)
  • packages/sdk/src/prisma/prisma-schema-generator.ts (4 hunks)
  • tests/regression/test/issue-274/regression.test.ts (1 hunks)

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

Reworks Prisma schema generation to detect and exclude auth() default invocations using new isExpression/isAuthInvocation guards. Renames a helper accordingly. Extends test utilities to optionally write schema.zmodel. Updates an existing regression test structure and adds a new regression test for issue #274 covering auth().id default with relations and migrate.

Changes

Cohort / File(s) Summary
Prisma schema generator auth-invocation handling
packages/sdk/src/prisma/prisma-schema-generator.ts
Replaces isInvocationExpr checks with isExpression/isAuthInvocation; renames helper to isDefaultWithAuthInvocation; updates default filtering to exclude auth-based defaults in field and attribute processing.
Test project utility enhancement
packages/testtools/src/project.ts
Updates createTestProject signature to accept optional zmodelContent and conditionally writes schema.zmodel into the work directory.
Regression tests adjustments
tests/regression/test/issue-204/regression.test.ts, tests/regression/test/issue-274/regression.test.ts
Simplifies issue-204 test by removing describe wrapper. Adds new issue-274 test exercising auth().id default with relation and running migrate dev.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • docs: add README #27 — Also touches prisma-schema-generator.ts default-handling, aligning logic around filtering defaults; likely adjacent to the new isAuthInvocation-based approach.

Poem

A nibble of logic, a hop through the schema,
I sniff out auth() in Prisma’s arena.
Tests sprout like clover, green and bright,
Migrate now hums through the moonlit night.
With a twitch of whiskers and ears held high,
Defaults behave—no MemberAccess sigh! 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Out of Scope Changes Check ⚠️ Warning While the core implementation updates and the new regression test for issue #274 align with handling the auth invocation in default expressions, the removal of the describe wrapper and refactoring of the existing regression test for issue #204 are unrelated to fixing the unsupported MemberAccessExpr handling and constitute out-of-scope changes. The refactoring of the issue-204 regression test should be extracted into a separate pull request so that this PR remains focused solely on supporting the @default(auth().id) scenario as specified by issue #274.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The provided title succinctly describes the main change—fixing Prisma schema generation when using @default(auth())—and aligns directly with the bug in issue #274 without extraneous detail.
Linked Issues Check ✅ Passed The pull request introduces the isAuthInvocation type guard and updates the Prisma schema generator to recognize and exclude @default(auth().…) expressions, which directly addresses the unsupported MemberAccessExpr error described in issue #274, and it includes a regression test specifically verifying that default(auth().id) no longer fails.

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 Sep 30, 2025

Claude finished @ymc9's task —— View job


Code Review for PR #278

Reviewing the fix for Prisma schema generation issue with default() using auth().

Analysis Tasks:


Summary

This PR fixes an issue where Prisma schema generation was incorrectly filtering out @default() attributes that use auth() function calls. The fix refines the filtering logic to specifically target only auth() invocations rather than all plugin invocations.

Code Quality Assessment

Strengths

  1. Clear Problem Focus: The fix is precisely targeted - changing from generic "plugin invocation" detection to specific auth() invocation detection
  2. Proper Function Reuse: Uses the existing isAuthInvocation utility from @zenstackhq/language/utils instead of duplicating logic
  3. Improved Type Safety: Adds isExpression check before calling isAuthInvocation in the AST traversal
  4. Good Test Coverage: Includes a regression test that reproduces the exact issue scenario
  5. Clean Refactoring: Renames method from isDefaultWithPluginInvocation to isDefaultWithAuthInvocation for clarity

Implementation Details

Core Fix (packages/sdk/src/prisma/prisma-schema-generator.ts:264-275):

// Before: Generic plugin detection
return AstUtils.streamAst(expr).some((node) => 
  isInvocationExpr(node) && this.isFromPlugin(node.function.ref)
);

// After: Specific auth() detection  
return AstUtils.streamAst(expr).some((node) => 
  isExpression(node) && isAuthInvocation(node)
);

Test Enhancement (packages/testtools/src/project.ts:5):

  • Makes zmodelContent parameter optional to allow custom schema content in tests
  • Simple, backward-compatible change

Regression Test (tests/regression/test/issue-274/regression.test.ts):

  • Tests the exact scenario: @default(auth().id) in a foreign key field
  • Verifies the fix by running zenstack migrate dev, which would fail without the fix

Security & Performance

Security: No concerns identified

  • The fix actually improves security by being more specific about which invocations to filter
  • No exposure of sensitive data or authentication bypass risks

Performance: Neutral to positive impact

  • Uses existing utility function rather than duplicating logic
  • AST traversal complexity remains the same
  • Specific function check is more efficient than generic plugin detection

Potential Issues & Recommendations

⚠️ Minor Concerns

  1. Dead Code: The isFromPlugin method (lines 277-280) appears to be unused after this change. Consider removing it in a follow-up cleanup.

  2. Test Isolation: The regression test changes process.chdir(dir) globally, which could affect other tests. Consider using a more isolated approach or ensuring proper cleanup.

  3. Test Consistency: The change to issue-204 test (removing describe wrapper) seems unrelated to this fix. While not problematic, it's slightly out of scope.

💡 Suggestions

  1. Documentation: Consider adding a code comment explaining why @default with auth() needs special handling in Prisma schema generation.

  2. Edge Cases: The fix correctly handles nested expressions through AstUtils.streamAst, but consider adding test cases for more complex scenarios like @default(someFunction(auth().id)).

Test Coverage

Regression Test Quality

  • Realistic Scenario: Uses a practical Comment/User relationship model
  • End-to-End Validation: Tests the full CLI migration flow, not just unit logic
  • Specific Issue Reproduction: The @default(auth().id) case directly addresses the reported problem

Overall Assessment

Excellent Fix

  • Focused Solution: Addresses the specific issue without over-engineering
  • Maintains Compatibility: No breaking changes to existing functionality
  • Well Tested: Includes appropriate regression test coverage
  • Code Quality: Clean, readable implementation with proper separation of concerns

Recommendation: ✅ Approve with minor cleanup suggestions

The fix correctly solves the reported issue and follows good engineering practices. The minor suggestions above are for code hygiene but don't block approval.

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 fixes a Prisma schema generation issue where @default() attributes using auth() functions were not being properly handled. The fix ensures that default values using authentication context are excluded from Prisma schema generation since they need to be handled at runtime rather than by Prisma.

  • Replaced plugin-based detection with specific auth() function detection
  • Added regression test to verify the fix works correctly
  • Enhanced test utilities to support schema content injection

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
tests/regression/test/issue-274/regression.test.ts Adds regression test for auth() default value handling
tests/regression/test/issue-204/regression.test.ts Removes unnecessary describe wrapper from existing test
packages/testtools/src/project.ts Enhances createTestProject to accept optional schema content
packages/sdk/src/prisma/prisma-schema-generator.ts Fixes schema generation to properly handle auth() in default values

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@claude
Copy link

claude bot commented Sep 30, 2025

Claude finished @ymc9's task —— View job


Code Review for PR #278

Reviewing the fix for Prisma schema generation issue with default() using auth().

Analysis Tasks:


Summary

This PR fixes an issue where Prisma schema generation was incorrectly filtering out @default() attributes that use auth() function calls. The fix refines the filtering logic to specifically target only auth() invocations rather than all plugin invocations.

Code Quality Assessment

Strengths

  1. Clear Problem Focus: The fix is precisely targeted - changing from generic "plugin invocation" detection to specific auth() invocation detection
  2. Proper Function Reuse: Uses the existing isAuthInvocation utility from @zenstackhq/language/utils instead of duplicating logic
  3. Improved Type Safety: Adds isExpression check before calling isAuthInvocation in the AST traversal
  4. Good Test Coverage: Includes a regression test that reproduces the exact issue scenario
  5. Clean Refactoring: Renames method from isDefaultWithPluginInvocation to isDefaultWithAuthInvocation for clarity

Implementation Details

Core Fix (packages/sdk/src/prisma/prisma-schema-generator.ts:264-275):

// Before: Generic plugin detection
return AstUtils.streamAst(expr).some((node) => 
  isInvocationExpr(node) && this.isFromPlugin(node.function.ref)
);

// After: Specific auth() detection  
return AstUtils.streamAst(expr).some((node) => 
  isExpression(node) && isAuthInvocation(node)
);

Test Enhancement (packages/testtools/src/project.ts:5):

  • Makes zmodelContent parameter optional to allow custom schema content in tests
  • Simple, backward-compatible change

Regression Test (tests/regression/test/issue-274/regression.test.ts):

  • Tests the exact scenario: @default(auth().id) in a foreign key field
  • Verifies the fix by running zenstack migrate dev, which would fail without the fix

Security & Performance

Security: No concerns identified

  • The fix actually improves security by being more specific about which invocations to filter
  • No exposure of sensitive data or authentication bypass risks

Performance: Neutral to positive impact

  • Uses existing utility function rather than duplicating logic
  • AST traversal complexity remains the same
  • Specific function check is more efficient than generic plugin detection

Potential Issues & Recommendations

⚠️ Minor Concerns

  1. Dead Code: The isFromPlugin method (lines 277-280) appears to be unused after this change. Consider removing it in a follow-up cleanup.

  2. Test Isolation: The regression test changes process.chdir(dir) globally, which could affect other tests. Consider using a more isolated approach or ensuring proper cleanup.

  3. Test Consistency: The change to issue-204 test (removing describe wrapper) seems unrelated to this fix. While not problematic, it's slightly out of scope.

💡 Suggestions

  1. Documentation: Consider adding a code comment explaining why @default with auth() needs special handling in Prisma schema generation.

  2. Edge Cases: The fix correctly handles nested expressions through AstUtils.streamAst, but consider adding test cases for more complex scenarios like @default(someFunction(auth().id)).

Test Coverage

Regression Test Quality

  • Realistic Scenario: Uses a practical Comment/User relationship model
  • End-to-End Validation: Tests the full CLI migration flow, not just unit logic
  • Specific Issue Reproduction: The @default(auth().id) case directly addresses the reported problem

Overall Assessment

Excellent Fix

  • Focused Solution: Addresses the specific issue without over-engineering
  • Maintains Compatibility: No breaking changes to existing functionality
  • Well Tested: Includes appropriate regression test coverage
  • Code Quality: Clean, readable implementation with proper separation of concerns

Recommendation: ✅ Approve with minor cleanup suggestions

The fix correctly solves the reported issue and follows good engineering practices. The minor suggestions above are for code hygiene but don't block approval.


Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 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 b6e16ff and efa1768.

📒 Files selected for processing (4)
  • packages/sdk/src/prisma/prisma-schema-generator.ts (5 hunks)
  • packages/testtools/src/project.ts (2 hunks)
  • tests/regression/test/issue-204/regression.test.ts (1 hunks)
  • tests/regression/test/issue-274/regression.test.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**

📄 CodeRabbit inference engine (CLAUDE.md)

Place packages only under packages/, samples/, or tests/

Files:

  • packages/testtools/src/project.ts
  • tests/regression/test/issue-274/regression.test.ts
  • packages/sdk/src/prisma/prisma-schema-generator.ts
  • tests/regression/test/issue-204/regression.test.ts
🧬 Code graph analysis (3)
tests/regression/test/issue-274/regression.test.ts (1)
packages/testtools/src/project.ts (1)
  • createTestProject (5-71)
packages/sdk/src/prisma/prisma-schema-generator.ts (3)
packages/language/src/generated/ast.ts (3)
  • DataFieldAttribute (348-353)
  • DataFieldAttribute (355-355)
  • isExpression (119-121)
packages/language/src/utils.ts (1)
  • isAuthInvocation (64-66)
packages/sdk/src/ts-schema-generator.ts (1)
  • isAuthInvocation (651-655)
tests/regression/test/issue-204/regression.test.ts (1)
tests/regression/test/issue-204/models.ts (3)
  • Configuration (11-11)
  • ShirtColor (12-12)
  • ShirtColor (13-13)
⏰ 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)

@claude
Copy link

claude bot commented Sep 30, 2025

Claude finished @ymc9's task —— View job


Code Review for PR #278

Reviewing the fix for Prisma schema generation issue with default() using auth().

Analysis Tasks:


Summary

This PR fixes an issue where Prisma schema generation was incorrectly filtering out @default() attributes that use auth() function calls. The fix refines the filtering logic to specifically target only auth() invocations rather than all plugin invocations.

Code Quality Assessment

Strengths

  1. Clear Problem Focus: The fix is precisely targeted - changing from generic "plugin invocation" detection to specific auth() invocation detection
  2. Proper Function Reuse: Uses the existing isAuthInvocation utility from @zenstackhq/language/utils instead of duplicating logic
  3. Improved Type Safety: Addresses reviewer feedback by removing redundant isExpression check (commit aea1acb)
  4. Good Test Coverage: Includes a regression test that reproduces the exact issue scenario
  5. Clean Refactoring: Renames method from isDefaultWithPluginInvocation to isDefaultWithAuthInvocation for clarity
  6. Test Fix Applied: The process.chdir issue raised by reviewers was already fixed in commit e29c7e1

Implementation Details

Core Fix (packages/sdk/src/prisma/prisma-schema-generator.ts:272):

// Before: Generic plugin detection
return AstUtils.streamAst(expr).some((node) => 
  isInvocationExpr(node) && this.isFromPlugin(node.function.ref)
);

// After: Specific auth() detection  
return AstUtils.streamAst(expr).some(isAuthInvocation);

Test Enhancement (packages/testtools/src/project.ts:5):

  • Makes zmodelContent parameter optional to allow custom schema content in tests
  • Simple, backward-compatible change

Regression Test (tests/regression/test/issue-274/regression.test.ts):

  • Tests the exact scenario: @default(auth().id) in a foreign key field
  • Verifies the fix by running zenstack migrate dev, which would fail without the fix
  • Fixed: Now uses { cwd: dir } instead of process.chdir(dir) to avoid global state mutation

Security & Performance

Security: No concerns identified

  • The fix actually improves security by being more specific about which invocations to filter
  • No exposure of sensitive data or authentication bypass risks

Performance: Neutral to positive impact

  • Uses existing utility function rather than duplicating logic
  • AST traversal complexity remains the same
  • Specific function check is more efficient than generic plugin detection

Potential Issues & Recommendations

⚠️ Minor Concerns

  1. Dead Code: The isFromPlugin method appears to be unused after this change. Consider removing it in a follow-up cleanup.

  2. Test Consistency: The change to issue-204 test (removing describe wrapper) seems unrelated to this fix. While not problematic, it's slightly out of scope.

💡 Suggestions

  1. Documentation: Consider adding a code comment explaining why @default with auth() needs special handling in Prisma schema generation.

  2. Edge Cases: The fix correctly handles nested expressions through AstUtils.streamAst, but consider adding test cases for more complex scenarios like @default(someFunction(auth().id)).

Test Coverage

Regression Test Quality

  • Realistic Scenario: Uses a practical Comment/User relationship model
  • End-to-End Validation: Tests the full CLI migration flow, not just unit logic
  • Specific Issue Reproduction: The @default(auth().id) case directly addresses the reported problem

Overall Assessment

Excellent Fix

  • Focused Solution: Addresses the specific issue without over-engineering
  • Maintains Compatibility: No breaking changes to existing functionality
  • Well Tested: Includes appropriate regression test coverage
  • Code Quality: Clean, readable implementation with proper separation of concerns
  • Responsive to Feedback: Addressed reviewer concerns promptly

Recommendation: ✅ Approve with minor cleanup suggestions

The fix correctly solves the reported issue and follows good engineering practices. The minor suggestions above are for code hygiene but don't block approval.


@ymc9 ymc9 merged commit 546ba8e into dev Oct 1, 2025
5 checks passed
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