dualfroz
d03cd5d09c
splitKeyVal sliced the value from index+1, skipping only one byte of the key/value separator. With a multi-character sep tag (e.g. sep:"=>") the remaining separator bytes leaked into the value. Slice from index+len(sep) so the whole separator is consumed.
64 lines
1.7 KiB
Markdown
64 lines
1.7 KiB
Markdown
# fix(flag): honor full separator length when splitting map key=value pairs
|
|
|
|
## Problem
|
|
|
|
Map flags support a configurable key/value separator via the `sep` tag. When the
|
|
separator is more than one byte long, the tail of the separator leaks into the
|
|
parsed value:
|
|
|
|
```go
|
|
type T struct {
|
|
Map map[string]string `cli:"m" sep:"=>"`
|
|
}
|
|
// parsing -mkey=>val yields Map["key"] == ">val", want "val"
|
|
```
|
|
|
|
Internally:
|
|
|
|
```go
|
|
splitKeyVal("key=>val", "=>") // ("key", ">val"), want ("key", "val")
|
|
splitKeyVal("a::b", "::") // ("a", ":b"), want ("a", "b")
|
|
```
|
|
|
|
The default separator is `=` (single byte), so the common case is unaffected;
|
|
the defect only appears for multi-character separators.
|
|
|
|
## Root cause
|
|
|
|
`flag.go`, `splitKeyVal`:
|
|
|
|
```go
|
|
index := strings.Index(s, sep)
|
|
if index == -1 {
|
|
return s, "", nil
|
|
}
|
|
return s[:index], s[index+1:], nil
|
|
```
|
|
|
|
`strings.Index` returns the byte offset of the start of `sep`, but the value is
|
|
sliced from `index+1`, which only skips a single byte of the separator instead
|
|
of its whole length.
|
|
|
|
## Fix
|
|
|
|
Skip the full separator length:
|
|
|
|
```go
|
|
return s[:index], s[index+len(sep):], nil
|
|
```
|
|
|
|
## Test
|
|
|
|
Added `TestMapMultiCharSep` in `cli_test.go`, covering both `splitKeyVal`
|
|
directly and a map flag parsed through `parseArgv` with `sep:"=>"`.
|
|
|
|
- Gate: `go test ./...` -> ok (PASS, exit 0, all packages)
|
|
- `go build ./...` -> exit 0
|
|
- Counterfactual: reverting `flag.go` makes the test fail with
|
|
`splitKeyVal("key=>val", "=>") = ("key", ">val"), want ("key", "val")`.
|
|
|
|
Note: `go vet ./...` exits 1 both before and after this change, due to
|
|
pre-existing example issues in `example_test.go`
|
|
(`ExampleNumOptionFunc_* refers to unknown identifier: NumOptionFunc`), which
|
|
are unrelated to this fix.
|