-
Notifications
You must be signed in to change notification settings - Fork 18
feat: 支持按照插件格式返回流程配置 --story=130216170 @dengyh #594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: 支持按照插件格式返回流程配置 --story=130216170 @dengyh #594
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
代码审查总结
本次提交新增了 format=plugin 参数支持,核心功能正常。发现以下需要关注的问题:
关键问题
- 🔒 输入验证不足 -
parse_pipeline_tree_to_plugin_schema等函数缺少对空值的防护 ⚠️ 字符串处理不精确 - 使用.strip("${}")可能导致意外字符被移除⚠️ API 响应不一致 -with_mock_data=True时,字段是否返回取决于mock_scheme是否存在- ⚡ 类型推断逻辑 -
isinstance检查顺序可能导致类型误判
建议
- 优先修复输入验证和字符串处理问题,防止生产环境异常
- 考虑为
format=plugin功能添加单元测试覆盖边界情况
详细问题见下方行内评论 👇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
代码审查总结
本次提交新增了 format=plugin 参数支持,核心功能正常。发现以下需要关注的问题:
关键问题
- 🔒 输入验证不足 -
parse_pipeline_tree_to_plugin_schema等函数缺少对空值的防护 ⚠️ 字符串处理不精确 - 使用.strip("${}")可能导致意外字符被移除⚠️ API 响应不一致 -with_mock_data=True时,字段是否返回取决于mock_scheme是否存在- ⚡ 类型推断逻辑 -
isinstance检查顺序可能导致类型误判
建议
- 优先修复输入验证和字符串处理问题,防止生产环境异常
- 考虑为
format=plugin功能添加单元测试覆盖边界情况
bkflow/apigw/utils.py
Outdated
| # 输入参数:show_type 为 show 且不是组件输出 | ||
| if info.get("show_type") == "show" and info.get("source_type") != "component_outputs": | ||
| # 移除 key 中的 ${} 包装 | ||
| clean_key = key.strip("${}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
使用 .strip("${}") 会移除首尾所有出现的 $、{、} 字符(非整体匹配)。
问题示例:
"${my_var}$"→"my_var"✅"$my_var$"→"my_var"❌"{var}"→"var"❌
建议修复:
import re
clean_key = re.sub(r'^\$\{|\}$', '', key)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ 此问题已被解决:代码改用 key[2:-1] 精确截取,避免了 .strip() 的误删问题
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ 此问题已在最新提交中解决:代码改用 key[2:-1] 精确截取 ${var_name} 中的变量名,避免了 .strip("${}") 的字符误删问题。
| @param pipeline_tree: 流程树 | ||
| @return: JSON Schema 格式的 inputs | ||
| """ | ||
| constants = pipeline_tree.get("constants", {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 潜在空值异常风险
直接调用 .get() 而不验证返回值。如果 pipeline_tree 为 None 或缺少 constants 键,会导致 AttributeError。
建议:在函数开始添加验证:
if not pipeline_tree or not isinstance(pipeline_tree, dict):
return {"type": "object", "properties": {}, "required": [], "definitions": {}}| schema_type = "array" | ||
| elif custom_type in ["dict", "object"]: | ||
| schema_type = "object" | ||
| elif isinstance(value, bool): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚡ 类型推断顺序问题
当 value="" 时,按当前顺序检查可能导致误判。建议先检查 custom_type,再按 bool → int → float 顺序检查 isinstance(value, ...),避免空字符串被错误分类。
| pipeline_tree = template.pipeline_tree | ||
| exclude_task_nodes_id = PipelineTemplateWebPreviewer.get_template_exclude_task_nodes_with_appoint_nodes( | ||
| pipeline_tree, appoint_node_ids | ||
| if mock_scheme: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
当 with_mock_data=True 但 mock_scheme 为 None 时,不再返回 appoint_node_ids 和 mock_data 字段。
影响:可能破坏依赖这些字段的客户端。
建议:始终返回这些字段,无数据时设为空列表:
appoint_node_ids = mock_scheme.data.get("nodes", []) if mock_scheme else []
response["data"]["appoint_node_ids"] = appoint_node_idsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ 此问题已部分解决:添加了 if mock_scheme: 检查,避免访问 None 对象。但建议进一步改进:始终返回空列表确保 API 一致性
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #594 +/- ##
=========================================
Coverage ? 80.11%
=========================================
Files ? 239
Lines ? 13544
Branches ? 0
=========================================
Hits ? 10851
Misses ? 2693
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bkflow/apigw/utils.py:86
🚨 类型推断顺序错误:在 Python 中 isinstance(True, int) 为 True,需先检查 bool 再检查 int,否则布尔值会被误判为整数。建议将第 86 行移到第 88 行之前。
bkflow/apigw/utils.py:64
if not constant_info or not isinstance(constant_info, dict): return {"title": "", "type": "string"},避免传入空值时异常。
bkflow/apigw/views/get_template_detail.py:96
✅ if mock_scheme: 检查已添加,此问题已解决。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
代码审查
本次审查发现以下问题:
🚨 类型推断逻辑错误
在 Python 中 isinstance(True, int) 返回 True,bool 检查必须在 int 之前,否则布尔值会被误判为整数。
⚠️ 空值安全
_constant_to_json_schema_property 未对空参数做防护,可能导致异常。
✅ 已改进
mock_scheme 的空值检查已添加 (line 96),此问题已解决。
| schema_type = "array" | ||
| elif custom_type in ["dict", "object"]: | ||
| schema_type = "object" | ||
| elif isinstance(value, bool): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 类型推断顺序错误:在 Python 中 isinstance(True, int) 为 True,需先检查 bool 再检查 int,否则布尔值会被误判为整数
| @return: JSON Schema property 字典 | ||
| """ | ||
| # 根据 custom_type 或 source_tag 推断类型 | ||
| custom_type = constant_info.get("custom_type", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not constant_info: constant_info = {},避免传入 None 时异常
62236fb to
e0289cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
审查总结
本次提交新增 format=plugin 参数,整体实现正确。但仍存在以下关键问题需要优先修复:
🚨 严重问题
- 类型推断逻辑错误 (
utils.py:86) - 布尔值会被误判为整数,影响 JSON Schema 生成准确性 - 空值异常风险 (
utils.py:64,utils.py:122) - 缺少输入验证,可能导致运行时崩溃
⚠️ 改进建议
- API 响应一致性 (
get_template_detail.py:96) -with_mock_data=True时字段返回不稳定
✅ 已解决
- 字符串截取问题已通过
key[2:-1]修复
建议优先修复严重问题,确保生产环境稳定性。
| schema_type = "array" | ||
| elif custom_type in ["dict", "object"]: | ||
| schema_type = "object" | ||
| elif isinstance(value, bool): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 关键问题:isinstance(True, int) 在 Python 中返回 True,导致布尔值被误判为整数。必须将第 86-87 行移到第 88-89 行之前。
| raise Exception(message) | ||
|
|
||
|
|
||
| def _constant_to_json_schema_property(constant_info): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 空值安全:缺少对 constant_info 的验证。建议在函数开头添加:if not constant_info: constant_info = {}
| @param pipeline_tree: 流程树 | ||
| @return: JSON Schema 格式的 inputs | ||
| """ | ||
| constants = pipeline_tree.get("constants", {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 潜在异常:pipeline_tree 为 None 或非字典时会导致 AttributeError。建议函数开头添加空值检查并返回默认结构。
| pipeline_tree = template.pipeline_tree | ||
| exclude_task_nodes_id = PipelineTemplateWebPreviewer.get_template_exclude_task_nodes_with_appoint_nodes( | ||
| pipeline_tree, appoint_node_ids | ||
| if mock_scheme: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with_mock_data=True 但 mock_scheme 为 None 时,不返回 appoint_node_ids 和 mock_data 字段可能破坏客户端。建议始终返回空列表保持一致性。
No description provided.