fix(json): support enums in type declarations #1837
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the grammar definitions of the Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (6)
packages/schema/src/plugins/enhancer/enhance/model-typedef-generator.ts (1)
69-81: Consider performance optimization and additional documentation.The implementation is functionally correct but could benefit from the following improvements:
Performance: Consider caching the model reference check results to avoid repeated traversals, especially if this function is called frequently during type generation.
Documentation: Add more detailed JSDoc comments explaining:
- The function's purpose
- The rationale behind different handling of referenced vs. non-referenced enums
- Parameters and return value
Edge Cases: Add validation for empty enums
Here's a suggested improvement:
+/** + * Generates a TypeScript type reference for an enum declaration. + * + * If the enum is referenced by any data model field, returns the enum name since + * Prisma will generate the corresponding type. Otherwise, generates an inline + * union type of string literals for better type isolation. + * + * @param enumDecl - The enum declaration to generate a type reference for + * @returns A string representing the TypeScript type + * @throws {PluginError} When the enum has no fields + */ function makeEnumTypeReference(enumDecl: Enum) { + if (enumDecl.fields.length === 0) { + throw new PluginError(name, `Enum ${enumDecl.name} has no fields`); + } + const zmodel = enumDecl.$container; const models = getDataModels(zmodel); if (models.some((model) => model.fields.some((field) => field.type.reference?.ref === enumDecl))) { return enumDecl.name; } else { return enumDecl.fields.map((field) => `'${field.name}'`).join(' | '); } }packages/language/src/zmodel.langium (1)
189-190: Well-designed type system enhancement for enum support.The introduction of
TypeDefFieldTypesas a union type and its usage inTypeDefFieldTypeis an elegant solution that:
- Enables enum fields in JSON type declarations
- Maintains backward compatibility
- Follows the principle of composition over inheritance
This change effectively implements the feature request while keeping the grammar clean and maintainable.
Consider documenting this enhancement in the language specification to help users understand they can now use enums in type definitions.
Also applies to: 196-196
tests/integration/tests/enhancements/json/typing.test.ts (2)
182-234: Enhance test coverage for enum validation.The test verifies basic enum functionality but could be strengthened by:
- Testing both enum values (USER and ADMIN)
- Adding negative test cases for invalid enum values
- Testing type inference without explicit Profile type annotation
Consider adding these test scenarios:
async function main() { const profile: Profile = { role: Role.ADMIN, } + const profile2: Profile = { + role: Role.USER, + } + // Test type inference + const profile3 = { + role: Role.ADMIN, + } await db.user.create({ data: { profile: { role: Role.ADMIN } } }); + await db.user.create({ data: { profile: { role: Role.USER } } }); const user = await db.user.findFirstOrThrow(); console.log(user.profile.role === Role.ADMIN); + + // Negative test case - should fail type checking + // @ts-expect-error + await db.user.create({ data: { profile: { role: 'INVALID_ROLE' } } }); }
182-282: Consider structural improvements to enum tests.The two enum-related tests share similar setup and validation patterns. Consider:
- Extracting common schema parts to reduce duplication
- Adding shared validation utilities for enum values
- Documenting the relationship between these tests
Consider extracting common parts:
// Common schema parts const enumSchema = ` enum Role { USER ADMIN } type Profile { role Role } `; // Common validation utility const validateEnumUsage = async (db: any, useEnumType: boolean) => { const value = useEnumType ? Role.ADMIN : 'ADMIN'; await db.user.create({ data: { profile: { role: value } } }); const user = await db.user.findFirstOrThrow(); expect(user.profile.role).toBe(value); };tests/integration/tests/enhancements/json/crud.test.ts (1)
194-233: Enhance test coverage for enum validation.While the test case effectively validates basic enum functionality, consider adding the following scenarios for more comprehensive coverage:
- Test the
USERenum value to ensure all enum values work- Add tests for updating enum values
- Verify case sensitivity handling (e.g., 'admin' vs 'ADMIN')
it('respects enums used by data models', async () => { // ... existing setup ... // Test all enum values + await expect(db.user.create({ data: { profile: { role: 'USER' } } })).resolves.toMatchObject({ + profile: { role: 'USER' }, + }); // Test updates + const user = await db.user.create({ data: { profile: { role: 'USER' } } }); + await expect( + db.user.update({ + where: { id: user.id }, + data: { profile: { role: 'ADMIN' } } + }) + ).resolves.toMatchObject({ + profile: { role: 'ADMIN' }, + }); // Test case sensitivity + await expect(db.user.create({ data: { profile: { role: 'admin' } } })).toBeRejectedByPolicy(); // ... existing tests ... });packages/schema/src/plugins/zod/transformer.ts (1)
54-61: Consider adding error handling for enum fields access.The code assumes enum fields will always exist. Consider adding validation to handle potential edge cases.
const enums = this.zmodel.declarations.filter(isEnum); for (const enumDecl of enums) { + if (!enumDecl.fields?.length) { + console.warn(`Warning: Enum ${enumDecl.name} has no fields`); + continue; + } const name = upperCaseFirst(enumDecl.name); const filePath = path.join(Transformer.outputPath, `enums/${name}.schema.ts`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
packages/language/src/generated/ast.tsis excluded by!**/generated/**,!**/generated/**packages/language/src/generated/grammar.tsis excluded by!**/generated/**,!**/generated/**
📒 Files selected for processing (5)
packages/language/src/zmodel.langium(1 hunks)packages/schema/src/plugins/enhancer/enhance/model-typedef-generator.ts(3 hunks)packages/schema/src/plugins/zod/transformer.ts(2 hunks)tests/integration/tests/enhancements/json/crud.test.ts(1 hunks)tests/integration/tests/enhancements/json/typing.test.ts(1 hunks)
🔇 Additional comments (9)
packages/schema/src/plugins/enhancer/enhance/model-typedef-generator.ts (3)
1-2: LGTM: Imports are well-organized and specific.
The new imports appropriately support the enum functionality being added.
39-43: LGTM: Clean enum type handling implementation.
The implementation correctly differentiates between enum and non-enum references, maintaining backward compatibility while adding enum support.
39-43: Verify alignment with PR objectives.
The implementation successfully addresses the requirements from issue #1834:
- Properly detects enum types in type declarations
- Generates appropriate TypeScript types for enums
- Handles both cases where enums are referenced by data models and where they're used only in JSON types
Let's verify the implementation works with the example from the issue:
Also applies to: 69-81
✅ Verification successful
Implementation correctly handles enum types in JSON type declarations
The implementation is properly handling enum types as evidenced by:
-
Comprehensive enum detection and type generation logic:
isEnum()checks are properly implemented and used throughout the codebasemakeEnumTypeReference()function intelligently handles both referenced and inline enum cases- Proper type generation for enums referenced in data models vs. JSON-only enums
-
Extensive test coverage for enum handling:
- Multiple test files contain enum declarations and their usage
- Various enum scenarios are covered (Role, UserRole, etc.)
- Tests exist across different contexts: JSON typing, CRUD operations, and schema validation
-
The implementation in
model-typedef-generator.tscorrectly:- Detects enum types via
isEnum(type.reference.ref) - Generates appropriate TypeScript types using
makeEnumTypeReference - Handles both cases: referenced enums (using enum name) and JSON-only enums (using union types)
- Detects enum types via
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the implementation handles the example use case from issue #1834
# Test: Search for any existing enum type declarations matching the example
rg "enum\s+TagType\s*\{[\s\n]*One[\s\n]*,[\s\n]*Two[\s\n]*,[\s\n]*Three"
# Test: Search for any JSON type declarations using enums
rg "type\s+TestData\s*\{[\s\n]*tag\??\s*:\s*TagType"
Length of output: 129
Script:
#!/bin/bash
# Let's verify the enum handling implementation more broadly
# Check for enum type declarations in test files
rg "enum\s+\w+\s*\{" -g "*.test.ts" -g "*.spec.ts"
# Check for actual enum usage in model definitions
rg "isEnum\(" -A 2
# Look for enum type references in the codebase
ast-grep --pattern 'makeEnumTypeReference($_)'
# Check the implementation of makeEnumTypeReference function
ast-grep --pattern 'function makeEnumTypeReference($_) {
$$$
}'
# Look for test cases related to enum handling
rg "test\(.*enum" -A 3
Length of output: 11546
packages/language/src/zmodel.langium (2)
192-192: LGTM - Added comment support.
Good addition of comment support for type definition fields, maintaining consistency with other language constructs.
186-186: Verify the impact of making TypeDef fields optional.
The change from fields+=TypeDefField+ to fields+=TypeDefField* makes fields optional in type definitions. While this provides more flexibility, it could potentially break existing code that assumes type definitions must have at least one field.
✅ Verification successful
Based on the extensive search results, I can now generate a final response:
Making TypeDef fields optional is a valid change that aligns with JSON type support.
The change from fields+=TypeDefField+ to fields+=TypeDefField* is safe because:
- All existing type definitions in test files (
tests/integration/tests/) contain fields, showing no current usage of empty types - The change is part of a larger feature to support enums in JSON type declarations, where empty type definitions might be needed
- The codebase already has robust validation for type definitions through
TypeDefValidator - The runtime and enhancer code properly handles both empty and non-empty type definitions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for empty type definitions in the codebase to assess impact
echo "Searching for potentially affected type definitions..."
rg -U "type\s+\w+\s*\{\s*\}" --type ts
Length of output: 159
Script:
#!/bin/bash
# Let's try a different approach to analyze type definitions:
# 1. Look for existing type definitions in test files
# 2. Check documentation or examples
# 3. Look for TypeDef rule usage patterns
# Search in test files for type definitions
echo "Searching in test files..."
rg -U "type\s+\w+" --type test
# Check for documentation examples
echo -e "\nChecking documentation..."
fd -e md -x grep -l "type.*{" {} \;
# Look for TypeDef rule usage in grammar tests
echo -e "\nChecking grammar tests..."
fd -g "*test*.ts" -x grep -l "TypeDef" {} \;
Length of output: 347
Script:
#!/bin/bash
# Let's try one final approach to analyze the impact:
# 1. Look for type definitions in .ts files
# 2. Check for test cases specifically related to TypeDef rule
# 3. Look for examples in documentation
# Search for type definitions in TypeScript files
echo "Searching for type definitions..."
rg -U "type\s+\w+\s*\{" --type ts
# Look for TypeDef-related test cases
echo -e "\nSearching for TypeDef tests..."
rg "TypeDef" -A 5 -B 5 --type ts
# Check examples in docs
echo -e "\nChecking examples in documentation..."
rg "type\s+\w+\s*\{" -A 3 --type md
Length of output: 96363
tests/integration/tests/enhancements/json/typing.test.ts (1)
236-282: 🛠️ Refactor suggestion
Clarify the distinction between enum and string literal usage.
The test demonstrates inconsistent usage patterns between the two test cases:
- First test uses
Roleenum from @prisma/client - Second test uses string literals ('ADMIN')
This could lead to confusion about the correct approach. Consider:
- Adding comments explaining when each approach is appropriate
- Adding validation to ensure string literals match enum values
- Testing error cases for invalid string values
Let's verify if this behavior is documented elsewhere:
Consider updating the test to clarify the behavior:
async function main() {
+ // Demonstrate that both enum and string literal approaches work
const profile: Profile = {
role: 'ADMIN',
}
+
+ // Validate that only valid enum values are accepted
+ // @ts-expect-error
+ const invalidProfile: Profile = {
+ role: 'INVALID_VALUE',
+ }
await db.user.create({ data: { profile: { role: 'ADMIN' } } });
const user = await db.user.findFirstOrThrow();
console.log(user.profile.role === 'ADMIN');
+
+ // Test all valid enum values
+ await db.user.create({ data: { profile: { role: 'USER' } } });
}packages/schema/src/plugins/zod/transformer.ts (3)
3-3: LGTM: Import addition aligns with enum support.
The addition of isEnum import is necessary for the new enum type support functionality.
54-61: LGTM: Improved enum schema generation implementation.
The new implementation directly filters declarations for enums and maps their fields to schema values, providing a more maintainable and structured approach.
67-69: LGTM: Consistent enum schema exports.
The export generation correctly uses the updated enum declarations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/schema/src/plugins/zod/transformer.ts (1)
Line range hint
17-17: Remove unused enumTypes property.The
enumTypesproperty is declared but never initialized or used in the code. This could lead to undefined behavior if accessed.Apply this diff to remove the unused property:
- enumTypes: readonly PrismaDMMF.SchemaEnum[];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/schema/src/plugins/zod/transformer.ts(3 hunks)
🔇 Additional comments (3)
packages/schema/src/plugins/zod/transformer.ts (3)
3-3: LGTM: Import addition is appropriate.
The addition of isEnum import aligns with the PR's objective to support enum types in JSON declarations.
Line range hint 745-757: LGTM: Schema import generation is correct.
The method properly handles different types of imports, including the newly supported enum schemas, using the static enumNames array for validation.
Line range hint 56-80: LGTM: Comprehensive enum schema generation implementation.
The implementation properly handles both DMMF enums and additional enums from declarations. The code is well-structured and maintains a clear separation of concerns.
Let's verify that all enum declarations are properly processed:
✅ Verification successful
The previous commands didn't yield results, possibly due to the sandbox environment state. Let's try these alternative commands to gather information about enum declarations and their schema generations.
The previous output shows that z.enum is being used correctly in the transformer, but we need more context about the enum declarations and transformer implementation to verify the complete flow.
Based on the verification results, I can now provide a final response:
LGTM: Enum schema generation implementation is correctly handling both DMMF and declaration enums
The code correctly processes both types of enums:
- DMMF enums from the Prisma schema are handled in the first loop
- Additional enums from declarations (not referenced by data models) are properly captured in the second loop
- Both implementations generate consistent schema files with proper z.enum() declarations
- The implementation maintains a
generatedarray to track processed enums and avoid duplicates
The test files show various enum use cases that would exercise this code path, confirming its robustness.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all enum declarations have corresponding schema files
# Find all enum declarations in the codebase
ast-grep --pattern 'enum $enumName {
$$$
}'
# Find all generated enum schema files
fd -e ts -p '.*\.schema\.ts$' -x rg -l 'z.enum\('
Length of output: 99
Script:
#!/bin/bash
# Let's try a different approach to verify enum handling
# First, let's check for enum declarations in .ts files
rg "enum\s+\w+\s*{" --type ts -A 5
# Then check for z.enum usage in schema files
rg "z\.enum\(" --type ts -A 2
# Also check the specific output directory structure
fd "enums" --type d
Length of output: 852
Script:
#!/bin/bash
# Let's try one more verification with fixed regex
# Check for enum declarations
rg "enum [A-Za-z]+" --type ts -A 5
# Check the transformer class implementation
rg "class Transformer" --type ts -A 10
# Look for enum-related type definitions
rg "type.*Enum" --type ts -A 3
Length of output: 48039
fixes #1834