fix: generated code fails to compile when prisma's output path overlaps with zenstack #1765
Conversation
…ps with zenstack Fixes #1743
📝 WalkthroughWalkthroughThe changes in this pull request primarily involve the introduction of a new function, 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: 2
🧹 Outside diff range and nitpick comments (6)
packages/sdk/src/path.ts (1)
6-9: Consider adding error handling and unit tests.While the current implementation is correct, consider the following improvements for increased robustness:
- Add input validation to handle potential edge cases (e.g., empty strings, non-string inputs).
- Implement unit tests to verify the function's behavior with various input combinations.
These additions would further enhance the reliability of the
normalizedRelativefunction and prevent potential issues in the future.Would you like assistance in drafting input validation logic or unit tests for this function?
packages/schema/src/plugins/enhancer/index.ts (1)
Line range hint
1-41: Overall assessment: Changes look good and address the PR objective.The modifications in this file are minimal and focused, directly addressing the issue of incorrect import paths when Prisma and Zenstack output paths overlap. The introduction of
normalizedRelativefor path calculations is a solid approach to handle this issue more robustly.Some suggestions for further improvement:
- Consider adding comments explaining why
normalizedRelativeis used instead ofpath.relativeto help future maintainers understand the rationale behind this change.- As mentioned earlier, adding unit tests to verify the behavior of
normalizedRelativewith various path configurations would increase confidence in this change.packages/sdk/src/prisma.ts (1)
Line range hint
1-95: Summary: Effective solution for import path issues.The changes in this file, particularly the introduction of
normalizedRelativeand its use ingetPrismaClientImportSpec, directly address the PR objectives. This modification should resolve the issue of incorrect import paths when Prisma and Zenstack output to the same directory.The implementation is minimal and focused, affecting only the necessary parts of the code. The rest of the file's functionality remains intact, which is a positive aspect of this change.
Consider adding a brief comment above the
getPrismaClientImportSpecfunction explaining whynormalizedRelativeis used instead ofpath.relative. This would help future maintainers understand the rationale behind this specific implementation.packages/testtools/src/schema.ts (2)
267-272: LGTM: Improved Prisma client loading logicThe new logic for determining the Prisma client load path is well-implemented. It correctly handles both absolute and relative paths for the new
prismaLoadPathoption while maintaining backwards compatibility.One minor suggestion for improved readability:
Consider extracting the default path to a constant at the top of the file:
const DEFAULT_PRISMA_LOAD_PATH = 'node_modules/.prisma/client'; // Then in the loadSchema function: const prismaLoadPath = options?.prismaLoadPath ? path.isAbsolute(options.prismaLoadPath) ? options.prismaLoadPath : path.join(projectDir, options.prismaLoadPath) : path.join(projectDir, DEFAULT_PRISMA_LOAD_PATH);This change would make it easier to update the default path in the future if needed.
138-138: LGTM: Additional option for Prisma client customizationThe addition of
prismaClientOptionstoSchemaLoadOptionsis a good enhancement that provides more flexibility in configuring the Prisma client. While not directly related to the main PR objective, it's a valuable addition.Consider adding a brief comment or updating the documentation to explain the purpose and usage of this new option. This will help users understand how to leverage this new functionality effectively.
packages/schema/src/plugins/enhancer/enhance/index.ts (1)
Add Comprehensive Tests for EnhancerGenerator Path Resolution
The current changes improve path resolution within the
EnhancerGeneratorclass. However, there are no dedicated tests for this class to verify these modifications.
- Action Items:
- Implement unit tests for the
EnhancerGeneratorclass focusing on path resolution.- Ensure tests cover scenarios with custom output paths and various operating systems.
🔗 Analysis chain
Line range hint
1-638: Verify the impact on enhanced Prisma client generationWhile the changes are focused on improving path resolution for Zod imports, it's crucial to ensure that this modification doesn't inadvertently affect other parts of the
EnhancerGeneratorclass.Please run comprehensive tests to verify that the enhanced Prisma client generation process works correctly with these changes, especially in scenarios involving custom output paths and different operating systems.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the enhanced Prisma client generation process # Test: Check if there are any test files related to EnhancerGenerator echo "Searching for EnhancerGenerator test files..." fd -e test.ts -e spec.ts EnhancerGenerator # Test: Look for any error handling related to path resolution in the codebase echo "Checking for path resolution error handling..." rg -i 'path.*error' --type ts # Test: Search for usage of normalizedRelative function echo "Searching for usage of normalizedRelative function..." rg 'normalizedRelative' --type tsLength of output: 1855
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- packages/schema/src/plugins/enhancer/enhance/index.ts (2 hunks)
- packages/schema/src/plugins/enhancer/index.ts (2 hunks)
- packages/schema/src/plugins/prisma/index.ts (2 hunks)
- packages/sdk/src/index.ts (1 hunks)
- packages/sdk/src/path.ts (1 hunks)
- packages/sdk/src/prisma.ts (2 hunks)
- packages/testtools/src/schema.ts (2 hunks)
- tests/regression/tests/issue-1743.test.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (18)
packages/sdk/src/path.ts (3)
1-1: LGTM: Appropriate import for path operations.The import of the 'path' module is correct and necessary for the file path operations performed in this module.
3-5: LGTM: Clear and concise function documentation.The JSDoc comment effectively describes the purpose and behavior of the
normalizedRelativefunction, highlighting the normalization aspect.
6-9: LGTM: Effective implementation addressing the PR objectives.The
normalizedRelativefunction is well-implemented and directly addresses the issue of incorrect import paths mentioned in the PR objectives. By ensuring that the returned path always starts with './' or '../', it should resolve the compilation errors caused by incorrect imports.Some key points:
- Correct use of
path.relativefor calculating relative paths.- Proper normalization logic to ensure paths start with './' when necessary.
- Simple and focused implementation without unnecessary complexity.
This function should help resolve the issue described in #1743 where import statements were incorrectly referencing "prisma" instead of using relative paths.
packages/sdk/src/index.ts (1)
5-5: LGTM! New export added to address import path issues.The addition of
export * from './path';aligns with the PR objectives to resolve import path problems when Prisma and Zenstack output to the same directory. This change expands the public API of the SDK, potentially providing the necessary functionality to address the reported issues.To ensure the
pathmodule contains the expected functionality, particularly thenormalizedRelativefunction mentioned in the summary, please run the following script:This will help confirm that the
pathmodule includes the necessary changes to address the import path issues.✅ Verification successful
Verification Successful:
normalizedRelativefunction is correctly exported and utilized across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the contents of the 'path' module # Test: Check for the presence of the normalizedRelative function ast-grep --lang typescript --pattern 'export function normalizedRelative($_) { $$$ }' packages/sdk/src/path.ts # Test: Look for any usage of the new normalizedRelative function rg --type typescript 'normalizedRelative\(' packages/sdk/srcLength of output: 210
Script:
#!/bin/bash # Description: Verify the presence and export of the 'normalizedRelative' function in the 'path.ts' module # Test: Check for the presence of the normalizedRelative function ast-grep --lang typescript --pattern 'export function normalizedRelative($_) { $$$ }' packages/sdk/src/path.ts # Test: Look for any usage of the normalizedRelative function in .ts files rg 'normalizedRelative\(' packages/sdk/src/**/*.ts # Test: List all export statements in path.ts to verify correct exports rg '^export' packages/sdk/src/path.tsLength of output: 621
tests/regression/tests/issue-1743.test.ts (3)
1-3: LGTM: Proper test setup for regression testing.The import statement and test suite setup are correct. The use of a dedicated test for issue 1743 is a good practice for regression testing.
4-25: Excellent: Schema accurately represents the issue scenario.The schema definition effectively recreates the conditions described in issue #1743. Key points:
- Prisma client output is set to '../lib/zenstack/prisma'
- Enhancer plugin output is set to './lib/zenstack'
- These overlapping paths are crucial for testing the import path resolution issue.
The minimal User model is sufficient for this test case.
26-32: Well-configured: Options effectively test the import path issue.The
loadSchemaoptions are well-crafted to test the specific issue:
compile: trueensures that the code is generated, allowing for import path verification.outputandprismaLoadPathsettings recreate the overlapping directory scenario.These configurations are crucial for verifying that the fix correctly resolves import paths when Prisma and ZenStack outputs overlap.
packages/schema/src/plugins/enhancer/index.ts (1)
1-8: LGTM: Import statement updated correctly.The addition of
normalizedRelativeto the import statement from '@zenstackhq/sdk' is consistent with its usage in the file and aligns with the PR objective of addressing import path issues.packages/sdk/src/prisma.ts (2)
12-12: LGTM: New import statement added.The new import for
normalizedRelativefrom './path' is correctly added and will be used in thegetPrismaClientImportSpecfunction.
38-38: LGTM: Improved relative path calculation.The change from
path.relativetonormalizedRelativeshould address the issue of incorrect import paths when Prisma and Zenstack output to the same directory. This modification aligns with the PR objectives and should resolve the compilation errors mentioned in issue #1743.To ensure this change resolves the issue completely, please verify with the following steps:
- Set up a test configuration where Prisma and Zenstack output to the same directory.
- Generate the code using this updated version.
- Confirm that the import statements in the generated files (e.g.,
policy.ts) are correct and the code compiles without errors.packages/schema/src/plugins/prisma/index.ts (3)
1-8: LGTM: Import ofnormalizedRelativeaddresses path resolution issues.The addition of
normalizedRelativefrom@zenstackhq/sdkis a positive change that aligns with the PR's objective of fixing import path issues. This function is likely designed to handle path normalization more robustly, which should help address the compilation errors mentioned in the linked issue #1743.
Line range hint
1-69: Summary: Targeted fix improves path resolution for Prisma client imports.The changes in this file are focused and effective in addressing the issue of incorrect import paths when Prisma and Zenstack output to the same directory. The introduction of
normalizedRelativeand its use in calculating theprismaClientPathshould resolve the compilation errors reported in issue #1743. This implementation maintains the existing logic while improving cross-environment compatibility.Overall, these changes align well with the PR objectives and provide a targeted solution to the reported problem.
69-69: LGTM: Improved path resolution addresses compilation issues.The replacement of
path.relativewithnormalizedRelativeis a key improvement that directly addresses the issue reported in #1743. This change should provide more consistent and correct path resolution, especially when Prisma and Zenstack are configured to output to the same directory. The use ofnormalizedRelativeis likely to generate correct import paths in files likepolicy.ts, resolving the compilation errors mentioned in the PR objectives.To ensure this change resolves the reported issue, we can verify the generated import paths:
This script will help confirm that the import statements in the generated files are using the correct relative paths.
packages/testtools/src/schema.ts (2)
137-137: LGTM: New option enhances flexibility for Prisma client loadingThe addition of the
prismaLoadPathoption toSchemaLoadOptionsis a good enhancement. It allows users to specify a custom path for loading the Prisma client, which directly addresses the issue of overlapping output paths between Prisma and Zenstack.
Line range hint
1-424: Summary: Effective implementation addressing PR objectivesThe changes in this file successfully address the main PR objective of fixing compilation issues when Prisma and Zenstack output paths overlap. The new
prismaLoadPathoption and its associated logic provide a flexible solution while maintaining backwards compatibility. The additionalprismaClientOptionsfurther enhances customization capabilities.A few minor suggestions have been made for improved readability and documentation. Overall, these changes are well-implemented and align closely with the PR's goals.
packages/schema/src/plugins/enhancer/enhance/index.ts (3)
11-11: New import added for path normalizationThe
normalizedRelativefunction has been imported from@zenstackhq/sdk. This is likely to improve path resolution across different operating systems.
163-163: Improved path resolution for Zod importsThe use of
normalizedRelativeinstead ofpath.relativeis a good improvement. This change enhances cross-platform compatibility and addresses the issue of incorrect import paths being generated, which was the main objective of this PR.
Line range hint
1-638: Summary of changes and their impactThe modifications in this file are minimal but significant:
- Added import of
normalizedRelativefrom@zenstackhq/sdk.- Updated the
getZodImportmethod to usenormalizedRelativeinstead ofpath.relative.These changes directly address the PR objective of fixing incorrect import paths generation when Prisma and Zenstack output paths overlap. The use of
normalizedRelativeshould improve cross-platform compatibility and resolve the reported issues.While the changes look good, it's important to ensure that:
- The new path resolution works correctly across different operating systems.
- The changes don't negatively impact other parts of the
EnhancerGeneratorclass.- Comprehensive tests are run to verify the enhanced Prisma client generation process.
Overall, this update seems to be a well-targeted fix for the reported issue.
Fixes #1743