refactor(install): parse llard responses with cmdjsonl - #159
Conversation
81878b2 to
daef124
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: refactor(install) — parse llard responses with cmdjsonl
Clean, well-scoped refactor. Replaces the hand-written line parser with cmdjsonl.Parser, drops the temporary parser and its migration TODO, and pins qiniu/x v1.18.0. I verified go build ./... and go test ./cmd/llar/internal/ -run TestRequestInstallArtifacts both pass on the head branch. Behavior is preserved: streaming info output still flushes per line, artifact validation (ID/Type/URL non-empty) is kept, and the 64KB bufio.MaxScanTokenSize line cap is actually a small robustness improvement over the old unbounded ReadString when facing an untrusted llard body.
A few non-blocking maintainability notes below (2 inline). One additional note that isn't tied to a single diff line:
- Blank-line tolerance (defensive, not a current bug):
cmdjsonlrejects any line without a space asParseCommand: no space found. The old parser skipped empty lines. The server'swriteCommand(internal/build/http/http.go:264) always emits<cmd> <json>and never blank lines, so this is safe today — worth keeping in mind only if the wire format ever gains blank separators.
Security and documentation passes found nothing: no dangling references to the removed temporary parser / TODO, and the untrusted-response handling is equivalent-or-better.
| } | ||
| if err := parser.HandleFunc("error", func(message string) error { | ||
| responseErr = fmt.Errorf("llard: %s", message) | ||
| return responseErr |
There was a problem hiding this comment.
The error handler both sets responseErr and returns it, but the returned value's only real effect is to make Parse stop early (it aborts on the first non-nil handler error). The caller then discards Parse's wrapped *ParseError and re-returns the clean responseErr instead. This works, but the contract is non-obvious — a reader has to know Parse wraps handler errors and that the closure side-effect is the intended return channel. A one-line comment here (and at the if responseErr != nil guard) noting "return a sentinel to abort parsing; caller unwraps responseErr for a clean message" would save future maintainers.
Note the guard checks responseErr before err, so it depends on Parse returning immediately once the error handler fires — which it does, but that coupling is implicit.
| {name: "error JSON", contentType: "application/x-cmdjsonl", body: "error {\n", want: "1: parse error when UnmarshalParam: unexpected end of JSON input"}, | ||
| {name: "artifact JSON", contentType: "application/x-cmdjsonl", body: "artifact {\n", want: "1: parse error when UnmarshalParam: unexpected end of JSON input"}, | ||
| {name: "artifact fields", contentType: "application/x-cmdjsonl", body: "artifact {\"id\":\"test/root@v1?os=linux\"}\n", want: "1: parse error when CallHandler artifact: invalid llard artifact"}, | ||
| {name: "command", contentType: "application/x-cmdjsonl", body: "done {}\n", want: "1: parse error when ParseCommand: unknown command 'done'"}, |
There was a problem hiding this comment.
These assertions pin the full cmdjsonl error strings, including the line-number prefix and internal phrasing (parse error when ParseCommand, UnmarshalParam, etc.). That wording is owned by the third-party qiniu/x package, so a future upgrade could silently break these exact-match assertions without any behavior change in llar. Consider asserting on a stable substring (e.g. no space found, unknown command 'done') where the prefix/line number isn't the point of the test.
2 participants
No description provided.