- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 126
fix(zmodel): check cyclic inheritance #1931
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| 📝 WalkthroughWalkthroughThis pull request introduces enhanced validation for circular inheritance in data models. The changes span multiple files, focusing on detecting and preventing circular inheritance scenarios. A new method  Changes
 Sequence DiagramsequenceDiagram
    participant DM as DataModelValidator
    participant Utils as RecursiveBasesUtil
    participant Model as DataModel
    DM->>Model: Start inheritance validation
    Model-->>DM: Provide super types
    DM->>Utils: Get recursive base models
    Utils->>Utils: Track seen models
    Utils-->>DM: Return base models
    DM->>DM: Check for circular inheritance
    alt Circular Inheritance Detected
        DM->>DM: Report validation error
    end
Possibly Related PRs
 Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command  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 ( | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/sdk/src/utils.ts (1)
547-564: Consider clarifying recursion approach and namingThe new
seenset parameter works well to prevent infinite recursion. One minor enhancement is to renameseentovisitedand mention in a docstring that this is effectively a DFS traversal for clarity.packages/schema/src/language-server/validator/datamodel-validator.ts (1)
414-433: Validate edge cases when superType references are undefinedThe BFS-like approach to detect circular inheritance works well. However, consider gracefully handling cases where
superType.refmight be undefined (e.g., if references are unresolved for any reason). Doing so can prevent possible runtime errors in rare scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
- packages/schema/src/language-server/validator/datamodel-validator.ts(2 hunks)
- packages/schema/tests/schema/validation/cyclic-inheritance.test.ts(1 hunks)
- packages/sdk/src/utils.ts(1 hunks)
🔇 Additional comments (2)
packages/schema/tests/schema/validation/cyclic-inheritance.test.ts (1)
1-39: Tests cover circular inheritance effectively
Both test cases provide robust coverage for detecting circular inheritance. The use of toContain with the expected error messages is straightforward and ensures that the error output is validated properly. Overall, this test suite is well-structured and accurate.
packages/schema/src/language-server/validator/datamodel-validator.ts (1)
36-39: Inheritance check trigger is correct
Invoking validateInheritance only when superTypes.length > 0 is an efficient check. This ensures we skip unnecessary processing for models without super types.
fixes #1229