Apply feedback

Author: jpbetz

internal/fixture/state.go

@@ -100,7 +100,7 @@ func (s *State) UpdateObject(tv *typed.TypedValue, version fieldpath.APIVersion,
 	return nil
 }
 
-// Update the current state with the passed in object
+// Set the current state with the passed in object
 func (s *State) Update(obj typed.YAMLObject, version fieldpath.APIVersion, manager string) error {
 	tv, err := s.Parser.FromYAML(FixTabsOrDie(obj))
 	if err != nil {
@@ -308,7 +308,7 @@ func (f ForceApplyObject) preprocess(parser typed.ParseableType) (Operation, err
 	return f, nil
 }
 
-// Update is a type of operation. It is a controller type of
+// Set is a type of operation. It is a controller type of
 // update. Errors are passed along.
 type Update struct {
 	Manager    string

merge/real_test.go

@@ -101,7 +101,7 @@ func BenchmarkOperations(b *testing.B) {
 					},
 				},
 				{
-					name: "Update",
+					name: "Set",
 					ops: []Operation{
 						Update{
 							Manager:    "controller",

merge/update.go

@@ -114,7 +114,7 @@ func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpa
 	return managers, compare, nil
 }
 
-// Update is the method you should call once you've merged your final
+// Set is the method you should call once you've merged your final
 // object on CREATE/UPDATE/PATCH verbs. newObject must be the object
 // that you intend to persist (after applying the patch if this is for a
 // PATCH call), and liveObject must be the original object (empty if

typed/union.go

@@ -243,7 +243,7 @@ func (u *union) Normalize(old, new, out value.Map) error {
 		return fmt.Errorf("multiple fields set without discriminator change: %v", ns)
 	}
 
-	// Update discriminiator if it needs to be deduced.
+	// Set discriminiator if it needs to be deduced.
 	if u.deduceInvalidDiscriminator && len(ns) == 1 {
 		u.d.Set(out, u.dn.toDiscriminated(*ns.One()))
 	}
@@ -265,7 +265,7 @@ func (u *union) NormalizeApply(applied, merged, out value.Map) error {
 		return fmt.Errorf("applied discriminator (%v) doesn't match applied field (%v)", u.d.Get(applied), *as.One())
 	}
 
-	// Update discriminiator if needed
+	// Set discriminiator if needed
 	if u.deduceInvalidDiscriminator {
 		u.d.Set(out, u.dn.toDiscriminated(*as.One()))
 	}

value/jsontagutil.go

@@ -17,24 +17,24 @@ limitations under the License.
 package value
 
 import (
+	"fmt"
 	"reflect"
 	"strings"
 )
 
 // TODO: This implements the same functionality as https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/runtime/converter.go#L236
 // but is based on the highly efficient approach from https://golang.org/src/encoding/json/encode.go
 
-func lookupJsonTags(f reflect.StructField) (string, bool, bool) {
-	var name string
+func lookupJsonTags(f reflect.StructField) (name string, omit bool, inline bool, omitempty bool) {
 	tag := f.Tag.Get("json")
 	if tag == "-" {
-		return f.Name, false, false
+		return "", true, false, false
 	}
 	name, opts := parseTag(tag)
 	if name == "" {
 		name = f.Name
 	}
-	return name, opts.Contains("inline"), opts.Contains("omitempty")
+	return name, false, opts.Contains("inline"), opts.Contains("omitempty")
 }
 
 func isZero(v reflect.Value) bool {
@@ -51,6 +51,8 @@ func isZero(v reflect.Value) bool {
 		return v.Float() == 0
 	case reflect.Interface, reflect.Ptr:
 		return v.IsNil()
+	case reflect.Chan, reflect.Func:
+		panic(fmt.Sprintf("unsupported type: %v", v.Type()))
 	}
 	return false
 }

value/structreflect.go

@@ -53,14 +53,21 @@ func (c *structCache) Get(t reflect.Type) (map[string]*fieldCacheEntry, bool) {
 	return entry, ok
 }
 
-// Update sets the fieldCacheEntry for the given type via a copy-on-write update to the struct cache.
-func (c *structCache) Update(t reflect.Type, m map[string]*fieldCacheEntry) {
+// Set sets the fieldCacheEntry for the given type via a copy-on-write update to the struct cache.
+func (c *structCache) Set(t reflect.Type, m map[string]*fieldCacheEntry) {
 	c.mu.Lock()
 	defer c.mu.Unlock()
 
-	oldCacheMap := c.value.Load().(structCacheMap)
-	newCacheMap := make(structCacheMap, len(oldCacheMap)+1)
-	for k, v := range oldCacheMap {
+	currentCacheMap := c.value.Load().(structCacheMap)
+
+	if _, ok := currentCacheMap[t]; ok {
+		// Bail if the entry has been set while waiting for lock acquisition.
+		// This is safe since setting entries is idempotent.
+		return
+	}
+
+	newCacheMap := make(structCacheMap, len(currentCacheMap)+1)
+	for k, v := range currentCacheMap {
 		newCacheMap[k] = v
 	}
 	newCacheMap[t] = m
@@ -99,14 +106,17 @@ func getStructCacheEntry(t reflect.Type) structCacheEntry {
 	hints := map[string]*fieldCacheEntry{}
 	buildStructCacheEntry(t, hints, nil)
 
-	reflectStructCache.Update(t, hints)
+	reflectStructCache.Set(t, hints)
 	return hints
 }
 
 func buildStructCacheEntry(t reflect.Type, infos map[string]*fieldCacheEntry, fieldPath [][]int) {
 	for i := 0; i < t.NumField(); i++ {
 		field := t.Field(i)
-		jsonName, isInline, isOmitempty := lookupJsonTags(field)
+		jsonName, omit, isInline, isOmitempty := lookupJsonTags(field)
+		if omit {
+			continue
+		}
 		if isInline {
 			buildStructCacheEntry(field.Type, infos, append(fieldPath, field.Index))
 			continue
@@ -131,19 +141,19 @@ func (r structReflect) Length() int {
 }
 
 func (r structReflect) Get(key string) (Value, bool) {
-	if val, ok := r.findJsonNameField(key); ok {
+	if val, ok, _ := r.findJsonNameField(key); ok {
 		return mustWrapValueReflect(val), true
 	}
 	return nil, false
 }
 
 func (r structReflect) Has(key string) bool {
-	_, ok := r.findJsonNameField(key)
+	_, ok, _ := r.findJsonNameField(key)
 	return ok
 }
 
 func (r structReflect) Set(key string, val Value) {
-	fieldVal, ok := r.findJsonNameField(key)
+	fieldVal, ok, _ := r.findJsonNameField(key)
 	if !ok {
 		panic(fmt.Sprintf("key %s may not be set on struct %T: field does not exist", key, r.Value.Interface()))
 	}
@@ -155,14 +165,17 @@ func (r structReflect) Set(key string, val Value) {
 }
 
 func (r structReflect) Delete(key string) {
-	fieldVal, ok := r.findJsonNameField(key)
+	fieldVal, ok, omitempty := r.findJsonNameField(key)
 	if !ok {
 		panic(fmt.Sprintf("key %s may not be deleted on struct %T: field does not exist", key, r.Value.Interface()))
 	}
 	if !fieldVal.CanSet() {
 		// See https://blog.golang.org/laws-of-reflection for details on why a struct may not be settable
 		panic(fmt.Sprintf("key %s may not be deleted on struct: %T: struct is not settable", key, r.Value.Interface()))
 	}
+	if fieldVal.Kind() != reflect.Ptr && !omitempty {
+		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()))
+	}
 	fieldVal.Set(reflect.Zero(fieldVal.Type()))
 }
 
@@ -228,11 +241,11 @@ func (r structReflect) findJsonNameFieldAndNotEmpty(jsonName string) (reflect.Va
 	return fieldVal, !omit
 }
 
-func (r structReflect) findJsonNameField(jsonName string) (reflect.Value, bool) {
+func (r structReflect) findJsonNameField(jsonName string) (val reflect.Value, ok bool, omitEmpty bool) {
 	structCacheEntry, ok := getStructCacheEntry(r.Value.Type())[jsonName]
 	if !ok {
-		return reflect.Value{}, false
+		return reflect.Value{}, false, false
 	}
 	fieldVal := structCacheEntry.getFieldFromStruct(r.Value)
-	return fieldVal, true
+	return fieldVal, true, structCacheEntry.isOmitEmpty
 }

value/valuereflect_test.go

@@ -179,6 +179,10 @@ type testBasicStruct struct {
 	I int64 `json:"int"`
 	S string
 }
+type testOmitStruct struct {
+	I int64 `json:"-"`
+	S string
+}
 type testInlineStruct struct {
 	Inline T `json:",inline"`
 	S      string
@@ -213,6 +217,12 @@ func TestReflectStruct(t *testing.T) {
 			expectedMap:          map[string]interface{}{"int": int64(10), "S": "string"},
 			expectedUnstructured: map[string]interface{}{"int": int64(10), "S": "string"},
 		},
+		{
+			name:                 "omit",
+			val:                  testOmitStruct{I: 10, S: "string"},
+			expectedMap:          map[string]interface{}{"S": "string"},
+			expectedUnstructured: map[string]interface{}{"S": "string"},
+		},
 		{
 			name:                 "inline",
 			val:                  &testInlineStruct{Inline: T{I: 10}, S: "string"},