perf: improve polymorphism code generation speed #1073
Conversation
WalkthroughWalkthroughThis update introduces significant enhancements to the schema and Prisma plugins, focusing on the addition of new types, functions, and support for named constraints across different database providers. It also modifies the initialization and generation processes for Prisma schemas, improving the handling of polymorphic associations and refining the structure and capabilities of the Prisma namespace. Integration tests have been updated to reflect these changes and address specific issues. Changes
Related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (7)
- packages/schema/src/plugins/enhancer/delegate/index.ts (1 hunks)
- packages/schema/src/plugins/enhancer/enhance/index.ts (4 hunks)
- packages/schema/src/plugins/prisma/index.ts (1 hunks)
- packages/schema/src/plugins/prisma/schema-generator.ts (7 hunks)
- tests/integration/tests/enhancements/with-delegate/issue-1058.test.ts (1 hunks)
- tests/integration/tests/enhancements/with-delegate/issue-1064.test.ts (1 hunks)
- tests/integration/tests/enhancements/with-delegate/polymorphism.test.ts (2 hunks)
Additional comments: 10
packages/schema/src/plugins/prisma/index.ts (1)
- 8-8: The modification to pass
modeldirectly to thePrismaSchemaGeneratorconstructor simplifies the generator's usage and aligns with the PR's objectives to enhance polymorphism code generation. This change is correctly implemented.packages/schema/src/plugins/enhancer/delegate/index.ts (1)
- 8-9: Initializing
PrismaSchemaGeneratorwithmodeldirectly in the constructor simplifies its usage and is consistent with the PR's objectives. This change is correctly implemented.tests/integration/tests/enhancements/with-delegate/issue-1058.test.ts (1)
- 3-4: The updates to the test suite and test case descriptions are appropriate and align with the PR's objectives to introduce regression tests for specific issues. The test case is correctly implemented and relevant.
tests/integration/tests/enhancements/with-delegate/issue-1064.test.ts (1)
- 3-4: The test suite and test case for regression issue 1064 are well-structured and comprehensive, covering a wide range of models and relationships. This aligns with the PR's objectives to ensure the robustness of enhancements in handling complex schemas.
packages/schema/src/plugins/enhancer/enhance/index.ts (1)
- 24-26: Introducing the
DelegateInfotype is a good practice for encapsulating information about delegate models and their sub-models, enhancing code readability and maintainability.packages/schema/src/plugins/prisma/schema-generator.ts (4)
- 83-83: The introduction of
PROVIDERS_SUPPORTING_NAMED_CONSTRAINTSis a good practice for maintaining a clear list of supported database providers for named constraints.- 100-100: The modification to the constructor of
PrismaSchemaGeneratorto accept azmodelparameter simplifies its instantiation and usage, aligning with the PR's objectives.- 102-102: Removing the
modelparameter from thegeneratemethod and relying on thezmodelprovided at construction simplifies the interface and usage ofPrismaSchemaGenerator.- 484-497: The addition of the
supportNamedConstraintsgetter is a well-implemented feature that checks for support of named constraints based on the database provider, aligning with the PR's objectives.tests/integration/tests/enhancements/with-delegate/polymorphism.test.ts (1)
- 2-4: The addition of imports for
run,fs, andpathis appropriate given their usage in the newly added TypeScript compilation test case. These modules are essential for setting up the test environment, writing the TypeScript script to a file, and executing the TypeScript compiler.
| it('typescript compilation', async () => { | ||
| const { projectDir } = await loadSchema(schema, { enhancements: ['delegate'] }); | ||
| const src = ` | ||
| import { PrismaClient } from '@prisma/client'; | ||
| import { enhance } from '.zenstack/enhance'; | ||
|
|
||
| const prisma = new PrismaClient(); | ||
|
|
||
| async function main() { | ||
| await prisma.user.deleteMany(); | ||
| const db = enhance(prisma); | ||
|
|
||
| const user1 = await db.user.create({ data: { } }); | ||
|
|
||
| await db.ratedVideo.create({ | ||
| data: { | ||
| owner: { connect: { id: user1.id } }, | ||
| duration: 100, | ||
| url: 'abc', | ||
| rating: 10, | ||
| }, | ||
| }); | ||
|
|
||
| await db.image.create({ | ||
| data: { | ||
| owner: { connect: { id: user1.id } }, | ||
| format: 'webp', | ||
| }, | ||
| }); | ||
|
|
||
| const video = await db.video.findFirst({ include: { owner: true } }); | ||
| console.log(video?.duration); | ||
| console.log(video?.viewCount); | ||
|
|
||
| const asset = await db.asset.findFirstOrThrow(); | ||
| console.log(asset.assetType); | ||
| console.log(asset.viewCount); | ||
|
|
||
| if (asset.assetType === 'Video') { | ||
| console.log('Video: duration', asset.duration); | ||
| } else { | ||
| console.log('Image: format', asset.format); | ||
| } | ||
| } | ||
|
|
||
| main() | ||
| .then(async () => { | ||
| await prisma.$disconnect(); | ||
| }) | ||
| .catch(async (e) => { | ||
| console.error(e); | ||
| await prisma.$disconnect(); | ||
| process.exit(1); | ||
| }); | ||
| `; | ||
|
|
||
| fs.writeFileSync(path.join(projectDir, 'script.ts'), src); | ||
| fs.writeFileSync( | ||
| path.join(projectDir, 'tsconfig.json'), | ||
| JSON.stringify({ | ||
| compilerOptions: { | ||
| outDir: 'dist', | ||
| strict: true, | ||
| lib: ['esnext'], | ||
| esModuleInterop: true, | ||
| }, | ||
| }) | ||
| ); | ||
|
|
||
| run('npm i -D @types/node', undefined, projectDir); | ||
| run('npx tsc --noEmit --skipLibCheck script.ts', undefined, projectDir); | ||
| }); |
There was a problem hiding this comment.
The new test case for TypeScript compilation is well-structured and serves its purpose of validating the TypeScript compilation with PrismaClient operations. However, there are a few points to consider for improvement:
-
Error Handling: The catch block in lines 1067-1071 properly logs errors and disconnects the Prisma client, which is good practice. However, it's important to ensure that any potential errors during the setup phase (e.g., writing files or installing dependencies) are also caught and handled appropriately.
-
Dependency Installation: The command in line 1087 installs
@types/nodeevery time the test runs. Consider checking if this dependency could be included in the project'sdevDependenciesto avoid repeated installations, which could slow down the test execution. -
Compiler Options: The TypeScript compiler options specified in lines 1078-1083 are focused on strict typing and modern JavaScript features. Ensure these settings align with the project's overall TypeScript configuration to maintain consistency.
-
Script Execution: While the test verifies the TypeScript compilation (line 1088), it does not execute the compiled JavaScript. If the objective includes runtime validation, consider adding a step to execute the compiled output.
Overall, the test case is a valuable addition to the test suite, ensuring that the TypeScript code involving PrismaClient operations compiles correctly. Just ensure that the points mentioned above are considered to optimize the test execution and alignment with project standards.
- run('npm i -D @types/node', undefined, projectDir);
+ // Consider moving '@types/node' to project's devDependencies to avoid repeated installationsCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| it('typescript compilation', async () => { | |
| const { projectDir } = await loadSchema(schema, { enhancements: ['delegate'] }); | |
| const src = ` | |
| import { PrismaClient } from '@prisma/client'; | |
| import { enhance } from '.zenstack/enhance'; | |
| const prisma = new PrismaClient(); | |
| async function main() { | |
| await prisma.user.deleteMany(); | |
| const db = enhance(prisma); | |
| const user1 = await db.user.create({ data: { } }); | |
| await db.ratedVideo.create({ | |
| data: { | |
| owner: { connect: { id: user1.id } }, | |
| duration: 100, | |
| url: 'abc', | |
| rating: 10, | |
| }, | |
| }); | |
| await db.image.create({ | |
| data: { | |
| owner: { connect: { id: user1.id } }, | |
| format: 'webp', | |
| }, | |
| }); | |
| const video = await db.video.findFirst({ include: { owner: true } }); | |
| console.log(video?.duration); | |
| console.log(video?.viewCount); | |
| const asset = await db.asset.findFirstOrThrow(); | |
| console.log(asset.assetType); | |
| console.log(asset.viewCount); | |
| if (asset.assetType === 'Video') { | |
| console.log('Video: duration', asset.duration); | |
| } else { | |
| console.log('Image: format', asset.format); | |
| } | |
| } | |
| main() | |
| .then(async () => { | |
| await prisma.$disconnect(); | |
| }) | |
| .catch(async (e) => { | |
| console.error(e); | |
| await prisma.$disconnect(); | |
| process.exit(1); | |
| }); | |
| `; | |
| fs.writeFileSync(path.join(projectDir, 'script.ts'), src); | |
| fs.writeFileSync( | |
| path.join(projectDir, 'tsconfig.json'), | |
| JSON.stringify({ | |
| compilerOptions: { | |
| outDir: 'dist', | |
| strict: true, | |
| lib: ['esnext'], | |
| esModuleInterop: true, | |
| }, | |
| }) | |
| ); | |
| run('npm i -D @types/node', undefined, projectDir); | |
| run('npx tsc --noEmit --skipLibCheck script.ts', undefined, projectDir); | |
| }); | |
| it('typescript compilation', async () => { | |
| const { projectDir } = await loadSchema(schema, { enhancements: ['delegate'] }); | |
| const src = ` | |
| import { PrismaClient } from '@prisma/client'; | |
| import { enhance } from '.zenstack/enhance'; | |
| const prisma = new PrismaClient(); | |
| async function main() { | |
| await prisma.user.deleteMany(); | |
| const db = enhance(prisma); | |
| const user1 = await db.user.create({ data: { } }); | |
| await db.ratedVideo.create({ | |
| data: { | |
| owner: { connect: { id: user1.id } }, | |
| duration: 100, | |
| url: 'abc', | |
| rating: 10, | |
| }, | |
| }); | |
| await db.image.create({ | |
| data: { | |
| owner: { connect: { id: user1.id } }, | |
| format: 'webp', | |
| }, | |
| }); | |
| const video = await db.video.findFirst({ include: { owner: true } }); | |
| console.log(video?.duration); | |
| console.log(video?.viewCount); | |
| const asset = await db.asset.findFirstOrThrow(); | |
| console.log(asset.assetType); | |
| console.log(asset.viewCount); | |
| if (asset.assetType === 'Video') { | |
| console.log('Video: duration', asset.duration); | |
| } else { | |
| console.log('Image: format', asset.format); | |
| } | |
| } | |
| main() | |
| .then(async () => { | |
| await prisma.$disconnect(); | |
| }) | |
| .catch(async (e) => { | |
| console.error(e); | |
| await prisma.$disconnect(); | |
| process.exit(1); | |
| }); | |
| `; | |
| fs.writeFileSync(path.join(projectDir, 'script.ts'), src); | |
| fs.writeFileSync( | |
| path.join(projectDir, 'tsconfig.json'), | |
| JSON.stringify({ | |
| compilerOptions: { | |
| outDir: 'dist', | |
| strict: true, | |
| lib: ['esnext'], | |
| esModuleInterop: true, | |
| }, | |
| }) | |
| ); | |
| // Consider moving '@types/node' to project's devDependencies to avoid repeated installations | |
| run('npx tsc --noEmit --skipLibCheck script.ts', undefined, projectDir); | |
| }); |
| InvocationExpr, | ||
| isArrayExpr, | ||
| isDataModel, | ||
| isDataSource, | ||
| isInvocationExpr, | ||
| isLiteralExpr, | ||
| isNullExpr, |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [707-707]
Potential security issue detected: user input is used in path.resolve within the getDefaultPrismaOutputFile function, which could lead to a path traversal vulnerability. Ensure to sanitize or validate user input before using it in path resolution functions.
- return path.resolve(path.dirname(pkgJsonPath), pkgJson.zenstack.prisma);
+ // Ensure pkgJson.zenstack.prisma is sanitized or validated before use
+ return path.resolve(path.dirname(pkgJsonPath), sanitizedInput);| import path from 'path'; | ||
|
|
||
| export async function generate(model: Model, options: PluginOptions, project: Project, outDir: string) { | ||
| const prismaGenerator = new PrismaSchemaGenerator(); | ||
| await prismaGenerator.generate(model, { | ||
| const prismaGenerator = new PrismaSchemaGenerator(model); | ||
| await prismaGenerator.generate({ | ||
| provider: '@internal', | ||
| schemaPath: options.schemaPath, | ||
| output: path.join(outDir, 'delegate.prisma'), |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [12-13]
The use of path.join with options.schemaPath and outDir could potentially lead to path traversal vulnerabilities if user input is not properly sanitized or validated. Ensure that any user-supplied paths are rigorously checked to prevent unauthorized file system access.
- output: path.join(outDir, 'delegate.prisma'),
- overrideClientGenerationPath: path.join(outDir, '.delegate'),
+ output: sanitizePath(path.join(outDir, 'delegate.prisma')),
+ overrideClientGenerationPath: sanitizePath(path.join(outDir, '.delegate')),Consider implementing a sanitizePath function that checks for and mitigates any path traversal patterns.
| } from 'ts-morph'; | ||
| import { PrismaSchemaGenerator } from '../../prisma/schema-generator'; | ||
|
|
||
| // information of delegate models and their sub models | ||
| type DelegateInfo = [DataModel, DataModel[]][]; | ||
|
|
||
| export async function generate(model: Model, options: PluginOptions, project: Project, outDir: string) { | ||
| const outFile = path.join(outDir, 'enhance.ts'); |
There was a problem hiding this comment.
The use of path.join with outDir could potentially lead to path traversal vulnerabilities if user input is not properly sanitized or validated. Ensure that any user-supplied paths are rigorously checked to prevent unauthorized file system access.
- const outFile = path.join(outDir, 'enhance.ts');
+ const outFile = sanitizePath(path.join(outDir, 'enhance.ts'));Consider implementing a sanitizePath function that checks for and mitigates any path traversal patterns.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const outFile = path.join(outDir, 'enhance.ts'); | |
| const outFile = sanitizePath(path.join(outDir, 'enhance.ts')); |
| const delegateCreateUpdateInputRegex = new RegExp( | ||
| `\\${delegateModelNames.join('|')}(Unchecked)?(Create|Update).*Input` | ||
| ); |
There was a problem hiding this comment.
The dynamic construction of a regular expression from user input or variable names could potentially lead to a Regular Expression Denial-of-Service (ReDoS) vulnerability. Consider using hardcoded regexes or performing input validation to mitigate this risk.
Consider refactoring to use hardcoded regexes or adding input validation to ensure the regex does not become a vector for ReDoS.
| const concreteCreateUpdateInputRegex = new RegExp( | ||
| `(${concreteModelNames.join('|')})(Unchecked)?(Create|Update).*Input` | ||
| ); |
There was a problem hiding this comment.
Similar to the previous comment, dynamically constructing a regular expression from user input or variable names could lead to a Regular Expression Denial-of-Service (ReDoS) vulnerability. It's important to use hardcoded regexes or perform input validation to mitigate this risk.
Consider refactoring to use hardcoded regexes or adding input validation to ensure the regex does not become a vector for ReDoS.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/schema/tests/generator/prisma-generator.test.ts (13 hunks)
Additional comments: 1
packages/schema/tests/generator/prisma-generator.test.ts (1)
- 52-52: The modification to pass
modelas a parameter toPrismaSchemaGenerator'sgeneratemethod instead of creating a new instance each time is a significant improvement in terms of efficiency. This change allows for the reuse of thePrismaSchemaGeneratorinstance across multiple calls togenerate, reducing the overhead of instance creation and potentially improving performance, especially in scenarios wheregenerateis called multiple times.
|
Fixes #1064 |
Summary by CodeRabbit