feat: pipeline_tree静态检查增强 --story=130810130#758
Conversation
|
v_ghluguo seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
整体来看，这个 PR 将分散的 pipeline 校验逻辑重构为基于注册机制的 ValidatorHandler 架构，设计清晰、职责分明。以下是几点建议：
架构设计 ✨
- Handler + BasePipelineValidator +
__init_subclass__自动注册的模式很好，易于扩展新校验器。 ValidateType枚举 + GENERAL 兜底的设计合理。
关注点
validate_pipeline_tree_constants中re.search(re.escape(key), value)可简化为key in value（因为 escape 后就是纯文本匹配）。ConstantsValidator直接访问web_pipeline_tree["constants"]缺少异常保护——虽然 schema 要求该字段存在，但作为 GENERAL 类型校验器，它可能在SchemaValidator之前执行。broadcast_task_events的条件检查是合理的防御性编程。
总体质量良好，建议关注行内评论中标注的几处细节。
| # 检查当前参数值中引用的所有参数 | ||
| referenced_keys = set() | ||
| for other_key in constant_values: | ||
| if re.search(re.escape(other_key), value): |
There was a problem hiding this comment.
⚡ re.search(re.escape(other_key), value) 等价于纯文本匹配，可直接用 other_key in value，避免 regex 编译开销（O(n²) 循环中每次都调用 re.search + re.escape）。
| @classmethod | ||
| def validate(cls, web_pipeline_tree: dict) -> ValidatorResult: | ||
| try: | ||
| validate_pipeline_tree_constants(web_pipeline_tree["constants"]) |
There was a problem hiding this comment.
ValidatorHandler.validate 遍历 __hub dict，校验器执行顺序取决于注册顺序。ConstantsValidator (GENERAL) 直接访问 web_pipeline_tree["constants"]，如果在 SchemaValidator 之前执行且输入缺少 constants 字段，会抛出 KeyError 而非友好的校验错误。建议用 .get("constants", {}) 防御或确保执行顺序。
| @classmethod | ||
| def validate(cls, web_pipeline_tree: dict, validate_type: Optional[ValidateType] = None): | ||
| validators_to_run = [] | ||
| for validator_name, validator_cls in cls.__hub.items(): |
There was a problem hiding this comment.
ValidatorHandler.validate 中校验器的执行顺序依赖 __hub dict 的插入顺序（即 apps.ready() 中的 import 顺序）。建议在 BasePipelineValidator 中增加 priority 属性，validate 方法按 priority 排序后执行，避免隐式的顺序依赖。
| class ValidatorHandler: | ||
| """校验器处理器""" | ||
|
|
||
| __hub = {} |
There was a problem hiding this comment.
✨ __hub 作为可变类变量在测试场景下可能存在状态污染（上一个测试注册的 validator 会影响下一个）。考虑提供 clear() 或 unregister() 类方法供测试清理使用。
|
|
||
| class MakoKeywordValidator(BasePipelineValidator): | ||
| name = "mako_keyword_validator" | ||
| validate_type = ValidateType.TASK.value |
There was a problem hiding this comment.
MakoKeywordValidator 的 validate_type 是 TASK，但 Mako 保留关键字检查在模板保存阶段同样有意义（模板编辑时就应该阻止用户使用保留字命名变量）。是否应该改为 GENERAL？
| WHITE, GRAY, BLACK = 0, 1, 2 | ||
| color = {node: WHITE for node in graph} | ||
|
|
||
| def has_cycle(node, path): |
There was a problem hiding this comment.
has_cycle 使用递归实现，如果常量间依赖链很深（如 A→B→C→...→Z），会导致 Python 默认递归深度限制（1000）的风险。对于极端用户输入场景，考虑改用迭代式拓扑排序（Kahn's algorithm）检测环路。
| from pipeline.parser.pipeline_parser import PipelineParser | ||
|
|
||
| from bkflow.constants import ValidateType | ||
| from bkflow.pipeline_validate.handler import ValidatorHandler |
There was a problem hiding this comment.
✨ 旧的 validate_web_pipeline_tree 函数（bkflow/pipeline_web/parser/validator.py）已无调用方，建议在本 PR 中删除该文件或添加 deprecation 标注，避免后续维护者困惑。
| # 首先构建参数值映射表 | ||
| for key, const in constants.items(): | ||
| value = const.get("value") if isinstance(const, dict) else const | ||
| constant_values[key] = str(value) if value is not None else "" |
There was a problem hiding this comment.
validate_pipeline_tree_constants 将 value 通过 str(value) 转为字符串后做子串搜索。如果 value 是复杂对象（dict/list），str() 的输出格式不确定（如 {'key': '${var}'}），可能导致误判或漏判。建议只对 source_type == 'custom' 且 value 为字符串类型的常量做引用检查。
| @@ -0,0 +1,58 @@ | |||
| """ | |||
There was a problem hiding this comment.
✨ 项目规范要求新增 Python 文件顶部包含 # -*- coding: utf-8 -*- 及版权注释。本 PR 新增的文件缺少 # -*- coding: utf-8 -*- 行（虽然 Python 3 默认 UTF-8，但项目有此约定）。
There was a problem hiding this comment.
Incremental Review (commit 2f59571)
本次提交仅修复了测试文件中 mock 路径引用（validate_pipeline_tree → ValidatorHandler.validate），改动正确且对称。
前轮 Review 问题状态：前轮提出的 9 条建议（执行顺序、re.search 性能、递归深度、MakoKeywordValidator 作用域、str(value) 误判等）在本次提交中均未涉及修改，仍然 open。
本次未发现新的 Critical / Important 级别问题。
✨ 小建议：build_pipeline_tree 测试辅助方法重构为手工构造 dict 的方式更贴合 ValidatorHandler 的实际输入格式，测试覆盖更真实，改动合理。
There was a problem hiding this comment.
Incremental Review (commit b8133f2)
本次提交修复了前轮 review 中提出的问题并进行了代码重组：
已解决的前轮问题：
- ✅
re.search(re.escape(other_key), value)已简化为other_key in value，消除了 O(n²) 循环中不必要的 regex 编译开销 - ✅
MakoKeywordValidator从task.py移至general.py，代码组织更合理（校验器与同类校验器放在一起）
前轮仍 open 的建议（均为 Minor/改进级别，非阻塞）：
- 执行顺序依赖 dict 插入顺序（建议加
priority） ConstantsValidator缺少KeyError保护- 递归深度风险
str(value)对复杂对象的误判- 旧
validator.py文件未删除
本次提交未发现新的 Critical / Important 级别问题。 改动干净、对称，LGTM。
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #758 +/- ##
===========================================
+ Coverage 82.14% 83.06% +0.91%
===========================================
Files 296 313 +17
Lines 17925 18431 +506
===========================================
+ Hits 14725 15309 +584
+ Misses 3200 3122 -78 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3 participants
No description provided.