fix(config): implement circular reference detection#21
Merged
Conversation
leaperone-bot
left a comment
There was a problem hiding this comment.
🤖 Code Review 🟡
总体评价
本 PR 将原来的循环引用检测空壳(stub)实现为了真正的递归检测逻辑,方向正确。但存在一个语义层面的问题需要讨论。
🔶 主要问题:跨项目引用的误判
extractReferencedKey 中对 @username/project/key 格式的处理存在语义问题:
typescript
// @username/project/key 格式:提取最后一段作为 key
const match = target.match(/^@[^/]+/[^/]+/([^/]+)$/);
if (match) {
const key = match[1];
if (key in config.env) {
return key;
}
}
当 target 为 @other-user/other-project/DB_URL 时,如果本地 config 恰好也有一个 DB_URL 变量,该方法会错误地将其视为对本地 DB_URL 的引用。这会导致假阳性的循环引用错误。
示例场景:
env:
DB_URL:
target: "@team/shared-config/DB_URL" # 引用外部项目这里 DB_URL 引用的是外部项目的值,但当前逻辑会认为它引用了自身,从而报循环引用错误。
建议:应该判断 @username/project 是否指向当前项目自身,只有自引用才应该纳入循环检测。对于外部项目引用应直接返回 null。如果当前无法获取本项目的 namespace/project 信息,那么更安全的做法是:对 @username/project/key 格式一律不做本地循环引用检测(因为大多数情况下这是跨项目引用),仅对"直接键名引用"做循环检测。
✅ 正确的部分
- DFS + visited 回溯:
visited.add→ 递归 →visited.delete的模式正确实现了有向图的环检测 - 对非对象类型的处理:当
config.env[key]是字符串(EnvTarget)时,typeof current === 'object'为 false,不会递归,逻辑正确 - 对不可追踪 target 的处理:
local、<url>和@user/project(无 key)格式会返回null,不进入递归,这是合理的
📝 其他建议
- 缺少测试:PR 描述中列出了测试计划但以 checkbox 形式呈现,仓库中没有自动化测试。建议至少补充单元测试覆盖以下场景:
- A → B → A 直接循环
- A → B → C → A 传递循环
- 外部项目引用(不应报循环)
- 自引用(A → A)
- 错误提示可以更详细:当前循环引用错误只提示了起始 key,可以考虑输出完整的引用链路(如
A -> B -> C -> A),便于用户排查
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #9
Test plan