fix: MapKeys returns empty slice for natively-typed maps
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.
This commit is contained in:
1 parent
15e76cb3d5
commit
9f860a7fd4
3 files changed
+116
-5
No files matched your search
@@ -0,0 +1,95 @@
|
||||
# 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.
|
||||
@@ -416,13 +416,18 @@ func (ko *Koanf) MapKeys(path string) []string {
|
||||
return out
|
||||
}
|
||||
|
||||
mp, ok := o.(map[string]any)
|
||||
if !ok {
|
||||
// Not every map at a key path is loaded as map[string]any. A value
|
||||
// set directly with Set() or via a Provider such as structs keeps
|
||||
// its native map[string]T type, so the keys have to be read through
|
||||
// reflection to cover those too.
|
||||
rv := reflect.ValueOf(o)
|
||||
if rv.Kind() != reflect.Map || rv.Type().Key().Kind() != reflect.String {
|
||||
return out
|
||||
}
|
||||
out = make([]string, 0, len(mp))
|
||||
for k := range mp {
|
||||
out = append(out, k)
|
||||
|
||||
out = make([]string, 0, rv.Len())
|
||||
for _, k := range rv.MapKeys() {
|
||||
out = append(out, k.String())
|
||||
}
|
||||
sort.Strings(out)
|
||||
return out
|
||||
|
||||
@@ -1533,6 +1533,17 @@ func TestBoolsNativeSlice(t *testing.T) {
|
||||
assert.Equal([]bool{true, false, true}, k.Bools("bools"))
|
||||
}
|
||||
|
||||
func TestMapKeysNativeMap(t *testing.T) {
|
||||
assert := assert.New(t)
|
||||
|
||||
k := koanf.New(delim)
|
||||
assert.NoError(k.Load(confmap.Provider(map[string]any{
|
||||
"ints": map[string]int64{"a": 1, "b": 2},
|
||||
}, "."), nil))
|
||||
|
||||
assert.Equal([]string{"a", "b"}, k.MapKeys("ints"), "map keys mismatch for a natively-typed map")
|
||||
}
|
||||
|
||||
// waitTimeout waits for the waitgroup for the specified max timeout.
|
||||
// Returns true if waiting timed out.
|
||||
func waitTimeout(wg *sync.WaitGroup, timeout time.Duration) bool {
|
||||
|
||||
Reference in new issue
Block a user