Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for getting array index in GetValue #321

Merged
merged 4 commits into from
Apr 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions pkg/data/values.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
// Package data contains functions for working with unstructured values like []interface or map[string]interface{}.
// It allows reading/writing to these values without having to convert to structured items.
package data

import (
"strconv"
)

// RemoveValue removes a value from data. Keys should be in order denoting the path to the value in the nested
// structure of the map. For example, passing []string{"metadata", "annotations"} will make the function remove the
// "annotations" key from the "metadata" sub-map. Returns the removed value (if any) and a bool indicating if the value
// was found.
func RemoveValue(data map[string]interface{}, keys ...string) (interface{}, bool) {
for i, key := range keys {
if i == len(keys)-1 {
Expand All @@ -18,6 +28,8 @@ func GetValueN(data map[string]interface{}, keys ...string) interface{} {
return val
}

// GetValue works similar to GetValueFromAny, but can only process maps. Kept this way to avoid breaking changes with
// the previous interface, GetValueFromAny should be used in most cases since that can handle slices as well.
func GetValue(data map[string]interface{}, keys ...string) (interface{}, bool) {
for i, key := range keys {
if i == len(keys)-1 {
Expand All @@ -26,10 +38,48 @@ func GetValue(data map[string]interface{}, keys ...string) (interface{}, bool) {
}
data, _ = data[key].(map[string]interface{})
}
return nil, false
}

// GetValueFromAny retrieves a value from the provided collection, which must be a map[string]interface or a []interface.
// Keys are always strings.
// For a map, a key denotes the key in the map whose value we want to retrieve.
// For the slice, it denotes the index (starting at 0) of the value we want to retrieve.
// Returns the retrieved value (if any) and a bool indicating if the value was found.
func GetValueFromAny(data interface{}, keys ...string) (interface{}, bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder whether it would be a good idea to allows keys to be any and validate that they're string or a number..
This way we could be sure that -1 passed in expected a list vs "-1" which would expect a map.

This does add more complexity with this function like what to return if you receive an unexpected type, etc. So I think it's okay as is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the way this function is used in Steve is that keys are essentially user provided input. So if we wen that direction Steve would need to do some selective parsing there, which I don't think is desirable.

That being said, I think that you are correct that the underlying library really should be more specific than this. I tried to implement a more comprehensive approach that (IIRC) did what you suggested in #325 (though I doubt that will merge). I'd say if we want an improvement like that we should consider a rewrite like that Pr looks for.

for i, key := range keys {
if i == len(keys)-1 {
if dataMap, ok := data.(map[string]interface{}); ok {
val, ok := dataMap[key]
return val, ok
}
if dataSlice, ok := data.([]interface{}); ok {
return itemByIndex(dataSlice, key)
}
}
if dataMap, ok := data.(map[string]interface{}); ok {
data, _ = dataMap[key]
} else if dataSlice, ok := data.([]interface{}); ok {
data, _ = itemByIndex(dataSlice, key)
}
}

return nil, false
}

func itemByIndex(dataSlice []interface{}, key string) (interface{}, bool) {
keyInt, err := strconv.Atoi(key)
if err != nil {
return nil, false
}
if keyInt >= len(dataSlice) || keyInt < 0 {
return nil, false
}
return dataSlice[keyInt], true
}

// PutValue updates the value of a given map at the index specified by keys that denote the path to the value in the
// nested structure of the map. If there is no current entry at a key, a new map is created for that value.
func PutValue(data map[string]interface{}, val interface{}, keys ...string) {
if data == nil {
return
Expand Down
224 changes: 224 additions & 0 deletions pkg/data/values_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,224 @@
package data

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestGetValueFromAny(t *testing.T) {
t.Parallel()
tests := []struct {
MbolotSuse marked this conversation as resolved.
Show resolved Hide resolved
name string
data interface{}
keys []string
wantValue interface{}
wantSuccess bool
}{
{
name: "nil map",
data: nil,
keys: []string{"somekey"},
wantValue: nil,
wantSuccess: false,
},
{
name: "key is not in map",
data: map[string]interface{}{
"realKey": "realVal",
},
keys: []string{"badKey"},
wantValue: nil,
wantSuccess: false,
},
{
name: "key is in first level of map",
data: map[string]interface{}{
"realKey": "realVal",
},
keys: []string{"realKey"},
wantValue: "realVal",
wantSuccess: true,
},
{
name: "key is nested in map",
MbolotSuse marked this conversation as resolved.
Show resolved Hide resolved
data: map[string]interface{}{
"parent": map[string]interface{}{
"child": map[string]interface{}{
"grandchild": "someValue",
},
},
},
keys: []string{"parent", "child", "grandchild"},
wantValue: "someValue",
wantSuccess: true,
},
{
name: "incorrected nested key",
data: map[string]interface{}{
"parent": map[string]interface{}{
"child": map[string]interface{}{
"grandchild": "someValue",
},
},
},
keys: []string{"parent", "grandchild", "child"},
wantValue: nil,
wantSuccess: false,
},
{
name: "get index of slice",
data: map[string]interface{}{
"parent": map[string]interface{}{
"children": []interface{}{
"alice",
"bob",
"eve",
},
},
},
keys: []string{"parent", "children", "2"},
wantValue: "eve",
wantSuccess: true,
},
{
name: "get index of top level slice",
data: []interface{}{
"alice",
"bob",
"eve",
},
keys: []string{"2"},
wantValue: "eve",
wantSuccess: true,
},
{
name: "slice of maps",
data: []interface{}{
map[string]interface{}{
"notthisone": "val",
},
map[string]interface{}{
"parent": map[string]interface{}{
"children": []interface{}{
"alice",
"bob",
"eve",
},
},
},
},
keys: []string{"1", "parent", "children", "0"},
wantValue: "alice",
wantSuccess: true,
},
{
name: "index is too big",
data: map[string]interface{}{
"parent": map[string]interface{}{
"children": []interface{}{
"alice",
"bob",
"eve",
},
},
},
keys: []string{"parent", "children", "3"},
wantValue: nil,
wantSuccess: false,
},
{
name: "index is negative",
data: map[string]interface{}{
"parent": map[string]interface{}{
"children": []interface{}{
"alice",
"bob",
"eve",
},
},
},
keys: []string{"parent", "children", "-3"},
wantValue: nil,
wantSuccess: false,
},
{
name: "index not parseable to int",
data: map[string]interface{}{
"parent": map[string]interface{}{
"children": []interface{}{
"alice",
"bob",
"eve",
},
},
},
keys: []string{"parent", "children", "notanint"},
wantValue: nil,
wantSuccess: false,
},
{
name: "slice blank index",
data: []interface{}{
"bob",
},
keys: []string{""},
wantValue: nil,
wantSuccess: false,
},
{
name: "slice no index",
data: []interface{}{
"bob",
},
wantValue: nil,
wantSuccess: false,
},
{
name: "keys nested too far",
data: []interface{}{
"alice",
"bob",
"eve",
},
keys: []string{"2", "1"},
wantValue: nil,
wantSuccess: false,
},
{
name: "map blank key with value",
data: map[string]interface{}{
"": "bob",
},
keys: []string{""},
wantValue: "bob",
wantSuccess: true,
},
{
name: "map blank key no value",
data: map[string]interface{}{
"alice": "bob",
},
keys: []string{""},
wantValue: nil,
wantSuccess: false,
},
{
name: "map no key",
data: map[string]interface{}{
"": "bob",
},
wantValue: nil,
wantSuccess: false,
},
}
for _, test := range tests {
test := test
t.Run(test.name, func(t *testing.T) {
MbolotSuse marked this conversation as resolved.
Show resolved Hide resolved
t.Parallel()
gotValue, gotSuccess := GetValueFromAny(test.data, test.keys...)
assert.Equal(t, test.wantValue, gotValue)
assert.Equal(t, test.wantSuccess, gotSuccess)
})
}
}
Loading