fix: monorepo's wrong auth error #2295
Conversation
📝 WalkthroughWalkthroughRefactoring of data-model resolution logic to use a narrower scope. New utility functions added to resolve data models from the current node and reachable documents, replacing the broader all-loaded-documents approach. Call sites updated to use the new functions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/schema/src/language-server/zmodel-workspace-manager.ts (3)
163-178: Consider whether the file existence check is too restrictive.For relative paths, the code uses
fs.existsSyncto validate entries before including them. This approach means entries pointing to files that don't exist yet (e.g., generated files, files not yet created in a new monorepo setup) will be silently ignored.If entry documents are always expected to exist at configuration time, this is fine. However, if users might configure entries for files that will be created later, consider one of these alternatives:
- Skip the existence check and resolve relative paths directly
- Add a configuration option to control this behavior
- Document this requirement clearly
139-193: Verify that entry URIs reference loaded documents.The configuration reading logic resolves and stores entry URIs, but there's no validation that these URIs:
- Reference documents that were actually loaded into the workspace
- Have the correct file extension (
.zmodel)- Are accessible to the language server
Without validation, users might configure entries that point to non-existent, unloaded, or invalid documents, which could cause issues downstream when these entry URIs are used.
Consider adding validation after line 185:
// Validate that entry URIs reference loaded documents for (const uri of this.entryDocumentUris) { const parsedUri = URI.parse(uri); if (!this.langiumDocuments.hasDocument(parsedUri)) { console.warn(`Entry document not found in workspace: ${uri}`); } // Optionally check file extension const fileName = Utils.basename(parsedUri); if (!fileName.endsWith('.zmodel')) { console.warn(`Entry document is not a .zmodel file: ${uri}`); } }Would you like me to help implement this validation?
191-193: Improve error logging for debugging.The catch block logs a generic warning but doesn't include error details. This makes it difficult to diagnose configuration issues.
Apply this diff:
} catch (err) { - console.warn('Failed to read zmodel.entries configuration:', err); + console.warn('Failed to read zmodel.entries configuration:', err instanceof Error ? err.message : String(err)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/schema/package.jsonis excluded by!**/*.json
📒 Files selected for processing (1)
packages/schema/src/language-server/zmodel-workspace-manager.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/schema/src/language-server/zmodel-workspace-manager.ts (1)
packages/language/src/generated/module.ts (1)
ZModelLanguageMetaData(10-14)
🔇 Additional comments (3)
packages/schema/src/language-server/zmodel-workspace-manager.ts (3)
3-15: LGTM: Imports are properly structured.The addition of
ConfigurationProviderandZModelLanguageMetaDataimports supports the new entry configuration feature, and the formatting improvements enhance readability.
21-27: LGTM: ConfigurationProvider properly initialized.The constructor correctly initializes the
ConfigurationProviderfromLangiumSharedServices, following standard Langium patterns.
29-38: LGTM: Well-designed public API.The public methods provide clean access to entry document information:
isEntryDocumenthandles both string and URI types.getEntryDocumentUrisreturns a copy to prevent external mutation.Good encapsulation practices.
cqh963852
force-pushed
the
feat/workspaces
branch
from
d78acfd to
92f4db4
Compare
cqh963852
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/schema/src/utils/ast-utils.ts (1)
289-313: Consider refactoring to reduce duplication and improve performance.This new function duplicates significant logic from
getAllLoadedAndReachableDataModelsAndTypeDefs(lines 318-339). The only difference is the starting set of models. Additionally, usingincludes()for deduplication is O(n²) in the worst case.Consider this refactor to eliminate duplication and improve performance:
+function getDataModelsAndTypeDefsWithTransitiveImports( + langiumDocuments: LangiumDocuments, + initialModels: (DataModel | TypeDef)[], + fromModel?: DataModel +) { + const result = [...initialModels]; + const seen = new Set(initialModels); + + if (fromModel) { + const model = getContainerOfType(fromModel, isModel); + if (model) { + const transitiveDataModels = getAllDataModelsIncludingImports(langiumDocuments, model); + transitiveDataModels.forEach((dm) => { + if (!seen.has(dm)) { + seen.add(dm); + result.push(dm); + } + }); + } + } + + return result; +} export function getCurrentNodeAndReachableDataModelsAndTypeDefs( langiumDocuments: LangiumDocuments, node: AstNode, fromModel?: DataModel ) { const document = getDocument(node); - const allDataModels = (document.parseResult.value as Model).declarations.filter( + const initialModels = (document.parseResult.value as Model).declarations.filter( (d): d is DataModel | TypeDef => isDataModel(d) || isTypeDef(d) ); - - if (fromModel) { - // merge data models transitively reached from the current model - const model = getContainerOfType(fromModel, isModel); - if (model) { - const transitiveDataModels = getAllDataModelsIncludingImports(langiumDocuments, model); - transitiveDataModels.forEach((dm) => { - if (!allDataModels.includes(dm)) { - allDataModels.push(dm); - } - }); - } - } - - return allDataModels; + return getDataModelsAndTypeDefsWithTransitiveImports(langiumDocuments, initialModels, fromModel); } export function getAllLoadedAndReachableDataModelsAndTypeDefs( langiumDocuments: LangiumDocuments, fromModel?: DataModel ) { - // get all data models from loaded documents - const allDataModels = getAllLoadedDataModelsAndTypeDefs(langiumDocuments); - - if (fromModel) { - // merge data models transitively reached from the current model - const model = getContainerOfType(fromModel, isModel); - if (model) { - const transitiveDataModels = getAllDataModelsIncludingImports(langiumDocuments, model); - transitiveDataModels.forEach((dm) => { - if (!allDataModels.includes(dm)) { - allDataModels.push(dm); - } - }); - } - } - - return allDataModels; + const initialModels = getAllLoadedDataModelsAndTypeDefs(langiumDocuments); + return getDataModelsAndTypeDefsWithTransitiveImports(langiumDocuments, initialModels, fromModel); }This refactor:
- Eliminates code duplication between the two functions
- Uses a Set for O(1) deduplication checks
- Makes the shared logic explicit and reusable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/schema/src/language-server/zmodel-linker.ts(2 hunks)packages/schema/src/language-server/zmodel-scope.ts(2 hunks)packages/schema/src/utils/ast-utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/schema/src/language-server/zmodel-scope.ts (1)
packages/schema/src/utils/ast-utils.ts (1)
getCurrentNodeAndReachableDataModelsAndTypeDefs(289-313)
packages/schema/src/language-server/zmodel-linker.ts (1)
packages/schema/src/utils/ast-utils.ts (1)
getCurrentNodeAndReachableDataModelsAndTypeDefs(289-313)
packages/schema/src/utils/ast-utils.ts (2)
packages/language/src/ast.ts (3)
AstNode(4-4)AstNode(78-83)DataModel(56-66)packages/language/src/generated/ast.ts (9)
DataModel(277-287)DataModel(289-289)Model(503-507)Model(509-509)TypeDef(639-646)TypeDef(648-648)isDataModel(291-293)isTypeDef(650-652)isModel(511-513)
🔇 Additional comments (3)
packages/schema/src/language-server/zmodel-scope.ts (1)
231-238: LGTM - Function call correctly updated.The call site has been properly updated to use the new function signature with the
nodeparameter. The use ofgetContainerOfType(node, isDataModel)to derivefromModelis appropriate.packages/schema/src/language-server/zmodel-linker.ts (1)
286-292: LGTM - Consistent with scope provider changes.The update to use
getCurrentNodeAndReachableDataModelsAndTypeDefsis implemented correctly and consistently with the changes inzmodel-scope.ts. The function parameters are passed appropriately.packages/schema/src/utils/ast-utils.ts (1)
289-313: Change is intentional and working as designed—no issues found.Git history shows this is a recent fix commit ("only get current node and imports if getAuthDecl") that narrows auth() resolution to the current document and its transitive imports. The
multi-file.test.tsintegration test confirms this works correctly across multiple schemas when files import each other, which is the intended pattern for monorepo support. All existing tests pass with this narrower scope.
|
Excuse me, could you please review my changes? Do you think they are reasonable, and would such changes cause any disruptive changes? |
|
Hi @cqh963852 , thanks for making this PR! The original design was for a convenience concern. Consider the following example: model Post {
...
@@allow('all', auth().role == 'ADMIN')
}The For your specific problem, does the resolution issue only happen inside IDE, or with the |
Only happened inside vscode. ZenStack Generate works correctly. |
Could we adjust this design? maybe
{
"zmodel.entries":["package/abc", "package/def"]
}package/abc/post.zmodel ,only load package/abc's documents package/def/post.zmodel ,only load package/def's documents |
2 participants
Change
When performing auth() expression validation, the current approach is to retrieve the relevant decl data from all documents.
However, this is prone to errors in a monorepo, as described in the issue.
Therefore, my improvement is to start from the current document, retrieve all documents corresponding to the imports, and then parse them.
related issue #1532