fix(cli): bogus validation error when merging imported enums #1861
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (1)
tests/regression/tests/issue-1849.test.ts (1)
4-23: Enhance test coverage with negative test cases.While the current test verifies the fix for using imported enums as default values, consider adding negative test cases to ensure proper validation errors are thrown for invalid scenarios.
Consider adding these test cases:
it('should fail with invalid enum value', async () => { await expect(loadSchema( `schema.zmodel import './enum' model Post { id Int @id status Status @default(INVALID_VALUE) } ${FILE_SPLITTER}enum.zmodel enum Status { PENDING PUBLISHED }`, { provider: 'postgresql', pushDb: false } )).rejects.toThrow(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/schema/src/cli/cli-util.ts(1 hunks)packages/schema/src/language-server/validator/utils.ts(0 hunks)tests/regression/tests/issue-1849.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/schema/src/language-server/validator/utils.ts
🔇 Additional comments (4)
tests/regression/tests/issue-1849.test.ts (2)
21-21: Configuration matches reported environment.
The test configuration correctly uses PostgreSQL as specified in issue #1849, with pushDb: false for isolation.
6-20: Verify schema structure matches the issue report.
The test schema correctly reproduces the scenario from issue #1849 where an imported enum is used as a default value.
Let's verify this matches the original issue:
✅ Verification successful
Test case structure is valid for verifying enum imports and defaults
The test case correctly verifies:
- Import of an enum from a separate file using relative import
- Usage of the imported enum as a field type
- Setting a default value from the imported enum
- Proper separation of schema files using FILE_SPLITTER
The test structure is appropriate for regression testing of enum imports and default values, even though we couldn't access the original issue. The test follows ZenStack's schema testing patterns and includes all necessary components to verify the functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the test schema matches the issue report
# Expected: Find similar schema structure in the issue description
gh issue view 1849 --json body | jq -r '.body' | grep -A 10 "schema.zmodel"
Length of output: 73
Script:
#!/bin/bash
# Let's try searching for the issue number in the test file and surrounding context
rg -B 5 -A 5 "1849" tests/regression/tests/issue-1849.test.ts
Length of output: 289
Script:
#!/bin/bash
# Let's check the entire test file content to understand the test case better
cat tests/regression/tests/issue-1849.test.ts
Length of output: 620
Script:
#!/bin/bash
# Let's search for any related test files or documentation about importing enums
rg -g "*.md" -g "*.test.ts" -i "import.*enum" --multiline
Length of output: 280
packages/schema/src/cli/cli-util.ts (2)
108-111: Verify the fix addresses the original issue.
The changes modify document cleanup and add validation, but we should verify they fully address the "Reference not resolved: Status" error mentioned in issue #1849.
#!/bin/bash
# Description: Verify the changes fix the enum reference resolution issue
# Search for test cases covering imported enum default values
rg -l "enum.*Status.*@default" tests/
# Check for any remaining "Reference not resolved" error messages in the codebase
rg "Reference not resolved.*Status"
# Look for similar patterns that might be affected
ast-grep --pattern 'enum $enumName {
$$$
}
model $modelName {
$$$
$field $type @default($enumValue)
$$$
}'108-109: Review the impact of the new document cleanup approach.
The change from using DocumentBuilder.update to a two-step cleanup process (direct deletion + index removal) might affect how imported declarations are handled. While this approach is more explicit, we should verify it doesn't introduce side effects for imported enum references.
✅ Verification successful
Document cleanup change is safe and consistent
The new two-step cleanup approach (using deleteDocument and IndexManager.remove) is a direct replacement for the previous DocumentBuilder.update pattern. Both approaches aim to clean up imported documents, but the new implementation is more explicit in handling both the document and its index. The search results show that:
- The old pattern used
DocumentBuilder.update([], [doc.uri])to remove documents - The new approach properly handles both document cleanup and index removal in a coordinated way
- No other parts of the codebase rely on the specific cleanup mechanism
This change maintains the same cleanup semantics while being more explicit about the operations being performed. The modification is an implementation detail that doesn't affect how imported declarations (including enums) are processed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any references to imported enums in the codebase
# that might be affected by the new document cleanup approach
# Search for enum imports and their usage patterns
rg -A 5 "import.*enum"
# Search for @default directives using enum values
rg -A 5 "@default.*enum"
Length of output: 195
Script:
#!/bin/bash
# Let's try a different approach to verify the document cleanup impact
# 1. Find all references to DocumentBuilder.update to understand the old pattern
rg -A 3 "DocumentBuilder\.update"
# 2. Check for any existing usage of deleteDocument and IndexManager.remove
rg -A 3 "deleteDocument|IndexManager\.remove"
# 3. Look for import-related code patterns
ast-grep --pattern 'import { $_ } from $_'
# 4. Search for enum validation or processing logic
rg -A 5 "validateEnum|processEnum|enum.*validation"
# 5. Check for any test cases related to enum imports
fd -e test.ts -e spec.ts -X rg -l "enum.*import"
Length of output: 1052
fixes #1849