Skip to content

Commit a584cfc

Browse files
jpbetzjpbetz
committed
Support updates to structs in maps
1 parent 321bfd6 commit a584cfc

File tree

5 files changed

+138
-29
lines changed

5 files changed

+138
-29
lines changed

‎typed/validate_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -354,7 +354,7 @@ func BenchmarkValidateStructured(b *testing.B) {
354354
type:
355355
scalar: boolean
356356
`,
357-
object: Example{
357+
object: &Example{
358358
listOfPrimitives: []Primitives{primitive1, primitive2, primitive3, primitive4},
359359
mapOfPrimitives: map[string]Primitives{"1": primitive1, "2": primitive2, "3": primitive3, "4": primitive4},
360360
mapOfLists: map[string][]Primitives{

‎value/mapreflect.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ package value
1919
import "reflect"
2020

2121
type mapReflect struct {
22-
Value reflect.Value
22+
valueReflect
2323
}
2424

2525
func (r mapReflect) Length() int {
@@ -28,11 +28,12 @@ func (r mapReflect) Length() int {
2828
}
2929

3030
func (r mapReflect) Get(key string) (Value, bool) {
31-
val := r.Value.MapIndex(r.toMapKey(key))
31+
mapKey := r.toMapKey(key)
32+
val := r.Value.MapIndex(mapKey)
3233
if !val.IsValid() {
3334
return nil, false
3435
}
35-
return mustWrapValueReflect(val), val != reflect.Value{}
36+
return mustWrapValueReflectMapItem(&r.Value, &mapKey, val), val != reflect.Value{}
3637
}
3738

3839
func (r mapReflect) Has(key string) bool {

‎value/structreflect.go

Lines changed: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ func buildStructCacheEntry(t reflect.Type, infos map[string]*fieldCacheEntry, fi
128128
}
129129

130130
type structReflect struct {
131-
Value reflect.Value
131+
valueReflect
132132
}
133133

134134
func (r structReflect) Length() int {
@@ -153,30 +153,48 @@ func (r structReflect) Has(key string) bool {
153153
}
154154

155155
func (r structReflect) Set(key string, val Value) {
156-
fieldVal, ok, _ := r.findJsonNameField(key)
156+
fieldEntry, ok := getStructCacheEntry(r.Value.Type())[key]
157157
if !ok {
158158
panic(fmt.Sprintf("key %s may not be set on struct %T: field does not exist", key, r.Value.Interface()))
159159
}
160-
if !fieldVal.CanSet() {
161-
// See https://blog.golang.org/laws-of-reflection for details on why a struct may not be settable
162-
panic(fmt.Sprintf("key %s may not be set on struct: %T: struct is not settable", key, r.Value.Interface()))
163-
}
164-
fieldVal.Set(reflect.ValueOf(val.Unstructured()))
160+
oldVal := fieldEntry.getFieldFromStruct(r.Value)
161+
newVal := reflect.ValueOf(val.Unstructured())
162+
r.update(fieldEntry, key, oldVal, newVal)
165163
}
166164

167165
func (r structReflect) Delete(key string) {
168-
fieldVal, ok, omitempty := r.findJsonNameField(key)
166+
fieldEntry, ok := getStructCacheEntry(r.Value.Type())[key]
169167
if !ok {
170168
panic(fmt.Sprintf("key %s may not be deleted on struct %T: field does not exist", key, r.Value.Interface()))
171169
}
172-
if !fieldVal.CanSet() {
173-
// See https://blog.golang.org/laws-of-reflection for details on why a struct may not be settable
174-
panic(fmt.Sprintf("key %s may not be deleted on struct: %T: struct is not settable", key, r.Value.Interface()))
175-
}
176-
if fieldVal.Kind() != reflect.Ptr && !omitempty {
170+
oldVal := fieldEntry.getFieldFromStruct(r.Value)
171+
if oldVal.Kind() != reflect.Ptr && !fieldEntry.isOmitEmpty {
177172
panic(fmt.Sprintf("key %s may not be deleted on struct: %T: value is neither a pointer nor an omitempty field", key, r.Value.Interface()))
178173
}
179-
fieldVal.Set(reflect.Zero(fieldVal.Type()))
174+
r.update(fieldEntry, key, oldVal, reflect.Zero(oldVal.Type()))
175+
}
176+
177+
func (r structReflect) update(fieldEntry *fieldCacheEntry, key string, oldVal, newVal reflect.Value) {
178+
if oldVal.CanSet() {
179+
oldVal.Set(newVal)
180+
return
181+
}
182+
183+
// map items are not addressable, so if a struct is contained in a map, the only way to modify it is
184+
// to write a replacement fieldEntry into the map.
185+
if r.ParentMap != nil {
186+
if r.ParentMapKey == nil {
187+
panic("ParentMapKey must not be nil if ParentMap is not nil")
188+
}
189+
replacement := reflect.New(r.Value.Type()).Elem()
190+
fieldEntry.getFieldFromStruct(replacement).Set(newVal)
191+
r.ParentMap.SetMapIndex(*r.ParentMapKey, replacement)
192+
return
193+
}
194+
195+
// This should never happen since NewValueReflect ensures that the root object reflected on is a pointer and map
196+
// item replacement is handled above.
197+
panic(fmt.Sprintf("key %s may not be modified on struct: %T: struct is not settable", key, r.Value.Interface()))
180198
}
181199

182200
func (r structReflect) Iterate(fn func(string, Value) bool) bool {

‎value/valuereflect.go

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,16 +33,22 @@ var reflectPool = sync.Pool{
3333

3434
// NewValueReflect creates a Value backed by an "interface{}" type,
3535
// typically an structured object in Kubernetes world that is uses reflection to expose.
36+
// The provided "interface{}" value must be a pointer so that the value can be modified via reflection.
3637
// The provided "interface{}" may contain structs and types that are converted to Values
3738
// by the jsonMarshaler interface.
3839
func NewValueReflect(value interface{}) (Value, error) {
3940
if value == nil {
4041
return NewValueInterface(nil), nil
4142
}
42-
return wrapValueReflect(reflect.ValueOf(value))
43+
v := reflect.ValueOf(value)
44+
if v.Kind() != reflect.Ptr {
45+
// The root value to reflect on must be a pointer so that map.Set() and map.Delete() operations are possible.
46+
return nil, fmt.Errorf("value provided to NewValueReflect must be a pointer")
47+
}
48+
return wrapValueReflect(nil, nil, v)
4349
}
4450

45-
func wrapValueReflect(value reflect.Value) (Value, error) {
51+
func wrapValueReflect(parentMap, parentMapKey *reflect.Value, value reflect.Value) (Value, error) {
4652
// TODO: conversion of json.Marshaller interface types is expensive. This can be mostly optimized away by
4753
// introducing conversion functions that do not require going through JSON and using those here.
4854
if marshaler, ok := getMarshaler(value); ok {
@@ -51,11 +57,21 @@ func wrapValueReflect(value reflect.Value) (Value, error) {
5157
value = dereference(value)
5258
val := reflectPool.Get().(*valueReflect)
5359
val.Value = value
60+
val.ParentMap = parentMap
61+
val.ParentMapKey = parentMapKey
5462
return Value(val), nil
5563
}
5664

5765
func mustWrapValueReflect(value reflect.Value) Value {
58-
v, err := wrapValueReflect(value)
66+
v, err := wrapValueReflect(nil, nil, value)
67+
if err != nil {
68+
panic(err)
69+
}
70+
return v
71+
}
72+
73+
func mustWrapValueReflectMapItem(parentMap, parentMapKey *reflect.Value, value reflect.Value) Value {
74+
v, err := wrapValueReflect(parentMap, parentMapKey, value)
5975
if err != nil {
6076
panic(err)
6177
}
@@ -71,7 +87,9 @@ func dereference(val reflect.Value) reflect.Value {
7187
}
7288

7389
type valueReflect struct {
74-
Value reflect.Value
90+
ParentMap *reflect.Value
91+
ParentMapKey *reflect.Value
92+
Value reflect.Value
7593
}
7694

7795
func (r valueReflect) IsMap() bool {
@@ -135,9 +153,9 @@ func (r valueReflect) AsMap() Map {
135153
val := r.Value
136154
switch val.Kind() {
137155
case reflect.Struct:
138-
return structReflect{Value: r.Value}
156+
return structReflect{r}
139157
case reflect.Map:
140-
return mapReflect{Value: r.Value}
158+
return mapReflect{r}
141159
default:
142160
panic("value is not a map or struct")
143161
}
@@ -196,9 +214,9 @@ func (r valueReflect) Unstructured() interface{} {
196214
case r.IsNull():
197215
return nil
198216
case val.Kind() == reflect.Struct:
199-
return structReflect{Value: r.Value}.Unstructured()
217+
return structReflect{r}.Unstructured()
200218
case val.Kind() == reflect.Map:
201-
return mapReflect{Value: r.Value}.Unstructured()
219+
return mapReflect{r}.Unstructured()
202220
case r.IsList():
203221
return listReflect{Value: r.Value}.Unstructured()
204222
case r.IsString():

‎value/valuereflect_test.go

Lines changed: 75 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,10 @@ import (
2525
)
2626

2727
func MustReflect(i interface{}) Value {
28-
v, err := NewValueReflect(i)
28+
if i == nil {
29+
return NewValueInterface(nil)
30+
}
31+
v, err := wrapValueReflect(nil, nil, reflect.ValueOf(i))
2932
if err != nil {
3033
panic(err)
3134
}
@@ -279,10 +282,10 @@ func TestReflectStructMutate(t *testing.T) {
279282
m := rv.AsMap()
280283
atKey1, ok := m.Get("key1")
281284
if !ok {
282-
t.Errorf("expected map.Get(key1) to be 1 but got !ok")
285+
t.Fatalf("expected map.Get(key1) to be 1 but got !ok")
283286
}
284287
if atKey1.AsInt() != 1 {
285-
t.Errorf("expected map.Get(key1) to be 1 but got: %v", atKey1)
288+
t.Fatalf("expected map.Get(key1) to be 1 but got: %v", atKey1)
286289
}
287290
m.Set("key1", NewValueInterface(int64(2)))
288291
m.Delete("key2")
@@ -296,6 +299,75 @@ func TestReflectStructMutate(t *testing.T) {
296299
}
297300
}
298301

302+
// TestReflectMutateNestedStruct ensures a structs nested within various typed can be modified.
303+
func TestReflectMutateNestedStruct(t *testing.T) {
304+
type nested struct {
305+
S string `json:"s,omitempty"`
306+
}
307+
type root struct {
308+
Field nested `json:"field"`
309+
List []nested `json:"list"`
310+
Map map[string]nested `json:"map"`
311+
MapOfMaps map[string]map[string]nested `json:"mapOfMaps"`
312+
MapOfLists map[string][]nested `json:"mapOfLists"`
313+
}
314+
rv := MustReflect(&root{
315+
Field: nested{S: "field"},
316+
List: []nested{{S: "listItem"}},
317+
Map: map[string]nested{"mapKey": {S: "mapItem"}},
318+
MapOfMaps: map[string]map[string]nested{"outer": {"inner": {S: "mapOfMapItem"}}},
319+
MapOfLists: map[string][]nested{"outer": {{S: "mapOfListsItem"}}},
320+
})
321+
rootMap := rv.AsMap()
322+
field, _ := rootMap.Get("field")
323+
list, _ := rootMap.Get("list")
324+
listItem := list.AsList().At(0)
325+
m, _ := rootMap.Get("map")
326+
mapItem, _ := m.AsMap().Get("mapKey")
327+
mapOfMaps, _ := rootMap.Get("mapOfMaps")
328+
innerMap, _ := mapOfMaps.AsMap().Get("outer")
329+
mapOfMapsItem, _ := innerMap.AsMap().Get("inner")
330+
mapOfLists, _ := rootMap.Get("mapOfLists")
331+
innerList, _ := mapOfLists.AsMap().Get("outer")
332+
mapOfListsItem := innerList.AsList().At(0)
333+
334+
field.AsMap().Set("s", NewValueInterface("field2"))
335+
listItem.AsMap().Set("s", NewValueInterface("listItem2"))
336+
mapItem.AsMap().Set("s", NewValueInterface("mapItem2"))
337+
mapOfMapsItem.AsMap().Set("s", NewValueInterface("mapOfMapItem2"))
338+
mapOfListsItem.AsMap().Set("s", NewValueInterface("mapOfListsItem2"))
339+
340+
unstructured := rv.Unstructured()
341+
expectedMap := map[string]interface{}{
342+
"field": map[string]interface{}{"s": "field2"},
343+
"list": []interface{}{map[string]interface{}{"s": "listItem2"}},
344+
"map": map[string]interface{}{"mapKey": map[string]interface{}{"s": "mapItem2"}},
345+
"mapOfMaps": map[string]interface{}{"outer": map[string]interface{}{"inner": map[string]interface{}{"s": "mapOfMapItem2"}}},
346+
"mapOfLists": map[string]interface{}{"outer": []interface{}{map[string]interface{}{"s": "mapOfListsItem2"}}},
347+
}
348+
if !reflect.DeepEqual(unstructured, expectedMap) {
349+
t.Errorf("expected %v but got: %v", expectedMap, unstructured)
350+
}
351+
352+
field.AsMap().Delete("s")
353+
listItem.AsMap().Delete("s")
354+
mapItem.AsMap().Delete("s")
355+
mapOfMapsItem.AsMap().Delete("s")
356+
mapOfListsItem.AsMap().Delete("s")
357+
358+
unstructured = rv.Unstructured()
359+
expectedMap = map[string]interface{}{
360+
"field": map[string]interface{}{},
361+
"list": []interface{}{map[string]interface{}{}},
362+
"map": map[string]interface{}{"mapKey": map[string]interface{}{}},
363+
"mapOfMaps": map[string]interface{}{"outer": map[string]interface{}{"inner": map[string]interface{}{}}},
364+
"mapOfLists": map[string]interface{}{"outer": []interface{}{map[string]interface{}{}}},
365+
}
366+
if !reflect.DeepEqual(unstructured, expectedMap) {
367+
t.Errorf("expected %v but got: %v", expectedMap, unstructured)
368+
}
369+
}
370+
299371
func TestReflectMap(t *testing.T) {
300372
cases := []struct {
301373
name string

0 commit comments

Comments
 (0)