fix(zmodel): improve attribute argument assignability check#588
Conversation
📝 WalkthroughWalkthroughValidator return types were changed to a discriminated union with detailed error messages; JSON/@default validation was hardened. A regression test for JSON and JSON[] defaults was added. SDK default-mapping now explicitly handles null defaults and updates related signatures. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
Claude finished @ymc9's task —— View job Code Review Complete ✅Reviewed PR that fixes issue #584 - improving attribute argument assignability check. Todo List:
Code Quality & Best Practices ✅Excellent improvements overall! The refactor significantly enhances error reporting and maintainability:
JSON Default Value Validation 🎯The core improvement correctly addresses JSON field default validation: Before ( const dstIsTypedJson = hasAttribute(attr.$container, '@json');
if (dstIsTypedJson && attr.decl.ref?.name === '@default') {
return argResolvedType.decl === 'String';
}After ( const dstIsJson = attr.$container.type.type === 'Json' || hasAttribute(attr.$container, '@json');
if (dstIsJson && attr.decl.ref?.name === '@default') {
if (isLiteralJsonString(arg.value)) {
return success;
} else {
return {
result: false,
error: 'expected a JSON string literal',
};
}
}✅ Key Improvements:
Test Coverage ✅The test suite in
Minor Suggestions
error: `
expected ${dstIsArray ? 'array' : 'non-array'}`,Should be: error: `expected ${dstIsArray ? 'array' : 'non-array'}`,
Security & Performance ✅
Overall Assessment 🎉This is a high-quality fix that:
Recommendation: Approve with minor formatting fix suggestion. |
There was a problem hiding this comment.
Pull request overview
This PR improves attribute argument validation for ZModel schema files by enhancing error messages and adding specific validation for JSON default values. The changes address issue #584 where JSON field default values were not properly validated.
Changes:
- Enhanced
assignableToAttributeParamfunction to return descriptive error messages instead of just boolean values - Added
isLiteralJsonStringfunction to validate that JSON field default values are properly formatted JSON string literals - Added comprehensive test coverage for JSON default value validation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/regression/test/issue-584.test.ts | New test file validating that JSON default values must be valid JSON string literals, covering edge cases like null, plain strings, and valid JSON |
| packages/language/src/validators/attribute-application-validator.ts | Refactored validation logic to return structured error objects with descriptive messages; added JSON string literal validation for JSON/typed JSON fields |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/language/src/validators/attribute-application-validator.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/language/src/validators/attribute-application-validator.ts:
- Around line 453-455: The error string for the validation result (the object
with the error property in attribute-application-validator.ts) contains a
leading newline/whitespace because it uses a multi-line template literal; update
the error value to be a single-line string without leading whitespace (e.g.,
remove the leading newline/indentation from the template literal) so the message
becomes "expected array" or "expected non-array" based on dstIsArray.
🧹 Nitpick comments (1)
tests/regression/test/issue-584.test.ts (1)
1-66: LGTM! Comprehensive regression test for issue #584.The test suite effectively validates the JSON default value validation logic:
- Correctly rejects non-JSON-string-literal defaults (
null,'hello')- Correctly accepts valid JSON string literals (
'"hello"','[{"hello":1}]','null')The test coverage aligns well with the fix in the validator.
💡 Optional: Consider additional edge cases
While the current test coverage is sufficient, you could optionally add tests for:
- Empty JSON objects:
@default('{}')- Non-nullable JSON fields:
data Json @default('null')- Complex nested structures
- JSON with special characters/unicode
These would provide even more comprehensive coverage but are not essential for this regression fix.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/language/src/validators/attribute-application-validator.tstests/regression/test/issue-584.test.ts
🧰 Additional context used
🧠 Learnings (4)
📚 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:
tests/regression/test/issue-584.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 tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests
Applied to files:
tests/regression/test/issue-584.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/regression/test/issue-584.test.ts
📚 Learning: 2025-12-30T15:07:06.254Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:06.254Z
Learning: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
Applied to files:
packages/language/src/validators/attribute-application-validator.ts
🧬 Code graph analysis (1)
tests/regression/test/issue-584.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(101-248)
⏰ 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: CodeQL analysis (javascript-typescript)
- GitHub Check: build-test (22.x, postgresql)
- GitHub Check: build-test (22.x, sqlite)
🔇 Additional comments (4)
packages/language/src/validators/attribute-application-validator.ts (4)
106-110: LGTM! Improved error reporting.The refactor to use structured results enables precise error messages for each validation failure, improving developer experience.
395-405: LGTM! Well-structured discriminated union.The function signature change provides better type safety and enables detailed error reporting. The constants for success and error cases improve maintainability.
418-430: LGTM! Core fix for issue #584.The JSON field validation correctly enforces Prisma's design requirement that
@defaultvalues for JSON fields must be JSON string literals (e.g.,'null','"hello"','[1,2,3]') rather than raw values. The error message is clear and actionable.
607-617: LGTM! Well-implemented JSON validation helper.The function correctly validates JSON string literals by attempting to parse them. Since this runs only during schema validation (not at runtime), the performance characteristics of JSON.parse are acceptable.
packages/language/src/validators/attribute-application-validator.ts
Outdated
Show resolved
Hide resolved
|
Claude finished @ymc9's task —— View job Code Review Complete ✅Reviewed PR that fixes issue #584 - improving attribute argument assignability check. Todo List:
Code Quality & Best Practices ✅Excellent improvements overall! The refactor significantly enhances error reporting and maintainability:
JSON Default Value Validation 🎯The core improvement correctly addresses JSON field default validation: Before ( const dstIsTypedJson = hasAttribute(attr.$container, '@json');
if (dstIsTypedJson && attr.decl.ref?.name === '@default') {
return argResolvedType.decl === 'String';
}After ( const dstIsJson = attr.$container.type.type === 'Json' || hasAttribute(attr.$container, '@json');
if (dstIsJson && attr.decl.ref?.name === '@default') {
if (isLiteralJsonString(arg.value)) {
return success;
} else {
return {
result: false,
error: 'expected a JSON string literal',
};
}
}✅ Key Improvements:
Test Coverage ✅The test suite in
Minor Suggestions
error: `
expected ${dstIsArray ? 'array' : 'non-array'}`,Should be: error: `expected ${dstIsArray ? 'array' : 'non-array'}`,
Security & Performance ✅
Overall Assessment 🎉This is a high-quality fix that:
Recommendation: Approve with minor formatting fix suggestion. |
|
Claude finished @ymc9's task —— View job I'll analyze this PR and provide comprehensive feedback on code quality, potential issues, performance, security, and test coverage. Todo List:
Code Review Complete ✅Reviewed PR that fixes issue #584 - improving attribute argument assignability check. Todo List:
Code Quality & Best Practices ✅Excellent improvements overall! The refactor significantly enhances error reporting and maintainability:
JSON Default Value Validation 🎯The core improvement correctly addresses JSON field default validation: Before ( const dstIsTypedJson = hasAttribute(attr.$container, '@json');
if (dstIsTypedJson && attr.decl.ref?.name === '@default') {
return argResolvedType.decl === 'String';
}After ( const dstIsJson = attr.$container.type.type === 'Json' || hasAttribute(attr.$container, '@json');
if (dstIsJson && attr.decl.ref?.name === '@default') {
if (isLiteralJsonString(arg.value)) {
return success;
} else {
return {
result: false,
error: 'expected a JSON string literal',
};
}
}✅ Key Improvements:
Test Coverage ✅The test suite in
Minor Suggestions
error: `
expected ${dstIsArray ? 'array' : 'non-array'}`,Should be: error: `expected ${dstIsArray ? 'array' : 'non-array'}`,
Security & Performance ✅
Overall Assessment 🎉This is a high-quality fix that:
Recommendation: Approve with minor formatting fix suggestion. |
|
Claude finished @ymc9's task —— View job Code Review for PR #588
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/language/src/validators/attribute-application-validator.ts (2)
472-490: Simplify the array-of-field-references check for readability.
The double-negative!some(!...)is correct but harder to scan.Proposed diff
- if ( - isArrayExpr(arg.value) && - !arg.value.items.some((item) => !isReferenceExpr(item) || !isDataField(item.target.ref)) - ) { + if ( + isArrayExpr(arg.value) && + arg.value.items.every((item) => isReferenceExpr(item) && isDataField(item.target.ref)) + ) { return success; } else { return { result: false, error: 'expected an array of field references' }; }
619-629:isLiteralJsonStringis a good focused helper; consider a clearer error phrase.
Right now failures show “expected a JSON string literal”, but the actual requirement is “a string literal containing valid JSON” (important for JSON string values, which require escaped quotes).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/language/src/validators/attribute-application-validator.tspackages/sdk/src/ts-schema-generator.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-30T15:07:06.254Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:06.254Z
Learning: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
Applied to files:
packages/language/src/validators/attribute-application-validator.tspackages/sdk/src/ts-schema-generator.ts
🧬 Code graph analysis (1)
packages/sdk/src/ts-schema-generator.ts (1)
packages/language/src/generated/ast.ts (1)
isNullExpr(593-595)
⏰ 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 (22.x, postgresql)
- GitHub Check: build-test (22.x, sqlite)
🔇 Additional comments (7)
packages/sdk/src/ts-schema-generator.ts (3)
608-666: LGTM! Null default value handling implemented correctly.The explicit null check on line 610 correctly precedes the object check on line 614. This ordering is essential because
typeof null === 'object'in JavaScript, so checking for null first prevents it from being mishandled as an object. The generatedExpressionUtils._null()call is consistent with the pattern used increateNullExpression()(line 1255).This properly addresses the PR objective of handling nullable JSON fields with
@default(null).
729-747: LGTM! Return type correctly updated to include null.The expanded return type properly distinguishes between:
undefined: No@defaultattribute presentnull: Explicit@default(null)value- Other types: Actual default values
This enhances type safety and aligns with the new null handling logic.
749-778: LGTM! Null expression handling and error message improved.The explicit handling of
isNullExpr(lines 773-774) correctly returnsnull, which propagates through the mapping flow. The updated error message on line 776 is an improvement—it's more generic and doesn't assume this method is only used for default values, making it more maintainable as the codebase evolves.The return type update to include
| nullproperly reflects the new capability.packages/language/src/validators/attribute-application-validator.ts (4)
25-27: ImportingisStringLiteralis a clean way to type-guard JSON-default literals.
107-111: Nice: propagate the precise type-mismatch error to the user and short-circuit.
This should make schema diagnostics materially better than the prior boolean result.
396-414: Discriminated union return forassignableToAttributeParamis a solid API upgrade.
The{ result: false; error: string }shape is easy to extend without widening call-site logic.
416-446: The validator code is correct and the concern is based on a misunderstanding.Issue #584 test explicitly validates that
@default(null)should be rejected and only@default('null')(a JSON string literal containing null) should be accepted. This is the intended design—JSON default values must be strings containing valid JSON, following Prisma's convention.The test cases confirm:
@default(null)→ REJECTED with "expected a JSON string literal" ✓ (correct)@default('null')→ ACCEPTED ✓ (correct)@default('"hello"')→ ACCEPTED ✓ (correct)The code is working as designed. No changes needed.
Likely an incorrect or invalid review comment.
1 participant
fixes #584
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.