-
Notifications
You must be signed in to change notification settings - Fork 11
fix: literal function arguments with @default()
#288
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: literal function arguments with @default()
#288
Conversation
WalkthroughUpdates the TypeScript schema generator to wrap default function-call arguments with ExpressionUtils.literal and adds tests validating schema generation for default uuid(7) calls, including id fields, @id, @default with call args, and model metadata. Duplicate test cases verify the same behavior in the test file. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/cli/test/ts-schema-gen.test.ts (1)
364-424
: Test correctly validates the fix but consider expanding coverage.The test properly validates that literal arguments passed to default functions (e.g.,
uuid(7)
) are correctly wrapped as expression literals in the generated schema, which addresses the PR objective.However:
- The PR description mentions both
uuid()
andnanoid()
functions, but onlyuuid()
is tested.- Consider adding test cases for other scenarios: multiple arguments (e.g.,
uuid(7, 8)
), string literals (e.g.,uuid('v4')
), and thenanoid()
function.- The AI summary mentions "duplicate test cases," but only one test is visible in the annotated code. If there's a duplicate test elsewhere in the file, consider consolidating them.
To enhance test coverage, consider adding:
it('generates correct default literal function arguments for various functions', async () => { const { schema } = await generateTsSchema(` model User { id String @id @default(nanoid(10)) uuid String @default(uuid(7)) } `); expect(schema.models.User.fields.id.default).toMatchObject({ kind: "call", function: "nanoid", args: [{ kind: "literal", value: 10 }] }); expect(schema.models.User.fields.uuid.default).toMatchObject({ kind: "call", function: "uuid", args: [{ kind: "literal", value: 7 }] }); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli/test/ts-schema-gen.test.ts
(1 hunks)packages/sdk/src/ts-schema-generator.ts
(1 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/sdk/src/ts-schema-generator.ts
packages/cli/test/ts-schema-gen.test.ts
🧬 Code graph analysis (1)
packages/cli/test/ts-schema-gen.test.ts (2)
packages/testtools/src/schema.ts (1)
generateTsSchema
(35-65)samples/blog/zenstack/schema.ts (1)
schema
(9-230)
🔇 Additional comments (1)
packages/sdk/src/ts-schema-generator.ts (1)
510-512
: LGTM! The fix correctly resolves the TypeScript error.The change properly wraps each default function argument in
ExpressionUtils.literal
, ensuring type compatibility with the Expression type. This is consistent with how literal expressions are handled elsewhere in the codebase (e.g.,createLiteralExpression
at lines 1099-1108).The fix correctly handles the flow where:
getMappedValue
(line 626) extracts literal values from function arguments viagetLiteral
- Those primitive values (string|number|boolean) are converted to TypeScript AST nodes via
createLiteralNode
- Each node is now wrapped in
ExpressionUtils.literal
to match the Expression type signatureThis enables default function calls like
@default(uuid(7))
to compile successfully.
@default()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making the fix @sanny-io ! Looks great to me.
v3 produces an error when generating the .ts schema with literals passed to functions like
uuid()
andnanoid()
error TS2322: Type 'number' is not assignable to type 'Expression'.
This PR fixes that.
Summary by CodeRabbit