fix: avoid reference unused enums in generated policy code #1851
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a new function 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 (
|
|
fixes #1835 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/regression/tests/issue-1835.test.ts (1)
3-4: Add test description documenting the regression caseConsider adding a comment before the test case explaining:
- What issue this regression test is preventing
- The expected behavior regarding enum references in ignored models
Example:
describe('issue 1835', () => { + // Ensures enums referenced only in ignored models are properly handled + // and not incorrectly marked as unused in generated policy code it('regression', async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/schema/src/plugins/enhancer/policy/utils.ts(2 hunks)tests/regression/tests/issue-1835.test.ts(1 hunks)
🔇 Additional comments (5)
tests/regression/tests/issue-1835.test.ts (3)
1-4: LGTM: Test structure follows best practices
The test setup is clean and follows standard Jest/testing conventions with proper describe/it blocks and async handling.
22-25: LGTM: Test configuration is appropriate
The PostgreSQL provider with pushDb: false is suitable for this test case as we're testing schema validation rather than actual database operations.
7-16: Verify test coverage for enum reference scenarios
The test case defines an enum that's only referenced in an ignored model (@@ignore). While this tests one scenario, consider adding additional test cases for:
- Enums referenced in multiple models
- Enums referenced in models with different directives
- Completely unused enums
Would you like me to help generate additional test cases to improve coverage?
✅ Verification successful
Let me analyze the test coverage patterns for enums with this broader search approach.
Test coverage for enum references is comprehensive
Based on the search results, the codebase already has good test coverage for enum references across various scenarios:
-
Enums referenced in multiple models:
issue-1745.test.tshas multiple enums (BuyerType,ChainStore) used in modelsissue-1381.test.tstests multiple enums (MemberRole,SpaceType) in related models
-
Enums with different directives:
issue-1210.test.tstests enum with@defaultand@denydirectivesissue-1257.test.tstests enum with@@authdirectiveissue-714.test.tstests enum in relation fields
-
Unused enums:
issue-674.test.tsexplicitly tests an unused enum withMyUnUsedEnum- Current test case (
issue-1835.test.ts) covers enum in ignored model
The existing test suite comprehensively covers the suggested scenarios, making additional test cases unnecessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are other test cases covering enum references
rg -l "enum.*model.*@@ignore" tests/
Length of output: 38
Script:
#!/bin/bash
# Search for enum definitions and their usage patterns in test files
rg -A 10 "enum\s+\w+" tests/ --type ts
# Also search for test files containing both enum and model definitions
rg -l "enum.*\n.*model" tests/ --type ts
Length of output: 16814
packages/schema/src/plugins/enhancer/policy/utils.ts (2)
11-11: LGTM: Import addition is clean and well-placed
The new import is properly grouped with other imports from the same package and is utilized in the code.
541-553: LGTM: Enhanced enum reference checking implementation
The implementation properly extends enum reference checking to all related data models, which should fix the issue with unused enums in generated policy code. The logic maintains thorough checking for both field types and direct enum references.
Let's verify the performance impact of checking all data models:
No description provided.