-
-
Notifications
You must be signed in to change notification settings - Fork 89
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
fix(zmodel): check cyclic inheritance #1931
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
seen
set parameter works well to prevent infinite recursion. One minor enhancement is to renameseen
tovisited
and 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.ref
might 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