merge dev to main (v2.8.1) #1838
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a series of changes across multiple files, primarily focusing on version updates, grammar modifications, validation logic enhancements, and the addition of new test cases. The version number for the JetBrains plugin is incremented from "2.8.0" to "2.8.1". The grammar for the Changes
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: 2
🧹 Outside diff range and nitpick comments (8)
tests/integration/tests/enhancements/json/validation.test.ts (1)
Line range hint
6-19: LGTM! Improved error handling withloadSchema.The switch from
loadModelWithErrortoloadSchemaprovides more specific error handling for provider compatibility checks. The error message clearly indicates that SQLite doesn't support@jsonfields.Consider documenting these provider limitations in the main documentation to help developers choose the appropriate database provider based on their JSON field requirements.
packages/schema/src/plugins/enhancer/enhance/model-typedef-generator.ts (2)
39-43: Consider adding type validation for non-enum referencesThe enum handling logic is well-implemented. However, for non-enum references, we might want to validate that the referenced type is actually a TypeDef to prevent potential runtime issues.
Consider adding a type check:
if (isEnum(type.reference.ref)) { result = makeEnumTypeReference(type.reference.ref); } else { + if (!type.reference.ref.$type === 'TypeDef') { + throw new PluginError(name, `Invalid type reference: expected TypeDef, got ${type.reference.ref.$type}`); + } result = type.reference.ref.name; }
69-81: Consider adding error handling and performance optimizationThe function implementation is clean and well-documented. However, there are a few potential improvements to consider:
- Error handling for edge cases (e.g., empty enums)
- Performance optimization for large models by caching the model traversal results
Consider this enhanced implementation:
+const enumUsageCache = new WeakMap<Enum, boolean>(); + function makeEnumTypeReference(enumDecl: Enum) { + if (enumDecl.fields.length === 0) { + throw new PluginError(name, `Empty enum ${enumDecl.name} is not supported`); + } + + // Check cache first + const cachedResult = enumUsageCache.get(enumDecl); + if (cachedResult !== undefined) { + return cachedResult ? enumDecl.name : enumDecl.fields.map((field) => `'${field.name}'`).join(' | '); + } + const zmodel = enumDecl.$container; const models = getDataModels(zmodel); + const isUsedInModels = models.some((model) => + model.fields.some((field) => field.type.reference?.ref === enumDecl) + ); - if (models.some((model) => model.fields.some((field) => field.type.reference?.ref === enumDecl))) { + // Cache the result + enumUsageCache.set(enumDecl, isUsedInModels); + + if (isUsedInModels) { // if the enum is referenced by any data model, Prisma already generates its type, // we just need to reference it return enumDecl.name;tests/integration/tests/enhancements/json/typing.test.ts (2)
182-234: Enhance test coverage for enum handling.While the test successfully validates basic enum functionality, consider expanding it to:
- Test both enum values (USER and ADMIN)
- Add negative test cases for invalid enum values
- Include tests for the
Foomodel since it's defined but not usedHere's a suggested addition to the test:
// Test both enum values await db.user.create({ data: { profile: { role: Role.USER } } }); const userWithUserRole = await db.user.findFirstOrThrow(); console.log(userWithUserRole.profile.role === Role.USER); // Test Foo model await db.foo.create({ data: { role: Role.ADMIN } }); const foo = await db.foo.findFirstOrThrow(); console.log(foo.role === Role.ADMIN); // Negative test await expect( db.user.create({ data: { profile: { role: 'INVALID_ROLE' } } }) ).rejects.toThrow();
236-237: Rename test case for clarity.The test name "works with enums unused in models" is misleading since the enum is used in the Profile type. Consider renaming to "works with enum string literals" or "works with string-based enum values" to better reflect the test's purpose.
tests/integration/tests/enhancements/json/crud.test.ts (1)
194-233: Enhance test coverage for enum validation.While the test covers basic enum validation, consider adding the following test cases for more comprehensive coverage:
- Test the 'USER' enum value
- Test updating enum values
- Test the interaction between JSON field (
profile.role) and regular field (Foo.role)Here's a suggested addition:
// Test USER enum value await expect(db.user.create({ data: { profile: { role: 'USER' } } })).resolves.toMatchObject({ profile: { role: 'USER' }, }); // Test updating enum values 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 interaction with regular enum field await expect(db.foo.create({ data: { role: 'ADMIN' } })).resolves.toMatchObject({ role: 'ADMIN' });packages/schema/src/plugins/prisma/schema-generator.ts (1)
852-857: Method should be marked as private.The method is only used internally within the class and should be marked as private for better encapsulation.
- private ensureSupportingTypeDefFields(zmodel: Model) { + private ensureSupportingTypeDefFields(zmodel: Model) { const dsProvider = getDataSourceProvider(zmodel); if (dsProvider && !PROVIDERS_SUPPORTING_TYPEDEF_FIELDS.includes(dsProvider)) { - throw new PluginError(name, `Datasource provider "${dsProvider}" does not support "@json" fields`); + throw new PluginError(name, `Datasource provider "${dsProvider}" does not support type definition fields`); } }packages/schema/src/plugins/zod/transformer.ts (1)
Line range hint
56-86: Use aSetfor 'generated' to improve lookup performance and prevent duplicatesAs the number of enums increases, using an array with
includesfor checking existing enum names ingeneratedcan become inefficient. Switching to aSetimproves performance for lookups and ensures that duplicates are not possible.Apply the following changes:
- const generated: string[] = []; + const generated: Set<string> = new Set(); ... - generated.push(enumType.name); + generated.add(enumType.name); ... - generated.push(enumDecl.name); + generated.add(enumDecl.name); ... - const extraEnums = this.zmodel.declarations.filter((d): d is Enum => isEnum(d) && !generated.includes(d.name)); + const extraEnums = this.zmodel.declarations.filter((d): d is Enum => isEnum(d) && !generated.has(d.name)); ... - generated.map((name) => `export * from './${upperCaseFirst(name)}.schema';`).join('\n'), + Array.from(generated).map((name) => `export * from './${upperCaseFirst(name)}.schema';`).join('\n'),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (15)
package.jsonis excluded by!**/*.jsonpackages/ide/jetbrains/package.jsonis excluded by!**/*.jsonpackages/language/package.jsonis excluded by!**/*.jsonpackages/language/src/generated/ast.tsis excluded by!**/generated/**,!**/generated/**packages/language/src/generated/grammar.tsis excluded by!**/generated/**,!**/generated/**packages/misc/redwood/package.jsonis excluded by!**/*.jsonpackages/plugins/openapi/package.jsonis excluded by!**/*.jsonpackages/plugins/swr/package.jsonis excluded by!**/*.jsonpackages/plugins/tanstack-query/package.jsonis excluded by!**/*.jsonpackages/plugins/trpc/package.jsonis excluded by!**/*.jsonpackages/runtime/package.jsonis excluded by!**/*.jsonpackages/schema/package.jsonis excluded by!**/*.jsonpackages/sdk/package.jsonis excluded by!**/*.jsonpackages/server/package.jsonis excluded by!**/*.jsonpackages/testtools/package.jsonis excluded by!**/*.json
📒 Files selected for processing (9)
packages/ide/jetbrains/build.gradle.kts(1 hunks)packages/language/src/zmodel.langium(1 hunks)packages/schema/src/language-server/validator/datamodel-validator.ts(0 hunks)packages/schema/src/plugins/enhancer/enhance/model-typedef-generator.ts(3 hunks)packages/schema/src/plugins/prisma/schema-generator.ts(4 hunks)packages/schema/src/plugins/zod/transformer.ts(3 hunks)tests/integration/tests/enhancements/json/crud.test.ts(1 hunks)tests/integration/tests/enhancements/json/typing.test.ts(1 hunks)tests/integration/tests/enhancements/json/validation.test.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/schema/src/language-server/validator/datamodel-validator.ts
✅ Files skipped from review due to trivial changes (1)
- packages/ide/jetbrains/build.gradle.kts
🔇 Additional comments (10)
tests/integration/tests/enhancements/json/validation.test.ts (1)
1-1: LGTM! Clean import statement.
The import statement correctly includes both required functions from the test tools package.
packages/schema/src/plugins/enhancer/enhance/model-typedef-generator.ts (1)
1-1: LGTM: Import addition is appropriate
The addition of getDataModels import is well-placed and necessary for the new enum reference functionality.
packages/language/src/zmodel.langium (4)
189-190: LGTM! Well-structured type alias addition.
The new TypeDefFieldTypes type alias effectively groups related type references, improving code organization.
192-192: LGTM! Improved comment handling.
The explicit whitespace handling for comments improves grammar consistency.
196-196: LGTM! Enhanced type reference flexibility.
The update to use TypeDefFieldTypes allows fields to reference both TypeDef and Enum types, which aligns with the goal of improving type definition flexibility.
Let's verify existing type references are compatible:
#!/bin/bash
# Search for existing type references to ensure they're compatible
# with the new TypeDefFieldTypes system
rg -A 2 'type\s+\w+\s*\{.*\btype\b.*:.*\b(enum|type)\b'186-186: Verify the impact of allowing empty type definitions.
The cardinality change from + to * means type definitions can now be empty. While this adds flexibility, it's a potentially breaking change that might affect existing code that assumes types always have fields.
Consider:
- Documenting this change in the changelog
- Adding validation rules if empty types should be restricted in certain contexts
✅ Verification successful
Based on the search results, I can now make a definitive assessment. The grammar changes and AST definitions show that:
- The TypeDef model is a core part of the language structure, used in multiple places including abstract declarations and type references
- There are no existing empty type definitions in the codebase
- The change from '+' to '*' is intentional to support optional fields in type definitions
Empty type definitions are now supported by design
The cardinality change from + to * for TypeDef fields is a deliberate enhancement to support empty type definitions, not a breaking change. This aligns with common type system practices where empty types can serve valid use cases like:
- Marker types
- Future extensibility
- Placeholder types in API contracts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing empty type definitions that might now be valid
# and type validation logic that might need updates
rg -l 'type\s+\w+\s*\{\s*\}'
ast-grep --pattern 'type $name {}'
Length of output: 156
Script:
#!/bin/bash
# Let's try a more comprehensive search strategy:
# 1. Look for type definitions in test files that might demonstrate empty types
rg -A 5 "type\s+\w+\s*\{" test/ || true
# 2. Search for validation rules or constraints related to type definitions
rg -A 3 "validateType|TypeDef" packages/language/src/ || true
# 3. Look for any documentation or examples mentioning type definitions
fd "README|CHANGELOG" --exec cat {} || true
# 4. Check for existing type definition usages
ast-grep --pattern 'type $name {
$$$
}'
Length of output: 56914
tests/integration/tests/enhancements/json/typing.test.ts (1)
213-217: LGTM: Proper import structure.
The import statements are well-organized and include all necessary dependencies for type safety and database operations.
Also applies to: 262-264
packages/schema/src/plugins/prisma/schema-generator.ts (2)
85-85: LGTM: Provider support constant is well-defined.
The constant follows the existing naming convention and clearly defines PostgreSQL as the only supported provider for typedef fields.
799-799: LGTM: Validation is called at the right place.
The validation is appropriately placed before processing typedef fields in the generateModelField method.
packages/schema/src/plugins/zod/transformer.ts (1)
3-3: Necessary import of 'Enum' type for enum processing
The addition of the Enum type import is appropriate for handling enums not referenced by data models.
No description provided.