dualfroz
9f860a7fd4
Get() preserves the concrete type of a map[string]T value loaded via Set() or a provider like structs, but MapKeys() only type-asserted map[string]any, so it silently reported no keys for anything else. Read keys via reflection instead so any map with string keys works.
96 lines
3.1 KiB
Markdown
96 lines
3.1 KiB
Markdown
# MapKeys returns an empty slice for a natively-typed map
|
|
|
|
## Problem
|
|
|
|
`Koanf.MapKeys(path)` is documented to return the sorted keys of the map at
|
|
`path`, or an empty slice "if the path is not a map". A value at a key path
|
|
does not have to be `map[string]any` to be a map, though: `Set()`, or a
|
|
provider such as `structs`, can put a `map[string]T` (`map[string]int64`,
|
|
`map[string]bool`, ...) directly into the config tree, and `Koanf.Get()`
|
|
returns it verbatim with its concrete type intact (`koanf.go`'s `Get` only
|
|
special-cases `map[string]any` for its copy, everything else falls through
|
|
to `copystructure.Copy`, which preserves the type).
|
|
|
|
`MapKeys` only checked `o.(map[string]any)`, so a natively-typed map failed
|
|
that assertion and produced `[]string{}` even though the map plainly has
|
|
keys - a silent contract violation, not a "not a map" case.
|
|
|
|
```go
|
|
k := koanf.New(".")
|
|
k.Load(confmap.Provider(map[string]any{
|
|
"ints": map[string]int64{"a": 1, "b": 2},
|
|
}, "."), nil)
|
|
|
|
k.Get("ints") // map[string]int64{"a":1, "b":2}
|
|
k.MapKeys("ints") // []string{} -- should be []string{"a", "b"}
|
|
```
|
|
|
|
## Root cause
|
|
|
|
`koanf.go:410-429`, `MapKeys`:
|
|
|
|
```go
|
|
mp, ok := o.(map[string]any)
|
|
if !ok {
|
|
return out
|
|
}
|
|
```
|
|
|
|
This is the same shape of bug already fixed for `Int64Map`, `Float64Map` and
|
|
`BoolMap` in #444 (checking only `map[string]any` and silently dropping the
|
|
natively-typed sibling), applied to a different getter that PR does not
|
|
touch.
|
|
|
|
## Fix
|
|
|
|
Since `MapKeys` only needs the keys and does not care about the value type,
|
|
the fix reads them through reflection instead of enumerating every possible
|
|
`map[string]T`: any map whose key kind is `string` is accepted, `map[string]any`
|
|
included.
|
|
|
|
```go
|
|
rv := reflect.ValueOf(o)
|
|
if rv.Kind() != reflect.Map || rv.Type().Key().Kind() != reflect.String {
|
|
return out
|
|
}
|
|
|
|
out = make([]string, 0, rv.Len())
|
|
for _, k := range rv.MapKeys() {
|
|
out = append(out, k.String())
|
|
}
|
|
sort.Strings(out)
|
|
```
|
|
|
|
`reflect` and `sort` are already imported in `koanf.go` for other methods.
|
|
|
|
## Verification
|
|
|
|
`TestMapKeysNativeMap` in `tests/koanf_test.go` loads a `map[string]int64`
|
|
and asserts `MapKeys` returns its keys.
|
|
|
|
Reverting only the `MapKeys` body in `koanf.go` (test left in place) fails it:
|
|
|
|
```
|
|
--- FAIL: TestMapKeysNativeMap
|
|
Error: Not equal:
|
|
expected: []string{"a", "b"}
|
|
actual : []string{}
|
|
Messages: map keys mismatch for a natively-typed map
|
|
```
|
|
|
|
`go build ./...`, `go vet ./...` and `gofmt -l .` are clean in both the root
|
|
module and `tests/` (the only `gofmt -l` hit is the pre-existing
|
|
`examples/read-huml/main.go`, untouched by this change).
|
|
|
|
`go test ./... -count=1` and `go test ./... -count=1 -race` in `tests/`:
|
|
60 tests, all passing, including the new one.
|
|
|
|
## Flags for the account owner
|
|
|
|
- No CLA/DCO step found in `CONTRIBUTING.md` or `.github/`.
|
|
- Behaviour change: code that relied on `MapKeys` returning `[]string{}` for
|
|
a natively-typed map (rather than actually reading its keys) would see a
|
|
different result now. I think the old behaviour was simply wrong given the
|
|
doc comment, but flagging it since #444 called out the same kind of change
|
|
for its three getters.
|