feat: Add traffic-editor plugin #2825
Conversation
新增 traffic-editor 插件以支持请求和响应编辑功能变更概述
变更文件
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
![lingma-agents[bot]](https://avatars.githubusercontent.com/in/1198481?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 0 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 2 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 0 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 6 | 次要问题，酬情优化。例如：代码格式不规范或注释缺失。 |
总计: 8 个问题
📋 评审意见详情
💡 代码实现建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
🔹 plugins/wasm-go/extensions/traffic-editor/command.go (4 💬)
🔹 plugins/wasm-go/extensions/traffic-editor/condition.go (1 💬)
- regexCondition 编译正则表达式失败时未提供原始 pattern 信息，不利于调试。 (L274-L277)
🔹 plugins/wasm-go/extensions/traffic-editor/context.go (2 💬)
- EditorContext 的 map 初始化可能导致空指针异常。 (L102-L120)
- DeleteRefValues 方法也存在未初始化 map 的风险。 (L122-L140)
🔹 plugins/wasm-go/extensions/traffic-editor/main.go (1 💬)
- 在保存响应元数据更改时，应检查是否有任何修改再执行替换操作。 (L179-L185)
🚀 架构设计建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍1. 架构一致性问题：命令和条件处理逻辑缺乏统一抽象
在 command.go 和 condition.go 中，虽然都使用了类似的工厂模式和接口设计，但两者之间缺乏更高层次的抽象统一。例如，Command 和 Condition 都有 GetType、GetRefs 等方法，但没有共同的基接口或抽象类来统一管理。这可能导致未来扩展新类型时代码重复，维护成本增加。建议引入一个统一的接口如 TrafficEntity 来抽象 Command 和 Condition 的共同行为。
未来新增类型时需要重复实现相似逻辑，增加维护难度和出错风险。
🔍2. 跨文件问题：Stage 管理逻辑分散且不一致
Stage 的定义在 context.go 中，但相关的检查逻辑分散在 command.go（如 copyCommand 的 stage 顺序检查）和 condition.go（如 condition 只支持 request refs 的限制）。这种分散的逻辑容易导致维护困难和不一致的约束实施。建议将所有 Stage 相关的验证逻辑集中到 context.go 或一个专门的 stage 管理模块中。
📌 关键代码
if sourceRef.GetStage() > targetRef.GetStage() {
return nil, fmt.Errorf("setCommand: the processing stage of source [%s] cannot be after the stage of target [%s]", Stage2String[sourceRef.GetStage()], Stage2String[targetRef.GetStage()])
}if ref.GetStage() >= StageResponseHeaders {
return nil, fmt.Errorf("condition only supports request refs")
}Stage 相关的约束可能在不同地方实现不一致，导致难以维护和潜在的逻辑错误。
🔍3. 技术债务：Ref 解析和 Stage 映射逻辑紧耦合
在 ref.go 中，Ref 的解析和 Stage 映射逻辑紧密耦合在一起。当需要修改 Stage 映射规则或添加新的 Ref 类型时，必须同时修改 ref.go 和 context.go 中的相关代码。这种紧耦合增加了未来维护的复杂性。建议将 Stage 映射逻辑抽象为独立的映射服务，通过依赖注入的方式提供给 Ref 使用。
📌 关键代码
var (
refType2Stage = map[string]Stage{
refTypeRequestHeader: StageRequestHeaders,
refTypeRequestQuery: StageRequestHeaders,
refTypeResponseHeader: StageResponseHeaders,
}
)未来添加新的 Ref 类型或修改 Stage 映射规则时，需要同时修改多个文件，增加出错风险。
🔍4. 可测试性问题：Executor 的执行逻辑难以独立测试
在 command.go 中，各种 Executor 的 Run 方法直接依赖 EditorContext 和 Stage，这使得单元测试时需要构造复杂的 EditorContext 对象。建议将 Executor 的核心逻辑抽象为纯函数，接收必要的参数并返回结果，这样可以更容易地进行单元测试。
📌 关键代码
func (e *setExecutor) Run(editorContext *EditorContext, stage Stage) error {
if e.finished {
return nil
}
command := e.command
if command.targetRef.GetStage() == stage {
editorContext.SetRefValue(command.targetRef, command.value)
e.finished = true
}
return nil
}测试覆盖率难以提升，复杂场景下的逻辑验证成本高，容易遗漏边界情况。
🔍5. 性能影响：频繁的 header/query map 操作可能影响性能
在 context.go 和 http.go 中，频繁地将 header/query 转换为 map 结构进行操作，然后又转换回 slice 结构。在高并发场景下，这种频繁的转换可能带来性能开销。建议评估是否可以通过直接操作 slice 结构或使用更高效的数据结构来优化性能。
📌 关键代码
func headerSlice2Map(headerSlice [][2]string) map[string][]string {
headerMap := make(map[string][]string)
for _, header := range headerSlice {
k, v := strings.ToLower(header[0]), header[1]
headerMap[k] = append(headerMap[k], v)
}
return headerMap
}func (ctx *EditorContext) SetRefValues(ref *Ref, values []string) {
if ref == nil {
return
}
switch ref.Type {
case refTypeRequestHeader:
ctx.requestHeaders[ref.Name] = values
ctx.requestHeadersDirty = true
break
case refTypeRequestQuery:
ctx.requestQueries[ref.Name] = values
ctx.requestQueriesDirty = true
break
case refTypeResponseHeader:
ctx.responseHeaders[ref.Name] = values
ctx.responseHeadersDirty = true
break
}
}在高并发场景下可能成为性能瓶颈，影响整体系统吞吐量。
🔍6. 安全风险：正则表达式编译未做复杂度限制
在 condition.go 中，regexCondition 的正则表达式编译未做任何复杂度限制或超时控制。恶意构造的正则表达式可能导致 ReDoS（正则表达式拒绝服务）攻击。建议对正则表达式的复杂度进行限制，并设置编译和匹配的超时时间。
📌 关键代码
pattern, err := regexp.Compile(patternStr)
if err != nil {
return nil, errors.New("regexCondition: failed to compile pattern: " + err.Error())
}可能遭受 ReDoS 攻击，导致系统资源耗尽，影响服务可用性。
审查详情
📒 文件清单 （10 个文件）
✅ 新增: 10 个文件
✅ 新增文件:
plugins/wasm-go/extensions/traffic-editor/VERSIONplugins/wasm-go/extensions/traffic-editor/command.goplugins/wasm-go/extensions/traffic-editor/condition.goplugins/wasm-go/extensions/traffic-editor/config.goplugins/wasm-go/extensions/traffic-editor/context.goplugins/wasm-go/extensions/traffic-editor/docker-compose.yamlplugins/wasm-go/extensions/traffic-editor/go.modplugins/wasm-go/extensions/traffic-editor/http.goplugins/wasm-go/extensions/traffic-editor/main.goplugins/wasm-go/extensions/traffic-editor/ref.go
💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@lingma-agents 分析这个方法的性能瓶颈并提供优化建议。
-
@lingma-agents 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@lingma-agents 请总结上述讨论并提出解决方案。
-
@lingma-agents 请根据讨论内容生成优化代码。
| func (s *CommandSet) CreatExecutors() []Executor { | ||
| executors := make([]Executor, 0, len(s.Commands)) | ||
| for _, command := range s.Commands { | ||
| executor := command.CreateExecutor() | ||
| executors = append(executors, executor) | ||
| } | ||
| return executors | ||
| } |
There was a problem hiding this comment.
CreatExecutors 方法名拼写错误，应为 CreateExecutors。
🟢 Minor | 🧹 Code Smells
📋 问题详情
方法名 CreatExecutors 拼写错误，正确的英文应该是 CreateExecutors。这会影响代码的可读性和一致性。
💡 解决方案
修正方法名为 CreateExecutors。
-func (s *CommandSet) CreatExecutors() []Executor {
+func (s *CommandSet) CreateExecutors() []Executor {
executors := make([]Executor, 0, len(s.Commands))
for _, command := range s.Commands {
executor := command.CreateExecutor()
executors = append(executors, executor)
}
return executors
}您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| if sourceRef.GetStage() > targetRef.GetStage() { | ||
| return nil, fmt.Errorf("setCommand: the processing stage of source [%s] cannot be after the stage of target [%s]", Stage2String[sourceRef.GetStage()], Stage2String[targetRef.GetStage()]) | ||
| } |
There was a problem hiding this comment.
copyCommand 中 stage 顺序检查错误地引用了 setCommand 的错误信息。
🟢 Minor | 🧹 Code Smells
📋 问题详情
在 copyCommand 的构造函数中，当 sourceRef 的 stage 大于 targetRef 的 stage 时，抛出的错误信息错误地使用了 'setCommand' 字样，应更正为 'copyCommand' 以准确反映上下文。
💡 解决方案
将错误信息中的 'setCommand' 更改为 'copyCommand' 以匹配当前命令类型。
if sourceRef.GetStage() > targetRef.GetStage() {
- return nil, fmt.Errorf("setCommand: the processing stage of source [%s] cannot be after the stage of target [%s]", Stage2String[sourceRef.GetStage()], Stage2String[targetRef.GetStage()])
+ return nil, fmt.Errorf("copyCommand: the processing stage of source [%s] cannot be after the stage of target [%s]", Stage2String[sourceRef.GetStage()], Stage2String[targetRef.GetStage()])
}您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| func (e *copyExecutor) Run(editorContext *EditorContext, stage Stage) error { | ||
| if e.finished { | ||
| return nil | ||
| } | ||
|
|
||
| command := e.command | ||
|
|
||
| if command.sourceRef.GetStage() == stage { | ||
| e.valueToCopy = editorContext.GetRefValue(command.sourceRef) | ||
| log.Debugf("copyCommand: valueToCopy=%s", e.valueToCopy) | ||
| } | ||
|
|
||
| if e.valueToCopy == "" { | ||
| log.Debug("copyCommand: valueToCopy is empty. skip.") | ||
| e.finished = true | ||
| return nil | ||
| } | ||
|
|
||
| if command.targetRef.GetStage() == stage { | ||
| editorContext.SetRefValue(command.targetRef, e.valueToCopy) | ||
| log.Debugf("copyCommand: set %s to %s", e.valueToCopy, command.targetRef) | ||
| e.finished = true | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
在处理 copyCommand 时，如果 sourceRef 的值为空字符串，应提前终止执行以避免不必要的操作。
🟢 Minor | 🧹 Code Smells
📋 问题详情
当前 copyCommand 在 sourceRef 值为空时仍会继续执行到 targetRef 阶段，尽管不会设置任何值。这可能导致逻辑混淆或性能浪费。应在检测到值为空时立即标记为完成并返回。
💡 解决方案
在 sourceRef 阶段获取值后立即检查是否为空，若为空则提前结束执行，避免进入 targetRef 阶段。
func (e *copyExecutor) Run(editorContext *EditorContext, stage Stage) error {
if e.finished {
return nil
}
command := e.command
if command.sourceRef.GetStage() == stage {
e.valueToCopy = editorContext.GetRefValue(command.sourceRef)
log.Debugf("copyCommand: valueToCopy=%s", e.valueToCopy)
+ if e.valueToCopy == "" {
+ log.Debug("copyCommand: valueToCopy is empty. skip.")
+ e.finished = true
+ return nil
+ }
}
if command.targetRef.GetStage() == stage {
editorContext.SetRefValue(command.targetRef, e.valueToCopy)
log.Debugf("copyCommand: set %s to %s", e.valueToCopy, command.targetRef)
e.finished = true
}
return nil
}您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| if command.targetRef.GetStage() == stage { | ||
| if command.newName == command.targetRef.Name { | ||
| log.Debugf("renameCommand: skip renaming %s to itself", command.targetRef) | ||
| } else { | ||
| values := editorContext.GetRefValues(command.targetRef) | ||
| log.Debugf("renameCommand: rename %s to %s value=%v", command.targetRef, command.newName, values) | ||
| editorContext.SetRefValues(&Ref{ | ||
| Type: command.targetRef.Type, | ||
| Name: command.newName, | ||
| }, values) | ||
| editorContext.DeleteRefValues(command.targetRef) | ||
| } | ||
| e.finished = true | ||
| } |
There was a problem hiding this comment.
renameCommand 执行时未验证新名称与原名称是否相同即进行删除和设置操作，可优化为仅在名称不同时执行。
🟢 Minor | 🧹 Code Smells
📋 问题详情
虽然代码中已有日志表明跳过重命名为自身的情况，但实际仍会执行 GetRefValues、SetRefValues 和 DeleteRefValues 操作。这些操作是不必要的，应完全跳过以提升性能和减少副作用。
💡 解决方案
调整条件判断顺序，确保只有在新旧名称不一致时才执行 rename 操作。
if command.targetRef.GetStage() == stage {
- if command.newName == command.targetRef.Name {
- log.Debugf("renameCommand: skip renaming %s to itself", command.targetRef)
- } else {
+ if command.newName != command.targetRef.Name {
values := editorContext.GetRefValues(command.targetRef)
log.Debugf("renameCommand: rename %s to %s value=%v", command.targetRef, command.newName, values)
editorContext.SetRefValues(&Ref{
Type: command.targetRef.Type,
Name: command.newName,
}, values)
editorContext.DeleteRefValues(command.targetRef)
+ } else {
+ log.Debugf("renameCommand: skip renaming %s to itself", command.targetRef)
}
e.finished = true
}您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| pattern, err := regexp.Compile(patternStr) | ||
| if err != nil { | ||
| return nil, errors.New("regexCondition: failed to compile pattern: " + err.Error()) | ||
| } |
There was a problem hiding this comment.
regexCondition 编译正则表达式失败时未提供原始 pattern 信息，不利于调试。
🟢 Minor | 🧹 Code Smells
📋 问题详情
当 regexCondition 的正则表达式编译失败时，错误信息仅包含编译器返回的错误，缺少原始 pattern 内容，使得调试困难。应将 pattern 信息加入错误消息中。
💡 解决方案
使用 fmt.Errorf 包含 pattern 信息以便更好地调试。
pattern, err := regexp.Compile(patternStr)
if err != nil {
- return nil, errors.New("regexCondition: failed to compile pattern: " + err.Error())
+ return nil, fmt.Errorf("regexCondition: failed to compile pattern '%s': %w", patternStr, err)
}您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| func (ctx *EditorContext) SetRefValues(ref *Ref, values []string) { | ||
| if ref == nil { | ||
| return | ||
| } | ||
| switch ref.Type { | ||
| case refTypeRequestHeader: | ||
| ctx.requestHeaders[ref.Name] = values | ||
| ctx.requestHeadersDirty = true | ||
| break | ||
| case refTypeRequestQuery: | ||
| ctx.requestQueries[ref.Name] = values | ||
| ctx.requestQueriesDirty = true | ||
| break | ||
| case refTypeResponseHeader: | ||
| ctx.responseHeaders[ref.Name] = values | ||
| ctx.responseHeadersDirty = true | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
EditorContext 的 map 初始化可能导致空指针异常。
🟠 Critical | 🐞 Bugs
📋 问题详情
在 SetRefValues 和 DeleteRefValues 方法中，直接对 requestHeaders/requestQueries/responseHeaders 进行操作前未检查其是否已初始化，可能导致空指针异常。应在操作前确保 map 已初始化。
💡 解决方案
在每次访问 map 前检查其是否为 nil，并在必要时进行初始化。
func (ctx *EditorContext) SetRefValues(ref *Ref, values []string) {
if ref == nil {
return
}
switch ref.Type {
case refTypeRequestHeader:
+ if ctx.requestHeaders == nil {
+ ctx.requestHeaders = make(map[string][]string)
+ }
ctx.requestHeaders[ref.Name] = values
ctx.requestHeadersDirty = true
break
case refTypeRequestQuery:
+ if ctx.requestQueries == nil {
+ ctx.requestQueries = make(map[string][]string)
+ }
ctx.requestQueries[ref.Name] = values
ctx.requestQueriesDirty = true
break
case refTypeResponseHeader:
+ if ctx.responseHeaders == nil {
+ ctx.responseHeaders = make(map[string][]string)
+ }
ctx.responseHeaders[ref.Name] = values
ctx.responseHeadersDirty = true
break
}
}您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| func (ctx *EditorContext) DeleteRefValues(ref *Ref) { | ||
| if ref == nil { | ||
| return | ||
| } | ||
| switch ref.Type { | ||
| case refTypeRequestHeader: | ||
| delete(ctx.requestHeaders, ref.Name) | ||
| ctx.requestHeadersDirty = true | ||
| break | ||
| case refTypeRequestQuery: | ||
| delete(ctx.requestQueries, ref.Name) | ||
| ctx.requestQueriesDirty = true | ||
| break | ||
| case refTypeResponseHeader: | ||
| delete(ctx.responseHeaders, ref.Name) | ||
| ctx.responseHeadersDirty = true | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
DeleteRefValues 方法也存在未初始化 map 的风险。
🟠 Critical | 🐞 Bugs
📋 问题详情
与 SetRefValues 类似，DeleteRefValues 在操作 map 前未检查其是否已初始化，可能导致运行时 panic。需要添加初始化检查。
💡 解决方案
在删除 map 元素前检查 map 是否为 nil，防止空指针异常。
func (ctx *EditorContext) DeleteRefValues(ref *Ref) {
if ref == nil {
return
}
switch ref.Type {
case refTypeRequestHeader:
+ if ctx.requestHeaders != nil {
delete(ctx.requestHeaders, ref.Name)
ctx.requestHeadersDirty = true
+ }
break
case refTypeRequestQuery:
+ if ctx.requestQueries != nil {
delete(ctx.requestQueries, ref.Name)
ctx.requestQueriesDirty = true
+ }
break
case refTypeResponseHeader:
+ if ctx.responseHeaders != nil {
delete(ctx.responseHeaders, ref.Name)
ctx.responseHeadersDirty = true
+ }
break
}
}您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| func saveResponseMetaChanges(editorContext *EditorContext) error { | ||
| if !editorContext.responseHeadersDirty { | ||
| return nil | ||
| } | ||
| headerSlice := headerMap2Slice(editorContext.responseHeaders) | ||
| return proxywasm.ReplaceHttpResponseHeaders(headerSlice) | ||
| } |
There was a problem hiding this comment.
在保存响应元数据更改时，应检查是否有任何修改再执行替换操作。
🟢 Minor | 🧹 Code Smells
📋 问题详情
虽然代码中对请求头做了类似的检查（通过needSetHeaders标志），但在保存响应头更改时没有类似的逻辑，可能会导致不必要的系统调用和潜在的性能问题。
💡 解决方案
当前实现已经包含了对responseHeadersDirty的检查，因此无需更改。但为了提高可读性，可以添加注释说明此检查的目的。
func saveResponseMetaChanges(editorContext *EditorContext) error {
+ // Only proceed if response headers have been modified
if !editorContext.responseHeadersDirty {
return nil
}
headerSlice := headerMap2Slice(editorContext.responseHeaders)
return proxywasm.ReplaceHttpResponseHeaders(headerSlice)
}您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2825 +/- ##
==========================================
+ Coverage 35.91% 45.31% +9.40%
==========================================
Files 69 82 +13
Lines 11576 13224 +1648
==========================================
+ Hits 4157 5992 +1835
+ Misses 7104 6886 -218
- Partials 315 346 +31 🚀 New features to boost your workflow:
|
Ⅰ. Describe what this PR did
Add
traffic-editorplugin to provide request and response editing features with a more extensible code structure.Ⅱ. Does this pull request fix one issue?
fixes #2795
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews