feat: implement zmodel import

[ymc9]

No description provided.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing Touches

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/import

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Explain this complex logic.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai explain this code block.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and explain its main purpose.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai generate unit tests to generate unit tests for this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[claude]

Claude finished @ymc9's task —— View job

---

PR Review: ZModel Import Feature

I've completed my analysis of this PR implementing ZModel import functionality. Here's my comprehensive review:

Review Tasks

• Analyze core language implementation in packages/language/
• Review CLI integration changes
• Examine test coverage and quality
• Check for potential bugs and security issues
• Assess performance implications
• Provide comprehensive feedback

Overall Assessment

This PR successfully implements import functionality for ZModel files, allowing users to split their schemas across multiple files and import them. The implementation follows a clean architecture with proper separation of concerns.

Code Quality & Best Practices

✅ Good:

• Clean separation between import resolution logic and document loading
• Proper use of TypeScript types and error handling
• Good use of existing Langium framework patterns
• Consistent code style with the existing codebase
• Well-structured recursive import handling

⚠️ Areas for improvement:

• packages/language/src/index.ts:168-185: The linkContentToContainer function manually walks AST nodes and mutates container references. Consider if Langium provides built-in utilities for this.
• packages/language/src/utils.ts:441-447: Using require.resolve for module resolution - this ties the implementation to Node.js CommonJS. Consider using import.meta.resolve for better ESM compatibility.

Potential Bugs & Issues

🐛 Critical Issues:

1. Race condition in import loading (packages/language/src/index.ts:65-66): The loadImports function loads imported documents in parallel but doesn't wait for their own imports to be resolved before building:

   const importedDocuments = await Promise.all(importedURIs.map((uri) => langiumDocuments.getOrCreateDocument(uri)));

   This could cause issues with transitive imports.

2. File extension mutation (packages/language/src/utils.ts:418-420): The import path is mutated in place:

   if (!imp.path.endsWith('.zmodel')) {
       imp.path += '.zmodel';
   }

   This modifies the AST node directly, which could cause issues with IDE features or caching.

⚠️ Minor Issues:
3. Silent failures in resolveImport (packages/language/src/utils.ts:409-411): The function catches all exceptions and returns undefined, which could hide important errors like permission issues or malformed files.

4. Case sensitivity handling (packages/language/src/utils.ts:378,382): The path normalization uses toLowerCase() which may not work correctly on case-sensitive filesystems.

Performance Considerations

✅ Good:

• Proper cycle detection in import resolution prevents infinite loops
• Documents are cached and reused effectively
• Parallel loading of independent imports

⚠️ Concerns:

• Multiple file system reads for the same file could occur if imports are resolved multiple times
• No memoization of resolveImportUri results
• The findNodeModulesFile function uses require.resolve which can be expensive

Security Concerns

🔒 Potential Issues:

1. Path traversal vulnerability (packages/language/src/utils.ts:434-435): The path.resolve(dir, imp.path) could potentially be exploited with ../../../ patterns to access files outside the project directory.

2. Node modules access: The findNodeModulesFile function allows importing from node_modules, which could be a vector for dependency confusion attacks.

Recommendations:

• Add path validation to prevent directory traversal
• Consider adding a whitelist of allowed import locations
• Validate that resolved paths stay within expected boundaries

Test Coverage

✅ Excellent:

• Comprehensive tests covering basic imports, cyclic imports, and symbol resolution
• Both language-level and runtime integration tests
• Tests use temporary directories appropriately
• Good coverage of edge cases like cyclic imports

📝 Missing coverage:

• Error cases (malformed imports, missing files, permission errors)
• Node modules import functionality
• Import path edge cases (absolute paths, different extensions)
• Performance tests for large import graphs

Specific Code Issues

1. packages/language/test/import.test.ts:77-79: There's a duplicate fs.writeFileSync for the same file (b.zmodel) which overwrites the first write. This appears to be a copy-paste error.

2. packages/language/src/index.ts:115-122: The validationAfterImportMerge function could be more robust - it only checks for multiple datasources and auth declarations but doesn't validate other constraints that might be violated by imports.

Recommendations

1. Fix the race condition in import loading by ensuring transitive imports are resolved before building
2. Add path validation to prevent directory traversal attacks
3. Improve error handling in import resolution with more specific error messages
4. Add comprehensive error case testing
5. Consider performance optimizations like memoizing import resolution results
6. Fix the duplicate file write in the test case

Overall Score: B+

This is a solid implementation of an important feature. The core logic is sound and the test coverage is good. However, there are some critical issues around race conditions and security that should be addressed before merge.

---

[Copilot]

Pull Request Overview

This PR implements zmodel import functionality, allowing ZModel schema files to import declarations from other zmodel files. The implementation includes import resolution, transitive import handling, cyclic import support, and proper merging of imported declarations into the main document.

• Import directive parsing and resolution with support for relative paths
• Transitive and cyclic import handling with proper document loading
• Declaration merging from imported files with validation for conflicting constructs

Reviewed Changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
pnpm-workspace.yaml	Adds tmp and @types/tmp dependencies to catalog for test utilities
packages/testtools/src/schema.ts	Adds generateTsSchemaInPlace function and extracts compileAndLoad helper
packages/testtools/package.json	Updates tmp dependencies to use catalog versions
packages/runtime/test/utils.ts	Modifies createTestClient to support custom workDir parameter
packages/runtime/test/client-api/import.test.ts	Adds comprehensive test for import functionality with circular references
packages/language/test/import.test.ts	Adds unit tests for import resolution, symbol resolution, and cyclic imports
packages/language/src/validators/schema-validator.ts	Moves importPath variable declaration to fix undefined variable usage
packages/language/src/utils.ts	Adds utility functions getDocument and findRootNode for AST navigation
packages/language/src/index.ts	Implements core import loading, merging, and validation logic
packages/language/package.json	Adds tmp and @types/tmp as dev dependencies
packages/cli/test/ts-schema-gen.test.ts	Adds test for TypeScript schema generation with imports
packages/cli/src/actions/action-utils.ts	Adds datasource validation and warning display
packages/cli/package.json	Updates tmp dependencies to use catalog versions

Files not reviewed (1)

• pnpm-lock.yaml: Language not supported

[claude]

Claude finished @ymc9's task —— View job

---

PR Review: ZModel Import Feature

I've completed my analysis of this ZModel import implementation. Here's my comprehensive review:

Review Tasks

• Read repository guidelines and understand codebase structure
• Analyze core language implementation in packages/language/
• Review CLI integration changes
• Examine test coverage and quality
• Check for potential bugs and security issues
• Assess performance implications
• Provide comprehensive feedback

Overall Assessment

This PR successfully implements import functionality for ZModel files, allowing users to split their schemas across multiple files and import them. The implementation follows a clean architecture with proper separation of concerns between import resolution, document loading, and AST merging.

Code Quality & Best Practices

✅ Good:

• Clean separation between import resolution logic and document loading
• Proper use of TypeScript types and error handling
• Good use of existing Langium framework patterns
• Consistent code style with the existing codebase
• Well-structured recursive import handling with cycle detection

⚠️ Areas for improvement:

• packages/language/src/index.ts:167-184: The linkContentToContainer function manually walks AST nodes and mutates container references. Consider if Langium provides built-in utilities for this.
• Path resolution logic could be more robust with better error messages

Critical Bugs Found

🐛 Test Bug (packages/language/test/import.test.ts:77-79):
There's a stray closing brace } and duplicate fs.writeFileSync for the same file (b.zmodel) which overwrites the first write. This appears to be a copy-paste error that needs fixing.

🐛 Race Condition (packages/language/src/index.ts:65-69):
The loadImports function loads imported documents in parallel but doesn't wait for their own imports to be resolved before building:

for (const uri of importedURIs) {
    importedDocuments.push(await langiumDocuments.getOrCreateDocument(uri));
}

This could cause issues with transitive imports where imported documents haven't loaded their own dependencies yet.

🐛 Incomplete AST Linking (packages/language/src/index.ts:167-184):
The linkContentToContainer function doesn't handle nested structures recursively. It should call itself recursively for nested AST nodes to ensure all nodes are properly linked.

Security Concerns

🔒 Path Traversal Vulnerability (packages/language/src/utils.ts:422):
The path.resolve(dir, importPath) could potentially be exploited with ../../../ patterns to access files outside the project directory. Consider adding path validation to prevent directory traversal attacks.

🔒 Silent Error Handling (packages/language/src/utils.ts:409-412):
The resolveImport function catches all exceptions and returns undefined, which could hide important errors like permission issues or malformed files. More specific error handling would be beneficial.

Performance Considerations

✅ Good:

• Proper cycle detection prevents infinite loops
• Documents are cached and reused effectively
• Parallel loading of independent imports

⚠️ Potential optimizations:

• No memoization of resolveImportUri results for repeated imports
• Multiple file system reads could occur if the same import is resolved multiple times
• Case-sensitive filesystem handling could be improved

Test Coverage

✅ Excellent:

• Comprehensive tests covering basic imports, cyclic imports, and symbol resolution
• Both language-level and runtime integration tests
• Tests use temporary directories appropriately
• Good coverage of the happy path scenarios

📝 Missing coverage:

• Error cases (malformed imports, missing files, permission errors)
• Import path edge cases (absolute paths, different extensions)
• Node modules import functionality (if supported)
• Performance tests for large import graphs

Specific Recommendations

1. Fix the test bug by removing the stray closing brace and duplicate file write in packages/language/test/import.test.ts:77-79

2. Address the race condition by ensuring transitive imports are fully loaded before building documents

3. Improve AST linking by making linkContentToContainer recursive for nested nodes

4. Add path validation to prevent directory traversal attacks in import resolution

5. Enhance error handling with more specific error messages for different failure modes

6. Add comprehensive error case testing to cover malformed imports, missing files, and permission issues

Security & Safety

The implementation generally follows good practices but needs attention to path validation. The automatic .zmodel extension addition is good for UX but should be combined with proper path sanitization.

Overall Score: B+

This is a solid implementation of an important feature. The core logic is sound and follows good architectural patterns. The test coverage for the happy path is excellent. However, there are some critical issues around race conditions, test bugs, and security that should be addressed before merge.

The implementation successfully enables the key functionality of splitting ZModel schemas across multiple files while maintaining proper symbol resolution and cycle detection.

---

[claude]

Claude finished @ymc9's task —— View job

---

PR Review: ZModel Import Feature

I've completed my analysis of this ZModel import implementation. Here's my comprehensive review:

Review Tasks

• Read repository guidelines and understand codebase structure
• Analyze core language implementation in packages/language/
• Review CLI integration changes
• Examine test coverage and quality
• Check for potential bugs and security issues
• Assess performance implications
• Provide comprehensive feedback

Overall Assessment

This PR successfully implements import functionality for ZModel files, allowing users to split their schemas across multiple files and import them. The implementation follows a clean architecture with proper separation of concerns between import resolution, document loading, and AST merging.

Code Quality & Best Practices

✅ Good:

• Clean separation between import resolution logic and document loading
• Proper use of TypeScript types and error handling
• Good use of existing Langium framework patterns
• Consistent code style with the existing codebase
• Well-structured recursive import handling with cycle detection

⚠️ Areas for improvement:

• packages/language/src/index.ts:167-184: The linkContentToContainer function manually walks AST nodes and mutates container references. Consider if Langium provides built-in utilities for this.
• Path resolution logic could be more robust with better error messages

Critical Bugs Found

🐛 Test Bug (packages/language/test/import.test.ts:77-79):
There's a stray closing brace } and duplicate fs.writeFileSync for the same file (b.zmodel) which overwrites the first write. This appears to be a copy-paste error that needs fixing.

🐛 Race Condition (packages/language/src/index.ts:65-69):
The loadImports function loads imported documents in parallel but doesn't wait for their own imports to be resolved before building:

for (const uri of importedURIs) {
    importedDocuments.push(await langiumDocuments.getOrCreateDocument(uri));
}

This could cause issues with transitive imports where imported documents haven't loaded their own dependencies yet.

🐛 Incomplete AST Linking (packages/language/src/index.ts:167-184):
The linkContentToContainer function doesn't handle nested structures recursively. It should call itself recursively for nested AST nodes to ensure all nodes are properly linked.

Security Concerns

🔒 Path Traversal Vulnerability (packages/language/src/utils.ts:422):
The path.resolve(dir, importPath) could potentially be exploited with ../../../ patterns to access files outside the project directory. Consider adding path validation to prevent directory traversal attacks.

🔒 Silent Error Handling (packages/language/src/utils.ts:409-412):
The resolveImport function catches all exceptions and returns undefined, which could hide important errors like permission issues or malformed files. More specific error handling would be beneficial.

Performance Considerations

✅ Good:

• Proper cycle detection prevents infinite loops
• Documents are cached and reused effectively
• Parallel loading of independent imports

⚠️ Potential optimizations:

• No memoization of resolveImportUri results for repeated imports
• Multiple file system reads could occur if the same import is resolved multiple times
• Case-sensitive filesystem handling could be improved

Test Coverage

✅ Excellent:

• Comprehensive tests covering basic imports, cyclic imports, and symbol resolution
• Both language-level and runtime integration tests
• Tests use temporary directories appropriately
• Good coverage of the happy path scenarios

📝 Missing coverage:

• Error cases (malformed imports, missing files, permission errors)
• Import path edge cases (absolute paths, different extensions)
• Node modules import functionality (if supported)
• Performance tests for large import graphs

Specific Recommendations

1. Fix the test bug by removing the stray closing brace and duplicate file write in packages/language/test/import.test.ts:77-79

2. Address the race condition by ensuring transitive imports are fully loaded before building documents

3. Improve AST linking by making linkContentToContainer recursive for nested nodes

4. Add path validation to prevent directory traversal attacks in import resolution

5. Enhance error handling with more specific error messages for different failure modes

6. Add comprehensive error case testing to cover malformed imports, missing files, and permission issues

Security & Safety

The implementation generally follows good practices but needs attention to path validation. The automatic .zmodel extension addition is good for UX but should be combined with proper path sanitization.

Overall Score: B+

This is a solid implementation of an important feature. The core logic is sound and follows good architectural patterns. The test coverage for the happy path is excellent. However, there are some critical issues around race conditions, test bugs, and security that should be addressed before merge.

The implementation successfully enables the key functionality of splitting ZModel schemas across multiple files while maintaining proper symbol resolution and cycle detection.

---