Support for object, array and enum in ruleset builder #1025
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Parser
participant RulesetBuilder
participant TypeStack
participant Ruleset
Parser->>RulesetBuilder: Parse schema
loop For each type/property/union/literal
RulesetBuilder->>TypeStack: Push context (type/property/union)
TypeStack-->>RulesetBuilder: Current context
RulesetBuilder->>TypeStack: Pop context on exit
end
RulesetBuilder->>RulesetBuilder: build()
RulesetBuilder->>Ruleset: expandType("Document")
Ruleset-->>RulesetBuilder: Flat array of rules
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/schema/src/rulesets.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in ".eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. packages/schema/test/ruleset.test.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in ".eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. packages/schema/test/validator.test.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in ".eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code Graph Analysis (1)packages/schema/test/ruleset.test.ts (2)
🔇 Additional comments (22)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 comments (1)
packages/schema/src/rulesets.ts (1)
40-47: Missing 'null' primitive type definitionThe test file uses a 'null' type at line 51, but it's not included in the
PrimitiveTypedefinition. This will cause the parser to fail when encountering null types.Add 'null' to the PrimitiveType definition:
export type PrimitiveType = | 'boolean' | 'integer' | 'double' | 'long' | 'string' | 'date' - | 'bytes'; + | 'bytes' + | 'null';
🧹 Nitpick comments (2)
packages/schema/src/rulesets.ts (1)
296-302: Consider type consistency between UnionRule and expandTypeThe
UnionRuletype definition usestype: 'union', but theexpandTypemethod outputstype: 'enum'. While this aligns with treating enums as literal unions per the PR objectives, it creates a mismatch between the type definition and actual output.Either update the type definition to match the output:
export type UnionRule = { - type: 'union'; + type: 'enum'; values: Array<string | number | boolean>; } & RuleBase;Or update the RuleType to reflect this:
export type RuleType = | 'object' | 'array' - | 'union' | PrimitiveType | YorkieType | 'enum';Also applies to: 85-88
packages/schema/test/ruleset.test.ts (1)
363-434: Consider adding tests for edge casesThe current tests cover the happy path well, but consider adding tests for:
- Circular type references
- Duplicate type definitions
- Invalid literal values in unions
- Empty union types
Would you like me to generate additional test cases for these edge cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/schema/src/rulesets.ts(3 hunks)packages/schema/test/ruleset.test.ts(3 hunks)
🔇 Additional comments (1)
packages/schema/test/ruleset.test.ts (1)
102-140: Good test coverage for new featuresThe activation of these two test cases properly validates the enhanced functionality:
- Nested objects regardless of declaration order - confirms the Map-based approach works correctly
- Enum types with string and number literals - validates the union/enum type handling
Also applies to: 363-381
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/schema/src/rulesets.ts (2)
145-150: Duplicate type definition validation still missing.The previous review comment about adding validation for duplicate type definitions has not been addressed. This could lead to silent overwrites of type definitions.
Add validation to prevent duplicate type definitions:
exitTypeAliasDeclaration() { if (this.currentTypeName && this.typeStack.length > 0) { const typeDef = this.typeStack.pop()!; + if (this.typeDefinitions.has(this.currentTypeName)) { + throw new Error(`Duplicate type definition: ${this.currentTypeName}`); + } this.typeDefinitions.set(this.currentTypeName, typeDef); } this.currentTypeName = undefined; }
276-338: Circular reference detection still missing.The previous review comment about adding circular reference detection in
expandTypehas not been addressed. This could lead to stack overflow errors with circular type dependencies.Add circular reference detection as suggested in the previous review:
private expandType( typeDef: TypeDefinition, path: string, rules: Array<Rule>, + visited: Set<string> = new Set(), ): void { + // For reference types, check circular dependencies + if (typeDef.kind === 'reference') { + if (visited.has(typeDef.typeName)) { + console.warn(`Circular reference detected: ${typeDef.typeName}`); + return; + } + visited.add(typeDef.typeName); + } + switch (typeDef.kind) { // ... existing cases ... case 'reference': { const referencedType = this.typeDefinitions.get(typeDef.typeName); if (referencedType) { - this.expandType(referencedType, path, rules); + this.expandType(referencedType, path, rules, visited); } else { console.warn(`Type reference not found: ${typeDef.typeName}`); } + visited.delete(typeDef.typeName); break; } // Update recursive calls for properties case 'object': { // ... existing object handling ... for (const property of typeDef.properties) { const propertyPath = `${path}.${property.name}`; - this.expandType(property.type, propertyPath, rules); + this.expandType(property.type, propertyPath, rules, new Set(visited)); } break; } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/schema/src/rulesets.ts(3 hunks)
🔇 Additional comments (2)
packages/schema/src/rulesets.ts (2)
34-60: LGTM! Clean type system enhancement.The addition of
EnumRuleand the updatedRuleTypeunion properly supports the new enum functionality while maintaining type safety.
85-117: Well-designed internal type system.The
TypeDefinitionandPropertyDefinitiontypes provide a clean abstraction for building the hierarchical schema representation before expansion.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1025 +/- ##
==========================================
+ Coverage 78.10% 78.14% +0.03%
==========================================
Files 66 66
Lines 5754 5764 +10
Branches 1041 1043 +2
==========================================
+ Hits 4494 4504 +10
+ Misses 951 950 -1
- Partials 309 310 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sigmaith
changed the title
hackerwins
changed the title
hackerwins
changed the title
hackerwins
changed the title
What this PR does / why we need it?
Enhances the schema ruleset builder to support below types.
Introduces a Map-based approach to handle nested object types and type references, allowing for order-independent type definition resolution.
Introduces a
visitedSet,currentTypeNameto prevent infinite recursion.Any background context you want to provide?
As I worked on it, I thought that the enum should be treated as a literal union.
This is the part that connects to the complex union test case later.
Please let me know if I missed anything.
What are the relevant tickets?
Related yorkie-team/yorkie#971
Checklist
Summary by CodeRabbit
New Features
Refactor
Tests