fix(runs): preserve oneof fields when decoding revision spec - #621
Conversation
DecodeRevisionSpec used encoding/json to unmarshal the persisted project revision spec into the generated proto message. encoding/json does not understand proto oneof fields, so DriverSpec.config (and any other oneof) was silently dropped. A GetProject -> PatchProject round-trip then failed with "driver requires exactly one runtime config" because the returned driver carried only its name. Switch to protojson.Unmarshal with DiscardUnknown so oneof fields decode correctly while the legacy canonical workspace fields (still handled by restoreCanonicalRevisionWorkspaces) are tolerated.
|
PR Title: fix(runs): preserve oneof fields when decoding rev... Commit: 本次变更将 DecodeRevisionSpec 中用于解析项目 revision spec 的 encoding/json.Unmarshal 替换为 protojson.Unmarshal(UnmarshalOptions{DiscardUnknown: true}),并新增回归测试验证 driver 的 oneof 运行时配置被保留。 修复动机正确:encoding/json 无法给 protobuf 生成结构体的 oneof 接口字段赋值,导致 GetProject→PatchProject 往返中 driver config oneof 丢失,PatchProject 校验报 "driver requires exactly one runtime config"。protojson 是 protobuf 官方的 JSON 编解码器,能正确解析 oneof、enum 名称、64 位整数等 proto3 JSON 规范表示;测试输入(camelCase 且 oneof 成员直接作为键)也证实存储格式为 protojson 形式,因此新读取器与写入器一致,整体方向正确,未发现高置信度功能性缺陷。 需要注意的回归覆盖缺口:该替换影响所有已存储 revision spec 的解码路径,而 protojson 对 well-known 类型要求 proto3 JSON 规范表示、对重复键直接报错,与 encoding/json 的结构体字段表示存在差异;新增测试仅覆盖 driver 的 docker oneof 单一路径,未覆盖完整往返与既有存储数据兼容性,故提交一条低严重度发现建议补充往返与兼容性测试。 |
| return nil, fmt.Errorf("decode project revision spec: %w", err) | ||
| } | ||
| if err := json.Unmarshal(normalizedData, &spec); err != nil { | ||
| opts := protojson.UnmarshalOptions{DiscardUnknown: true} |
There was a problem hiding this comment.
protojson 解码切换缺少对既有存储 spec 兼容性与完整往返的回归覆盖
将 encoding/json.Unmarshal 替换为 protojson.Unmarshal 后,所有已存储的 project revision spec(revision.SpecJSON)的解码语义都发生了变化。encoding/json 按 protobuf 生成结构体的字段标签(camelCase + Go 字段结构)解析,而 protojson 遵循 proto3 JSON 规范:well-known 类型必须使用其规范表示(如 google.protobuf.Timestamp 为 RFC3339 字符串而非生成的 Go 结构体 {"seconds":...,"nanos":...}、Any 使用 @type),并且对同一对象的重复键直接报错而非 last-wins。若存在任何以非 protojson 规范序列化的历史 spec(例如旧写入路径使用 encoding/json.Marshal 产生的结构字段表示),切换后 DecodeRevisionSpec 将解码失败,进而导致 PrepareProjectRun 与 GetProject 中断;而 DiscardUnknown 又会使未知/无法识别的字段被静默丢弃。新增测试仅覆盖 driver 的 docker oneof 这一条最小路径(不含 workspaces/closed-sets 等经过 normalizeRevisionClosedSetsJSON 与 restoreCanonicalRevisionWorkspaces 处理的字段),既未验证 GetProject→PatchProject 的完整往返(解码后重新 protojson.Marshal 再被 PatchProject 接受),也未覆盖与既有存储数据格式的兼容性,回归风险面未被充分覆盖。
Problem code:
Changed code at pkg/runs/preparation.go:154-155
Recommendation:
补充覆盖完整往返与兼容性的测试:1) 构造一个包含 driver oneof、workspace、closed-sets 等字段的 protojson 序列化 spec,经 DecodeRevisionSpec 解码后重新 protojson.Marshal 再解码,断言 oneof 与其余字段在往返中保持(等价于 GetProject→PatchProject 场景);2) 覆盖 well-known 类型(若 ProjectSpec 含 Timestamp/Any 等)的 protojson 规范表示解码,以及非规范结构字段表示的报错/丢弃行为符合预期,防止解码器切换对既有存储数据造成静默数据丢失或运行中断。
Problem
GetProjectreturns a project spec whosedriverfield loses its runtime config oneof. AGetProject → PatchProjectround-trip fails with:Repro: apply a project whose agent has
driver: {docker: {}}, thenGetProject(includeSpec) →PatchProjectthe returned spec. The persistedspec_jsonis correct ({"name":"docker","docker":{}}), but the returned spec dropsdocker: {}, leaving only{"name":"docker"}.Root cause
DecodeRevisionSpecunmarshals the persisted spec into the generated proto message usingencoding/json:encoding/jsondoes not understand protooneoffields.DriverSpec.Configis anisDriverSpec_Configinterface with nojsontag, so thedockerkey is ignored andConfigstaysnil. This silently drops every oneof field, not just driver config.Fix
Switch to
protojson.Unmarshal(withDiscardUnknownso the legacy canonical workspace fields — still restored separately byrestoreCanonicalRevisionWorkspaces— remain tolerated):protojsonunderstands oneof and enum values, soDriverSpec.Config(and other oneofs) round-trip correctly.Test
TestDecodeRevisionSpecPreservesDriverRuntimeConfigasserts the driver oneof survives decoding.go test ./pkg/runs/→ 221 passedgo test ./pkg/agentcompose/api/ ./pkg/agentcompose/app/→ 321 passed