From 0e97094ebf2f1040e8421c657936e3ade880cd85 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Mon, 24 Mar 2025 20:37:39 -0400 Subject: [PATCH 01/20] Add omitzero support --- value/jsontagutil.go | 63 +++++++++++++++++++++-- value/reflectcache.go | 14 +++-- value/reflectcache_test.go | 103 ++++++++++++++++++++++++++++++++++++- 3 files changed, 171 insertions(+), 9 deletions(-) diff --git a/value/jsontagutil.go b/value/jsontagutil.go index d4adb8fc..3aadceb2 100644 --- a/value/jsontagutil.go +++ b/value/jsontagutil.go @@ -22,22 +22,77 @@ import ( "strings" ) +type isZeroer interface { + IsZero() bool +} + +var isZeroerType = reflect.TypeOf((*isZeroer)(nil)).Elem() + +func reflectIsZero(dv reflect.Value) bool { + return dv.IsZero() +} + +// OmitZeroFunc returns a function for a type for a given struct field +// which determines if the value for that field is a zero value, matching +// how the stdlib JSON implementation. +func OmitZeroFunc(t reflect.Type) func(reflect.Value) bool { + // Provide a function that uses a type's IsZero method. + // This matches the go 1.24 custom IsZero() implementation matching + switch { + case t.Kind() == reflect.Interface && t.Implements(isZeroerType): + return func(v reflect.Value) bool { + // Avoid panics calling IsZero on a nil interface or + // non-nil interface with nil pointer. + return safeIsNil(v) || + (v.Elem().Kind() == reflect.Pointer && v.Elem().IsNil()) || + v.Interface().(isZeroer).IsZero() + } + case t.Kind() == reflect.Pointer && t.Implements(isZeroerType): + return func(v reflect.Value) bool { + // Avoid panics calling IsZero on nil pointer. + return safeIsNil(v) || v.Interface().(isZeroer).IsZero() + } + case t.Implements(isZeroerType): + return func(v reflect.Value) bool { + return v.Interface().(isZeroer).IsZero() + } + case reflect.PointerTo(t).Implements(isZeroerType): + return func(v reflect.Value) bool { + if !v.CanAddr() { + // Temporarily box v so we can take the address. + v2 := reflect.New(v.Type()).Elem() + v2.Set(v) + v = v2 + } + return v.Addr().Interface().(isZeroer).IsZero() + } + default: + // default to the reflect.IsZero implementation + return reflectIsZero + } +} + // 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) (name string, omit bool, inline bool, omitempty bool) { +func lookupJsonTags(f reflect.StructField) (name string, omit bool, inline bool, omitempty bool, omitzero func(reflect.Value) bool) { tag := f.Tag.Get("json") if tag == "-" { - return "", true, false, false + return "", true, false, false, nil } name, opts := parseTag(tag) if name == "" { name = f.Name } - return name, false, opts.Contains("inline"), opts.Contains("omitempty") + + if opts.Contains("omitzero") { + omitzero = OmitZeroFunc(f.Type) + } + + return name, false, opts.Contains("inline"), opts.Contains("omitempty"), omitzero } -func isZero(v reflect.Value) bool { +func isEmpty(v reflect.Value) bool { switch v.Kind() { case reflect.Array, reflect.Map, reflect.Slice, reflect.String: return v.Len() == 0 diff --git a/value/reflectcache.go b/value/reflectcache.go index 88693b87..3b4a402e 100644 --- a/value/reflectcache.go +++ b/value/reflectcache.go @@ -59,6 +59,8 @@ type FieldCacheEntry struct { JsonName string // isOmitEmpty is true if the field has the json 'omitempty' tag. isOmitEmpty bool + // omitzero is set if the field has the json 'omitzero' tag. + omitzero func(reflect.Value) bool // fieldPath is a list of field indices (see FieldByIndex) to lookup the value of // a field in a reflect.Value struct. The field indices in the list form a path used // to traverse through intermediary 'inline' fields. @@ -69,7 +71,13 @@ type FieldCacheEntry struct { } func (f *FieldCacheEntry) CanOmit(fieldVal reflect.Value) bool { - return f.isOmitEmpty && (safeIsNil(fieldVal) || isZero(fieldVal)) + if f.isOmitEmpty && (safeIsNil(fieldVal) || isEmpty(fieldVal)) { + return true + } + if f.omitzero != nil && f.omitzero(fieldVal) { + return true + } + return false } // GetFrom returns the field identified by this FieldCacheEntry from the provided struct. @@ -147,7 +155,7 @@ func typeReflectEntryOf(cm reflectCacheMap, t reflect.Type, updates reflectCache func buildStructCacheEntry(t reflect.Type, infos map[string]*FieldCacheEntry, fieldPath [][]int) { for i := 0; i < t.NumField(); i++ { field := t.Field(i) - jsonName, omit, isInline, isOmitempty := lookupJsonTags(field) + jsonName, omit, isInline, isOmitempty, omitzero := lookupJsonTags(field) if omit { continue } @@ -161,7 +169,7 @@ func buildStructCacheEntry(t reflect.Type, infos map[string]*FieldCacheEntry, fi } continue } - info := &FieldCacheEntry{JsonName: jsonName, isOmitEmpty: isOmitempty, fieldPath: append(fieldPath, field.Index), fieldType: field.Type} + info := &FieldCacheEntry{JsonName: jsonName, isOmitEmpty: isOmitempty, omitzero: omitzero, fieldPath: append(fieldPath, field.Index), fieldType: field.Type} infos[jsonName] = info } } diff --git a/value/reflectcache_test.go b/value/reflectcache_test.go index 33eae60c..c1a7c856 100644 --- a/value/reflectcache_test.go +++ b/value/reflectcache_test.go @@ -144,6 +144,7 @@ func TestTimeToUnstructured(t *testing.T) { func TestTypeReflectEntryOf(t *testing.T) { testString := "" + testCustomType := customOmitZeroType{} tests := map[string]struct { arg interface{} want *TypeReflectCacheEntry @@ -196,6 +197,62 @@ func TestTypeReflectEntryOf(t *testing.T) { }, }, }, + "StructWith*StringFieldOmitzero": { + arg: struct { + F1 *string `json:"f1,omitzero"` + }{}, + want: &TypeReflectCacheEntry{ + structFields: map[string]*FieldCacheEntry{ + "f1": { + JsonName: "f1", + omitzero: func(v reflect.Value) bool { return v.IsZero() }, + fieldPath: [][]int{{0}}, + fieldType: reflect.TypeOf(&testString), + TypeEntry: &TypeReflectCacheEntry{}, + }, + }, + orderedStructFields: []*FieldCacheEntry{ + { + JsonName: "f1", + omitzero: func(v reflect.Value) bool { return v.IsZero() }, + fieldPath: [][]int{{0}}, + fieldType: reflect.TypeOf(&testString), + TypeEntry: &TypeReflectCacheEntry{}, + }, + }, + }, + }, + "StructWith*CustomFieldOmitzero": { + arg: struct { + F1 customOmitZeroType `json:"f1,omitzero"` + }{}, + want: &TypeReflectCacheEntry{ + structFields: map[string]*FieldCacheEntry{ + "f1": { + JsonName: "f1", + omitzero: func(v reflect.Value) bool { return false }, + fieldPath: [][]int{{0}}, + fieldType: reflect.TypeOf(testCustomType), + TypeEntry: &TypeReflectCacheEntry{ + structFields: map[string]*FieldCacheEntry{}, + orderedStructFields: []*FieldCacheEntry{}, + }, + }, + }, + orderedStructFields: []*FieldCacheEntry{ + { + JsonName: "f1", + omitzero: func(v reflect.Value) bool { return false }, + fieldPath: [][]int{{0}}, + fieldType: reflect.TypeOf(testCustomType), + TypeEntry: &TypeReflectCacheEntry{ + structFields: map[string]*FieldCacheEntry{}, + orderedStructFields: []*FieldCacheEntry{}, + }, + }, + }, + }, + }, "StructWithInlinedField": { arg: struct { F1 string `json:",inline"` @@ -208,13 +265,55 @@ func TestTypeReflectEntryOf(t *testing.T) { } for name, tt := range tests { t.Run(name, func(t *testing.T) { - if got := TypeReflectEntryOf(reflect.TypeOf(tt.arg)); !reflect.DeepEqual(got, tt.want) { - t.Errorf("TypeReflectEntryOf() = %v, want %v", got, tt.want) + got := TypeReflectEntryOf(reflect.TypeOf(tt.arg)) + + // evaluate non-comparable omitzero functions + for k, v := range got.structFields { + compareOmitZero(t, v.fieldType, v.omitzero, tt.want.structFields[k].omitzero) + } + for i, v := range got.orderedStructFields { + compareOmitZero(t, v.fieldType, v.omitzero, tt.want.orderedStructFields[i].omitzero) + } + + // clear non-comparable omitzero functions + for k, v := range got.structFields { + v.omitzero = nil + tt.want.structFields[k].omitzero = nil + } + for i, v := range got.orderedStructFields { + v.omitzero = nil + tt.want.orderedStructFields[i].omitzero = nil + } + + // compare remaining fields + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("TypeReflectEntryOf() got\n%#v\nwant\n%#v", got, tt.want) } }) } } +type customOmitZeroType struct { +} + +func (c *customOmitZeroType) IsZero() bool { + return false +} + +func compareOmitZero(t *testing.T, fieldType reflect.Type, got, want func(reflect.Value) bool) { + t.Helper() + if (want == nil) != (got == nil) { + t.Fatalf("wanted omitzero=%v, got omitzero=%v", (want == nil), (got == nil)) + } + if want == nil { + return + } + v := reflect.New(fieldType).Elem() + if e, a := want(v), got(v); e != a { + t.Fatalf("wanted omitzero()=%v, got omitzero()=%v", e, a) + } +} + func TestUnmarshal(t *testing.T) { for _, tc := range []struct { JSON string From 133f75ec49cfc0c67c66edaf13d63170f435b4d0 Mon Sep 17 00:00:00 2001 From: Benjamin Elder Date: Thu, 17 Apr 2025 18:56:41 -0700 Subject: [PATCH 02/20] bump go version to 1.19+ go is currently on 1.24 / 1.23, 1.19 is old enough to still support back to Kubernetes 1.23 (10 releases behind the upcoming 1.33) switching to a newer go version gives us improvements to module management --- go.mod | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index dd09a8fe..a9cfd23f 100644 --- a/go.mod +++ b/go.mod @@ -3,9 +3,13 @@ module sigs.k8s.io/structured-merge-diff/v4 require ( github.com/google/go-cmp v0.5.9 github.com/json-iterator/go v1.1.12 - github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect sigs.k8s.io/randfill v0.0.0-20250304075658-069ef1bbf016 sigs.k8s.io/yaml v1.4.0 ) -go 1.13 +require ( + github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect + github.com/modern-go/reflect2 v1.0.2 // indirect +) + +go 1.19 From 0957b10a9d1e771dbab2e033c30856cfda9ad0c1 Mon Sep 17 00:00:00 2001 From: Benjamin Elder Date: Fri, 18 Apr 2025 12:03:59 -0700 Subject: [PATCH 03/20] stop exporting jsoniter based apis see: https://github.com/kubernetes-sigs/structured-merge-diff/issues/202 --- fieldpath/serialize-pe.go | 26 ++++++++++++++++++++++---- value/value.go | 17 +++++++++++------ 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/fieldpath/serialize-pe.go b/fieldpath/serialize-pe.go index cb18e7b1..3e25e05d 100644 --- a/fieldpath/serialize-pe.go +++ b/fieldpath/serialize-pe.go @@ -54,6 +54,24 @@ var ( peSepBytes = []byte(peSeparator) ) +// readJSONIter reads a Value from a JSON iterator. +// DO NOT EXPORT +// TODO: eliminate this https://github.com/kubernetes-sigs/structured-merge-diff/issues/202 +func readJSONIter(iter *jsoniter.Iterator) (value.Value, error) { + v := iter.Read() + if iter.Error != nil && iter.Error != io.EOF { + return nil, iter.Error + } + return value.NewValueInterface(v), nil +} + +// writeJSONStream writes a value into a JSON stream. +// DO NOT EXPORT +// TODO: eliminate this https://github.com/kubernetes-sigs/structured-merge-diff/issues/202 +func writeJSONStream(v value.Value, stream *jsoniter.Stream) { + stream.WriteVal(v.Unstructured()) +} + // DeserializePathElement parses a serialized path element func DeserializePathElement(s string) (PathElement, error) { b := []byte(s) @@ -75,7 +93,7 @@ func DeserializePathElement(s string) (PathElement, error) { case peValueSepBytes[0]: iter := readPool.BorrowIterator(b) defer readPool.ReturnIterator(iter) - v, err := value.ReadJSONIter(iter) + v, err := readJSONIter(iter) if err != nil { return PathElement{}, err } @@ -86,7 +104,7 @@ func DeserializePathElement(s string) (PathElement, error) { fields := value.FieldList{} iter.ReadObjectCB(func(iter *jsoniter.Iterator, key string) bool { - v, err := value.ReadJSONIter(iter) + v, err := readJSONIter(iter) if err != nil { iter.Error = err return false @@ -141,14 +159,14 @@ func serializePathElementToWriter(w io.Writer, pe PathElement) error { stream.WriteMore() } stream.WriteObjectField(field.Name) - value.WriteJSONStream(field.Value, stream) + writeJSONStream(field.Value, stream) } stream.WriteObjectEnd() case pe.Value != nil: if _, err := stream.Write(peValueSepBytes); err != nil { return err } - value.WriteJSONStream(*pe.Value, stream) + writeJSONStream(*pe.Value, stream) case pe.Index != nil: if _, err := stream.Write(peIndexSepBytes); err != nil { return err diff --git a/value/value.go b/value/value.go index f72e5cd2..06de6e34 100644 --- a/value/value.go +++ b/value/value.go @@ -23,6 +23,7 @@ import ( "strings" jsoniter "github.com/json-iterator/go" + yaml "sigs.k8s.io/yaml/goyaml.v2" ) @@ -90,7 +91,7 @@ func FromJSON(input []byte) (Value, error) { func FromJSONFast(input []byte) (Value, error) { iter := readPool.BorrowIterator(input) defer readPool.ReturnIterator(iter) - return ReadJSONIter(iter) + return readJSONIter(iter) } // ToJSON is a helper function for producing a JSon document. @@ -98,7 +99,7 @@ func ToJSON(v Value) ([]byte, error) { buf := bytes.Buffer{} stream := writePool.BorrowStream(&buf) defer writePool.ReturnStream(stream) - WriteJSONStream(v, stream) + writeJSONStream(v, stream) b := stream.Buffer() err := stream.Flush() // Help jsoniter manage its buffers--without this, the next @@ -109,8 +110,10 @@ func ToJSON(v Value) ([]byte, error) { return buf.Bytes(), err } -// ReadJSONIter reads a Value from a JSON iterator. -func ReadJSONIter(iter *jsoniter.Iterator) (Value, error) { +// readJSONIter reads a Value from a JSON iterator. +// DO NOT EXPORT +// TODO: eliminate this https://github.com/kubernetes-sigs/structured-merge-diff/issues/202 +func readJSONIter(iter *jsoniter.Iterator) (Value, error) { v := iter.Read() if iter.Error != nil && iter.Error != io.EOF { return nil, iter.Error @@ -118,8 +121,10 @@ func ReadJSONIter(iter *jsoniter.Iterator) (Value, error) { return NewValueInterface(v), nil } -// WriteJSONStream writes a value into a JSON stream. -func WriteJSONStream(v Value, stream *jsoniter.Stream) { +// writeJSONStream writes a value into a JSON stream. +// DO NOT EXPORT +// TODO: eliminate this https://github.com/kubernetes-sigs/structured-merge-diff/issues/202 +func writeJSONStream(v Value, stream *jsoniter.Stream) { stream.WriteVal(v.Unstructured()) } From 046e0a3da8b4708a0f2cb0f47cde1d6606f703b5 Mon Sep 17 00:00:00 2001 From: Benjamin Elder Date: Fri, 18 Apr 2025 12:12:40 -0700 Subject: [PATCH 04/20] bump module to v6 for breaking change dropping jsoniter from public APIs NOTE: v5 was used in a previous release attempt, so we skip to v6 --- fieldpath/element.go | 2 +- fieldpath/element_test.go | 2 +- fieldpath/fromvalue.go | 2 +- fieldpath/fromvalue_test.go | 2 +- fieldpath/managers_test.go | 2 +- fieldpath/path.go | 2 +- fieldpath/path_test.go | 2 +- fieldpath/pathelementmap.go | 2 +- fieldpath/pathelementmap_test.go | 2 +- fieldpath/serialize-pe.go | 2 +- fieldpath/set.go | 5 +++-- fieldpath/set_test.go | 5 +++-- go.mod | 2 +- internal/cli/operation.go | 4 ++-- internal/cli/options.go | 2 +- internal/fixture/state.go | 6 +++--- internal/fixture/state_test.go | 2 +- merge/conflict.go | 2 +- merge/conflict_test.go | 6 +++--- merge/deduced_test.go | 6 +++--- merge/default_keys_test.go | 8 ++++---- merge/duplicates_test.go | 8 ++++---- merge/extract_apply_test.go | 6 +++--- merge/field_level_overrides_test.go | 8 ++++---- merge/ignore_test.go | 4 ++-- merge/key_test.go | 6 +++--- merge/leaf_test.go | 8 ++++---- merge/multiple_appliers_test.go | 10 +++++----- merge/nested_test.go | 6 +++--- merge/obsolete_versions_test.go | 8 ++++---- merge/preserve_unknown_test.go | 6 +++--- merge/real_test.go | 4 ++-- merge/schema_change_test.go | 8 ++++---- merge/set_test.go | 6 +++--- merge/update.go | 7 ++++--- smd/main.go | 2 +- typed/compare.go | 6 +++--- typed/comparison_test.go | 4 ++-- typed/deduced_test.go | 4 ++-- typed/helpers.go | 6 +++--- typed/helpers_test.go | 4 ++-- typed/merge.go | 6 +++--- typed/merge_test.go | 4 ++-- typed/parser.go | 4 ++-- typed/parser_test.go | 2 +- typed/reconcile_schema.go | 4 ++-- typed/reconcile_schema_test.go | 4 ++-- typed/remove.go | 6 +++--- typed/remove_test.go | 6 +++--- typed/symdiff_test.go | 4 ++-- typed/tofieldset.go | 6 +++--- typed/toset_test.go | 6 +++--- typed/typed.go | 6 +++--- typed/validate.go | 6 +++--- typed/validate_test.go | 4 ++-- value/equals_test.go | 2 +- 56 files changed, 131 insertions(+), 128 deletions(-) diff --git a/fieldpath/element.go b/fieldpath/element.go index 1578f64c..90a48b95 100644 --- a/fieldpath/element.go +++ b/fieldpath/element.go @@ -21,7 +21,7 @@ import ( "sort" "strings" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/value" ) // PathElement describes how to select a child field given a containing object. diff --git a/fieldpath/element_test.go b/fieldpath/element_test.go index 05019a98..ae52c839 100644 --- a/fieldpath/element_test.go +++ b/fieldpath/element_test.go @@ -19,7 +19,7 @@ package fieldpath import ( "testing" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/value" ) func TestPathElementSet(t *testing.T) { diff --git a/fieldpath/fromvalue.go b/fieldpath/fromvalue.go index 20775ee0..78383e40 100644 --- a/fieldpath/fromvalue.go +++ b/fieldpath/fromvalue.go @@ -17,7 +17,7 @@ limitations under the License. package fieldpath import ( - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/value" ) // SetFromValue creates a set containing every leaf field mentioned in v. diff --git a/fieldpath/fromvalue_test.go b/fieldpath/fromvalue_test.go index 90e8b723..476aa683 100644 --- a/fieldpath/fromvalue_test.go +++ b/fieldpath/fromvalue_test.go @@ -19,7 +19,7 @@ package fieldpath import ( "testing" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/value" yaml "sigs.k8s.io/yaml/goyaml.v2" ) diff --git a/fieldpath/managers_test.go b/fieldpath/managers_test.go index d06b055b..bac9efeb 100644 --- a/fieldpath/managers_test.go +++ b/fieldpath/managers_test.go @@ -20,7 +20,7 @@ import ( "reflect" "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" ) var ( diff --git a/fieldpath/path.go b/fieldpath/path.go index 0413130b..a865ec42 100644 --- a/fieldpath/path.go +++ b/fieldpath/path.go @@ -20,7 +20,7 @@ import ( "fmt" "strings" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/value" ) // Path describes how to select a potentially deeply-nested child field given a diff --git a/fieldpath/path_test.go b/fieldpath/path_test.go index d3086732..22410e5c 100644 --- a/fieldpath/path_test.go +++ b/fieldpath/path_test.go @@ -19,7 +19,7 @@ package fieldpath import ( "testing" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/value" ) var ( diff --git a/fieldpath/pathelementmap.go b/fieldpath/pathelementmap.go index 41fc2474..ff7ee510 100644 --- a/fieldpath/pathelementmap.go +++ b/fieldpath/pathelementmap.go @@ -19,7 +19,7 @@ package fieldpath import ( "sort" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/value" ) // PathElementValueMap is a map from PathElement to value.Value. diff --git a/fieldpath/pathelementmap_test.go b/fieldpath/pathelementmap_test.go index 5685c7ca..8341370b 100644 --- a/fieldpath/pathelementmap_test.go +++ b/fieldpath/pathelementmap_test.go @@ -19,7 +19,7 @@ package fieldpath import ( "testing" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/value" ) func TestPathElementValueMap(t *testing.T) { diff --git a/fieldpath/serialize-pe.go b/fieldpath/serialize-pe.go index 3e25e05d..f4b00c2e 100644 --- a/fieldpath/serialize-pe.go +++ b/fieldpath/serialize-pe.go @@ -24,7 +24,7 @@ import ( "strings" jsoniter "github.com/json-iterator/go" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/value" ) var ErrUnknownPathElementType = errors.New("unknown path element type") diff --git a/fieldpath/set.go b/fieldpath/set.go index 77ae2511..0c285d17 100644 --- a/fieldpath/set.go +++ b/fieldpath/set.go @@ -18,11 +18,12 @@ package fieldpath import ( "fmt" - "sigs.k8s.io/structured-merge-diff/v4/value" "sort" "strings" - "sigs.k8s.io/structured-merge-diff/v4/schema" + "sigs.k8s.io/structured-merge-diff/v6/value" + + "sigs.k8s.io/structured-merge-diff/v6/schema" ) // Set identifies a set of fields. diff --git a/fieldpath/set_test.go b/fieldpath/set_test.go index 7efb374e..bb159c9a 100644 --- a/fieldpath/set_test.go +++ b/fieldpath/set_test.go @@ -20,10 +20,11 @@ import ( "bytes" "fmt" "math/rand" - "sigs.k8s.io/structured-merge-diff/v4/value" "testing" - "sigs.k8s.io/structured-merge-diff/v4/schema" + "sigs.k8s.io/structured-merge-diff/v6/value" + + "sigs.k8s.io/structured-merge-diff/v6/schema" yaml "sigs.k8s.io/yaml/goyaml.v2" ) diff --git a/go.mod b/go.mod index dd09a8fe..f416cf97 100644 --- a/go.mod +++ b/go.mod @@ -1,4 +1,4 @@ -module sigs.k8s.io/structured-merge-diff/v4 +module sigs.k8s.io/structured-merge-diff/v6 require ( github.com/google/go-cmp v0.5.9 diff --git a/internal/cli/operation.go b/internal/cli/operation.go index 10f75996..e80d5383 100644 --- a/internal/cli/operation.go +++ b/internal/cli/operation.go @@ -21,8 +21,8 @@ import ( "io" "io/ioutil" - "sigs.k8s.io/structured-merge-diff/v4/typed" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/typed" + "sigs.k8s.io/structured-merge-diff/v6/value" ) type Operation interface { diff --git a/internal/cli/options.go b/internal/cli/options.go index 9a4cbeb8..66626ffd 100644 --- a/internal/cli/options.go +++ b/internal/cli/options.go @@ -24,7 +24,7 @@ import ( "io/ioutil" "os" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) var ( diff --git a/internal/fixture/state.go b/internal/fixture/state.go index 98d9a77f..efa35480 100644 --- a/internal/fixture/state.go +++ b/internal/fixture/state.go @@ -22,9 +22,9 @@ import ( "github.com/google/go-cmp/cmp" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - "sigs.k8s.io/structured-merge-diff/v4/merge" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/merge" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) // For the sake of tests, a parser is something that can retrieve a diff --git a/internal/fixture/state_test.go b/internal/fixture/state_test.go index 0b88f755..5b8bae7a 100644 --- a/internal/fixture/state_test.go +++ b/internal/fixture/state_test.go @@ -20,7 +20,7 @@ import ( "fmt" "testing" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) func TestFixTabs(t *testing.T) { diff --git a/merge/conflict.go b/merge/conflict.go index f1aa2586..dea64707 100644 --- a/merge/conflict.go +++ b/merge/conflict.go @@ -21,7 +21,7 @@ import ( "sort" "strings" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" ) // Conflict is a conflict on a specific field with the current manager of diff --git a/merge/conflict_test.go b/merge/conflict_test.go index 49841856..85978420 100644 --- a/merge/conflict_test.go +++ b/merge/conflict_test.go @@ -19,9 +19,9 @@ package merge_test import ( "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - "sigs.k8s.io/structured-merge-diff/v4/merge" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/merge" + "sigs.k8s.io/structured-merge-diff/v6/value" ) var ( diff --git a/merge/deduced_test.go b/merge/deduced_test.go index 3bde04b9..fcd47c1e 100644 --- a/merge/deduced_test.go +++ b/merge/deduced_test.go @@ -19,9 +19,9 @@ package merge_test import ( "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - . "sigs.k8s.io/structured-merge-diff/v4/internal/fixture" - "sigs.k8s.io/structured-merge-diff/v4/merge" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + . "sigs.k8s.io/structured-merge-diff/v6/internal/fixture" + "sigs.k8s.io/structured-merge-diff/v6/merge" ) func TestDeduced(t *testing.T) { diff --git a/merge/default_keys_test.go b/merge/default_keys_test.go index 33a62b83..24b202d4 100644 --- a/merge/default_keys_test.go +++ b/merge/default_keys_test.go @@ -16,10 +16,10 @@ package merge_test import ( "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - . "sigs.k8s.io/structured-merge-diff/v4/internal/fixture" - "sigs.k8s.io/structured-merge-diff/v4/merge" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + . "sigs.k8s.io/structured-merge-diff/v6/internal/fixture" + "sigs.k8s.io/structured-merge-diff/v6/merge" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) // portListParser sets the default value of key "protocol" to "TCP" diff --git a/merge/duplicates_test.go b/merge/duplicates_test.go index e0724370..299da4ec 100644 --- a/merge/duplicates_test.go +++ b/merge/duplicates_test.go @@ -19,10 +19,10 @@ package merge_test import ( "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - . "sigs.k8s.io/structured-merge-diff/v4/internal/fixture" - "sigs.k8s.io/structured-merge-diff/v4/merge" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + . "sigs.k8s.io/structured-merge-diff/v6/internal/fixture" + "sigs.k8s.io/structured-merge-diff/v6/merge" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) var duplicatesParser = func() Parser { diff --git a/merge/extract_apply_test.go b/merge/extract_apply_test.go index 50e19e13..2ae1df09 100644 --- a/merge/extract_apply_test.go +++ b/merge/extract_apply_test.go @@ -19,9 +19,9 @@ package merge_test import ( "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - . "sigs.k8s.io/structured-merge-diff/v4/internal/fixture" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + . "sigs.k8s.io/structured-merge-diff/v6/internal/fixture" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) var extractParser = func() Parser { diff --git a/merge/field_level_overrides_test.go b/merge/field_level_overrides_test.go index 75ac3256..d47d0ef7 100644 --- a/merge/field_level_overrides_test.go +++ b/merge/field_level_overrides_test.go @@ -3,10 +3,10 @@ package merge_test import ( "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - "sigs.k8s.io/structured-merge-diff/v4/internal/fixture" - "sigs.k8s.io/structured-merge-diff/v4/merge" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/internal/fixture" + "sigs.k8s.io/structured-merge-diff/v6/merge" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) func TestFieldLevelOverrides(t *testing.T) { diff --git a/merge/ignore_test.go b/merge/ignore_test.go index 07fdbce0..debbee29 100644 --- a/merge/ignore_test.go +++ b/merge/ignore_test.go @@ -19,8 +19,8 @@ package merge_test import ( "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - . "sigs.k8s.io/structured-merge-diff/v4/internal/fixture" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + . "sigs.k8s.io/structured-merge-diff/v6/internal/fixture" ) func TestIgnoreFilter(t *testing.T) { diff --git a/merge/key_test.go b/merge/key_test.go index e76e67cb..61969a83 100644 --- a/merge/key_test.go +++ b/merge/key_test.go @@ -19,9 +19,9 @@ package merge_test import ( "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - . "sigs.k8s.io/structured-merge-diff/v4/internal/fixture" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + . "sigs.k8s.io/structured-merge-diff/v6/internal/fixture" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) var associativeListParser = func() Parser { diff --git a/merge/leaf_test.go b/merge/leaf_test.go index ab6d59ae..5eccdaf1 100644 --- a/merge/leaf_test.go +++ b/merge/leaf_test.go @@ -19,10 +19,10 @@ package merge_test import ( "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - . "sigs.k8s.io/structured-merge-diff/v4/internal/fixture" - "sigs.k8s.io/structured-merge-diff/v4/merge" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + . "sigs.k8s.io/structured-merge-diff/v6/internal/fixture" + "sigs.k8s.io/structured-merge-diff/v6/merge" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) var leafFieldsParser = func() Parser { diff --git a/merge/multiple_appliers_test.go b/merge/multiple_appliers_test.go index 0794cda9..49a84ee4 100644 --- a/merge/multiple_appliers_test.go +++ b/merge/multiple_appliers_test.go @@ -22,11 +22,11 @@ import ( "strings" "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - . "sigs.k8s.io/structured-merge-diff/v4/internal/fixture" - "sigs.k8s.io/structured-merge-diff/v4/merge" - "sigs.k8s.io/structured-merge-diff/v4/typed" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + . "sigs.k8s.io/structured-merge-diff/v6/internal/fixture" + "sigs.k8s.io/structured-merge-diff/v6/merge" + "sigs.k8s.io/structured-merge-diff/v6/typed" + "sigs.k8s.io/structured-merge-diff/v6/value" yaml "sigs.k8s.io/yaml/goyaml.v2" ) diff --git a/merge/nested_test.go b/merge/nested_test.go index eb65a5a9..ffdb07b8 100644 --- a/merge/nested_test.go +++ b/merge/nested_test.go @@ -19,9 +19,9 @@ package merge_test import ( "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - . "sigs.k8s.io/structured-merge-diff/v4/internal/fixture" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + . "sigs.k8s.io/structured-merge-diff/v6/internal/fixture" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) var nestedTypeParser = func() Parser { diff --git a/merge/obsolete_versions_test.go b/merge/obsolete_versions_test.go index 3073a198..226a8708 100644 --- a/merge/obsolete_versions_test.go +++ b/merge/obsolete_versions_test.go @@ -20,10 +20,10 @@ import ( "fmt" "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - . "sigs.k8s.io/structured-merge-diff/v4/internal/fixture" - "sigs.k8s.io/structured-merge-diff/v4/merge" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + . "sigs.k8s.io/structured-merge-diff/v6/internal/fixture" + "sigs.k8s.io/structured-merge-diff/v6/merge" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) // specificVersionConverter doesn't convert and return the exact same diff --git a/merge/preserve_unknown_test.go b/merge/preserve_unknown_test.go index 2d77384f..f7db382d 100644 --- a/merge/preserve_unknown_test.go +++ b/merge/preserve_unknown_test.go @@ -19,9 +19,9 @@ package merge_test import ( "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - . "sigs.k8s.io/structured-merge-diff/v4/internal/fixture" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + . "sigs.k8s.io/structured-merge-diff/v6/internal/fixture" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) var preserveUnknownParser = func() Parser { diff --git a/merge/real_test.go b/merge/real_test.go index 9a1cc9e9..c8254fce 100644 --- a/merge/real_test.go +++ b/merge/real_test.go @@ -21,8 +21,8 @@ import ( "path/filepath" "testing" - . "sigs.k8s.io/structured-merge-diff/v4/internal/fixture" - "sigs.k8s.io/structured-merge-diff/v4/typed" + . "sigs.k8s.io/structured-merge-diff/v6/internal/fixture" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) func testdata(file string) string { diff --git a/merge/schema_change_test.go b/merge/schema_change_test.go index f98fdf73..d733407c 100644 --- a/merge/schema_change_test.go +++ b/merge/schema_change_test.go @@ -19,10 +19,10 @@ package merge_test import ( "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - . "sigs.k8s.io/structured-merge-diff/v4/internal/fixture" - "sigs.k8s.io/structured-merge-diff/v4/merge" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + . "sigs.k8s.io/structured-merge-diff/v6/internal/fixture" + "sigs.k8s.io/structured-merge-diff/v6/merge" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) var structParser = func() *typed.Parser { diff --git a/merge/set_test.go b/merge/set_test.go index e1209926..73d7170d 100644 --- a/merge/set_test.go +++ b/merge/set_test.go @@ -19,9 +19,9 @@ package merge_test import ( "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - . "sigs.k8s.io/structured-merge-diff/v4/internal/fixture" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + . "sigs.k8s.io/structured-merge-diff/v6/internal/fixture" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) var setFieldsParser = func() Parser { diff --git a/merge/update.go b/merge/update.go index 455818ff..99d722f8 100644 --- a/merge/update.go +++ b/merge/update.go @@ -15,9 +15,10 @@ package merge import ( "fmt" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - "sigs.k8s.io/structured-merge-diff/v4/typed" - "sigs.k8s.io/structured-merge-diff/v4/value" + + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/typed" + "sigs.k8s.io/structured-merge-diff/v6/value" ) // Converter is an interface to the conversion logic. The converter diff --git a/smd/main.go b/smd/main.go index 34a904e9..b8b29813 100644 --- a/smd/main.go +++ b/smd/main.go @@ -22,7 +22,7 @@ import ( "flag" "log" - "sigs.k8s.io/structured-merge-diff/v4/internal/cli" + "sigs.k8s.io/structured-merge-diff/v6/internal/cli" ) func main() { diff --git a/typed/compare.go b/typed/compare.go index 5fffa5e2..488251f6 100644 --- a/typed/compare.go +++ b/typed/compare.go @@ -20,9 +20,9 @@ import ( "fmt" "strings" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - "sigs.k8s.io/structured-merge-diff/v4/schema" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/schema" + "sigs.k8s.io/structured-merge-diff/v6/value" ) // Comparison is the return value of a TypedValue.Compare() operation. diff --git a/typed/comparison_test.go b/typed/comparison_test.go index 321f90cc..73c93622 100644 --- a/typed/comparison_test.go +++ b/typed/comparison_test.go @@ -3,8 +3,8 @@ package typed_test import ( "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) func TestComparisonExcludeFields(t *testing.T) { diff --git a/typed/deduced_test.go b/typed/deduced_test.go index 0719eae1..2d737a2e 100644 --- a/typed/deduced_test.go +++ b/typed/deduced_test.go @@ -20,8 +20,8 @@ import ( "fmt" "testing" - "sigs.k8s.io/structured-merge-diff/v4/typed" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/typed" + "sigs.k8s.io/structured-merge-diff/v6/value" ) func TestValidateDeducedType(t *testing.T) { diff --git a/typed/helpers.go b/typed/helpers.go index 78fdb0e7..f10ac6e9 100644 --- a/typed/helpers.go +++ b/typed/helpers.go @@ -21,9 +21,9 @@ import ( "fmt" "strings" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - "sigs.k8s.io/structured-merge-diff/v4/schema" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/schema" + "sigs.k8s.io/structured-merge-diff/v6/value" ) // ValidationError reports an error about a particular field diff --git a/typed/helpers_test.go b/typed/helpers_test.go index d04d4339..2b0d1c4f 100644 --- a/typed/helpers_test.go +++ b/typed/helpers_test.go @@ -4,8 +4,8 @@ import ( "strings" "testing" - "sigs.k8s.io/structured-merge-diff/v4/internal/fixture" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/internal/fixture" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) func TestInvalidOverride(t *testing.T) { diff --git a/typed/merge.go b/typed/merge.go index fa227ac4..f8ca9aba 100644 --- a/typed/merge.go +++ b/typed/merge.go @@ -17,9 +17,9 @@ limitations under the License. package typed import ( - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - "sigs.k8s.io/structured-merge-diff/v4/schema" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/schema" + "sigs.k8s.io/structured-merge-diff/v6/value" ) type mergingWalker struct { diff --git a/typed/merge_test.go b/typed/merge_test.go index 1e8562f1..c799e32f 100644 --- a/typed/merge_test.go +++ b/typed/merge_test.go @@ -20,8 +20,8 @@ import ( "fmt" "testing" - "sigs.k8s.io/structured-merge-diff/v4/typed" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/typed" + "sigs.k8s.io/structured-merge-diff/v6/value" ) type mergeTestCase struct { diff --git a/typed/parser.go b/typed/parser.go index 0e9f7cc7..a7ebf3a2 100644 --- a/typed/parser.go +++ b/typed/parser.go @@ -19,8 +19,8 @@ package typed import ( "fmt" - "sigs.k8s.io/structured-merge-diff/v4/schema" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/schema" + "sigs.k8s.io/structured-merge-diff/v6/value" yaml "sigs.k8s.io/yaml/goyaml.v2" ) diff --git a/typed/parser_test.go b/typed/parser_test.go index b81a9372..b47dc65c 100644 --- a/typed/parser_test.go +++ b/typed/parser_test.go @@ -22,7 +22,7 @@ import ( "strings" "testing" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/typed" yaml "sigs.k8s.io/yaml/goyaml.v2" ) diff --git a/typed/reconcile_schema.go b/typed/reconcile_schema.go index 6a7697e3..9b20e54a 100644 --- a/typed/reconcile_schema.go +++ b/typed/reconcile_schema.go @@ -20,8 +20,8 @@ import ( "fmt" "sync" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - "sigs.k8s.io/structured-merge-diff/v4/schema" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/schema" ) var fmPool = sync.Pool{ diff --git a/typed/reconcile_schema_test.go b/typed/reconcile_schema_test.go index 72053af2..9f0ace86 100644 --- a/typed/reconcile_schema_test.go +++ b/typed/reconcile_schema_test.go @@ -20,8 +20,8 @@ import ( "fmt" "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) type reconcileTestCase struct { diff --git a/typed/remove.go b/typed/remove.go index ad071ee8..86de5105 100644 --- a/typed/remove.go +++ b/typed/remove.go @@ -14,9 +14,9 @@ limitations under the License. package typed import ( - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - "sigs.k8s.io/structured-merge-diff/v4/schema" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/schema" + "sigs.k8s.io/structured-merge-diff/v6/value" ) type removingWalker struct { diff --git a/typed/remove_test.go b/typed/remove_test.go index 3a959ceb..2cb469dc 100644 --- a/typed/remove_test.go +++ b/typed/remove_test.go @@ -20,9 +20,9 @@ import ( "fmt" "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - "sigs.k8s.io/structured-merge-diff/v4/typed" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/typed" + "sigs.k8s.io/structured-merge-diff/v6/value" ) type removeTestCase struct { diff --git a/typed/symdiff_test.go b/typed/symdiff_test.go index 0ef15a2c..50e7d425 100644 --- a/typed/symdiff_test.go +++ b/typed/symdiff_test.go @@ -20,8 +20,8 @@ import ( "fmt" "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) type symdiffTestCase struct { diff --git a/typed/tofieldset.go b/typed/tofieldset.go index d563a87e..a52e342e 100644 --- a/typed/tofieldset.go +++ b/typed/tofieldset.go @@ -19,9 +19,9 @@ package typed import ( "sync" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - "sigs.k8s.io/structured-merge-diff/v4/schema" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/schema" + "sigs.k8s.io/structured-merge-diff/v6/value" ) var tPool = sync.Pool{ diff --git a/typed/toset_test.go b/typed/toset_test.go index 1b5d4f25..8b3e9da9 100644 --- a/typed/toset_test.go +++ b/typed/toset_test.go @@ -20,9 +20,9 @@ import ( "fmt" "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - "sigs.k8s.io/structured-merge-diff/v4/typed" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/typed" + "sigs.k8s.io/structured-merge-diff/v6/value" ) type objSetPair struct { diff --git a/typed/typed.go b/typed/typed.go index 7edaa6d4..0f9968fd 100644 --- a/typed/typed.go +++ b/typed/typed.go @@ -19,9 +19,9 @@ package typed import ( "sync" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - "sigs.k8s.io/structured-merge-diff/v4/schema" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/schema" + "sigs.k8s.io/structured-merge-diff/v6/value" ) // ValidationOptions is the list of all the options available when running the validation. diff --git a/typed/validate.go b/typed/validate.go index c38234c5..3371f87b 100644 --- a/typed/validate.go +++ b/typed/validate.go @@ -19,9 +19,9 @@ package typed import ( "sync" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" - "sigs.k8s.io/structured-merge-diff/v4/schema" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/schema" + "sigs.k8s.io/structured-merge-diff/v6/value" ) var vPool = sync.Pool{ diff --git a/typed/validate_test.go b/typed/validate_test.go index 2eb0041e..b4c177a4 100644 --- a/typed/validate_test.go +++ b/typed/validate_test.go @@ -21,8 +21,8 @@ import ( "strings" "testing" - "sigs.k8s.io/structured-merge-diff/v4/schema" - "sigs.k8s.io/structured-merge-diff/v4/typed" + "sigs.k8s.io/structured-merge-diff/v6/schema" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) type validationTestCase struct { diff --git a/value/equals_test.go b/value/equals_test.go index 1f98da2f..f41d4458 100644 --- a/value/equals_test.go +++ b/value/equals_test.go @@ -21,7 +21,7 @@ import ( "path/filepath" "testing" - "sigs.k8s.io/structured-merge-diff/v4/value" + "sigs.k8s.io/structured-merge-diff/v6/value" yaml "sigs.k8s.io/yaml/goyaml.v2" ) From 1a373a96647e26a5a289b52cd93af051c3afdac6 Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Thu, 1 May 2025 11:49:39 -0400 Subject: [PATCH 05/20] Add copy and iterator utils --- fieldpath/element.go | 71 ++++++++++++++++++++++++++++++++ fieldpath/element_test.go | 85 ++++++++++++++++++++++++--------------- fieldpath/set.go | 38 +++++++++++++++++ fieldpath/set_test.go | 27 ++++++++++++- go.mod | 2 +- value/fields.go | 8 ++++ 6 files changed, 197 insertions(+), 34 deletions(-) diff --git a/fieldpath/element.go b/fieldpath/element.go index 90a48b95..73436912 100644 --- a/fieldpath/element.go +++ b/fieldpath/element.go @@ -18,6 +18,7 @@ package fieldpath import ( "fmt" + "iter" "sort" "strings" @@ -47,6 +48,36 @@ type PathElement struct { Index *int } +// FieldNameElement creates a new FieldName PathElement. +func FieldNameElement(name string) PathElement { + return PathElement{FieldName: &name} +} + +// KeyElement creates a new Key PathElement with the key fields. +func KeyElement(fields ...value.Field) PathElement { + l := value.FieldList(fields) + return PathElement{Key: &l} +} + +// KeyElementByFields creates a new Key PathElement from names and values. +// `nameValues` must have an even number of entries, alternating +// names (type must be string) with values (type must be value.Value). If these +// conditions are not met, KeyByFields will panic--it's intended for static +// construction and shouldn't have user-produced values passed to it. +func KeyElementByFields(nameValues ...any) PathElement { + return PathElement{Key: KeyByFields(nameValues...)} +} + +// ValueElement creates a new Value PathElement. +func ValueElement(value value.Value) PathElement { + return PathElement{Value: &value} +} + +// IndexElement creates a new Index PathElement. +func IndexElement(index int) PathElement { + return PathElement{Index: &index} +} + // Less provides an order for path elements. func (e PathElement) Less(rhs PathElement) bool { return e.Compare(rhs) < 0 @@ -156,6 +187,25 @@ func (e PathElement) String() string { } } +// Copy returns a copy of the PathElement. +// This is not a full deep copy as any contained value.Value is not copied. +func (e PathElement) Copy() PathElement { + if e.FieldName != nil { + return PathElement{FieldName: e.FieldName} + } + if e.Key != nil { + c := e.Key.Copy() + return PathElement{Key: &c} + } + if e.Value != nil { + return PathElement{Value: e.Value} + } + if e.Index != nil { + return PathElement{Index: e.Index} + } + return e // zero value +} + // KeyByFields is a helper function which constructs a key for an associative // list type. `nameValues` must have an even number of entries, alternating // names (type must be string) with values (type must be value.Value). If these @@ -193,6 +243,16 @@ func (spe sortedPathElements) Len() int { return len(spe) } func (spe sortedPathElements) Less(i, j int) bool { return spe[i].Less(spe[j]) } func (spe sortedPathElements) Swap(i, j int) { spe[i], spe[j] = spe[j], spe[i] } +// Copy returns a copy of the PathElementSet. +// This is not a full deep copy as any contained value.Value is not copied. +func (s PathElementSet) Copy() PathElementSet { + out := make(sortedPathElements, len(s.members)) + for i := range s.members { + out[i] = s.members[i].Copy() + } + return PathElementSet{members: out} +} + // Insert adds pe to the set. func (s *PathElementSet) Insert(pe PathElement) { loc := sort.Search(len(s.members), func(i int) bool { @@ -315,3 +375,14 @@ func (s *PathElementSet) Iterate(f func(PathElement)) { f(pe) } } + +// All iterates over each PathElement in the set. The order is deterministic. +func (s *PathElementSet) All() iter.Seq[PathElement] { + return func(yield func(element PathElement) bool) { + for _, pe := range s.members { + if !yield(pe) { + return + } + } + } +} diff --git a/fieldpath/element_test.go b/fieldpath/element_test.go index ae52c839..aba96b14 100644 --- a/fieldpath/element_test.go +++ b/fieldpath/element_test.go @@ -33,6 +33,10 @@ func TestPathElementSet(t *testing.T) { if !s2.Has(PathElement{}) { t.Errorf("expected to have something: %#v", s2) } + c2 := s2.Copy() + if !c2.Equals(s2) { + t.Errorf("expected copy to equal original: %#v, %#v", s2, c2) + } n1 := "aoeu" n2 := "asdf" @@ -60,6 +64,20 @@ func TestPathElementSet(t *testing.T) { } i++ }) + i = 0 + for pe := range s2.All() { + e, a := expected[i], pe.FieldName + if e == nil || a == nil { + if e != a { + t.Errorf("index %v wanted %#v, got %#v", i, e, a) + } + } else { + if *e != *a { + t.Errorf("index %v wanted %#v, got %#v", i, *e, *a) + } + } + i++ + } } func strptr(s string) *string { return &s } @@ -68,6 +86,9 @@ func valptr(i interface{}) *value.Value { v := value.NewValueInterface(i) return &v } +func val(i interface{}) value.Value { + return value.NewValueInterface(i) +} func TestPathElementLess(t *testing.T) { table := []struct { @@ -84,71 +105,71 @@ func TestPathElementLess(t *testing.T) { eq: true, }, { name: "FieldName-1", - a: PathElement{FieldName: strptr("anteater")}, - b: PathElement{FieldName: strptr("zebra")}, + a: FieldNameElement("anteater"), + b: FieldNameElement("zebra"), }, { name: "FieldName-2", - a: PathElement{FieldName: strptr("bee")}, - b: PathElement{FieldName: strptr("bee")}, + a: FieldNameElement("bee"), + b: FieldNameElement("bee"), eq: true, }, { name: "FieldName-3", - a: PathElement{FieldName: strptr("capybara")}, - b: PathElement{Key: KeyByFields("dog", 3)}, + a: FieldNameElement("capybara"), + b: KeyElementByFields("dog", 3), }, { name: "FieldName-4", - a: PathElement{FieldName: strptr("elephant")}, - b: PathElement{Value: valptr(4)}, + a: FieldNameElement("elephant"), + b: ValueElement(val(4)), }, { name: "FieldName-5", - a: PathElement{FieldName: strptr("falcon")}, - b: PathElement{Index: intptr(5)}, + a: FieldNameElement("falcon"), + b: IndexElement(5), }, { name: "Key-1", - a: PathElement{Key: KeyByFields("goat", 1)}, - b: PathElement{Key: KeyByFields("goat", 1)}, + a: KeyElementByFields("goat", 1), + b: KeyElementByFields("goat", 1), eq: true, }, { name: "Key-2", - a: PathElement{Key: KeyByFields("horse", 1)}, - b: PathElement{Key: KeyByFields("horse", 2)}, + a: KeyElementByFields("horse", 1), + b: KeyElementByFields("horse", 2), }, { name: "Key-3", - a: PathElement{Key: KeyByFields("ibex", 1)}, - b: PathElement{Key: KeyByFields("jay", 1)}, + a: KeyElementByFields("ibex", 1), + b: KeyElementByFields("jay", 1), }, { name: "Key-4", - a: PathElement{Key: KeyByFields("kite", 1)}, - b: PathElement{Key: KeyByFields("kite", 1, "kite-2", 1)}, + a: KeyElementByFields("kite", 1), + b: KeyElementByFields("kite", 1, "kite-2", 1), }, { name: "Key-5", - a: PathElement{Key: KeyByFields("kite", 1)}, - b: PathElement{Value: valptr(1)}, + a: KeyElementByFields("kite", 1), + b: ValueElement(val(1)), }, { name: "Key-6", - a: PathElement{Key: KeyByFields("kite", 1)}, - b: PathElement{Index: intptr(5)}, + a: KeyElementByFields("kite", 1), + b: IndexElement(5), }, { name: "Value-1", - a: PathElement{Value: valptr(1)}, - b: PathElement{Value: valptr(2)}, + a: ValueElement(val(1)), + b: ValueElement(val(2)), }, { name: "Value-2", - a: PathElement{Value: valptr(1)}, - b: PathElement{Value: valptr(1)}, + a: ValueElement(val(1)), + b: ValueElement(val(1)), eq: true, }, { name: "Value-3", - a: PathElement{Value: valptr(1)}, - b: PathElement{Index: intptr(1)}, + a: ValueElement(val(1)), + b: IndexElement(1), }, { name: "Index-1", - a: PathElement{Index: intptr(1)}, - b: PathElement{Index: intptr(2)}, + a: IndexElement(1), + b: IndexElement(2), }, { name: "Index-2", - a: PathElement{Index: intptr(1)}, - b: PathElement{Index: intptr(1)}, + a: IndexElement(1), + b: IndexElement(1), eq: true, }, } diff --git a/fieldpath/set.go b/fieldpath/set.go index 0c285d17..d2d8c8a4 100644 --- a/fieldpath/set.go +++ b/fieldpath/set.go @@ -18,6 +18,7 @@ package fieldpath import ( "fmt" + "iter" "sort" "strings" @@ -47,6 +48,15 @@ func NewSet(paths ...Path) *Set { return s } +// Copy returns a copy of the Set. +// This is not a full deep copy as any contained value.Value is not copied. +func (s *Set) Copy() *Set { + return &Set{ + Members: s.Members.Copy(), + Children: s.Children.Copy(), + } +} + // Insert adds the field identified by `p` to the set. Important: parent fields // are NOT added to the set; if that is desired, they must be added separately. func (s *Set) Insert(p Path) { @@ -386,6 +396,15 @@ func (s *Set) Iterate(f func(Path)) { s.iteratePrefix(Path{}, f) } +// All iterates over each Path in the set (preorder DFS). +func (s *Set) All() iter.Seq[Path] { + return func(yield func(Path) bool) { + s.Iterate(func(p Path) { + yield(p) + }) + } +} + func (s *Set) iteratePrefix(prefix Path, f func(Path)) { s.Members.Iterate(func(pe PathElement) { f(append(prefix, pe)) }) s.Children.iteratePrefix(prefix, f) @@ -455,6 +474,16 @@ func (s sortedSetNode) Len() int { return len(s) } func (s sortedSetNode) Less(i, j int) bool { return s[i].pathElement.Less(s[j].pathElement) } func (s sortedSetNode) Swap(i, j int) { s[i], s[j] = s[j], s[i] } +// Copy returns a copy of the SetNodeMap. +// This is not a full deep copy as any contained value.Value is not copied. +func (s *SetNodeMap) Copy() SetNodeMap { + out := make(sortedSetNode, len(s.members)) + for i, v := range s.members { + out[i] = setNode{pathElement: v.pathElement.Copy(), set: v.set.Copy()} + } + return SetNodeMap{members: out} +} + // Descend adds pe to the set if necessary, returning the associated subset. func (s *SetNodeMap) Descend(pe PathElement) *Set { loc := sort.Search(len(s.members), func(i int) bool { @@ -705,6 +734,15 @@ func (s *SetNodeMap) Iterate(f func(PathElement)) { } } +// All iterates over each PathElement in the set. +func (s *SetNodeMap) All() iter.Seq[PathElement] { + return func(yield func(PathElement) bool) { + s.Iterate(func(pe PathElement) { + yield(pe) + }) + } +} + func (s *SetNodeMap) iteratePrefix(prefix Path, f func(Path)) { for _, n := range s.members { pe := n.pathElement diff --git a/fieldpath/set_test.go b/fieldpath/set_test.go index bb159c9a..3826d4ee 100644 --- a/fieldpath/set_test.go +++ b/fieldpath/set_test.go @@ -233,7 +233,6 @@ func TestSetIterSize(t *testing.T) { ) s2 := NewSet() - addedCount := 0 s1.Iterate(func(p Path) { if s2.Size() != addedCount { @@ -246,6 +245,19 @@ func TestSetIterSize(t *testing.T) { addedCount++ }) + s2 = NewSet() + addedCount = 0 + for p := range s1.All() { + if s2.Size() != addedCount { + t.Errorf("added %v items to set, but size is %v", addedCount, s2.Size()) + } + if addedCount > 0 == s2.Empty() { + t.Errorf("added %v items to set, but s2.Empty() is %v", addedCount, s2.Empty()) + } + s2.Insert(p) + addedCount++ + } + if !s1.Equals(s2) { // No point in using String() if iterate is broken... t.Errorf("Iterate missed something?\n%#v\n%#v", s1, s2) @@ -756,6 +768,19 @@ func TestSetNodeMapIterate(t *testing.T) { t.Errorf("expected to have iterated over %v, but never did", pe) } } + + iteratedElements = make(map[string]bool, toAdd) + for pe := range set.All() { + iteratedElements[pe.String()] = true + } + if len(iteratedElements) != toAdd { + t.Errorf("expected %v elements to be iterated over, got %v", toAdd, len(iteratedElements)) + } + for _, pe := range addedElements { + if _, ok := iteratedElements[pe]; !ok { + t.Errorf("expected to have iterated over %v, but never did", pe) + } + } } func TestFilterByPattern(t *testing.T) { diff --git a/go.mod b/go.mod index 12c3c539..c29a9fd3 100644 --- a/go.mod +++ b/go.mod @@ -12,4 +12,4 @@ require ( github.com/modern-go/reflect2 v1.0.2 // indirect ) -go 1.19 +go 1.23 diff --git a/value/fields.go b/value/fields.go index be3c6724..042b0487 100644 --- a/value/fields.go +++ b/value/fields.go @@ -31,6 +31,14 @@ type Field struct { // have a different name. type FieldList []Field +// Copy returns a copy of the FieldList. +// Values are not copied. +func (f FieldList) Copy() FieldList { + c := make(FieldList, len(f)) + copy(c, f) + return c +} + // Sort sorts the field list by Name. func (f FieldList) Sort() { if len(f) < 2 { From 111a1dda68bf47b642860bf23142b1c247fcb2f1 Mon Sep 17 00:00:00 2001 From: Davanum Srinivas Date: Thu, 26 Jun 2025 07:47:03 -0400 Subject: [PATCH 06/20] Drop usage of forked copies of goyaml.v2 Signed-off-by: Davanum Srinivas --- fieldpath/fromvalue_test.go | 2 +- fieldpath/set_test.go | 2 +- go.mod | 2 +- go.sum | 4 ++-- merge/multiple_appliers_test.go | 2 +- typed/parser.go | 2 +- typed/parser_test.go | 2 +- value/equals_test.go | 2 +- value/value.go | 2 +- 9 files changed, 10 insertions(+), 10 deletions(-) diff --git a/fieldpath/fromvalue_test.go b/fieldpath/fromvalue_test.go index 476aa683..cd909a50 100644 --- a/fieldpath/fromvalue_test.go +++ b/fieldpath/fromvalue_test.go @@ -19,8 +19,8 @@ package fieldpath import ( "testing" + yaml "go.yaml.in/yaml/v2" "sigs.k8s.io/structured-merge-diff/v6/value" - yaml "sigs.k8s.io/yaml/goyaml.v2" ) func TestFromValue(t *testing.T) { diff --git a/fieldpath/set_test.go b/fieldpath/set_test.go index 3826d4ee..9b3a984d 100644 --- a/fieldpath/set_test.go +++ b/fieldpath/set_test.go @@ -24,8 +24,8 @@ import ( "sigs.k8s.io/structured-merge-diff/v6/value" + yaml "go.yaml.in/yaml/v2" "sigs.k8s.io/structured-merge-diff/v6/schema" - yaml "sigs.k8s.io/yaml/goyaml.v2" ) type randomPathAlphabet []PathElement diff --git a/go.mod b/go.mod index c29a9fd3..f5343b69 100644 --- a/go.mod +++ b/go.mod @@ -3,8 +3,8 @@ module sigs.k8s.io/structured-merge-diff/v6 require ( github.com/google/go-cmp v0.5.9 github.com/json-iterator/go v1.1.12 + go.yaml.in/yaml/v2 v2.4.2 sigs.k8s.io/randfill v0.0.0-20250304075658-069ef1bbf016 - sigs.k8s.io/yaml v1.4.0 ) require ( diff --git a/go.sum b/go.sum index bfd3bbea..e120aad1 100644 --- a/go.sum +++ b/go.sum @@ -16,9 +16,9 @@ github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZN github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.3.0 h1:TivCn/peBQ7UY8ooIcPgZFpTNSz0Q2U6UrFlUfqbe0Q= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= +go.yaml.in/yaml/v2 v2.4.2 h1:DzmwEr2rDGHl7lsFgAHxmNz/1NlQ7xLIrlN2h5d1eGI= +go.yaml.in/yaml/v2 v2.4.2/go.mod h1:081UH+NErpNdqlCXm3TtEran0rJZGxAYx9hb/ELlsPU= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= sigs.k8s.io/randfill v0.0.0-20250304075658-069ef1bbf016 h1:kXv6kKdoEtedwuqMmkqhbkgvYKeycVbC8+iPCP9j5kQ= sigs.k8s.io/randfill v0.0.0-20250304075658-069ef1bbf016/go.mod h1:XeLlZ/jmk4i1HRopwe7/aU3H5n1zNUcX6TM94b3QxOY= -sigs.k8s.io/yaml v1.4.0 h1:Mk1wCc2gy/F0THH0TAp1QYyJNzRm2KCLy3o5ASXVI5E= -sigs.k8s.io/yaml v1.4.0/go.mod h1:Ejl7/uTz7PSA4eKMyQCUTnhZYNmLIl+5c2lQPGR2BPY= diff --git a/merge/multiple_appliers_test.go b/merge/multiple_appliers_test.go index 49a84ee4..7fad7b15 100644 --- a/merge/multiple_appliers_test.go +++ b/merge/multiple_appliers_test.go @@ -22,12 +22,12 @@ import ( "strings" "testing" + yaml "go.yaml.in/yaml/v2" "sigs.k8s.io/structured-merge-diff/v6/fieldpath" . "sigs.k8s.io/structured-merge-diff/v6/internal/fixture" "sigs.k8s.io/structured-merge-diff/v6/merge" "sigs.k8s.io/structured-merge-diff/v6/typed" "sigs.k8s.io/structured-merge-diff/v6/value" - yaml "sigs.k8s.io/yaml/goyaml.v2" ) func TestMultipleAppliersSet(t *testing.T) { diff --git a/typed/parser.go b/typed/parser.go index a7ebf3a2..c46e69f2 100644 --- a/typed/parser.go +++ b/typed/parser.go @@ -19,9 +19,9 @@ package typed import ( "fmt" + yaml "go.yaml.in/yaml/v2" "sigs.k8s.io/structured-merge-diff/v6/schema" "sigs.k8s.io/structured-merge-diff/v6/value" - yaml "sigs.k8s.io/yaml/goyaml.v2" ) // YAMLObject is an object encoded in YAML. diff --git a/typed/parser_test.go b/typed/parser_test.go index b47dc65c..9ef51cb6 100644 --- a/typed/parser_test.go +++ b/typed/parser_test.go @@ -22,8 +22,8 @@ import ( "strings" "testing" + yaml "go.yaml.in/yaml/v2" "sigs.k8s.io/structured-merge-diff/v6/typed" - yaml "sigs.k8s.io/yaml/goyaml.v2" ) func testdata(file string) string { diff --git a/value/equals_test.go b/value/equals_test.go index f41d4458..ecfc7d03 100644 --- a/value/equals_test.go +++ b/value/equals_test.go @@ -21,8 +21,8 @@ import ( "path/filepath" "testing" + yaml "go.yaml.in/yaml/v2" "sigs.k8s.io/structured-merge-diff/v6/value" - yaml "sigs.k8s.io/yaml/goyaml.v2" ) func testdata(file string) string { diff --git a/value/value.go b/value/value.go index 06de6e34..140b9903 100644 --- a/value/value.go +++ b/value/value.go @@ -24,7 +24,7 @@ import ( jsoniter "github.com/json-iterator/go" - yaml "sigs.k8s.io/yaml/goyaml.v2" + yaml "go.yaml.in/yaml/v2" ) var ( From 5f3bed258e448dd3a8e19ad065504f573ff4a0a7 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 16 Jul 2025 08:36:57 +0200 Subject: [PATCH 07/20] fieldpath: convert to blackbox testing The tests were defined in the "fieldpath" package, which prevented using typed.NewParser (import cycle!). Defining them in "fieldpath_test" allows that. To avoid changing all references to the package's exported symbols, "fieldpath_test.go" defines aliases. typed.NewParser is more strict and finds a typo in the test schema. Also, constructing a set with NewSet(MakePath(...)) was found to not yield the same structs as parsing a value with a schema, so now parser.Type(typeName).FromYAML().ToFieldSet is used instead. --- fieldpath/fieldpath_test.go | 46 +++++++++++++++ fieldpath/managers_test.go | 6 -- fieldpath/serialize_test.go | 2 +- fieldpath/set_test.go | 109 ++++++++++++++++++++++++++---------- 4 files changed, 126 insertions(+), 37 deletions(-) create mode 100644 fieldpath/fieldpath_test.go diff --git a/fieldpath/fieldpath_test.go b/fieldpath/fieldpath_test.go new file mode 100644 index 00000000..34812d6a --- /dev/null +++ b/fieldpath/fieldpath_test.go @@ -0,0 +1,46 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package fieldpath_test + +import ( + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/value" +) + +// Type and function aliases to avoid typing... + +type ( + Filter = fieldpath.Filter + Path = fieldpath.Path + PathElement = fieldpath.PathElement + Set = fieldpath.Set + SetNodeMap = fieldpath.SetNodeMap +) + +var ( + KeyByFields = fieldpath.KeyByFields + MakePathOrDie = fieldpath.MakePathOrDie + MakePrefixMatcherOrDie = fieldpath.MakePrefixMatcherOrDie + MatchAnyPathElement = fieldpath.MatchAnyPathElement + NewIncludeMatcherFilter = fieldpath.NewIncludeMatcherFilter + NewSet = fieldpath.NewSet + + // Short names for readable test cases. + _V = value.NewValueInterface + _NS = fieldpath.NewSet + _P = fieldpath.MakePathOrDie +) diff --git a/fieldpath/managers_test.go b/fieldpath/managers_test.go index bac9efeb..6984176c 100644 --- a/fieldpath/managers_test.go +++ b/fieldpath/managers_test.go @@ -23,12 +23,6 @@ import ( "sigs.k8s.io/structured-merge-diff/v6/fieldpath" ) -var ( - // Short names for readable test cases. - _NS = fieldpath.NewSet - _P = fieldpath.MakePathOrDie -) - func TestManagersEquals(t *testing.T) { tests := []struct { name string diff --git a/fieldpath/serialize_test.go b/fieldpath/serialize_test.go index a3bcc9e6..35cb6084 100644 --- a/fieldpath/serialize_test.go +++ b/fieldpath/serialize_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package fieldpath +package fieldpath_test import ( "bytes" diff --git a/fieldpath/set_test.go b/fieldpath/set_test.go index 9b3a984d..0254e2da 100644 --- a/fieldpath/set_test.go +++ b/fieldpath/set_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package fieldpath +package fieldpath_test import ( "bytes" @@ -22,10 +22,9 @@ import ( "math/rand" "testing" - "sigs.k8s.io/structured-merge-diff/v6/value" - - yaml "go.yaml.in/yaml/v2" "sigs.k8s.io/structured-merge-diff/v6/schema" + "sigs.k8s.io/structured-merge-diff/v6/typed" + "sigs.k8s.io/structured-merge-diff/v6/value" ) type randomPathAlphabet []PathElement @@ -664,65 +663,95 @@ func TestSetDifference(t *testing.T) { } } -var nestedSchema = func() (*schema.Schema, schema.TypeRef) { - sc := &schema.Schema{} +var nestedSchema = func() (*typed.Parser, string) { name := "type" - err := yaml.Unmarshal([]byte(`types: + parser := mustParse(`types: - name: type map: elementType: namedType: type fields: + - name: keyAStr + type: + scalar: string + - name: keyBInt + type: + scalar: numeric - name: named type: namedType: type - name: list type: list: - elementRelationShip: associative - keys: ["name"] + elementRelationship: associative + keys: ["keyAStr", "keyBInt"] elementType: namedType: type + - name: a + type: + namedType: type - name: value type: scalar: numeric -`), &sc) +`) + return parser, name +} + +func mustParse(schema typed.YAMLObject) *typed.Parser { + parser, err := typed.NewParser(schema) if err != nil { panic(err) } - return sc, schema.TypeRef{NamedType: &name} + return parser } -var _P = MakePathOrDie - func TestEnsureNamedFieldsAreMembers(t *testing.T) { table := []struct { - set, expected *Set + value typed.YAMLObject + expectedBefore *Set + expectedAfter *Set }{ { - set: NewSet(_P("named", "named", "value")), - expected: NewSet( + value: `{"named": {"named": {"value": 0}}}`, + expectedBefore: NewSet( + _P("named", "named", "value"), + ), + expectedAfter: NewSet( _P("named", "named", "value"), _P("named", "named"), _P("named"), ), }, { - set: NewSet(_P("named", "a", "named", "value"), _P("a", "named", "value"), _P("a", "b", "value")), - expected: NewSet( + value: `{"named": {"a": {"named": {"value": 42}}}, "a": {"named": {"value": 1}}}`, + expectedBefore: NewSet( + _P("named", "a", "named", "value"), + _P("a", "named", "value"), + ), + expectedAfter: NewSet( _P("named", "a", "named", "value"), _P("named", "a", "named"), + _P("named", "a"), _P("named"), _P("a", "named", "value"), _P("a", "named"), - _P("a", "b", "value"), + _P("a"), ), }, { - set: NewSet(_P("named", "list", KeyByFields("name", "a"), "named", "a", "value")), - expected: NewSet( - _P("named", "list", KeyByFields("name", "a"), "named", "a", "value"), - _P("named", "list", KeyByFields("name", "a"), "named"), + value: `{"named": {"list": [{"keyAStr": "a", "keyBInt": 1, "named": {"value": 0}}]}}`, + expectedBefore: NewSet( + _P("named", "list", KeyByFields("keyAStr", "a", "keyBInt", 1), "keyAStr"), + _P("named", "list", KeyByFields("keyAStr", "a", "keyBInt", 1), "keyBInt"), + _P("named", "list", KeyByFields("keyAStr", "a", "keyBInt", 1), "named", "value"), + _P("named", "list", KeyByFields("keyAStr", "a", "keyBInt", 1)), + ), + expectedAfter: NewSet( + _P("named", "list", KeyByFields("keyAStr", "a", "keyBInt", 1), "keyAStr"), + _P("named", "list", KeyByFields("keyAStr", "a", "keyBInt", 1), "keyBInt"), + _P("named", "list", KeyByFields("keyAStr", "a", "keyBInt", 1), "named", "value"), + _P("named", "list", KeyByFields("keyAStr", "a", "keyBInt", 1), "named"), + _P("named", "list", KeyByFields("keyAStr", "a", "keyBInt", 1)), _P("named", "list"), _P("named"), ), @@ -730,14 +759,34 @@ func TestEnsureNamedFieldsAreMembers(t *testing.T) { } for _, test := range table { - t.Run(fmt.Sprintf("%v", test.set), func(t *testing.T) { - got := test.set.EnsureNamedFieldsAreMembers(nestedSchema()) - if !got.Equals(test.expected) { - t.Errorf("expected %v, got %v (missing: %v/superfluous: %v)", - test.expected, + t.Run(string(test.value), func(t *testing.T) { + parser, typeName := nestedSchema() + typeRef := schema.TypeRef{NamedType: &typeName} + typedValue, err := parser.Type(typeName).FromYAML(test.value) + if err != nil { + t.Fatalf("unexpected error parsing test value: %v", err) + } + set, err := typedValue.ToFieldSet() + if err != nil { + t.Fatalf("unexpected error converting test value to set: %v", err) + } + if !set.Equals(test.expectedBefore) { + t.Errorf("expected before EnsureNamedFieldsAreMembers:\n%v\n\ngot:\n%v\n\nmissing:\n%v\n\nsuperfluous:\n\n%v", + test.expectedBefore, + set, + test.expectedAfter.Difference(set), + set.Difference(test.expectedAfter), + ) + } + + schema := &parser.Schema + got := set.EnsureNamedFieldsAreMembers(schema, typeRef) + if !got.Equals(test.expectedAfter) { + t.Errorf("expected after EnsureNamedFieldsAreMembers:\n%v\n\ngot:\n%v\n\nmissing:\n%v\n\nsuperfluous:\n\n%v", + test.expectedAfter, got, - test.expected.Difference(got), - got.Difference(test.expected), + test.expectedAfter.Difference(got), + got.Difference(test.expectedAfter), ) } }) From cd2b88317f5a4b9bef02695919837a349659f355 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 9 Jul 2025 09:08:48 +0200 Subject: [PATCH 08/20] support optional map keys Optional keys of a list map (= associative lists) keys are simply left out of the set of keys, which is different from a key with an empty value like "" for a string and obviously also different from a non-empty value. The comparison of values already supported that and the comparison of list values supported lists with different number of entries. Completely empty key field lists continue to trigger an error ("associative list with keys has an element that omits all key fields (and doesn't have default values for any key fields)". Downgrading from a version which has support for a new optional key to a version which doesn't works as long as the optional key is not used, because the ManagedFields don't mention the new key and field and there are no list entries which have it set. It does not work when the new field and key are used because the older version doesn't know that it needs to consider the new key, as the key is not listed in the older version's OpenAPI spec. This is considered acceptable because new fields will be alpha initially and downgrades with an alpha feature enabled are not required to work. It is worth calling out in release notes, though. --- fieldpath/set_test.go | 105 +++++++++++++++++++++++++++++++++++-- merge/default_keys_test.go | 42 ++++++++++----- typed/helpers.go | 9 +++- typed/validate_test.go | 1 + 4 files changed, 140 insertions(+), 17 deletions(-) diff --git a/fieldpath/set_test.go b/fieldpath/set_test.go index 0254e2da..0685bc47 100644 --- a/fieldpath/set_test.go +++ b/fieldpath/set_test.go @@ -697,6 +697,58 @@ var nestedSchema = func() (*typed.Parser, string) { return parser, name } +var associativeListSchema = func() (*typed.Parser, string) { + name := "type" + parser := mustParse(`types: +- name: type + map: + fields: + - name: values + type: + list: + elementRelationship: associative + keys: ["keyAStr", "keyBInt"] + elementType: + map: + fields: + - name: keyAStr + type: + scalar: string + - name: keyBInt + type: + scalar: numeric + - name: value + type: + scalar: numeric +`) + return parser, name +} + +var oldAssociativeListSchema = func() (*typed.Parser, string) { + name := "type" + // No keyBInt yet! + parser := mustParse(`types: +- name: type + map: + fields: + - name: values + type: + list: + elementRelationship: associative + keys: ["keyAStr"] + elementType: + map: + fields: + - name: keyAStr + type: + scalar: string + - name: value + type: + scalar: numeric +`) + return parser, name +} + func mustParse(schema typed.YAMLObject) *typed.Parser { parser, err := typed.NewParser(schema) if err != nil { @@ -707,12 +759,15 @@ func mustParse(schema typed.YAMLObject) *typed.Parser { func TestEnsureNamedFieldsAreMembers(t *testing.T) { table := []struct { + schemaFn func() (*typed.Parser, string) + newSchemaFn func() (*typed.Parser, string) value typed.YAMLObject expectedBefore *Set expectedAfter *Set }{ { - value: `{"named": {"named": {"value": 0}}}`, + schemaFn: nestedSchema, + value: `{"named": {"named": {"value": 0}}}`, expectedBefore: NewSet( _P("named", "named", "value"), ), @@ -723,7 +778,8 @@ func TestEnsureNamedFieldsAreMembers(t *testing.T) { ), }, { - value: `{"named": {"a": {"named": {"value": 42}}}, "a": {"named": {"value": 1}}}`, + schemaFn: nestedSchema, + value: `{"named": {"a": {"named": {"value": 42}}}, "a": {"named": {"value": 1}}}`, expectedBefore: NewSet( _P("named", "a", "named", "value"), _P("a", "named", "value"), @@ -739,7 +795,8 @@ func TestEnsureNamedFieldsAreMembers(t *testing.T) { ), }, { - value: `{"named": {"list": [{"keyAStr": "a", "keyBInt": 1, "named": {"value": 0}}]}}`, + schemaFn: nestedSchema, + value: `{"named": {"list": [{"keyAStr": "a", "keyBInt": 1, "named": {"value": 0}}]}}`, expectedBefore: NewSet( _P("named", "list", KeyByFields("keyAStr", "a", "keyBInt", 1), "keyAStr"), _P("named", "list", KeyByFields("keyAStr", "a", "keyBInt", 1), "keyBInt"), @@ -756,11 +813,46 @@ func TestEnsureNamedFieldsAreMembers(t *testing.T) { _P("named"), ), }, + { + // Generate the value using the old schema to get missing key entries, + // then process with new schema which has keyBInt. + schemaFn: oldAssociativeListSchema, + newSchemaFn: associativeListSchema, + value: `{"values": [{"keyAStr": "a", "value": 0}]}`, + expectedBefore: NewSet( + _P("values", KeyByFields("keyAStr", "a"), "keyAStr"), + _P("values", KeyByFields("keyAStr", "a"), "value"), + _P("values", KeyByFields("keyAStr", "a")), + ), + expectedAfter: NewSet( + _P("values", KeyByFields("keyAStr", "a"), "keyAStr"), + _P("values", KeyByFields("keyAStr", "a"), "value"), + _P("values", KeyByFields("keyAStr", "a")), + _P("values"), + ), + }, + { + // Check that converting the value with the missing key and + // the recent schema doesn't add the missing key. + schemaFn: associativeListSchema, + value: `{"values": [{"keyAStr": "a", "value": 1}]}`, + expectedBefore: NewSet( + _P("values", KeyByFields("keyAStr", "a"), "keyAStr"), + _P("values", KeyByFields("keyAStr", "a"), "value"), + _P("values", KeyByFields("keyAStr", "a")), + ), + expectedAfter: NewSet( + _P("values", KeyByFields("keyAStr", "a"), "keyAStr"), + _P("values", KeyByFields("keyAStr", "a"), "value"), + _P("values", KeyByFields("keyAStr", "a")), + _P("values"), + ), + }, } for _, test := range table { t.Run(string(test.value), func(t *testing.T) { - parser, typeName := nestedSchema() + parser, typeName := test.schemaFn() typeRef := schema.TypeRef{NamedType: &typeName} typedValue, err := parser.Type(typeName).FromYAML(test.value) if err != nil { @@ -780,6 +872,11 @@ func TestEnsureNamedFieldsAreMembers(t *testing.T) { } schema := &parser.Schema + if test.newSchemaFn != nil { + newParser, _ := test.newSchemaFn() + schema = &newParser.Schema + } + got := set.EnsureNamedFieldsAreMembers(schema, typeRef) if !got.Equals(test.expectedAfter) { t.Errorf("expected after EnsureNamedFieldsAreMembers:\n%v\n\ngot:\n%v\n\nmissing:\n%v\n\nsuperfluous:\n\n%v", diff --git a/merge/default_keys_test.go b/merge/default_keys_test.go index 24b202d4..28f81038 100644 --- a/merge/default_keys_test.go +++ b/merge/default_keys_test.go @@ -163,6 +163,36 @@ func TestDefaultKeysFlat(t *testing.T) { ), }, }, + "apply_missing_undefaulted_defaulted_key": { + Ops: []Operation{ + Apply{ + Manager: "default", + APIVersion: "v1", + Object: ` + containerPorts: + - protocol: TCP + name: A + `, + }, + }, + APIVersion: "v1", + Object: ` + containerPorts: + - protocol: TCP + name: A + `, + Managed: fieldpath.ManagedFields{ + "default": fieldpath.NewVersionedSet( + _NS( + _P("containerPorts", _KBF("protocol", "TCP")), + _P("containerPorts", _KBF("protocol", "TCP"), "name"), + _P("containerPorts", _KBF("protocol", "TCP"), "protocol"), + ), + "v1", + true, + ), + }, + }, } for name, test := range tests { @@ -176,18 +206,6 @@ func TestDefaultKeysFlat(t *testing.T) { func TestDefaultKeysFlatErrors(t *testing.T) { tests := map[string]TestCase{ - "apply_missing_undefaulted_defaulted_key": { - Ops: []Operation{ - Apply{ - Manager: "default", - APIVersion: "v1", - Object: ` - containerPorts: - - protocol: TCP - `, - }, - }, - }, "apply_missing_defaulted_key_ambiguous_A": { Ops: []Operation{ Apply{ diff --git a/typed/helpers.go b/typed/helpers.go index f10ac6e9..8a9c0b50 100644 --- a/typed/helpers.go +++ b/typed/helpers.go @@ -217,9 +217,16 @@ func keyedAssociativeListItemToPathElement(a value.Allocator, s *schema.Schema, } else if def != nil { keyMap = append(keyMap, value.Field{Name: fieldName, Value: value.NewValueInterface(def)}) } else { - return pe, fmt.Errorf("associative list with keys has an element that omits key field %q (and doesn't have default value)", fieldName) + // Don't add the key to the key field list. + // A key field list where it is set then represents a different entry + // in the associate list. } } + + if len(list.Keys) > 0 && len(keyMap) == 0 { + return pe, fmt.Errorf("associative list with keys has an element that omits all key fields %q (and doesn't have default values for any key fields)", list.Keys) + } + keyMap.Sort() pe.Key = &keyMap return pe, nil diff --git a/typed/validate_test.go b/typed/validate_test.go index b4c177a4..92ba1416 100644 --- a/typed/validate_test.go +++ b/typed/validate_test.go @@ -213,6 +213,7 @@ var validationCases = []validationTestCase{{ `{"list":[{"key":"a","id":1,"value":{"a":"a"}}]}`, `{"list":[{"key":"a","id":1},{"key":"a","id":2},{"key":"b","id":1}]}`, `{"atomicList":["a","a","a"]}`, + `{"list":[{"key":"x","value":{"a":"a"},"bv":true,"nv":3.14}]}`, }, invalidObjects: []typed.YAMLObject{ `{"key":true,"value":1}`, From 2657e81a2627bbda8209f490e6b7404bb11817ad Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Wed, 16 Jul 2025 11:53:32 -0400 Subject: [PATCH 09/20] Add associative list key schema evolution changes --- merge/schema_change_test.go | 241 ++++++++++++++++++++++++++++++++++++ 1 file changed, 241 insertions(+) diff --git a/merge/schema_change_test.go b/merge/schema_change_test.go index d733407c..2b0b34ef 100644 --- a/merge/schema_change_test.go +++ b/merge/schema_change_test.go @@ -252,3 +252,244 @@ func TestAtomicToGranularSchemaChanges(t *testing.T) { }) } } + +var associativeListParserOld = func() *typed.Parser { + oldParser, err := typed.NewParser(`types: +- name: v1 + map: + fields: + - name: list + type: + namedType: associativeList +- name: associativeList + list: + elementType: + namedType: myElement + elementRelationship: associative + keys: + - name +- name: myElement + map: + fields: + - name: name + type: + scalar: string + - name: id + type: + scalar: numeric + - name: value + type: + scalar: numeric +`) + if err != nil { + panic(err) + } + return oldParser +}() + +var associativeListParserNewOptionalKey = func() *typed.Parser { + newParser, err := typed.NewParser(`types: +- name: v1 + map: + fields: + - name: list + type: + namedType: associativeList +- name: associativeList + list: + elementType: + namedType: myElement + elementRelationship: associative + keys: + - name + - id +- name: myElement + map: + fields: + - name: name + type: + scalar: string + - name: id + type: + scalar: numeric + - name: value + type: + scalar: numeric +`) + if err != nil { + panic(err) + } + return newParser +}() + +var associativeListParserNewKeyWithDefault = func() *typed.Parser { + newParser, err := typed.NewParser(`types: +- name: v1 + map: + fields: + - name: list + type: + namedType: associativeList +- name: associativeList + list: + elementType: + namedType: myElement + elementRelationship: associative + keys: + - name + - id +- name: myElement + map: + fields: + - name: name + type: + scalar: string + - name: id + type: + scalar: numeric + default: 1 + - name: value + type: + scalar: numeric +`) + if err != nil { + panic(err) + } + return newParser +}() + +func TestAssociativeListSchemaChanges(t *testing.T) { + tests := map[string]TestCase{ + "new required key with default": { + Ops: []Operation{ + Apply{ + Manager: "one", + Object: ` + list: + - name: a + value: 1 + - name: b + value: 1 + - name: c + value: 1 + `, + APIVersion: "v1", + }, + ChangeParser{Parser: associativeListParserNewKeyWithDefault}, + Apply{ + Manager: "one", + Object: ` + list: + - name: a + value: 2 + - name: b + id: 1 + value: 2 + - name: c + value: 1 + - name: c + id: 2 + value: 2 + - name: c + id: 3 + value: 3 + `, + APIVersion: "v1", + }, + }, + Object: ` + list: + - name: a + value: 2 + - name: b + id: 1 + value: 2 + - name: c + value: 1 + - name: c + id: 2 + value: 2 + - name: c + id: 3 + value: 3 + `, + APIVersion: "v1", + Managed: fieldpath.ManagedFields{ + "one": fieldpath.NewVersionedSet(_NS( + _P("list", _KBF("name", "a", "id", float64(1))), + _P("list", _KBF("name", "a", "id", float64(1)), "name"), + _P("list", _KBF("name", "a", "id", float64(1)), "value"), + _P("list", _KBF("name", "b", "id", float64(1))), + _P("list", _KBF("name", "b", "id", float64(1)), "name"), + _P("list", _KBF("name", "b", "id", float64(1)), "id"), + _P("list", _KBF("name", "b", "id", float64(1)), "value"), + _P("list", _KBF("name", "c", "id", float64(1))), + _P("list", _KBF("name", "c", "id", float64(1)), "name"), + _P("list", _KBF("name", "c", "id", float64(1)), "value"), + _P("list", _KBF("name", "c", "id", float64(2))), + _P("list", _KBF("name", "c", "id", float64(2)), "name"), + _P("list", _KBF("name", "c", "id", float64(2)), "id"), + _P("list", _KBF("name", "c", "id", float64(2)), "value"), + _P("list", _KBF("name", "c", "id", float64(3))), + _P("list", _KBF("name", "c", "id", float64(3)), "name"), + _P("list", _KBF("name", "c", "id", float64(3)), "id"), + _P("list", _KBF("name", "c", "id", float64(3)), "value"), + ), "v1", true), + }, + }, + "new optional key": { + Ops: []Operation{ + Apply{ + Manager: "one", + Object: ` + list: + - name: a + value: 1 + `, + APIVersion: "v1", + }, + ChangeParser{Parser: associativeListParserNewOptionalKey}, + Apply{ + Manager: "one", + Object: ` + list: + - name: a + value: 2 + - name: a + id: 1 + value: 1 + `, + APIVersion: "v1", + }, + }, + Object: ` + list: + - name: a + value: 2 + - name: a + id: 1 + value: 1 + `, + APIVersion: "v1", + Managed: fieldpath.ManagedFields{ + "one": fieldpath.NewVersionedSet(_NS( + _P("list", _KBF("name", "a")), + _P("list", _KBF("name", "a"), "name"), + _P("list", _KBF("name", "a"), "value"), + _P("list", _KBF("name", "a", "id", float64(1))), + _P("list", _KBF("name", "a", "id", float64(1)), "name"), + _P("list", _KBF("name", "a", "id", float64(1)), "id"), + _P("list", _KBF("name", "a", "id", float64(1)), "value"), + ), "v1", true), + }, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + if err := test.Test(associativeListParserOld); err != nil { + t.Fatal(err) + } + }) + } +} From 77efcc0d937669c53bf2a26d3f46cf0e1e50a6c1 Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Wed, 16 Jul 2025 16:23:34 -0400 Subject: [PATCH 10/20] Add tests based on feedback --- merge/schema_change_test.go | 221 +++++++++++++++++++++++++++++++++++- 1 file changed, 218 insertions(+), 3 deletions(-) diff --git a/merge/schema_change_test.go b/merge/schema_change_test.go index 2b0b34ef..db78ff56 100644 --- a/merge/schema_change_test.go +++ b/merge/schema_change_test.go @@ -274,9 +274,6 @@ var associativeListParserOld = func() *typed.Parser { - name: name type: scalar: string - - name: id - type: - scalar: numeric - name: value type: scalar: numeric @@ -493,3 +490,221 @@ func TestAssociativeListSchemaChanges(t *testing.T) { }) } } + +var associativeListParserPromoteKeyBefore = func() *typed.Parser { + p, err := typed.NewParser(`types: +- name: v1 + map: + fields: + - name: list + type: + namedType: associativeList +- name: associativeList + list: + elementType: + namedType: myElement + elementRelationship: associative + keys: + - name +- name: myElement + map: + fields: + - name: name + type: + scalar: string + - name: id + type: + scalar: numeric + - name: value + type: + scalar: numeric +`) + if err != nil { + panic(err) + } + return p +}() + +var associativeListParserPromoteKeyAfter = func() *typed.Parser { + p, err := typed.NewParser(`types: +- name: v1 + map: + fields: + - name: list + type: + namedType: associativeList +- name: associativeList + list: + elementType: + namedType: myElement + elementRelationship: associative + keys: + - name + - id +- name: myElement + map: + fields: + - name: name + type: + scalar: string + - name: id + type: + scalar: numeric + - name: value + type: + scalar: numeric +`) + if err != nil { + panic(err) + } + return p +}() + +func TestPromoteFieldToAssociativeListKey(t *testing.T) { + tests := map[string]TestCase{ + "identical item merges": { + Ops: []Operation{ + Apply{ + Manager: "one", + Object: ` + list: + - name: a + id: 1 + value: 1 + `, + APIVersion: "v1", + }, + ChangeParser{Parser: associativeListParserPromoteKeyAfter}, + Apply{ + Manager: "one", + Object: ` + list: + - name: a + id: 1 + value: 2 + `, + APIVersion: "v1", + }, + }, + Object: ` + list: + - name: a + id: 1 + value: 2 + `, + APIVersion: "v1", + Managed: fieldpath.ManagedFields{ + "one": fieldpath.NewVersionedSet(_NS( + _P("list", _KBF("name", "a", "id", float64(1))), + _P("list", _KBF("name", "a", "id", float64(1)), "name"), + _P("list", _KBF("name", "a", "id", float64(1)), "id"), + _P("list", _KBF("name", "a", "id", float64(1)), "value"), + ), "v1", true), + }, + }, + "distinct item added": { + Ops: []Operation{ + Apply{ + Manager: "one", + Object: ` + list: + - name: a + id: 1 + value: 1 + `, + APIVersion: "v1", + }, + ChangeParser{Parser: associativeListParserPromoteKeyAfter}, + Apply{ + Manager: "one", + Object: ` + list: + - name: a + id: 1 + value: 1 + - name: a + id: 2 + value: 2 + `, + APIVersion: "v1", + }, + }, + Object: ` + list: + - name: a + id: 1 + value: 1 + - name: a + id: 2 + value: 2 + `, + APIVersion: "v1", + Managed: fieldpath.ManagedFields{ + "one": fieldpath.NewVersionedSet(_NS( + _P("list", _KBF("name", "a", "id", float64(1))), + _P("list", _KBF("name", "a", "id", float64(1)), "name"), + _P("list", _KBF("name", "a", "id", float64(1)), "id"), + _P("list", _KBF("name", "a", "id", float64(1)), "value"), + _P("list", _KBF("name", "a", "id", float64(2))), + _P("list", _KBF("name", "a", "id", float64(2)), "name"), + _P("list", _KBF("name", "a", "id", float64(2)), "id"), + _P("list", _KBF("name", "a", "id", float64(2)), "value"), + ), "v1", true), + }, + }, + "item missing new key field is distinct": { + Ops: []Operation{ + Apply{ + Manager: "one", + Object: ` + list: + - name: a + value: 1 + `, + APIVersion: "v1", + }, + ChangeParser{Parser: associativeListParserPromoteKeyAfter}, + Apply{ + Manager: "one", + Object: ` + list: + - name: a + value: 1 + - name: a + id: 2 + value: 2 + `, + APIVersion: "v1", + }, + }, + Object: ` + list: + - name: a + value: 1 + - name: a + id: 2 + value: 2 + `, + APIVersion: "v1", + Managed: fieldpath.ManagedFields{ + "one": fieldpath.NewVersionedSet(_NS( + _P("list", _KBF("name", "a")), + _P("list", _KBF("name", "a"), "name"), + _P("list", _KBF("name", "a"), "value"), + _P("list", _KBF("name", "a", "id", float64(2))), + _P("list", _KBF("name", "a", "id", float64(2)), "name"), + _P("list", _KBF("name", "a", "id", float64(2)), "id"), + _P("list", _KBF("name", "a", "id", float64(2)), "value"), + ), "v1", true), + }, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + if err := test.Test(associativeListParserPromoteKeyBefore); err != nil { + t.Fatal(err) + } + }) + } +} From 544b1e331edfdd55198de3f0c9baec55e614c16c Mon Sep 17 00:00:00 2001 From: Lalit Chauhan Date: Tue, 9 Sep 2025 17:36:34 +0000 Subject: [PATCH 11/20] Add tests which are testing unicode and escape characters for SMD fields --- fieldpath/path_test.go | 59 +++++++++++++++++++++++++++++++++- fieldpath/serialize-pe_test.go | 8 +++++ fieldpath/serialize_test.go | 1 + fieldpath/set_test.go | 23 ++++++++++++- 4 files changed, 89 insertions(+), 2 deletions(-) diff --git a/fieldpath/path_test.go b/fieldpath/path_test.go index 22410e5c..c5eea4f0 100644 --- a/fieldpath/path_test.go +++ b/fieldpath/path_test.go @@ -50,9 +50,66 @@ func TestPathString(t *testing.T) { _V(false), _V(3.14159), ), `.foo[="b"][=5][=false][=3.14159]`}, + { + name: "simple field", + fp: MakePathOrDie("spec"), + expect: ".spec", + }, + { + name: "app container image", + fp: MakePathOrDie( + "spec", "apps", + KeyByFields("name", "app-🚀"), + "container", "image", + ), + expect: `.spec.apps[name="app-🚀"].container.image`, + }, + { + name: "app port", + fp: MakePathOrDie( + "spec", "apps", + KeyByFields("name", "app-💻"), + "container", "ports", + KeyByFields("name", "port-🔑"), + "containerPort", + ), + expect: ".spec.apps[name=\"app-💻\"].container.ports[name=\"port-🔑\"].containerPort", + }, + { + name: "field with space", + fp: MakePathOrDie("spec", "field with space"), + expect: ".spec.field with space", + }, + { + name: "value with space", + fp: MakePathOrDie( + "spec", "apps", + _V("app with space"), + "container", "image", + ), + expect: `.spec.apps[="app with space"].container.image`, + }, + { + name: "value with quotes", + fp: MakePathOrDie( + "spec", "apps", + _V("app with \"quotes\""), + "container", "image", + ), + expect: ".spec.apps[=\"app with \\\"quotes\\\"\"].container.image", + }, + + { + name: "value with unicode", + fp: MakePathOrDie( + "spec", "apps", + _V("app-with-unicøde"), + "container", "image", + ), + expect: ".spec.apps[=\"app-with-unicøde\"].container.image", + }, } for _, tt := range table { - tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() got := tt.fp.String() diff --git a/fieldpath/serialize-pe_test.go b/fieldpath/serialize-pe_test.go index 0a6ee8f1..45ef3739 100644 --- a/fieldpath/serialize-pe_test.go +++ b/fieldpath/serialize-pe_test.go @@ -25,6 +25,7 @@ func TestPathElementRoundTrip(t *testing.T) { `f:`, `f:spec`, `f:more-complicated-string`, + `f:abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789`, `k:{"name":"my-container"}`, `k:{"port":"8080","protocol":"TCP"}`, `k:{"optionalField":null}`, @@ -34,6 +35,13 @@ func TestPathElementRoundTrip(t *testing.T) { `v:"some-string"`, `v:1234`, `v:{"some":"json"}`, + `k:{"name":"app-🚀"}`, + `k:{"name":"app-💻"}`, + `k:{"name":"app with-unicøde"}`, + `k:{"name":"你好世界"}`, + `k:{"name":"Привет, мир"}`, + `k:{"name":"नमस्ते दुनिया"}`, + `k:{"name":"👋"}`, } for _, test := range tests { diff --git a/fieldpath/serialize_test.go b/fieldpath/serialize_test.go index 35cb6084..727e0a6a 100644 --- a/fieldpath/serialize_test.go +++ b/fieldpath/serialize_test.go @@ -51,6 +51,7 @@ func TestSerializeV1GoldenData(t *testing.T) { examples := []string{ `{"f:aaa":{},"f:aab":{},"f:aac":{},"f:aad":{},"f:aae":{},"f:aaf":{},"k:{\"name\":\"first\"}":{},"k:{\"name\":\"second\"}":{},"k:{\"port\":443,\"protocol\":\"tcp\"}":{},"k:{\"port\":443,\"protocol\":\"udp\"}":{},"v:1":{},"v:2":{},"v:3":{},"v:\"aa\"":{},"v:\"ab\"":{},"v:true":{},"i:1":{},"i:2":{},"i:3":{},"i:4":{}}`, `{"f:aaa":{"k:{\"name\":\"second\"}":{"v:3":{"f:aab":{}}},"v:3":{},"v:true":{}},"f:aab":{"f:aaa":{},"f:aaf":{"k:{\"port\":443,\"protocol\":\"udp\"}":{"k:{\"port\":443,\"protocol\":\"tcp\"}":{}}},"k:{\"name\":\"first\"}":{}},"f:aac":{"f:aaa":{"v:1":{}},"f:aac":{},"v:3":{"k:{\"name\":\"second\"}":{}}},"f:aad":{"f:aac":{"v:1":{}},"f:aaf":{"k:{\"name\":\"first\"}":{"k:{\"name\":\"first\"}":{}}},"i:1":{"i:1":{},"i:3":{"v:true":{}}}},"f:aae":{"f:aae":{},"k:{\"port\":443,\"protocol\":\"tcp\"}":{"k:{\"port\":443,\"protocol\":\"udp\"}":{}},"i:4":{"f:aaf":{}}},"f:aaf":{"i:1":{"f:aac":{}},"i:2":{},"i:3":{}},"k:{\"name\":\"first\"}":{"f:aad":{"f:aaf":{}}},"k:{\"port\":443,\"protocol\":\"tcp\"}":{"f:aaa":{"f:aad":{}}},"k:{\"port\":443,\"protocol\":\"udp\"}":{"f:aac":{},"k:{\"name\":\"first\"}":{"i:3":{}},"k:{\"port\":443,\"protocol\":\"udp\"}":{"i:4":{}}},"v:1":{"f:aac":{"i:4":{}},"f:aaf":{},"k:{\"port\":443,\"protocol\":\"tcp\"}":{}},"v:2":{"f:aad":{"f:aaf":{}},"i:1":{}},"v:3":{"f:aaa":{},"k:{\"name\":\"first\"}":{},"i:2":{}},"v:\"aa\"":{"f:aab":{"f:aaf":{}},"f:aae":{},"k:{\"name\":\"first\"}":{"f:aad":{}},"i:2":{}},"v:\"ab\"":{"f:aaf":{"i:4":{}},"k:{\"port\":443,\"protocol\":\"tcp\"}":{},"k:{\"port\":443,\"protocol\":\"udp\"}":{},"v:1":{"k:{\"port\":443,\"protocol\":\"udp\"}":{}},"i:1":{"f:aae":{"i:4":{}}}},"v:true":{"k:{\"name\":\"second\"}":{"f:aaa":{}},"i:2":{"k:{\"port\":443,\"protocol\":\"tcp\"}":{}}},"i:1":{"i:3":{"f:aaf":{}}},"i:2":{"f:aae":{},"k:{\"port\":443,\"protocol\":\"tcp\"}":{"v:1":{}}},"i:3":{"f:aab":{"v:true":{"v:\"aa\"":{}}},"f:aaf":{},"i:1":{}},"i:4":{"v:\"aa\"":{"f:aab":{"k:{\"name\":\"second\"}":{}}}}}`, + `{"f:spec":{".":{},"f:apps":{".":{},"k:{\"name\":\"app-💻\"}":{".":{},"f:container":{".":{},"f:image":{},"f:name":{},"f:ports":{".":{},"k:{\"name\":\"port-🔑\"}":{".":{},"f:containerPort":{},"f:name":{}}}},"f:name":{}},"k:{\"name\":\"app-🚀\"}":{".":{},"f:container":{".":{},"f:image":{},"f:name":{},"f:ports":{".":{},"k:{\"name\":\"port-✅\"}":{".":{},"f:containerPort":{},"f:name":{}}}},"f:name":{}}}}}`, } for i, str := range examples { t.Run(fmt.Sprintf("%v", i), func(t *testing.T) { diff --git a/fieldpath/set_test.go b/fieldpath/set_test.go index 0685bc47..63f76221 100644 --- a/fieldpath/set_test.go +++ b/fieldpath/set_test.go @@ -48,17 +48,38 @@ var randomPathMaker = randomPathAlphabet(MakePathOrDie( "aad", "aae", "aaf", + // All alphabets + "abcdefghijklmnopqrstuvwxyz", + "ABCDEFGHIJKLMNOPQRSTUVWXYZ", + // Keys + KeyByFields("name", "привет"), + KeyByFields("name", "你好"), + KeyByFields("name", "こんにちは"), + KeyByFields("name", "안녕하세요"), + KeyByFields("name", "مرحبا"), KeyByFields("name", "first"), KeyByFields("name", "second"), KeyByFields("port", 443, "protocol", "tcp"), KeyByFields("port", 443, "protocol", "udp"), + KeyByFields("key", "value"), + KeyByFields("lang", "en-US"), + KeyByFields("unicode-key", "unicode-value-🔥"), + // Values _V(1), _V(2), _V(3), _V("aa"), _V("ab"), _V(true), - 1, 2, 3, 4, + _V(0), + _V(-1), + _V(3.14159), + _V("string with spaces"), + _V("string with \"quotes\""), + _V("unicode-string-你好"), + _V(false), + // Indices + 1, 2, 3, 4, 0, 5, 100, 2147483648, )) func BenchmarkFieldSet(b *testing.B) { From 7e8442c796452a51481a7ab8563dd68504bc1597 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Wed, 10 Sep 2025 09:16:18 -0400 Subject: [PATCH 12/20] Cleanup inactive reviewers, add liggitt --- OWNERS | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/OWNERS b/OWNERS index e58cd518..5788a0a7 100644 --- a/OWNERS +++ b/OWNERS @@ -1,14 +1,15 @@ # See the OWNERS docs: https://go.k8s.io/owners approvers: - - apelisse - jpbetz + - liggitt emeritus_approvers: + - apelisse # 2025-09-10 - lavalamp # 2023-05-13 - jennybuckley # 2021-05-13 reviewers: - - apelisse - jefftree + - liggitt - yliaog - cici37 - yongruilin From 92b1eeb43b4beb53704ec3e101d0a95b2a1d6b95 Mon Sep 17 00:00:00 2001 From: Henrik Schmidt Date: Tue, 17 Jun 2025 18:13:41 +0200 Subject: [PATCH 13/20] Fix ExtractItems to preserve empty maps and lists Previously, ExtractItems converted empty collections ({} and []) to null. This change preserves the distinction between null and empty values. --- typed/remove.go | 8 ++++++++ typed/remove_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/typed/remove.go b/typed/remove.go index 86de5105..ac47ccdf 100644 --- a/typed/remove.go +++ b/typed/remove.go @@ -58,6 +58,10 @@ func (w *removingWalker) doList(t *schema.List) (errs ValidationErrors) { defer w.allocator.Free(l) // If list is null or empty just return if l == nil || l.Length() == 0 { + // For extraction, we just return the value as is (which is nil or empty). For extraction the difference matters. + if w.shouldExtract { + w.out = w.value.Unstructured() + } return nil } @@ -113,6 +117,10 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors { } // If map is null or empty just return if m == nil || m.Empty() { + // For extraction, we just return the value as is (which is nil or empty). For extraction the difference matters. + if w.shouldExtract { + w.out = w.value.Unstructured() + } return nil } diff --git a/typed/remove_test.go b/typed/remove_test.go index 2cb469dc..e82ce688 100644 --- a/typed/remove_test.go +++ b/typed/remove_test.go @@ -675,6 +675,38 @@ var removeCases = []removeTestCase{{ ), `{"mapOfMapsRecursive":{"a":{"b":null}}}`, `{"mapOfMapsRecursive": {"a":{"b":{"c":null}}}}`, + }, { + // empty list + `{"listOfLists": []}`, + _NS( + _P("listOfLists"), + ), + ``, + `{"listOfLists": []}`, + }, { + // null list + `{"listOfLists": null}`, + _NS( + _P("listOfLists"), + ), + ``, + `{"listOfLists": null}`, + }, { + // empty map + `{"mapOfMaps": {}}`, + _NS( + _P("mapOfMaps"), + ), + ``, + `{"mapOfMaps": {}}`, + }, { + // nil map + `{"mapOfMaps": null}`, + _NS( + _P("mapOfMaps"), + ), + ``, + `{"mapOfMaps": null}`, }}, }} From e4bed5453bc3c805d0a108eef2e8305bb6e0e202 Mon Sep 17 00:00:00 2001 From: Henrik Schmidt Date: Fri, 27 Jun 2025 23:59:24 +0200 Subject: [PATCH 14/20] Fix duplicate extraction of atomic elements from associative lists ExtractItems with WithAppendKeyFields was duplicating atomic elements when extracting from associative lists. The item was processed twice: once for the direct path match and again for sub-path checking. Added test with atomicElementList to verify the fix. Co-authored-by: Joe Betz --- typed/remove.go | 24 +++++++++++++----------- typed/remove_test.go | 22 ++++++++++++++++++++++ 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/typed/remove.go b/typed/remove.go index ac47ccdf..78ba6f50 100644 --- a/typed/remove.go +++ b/typed/remove.go @@ -84,22 +84,24 @@ func (w *removingWalker) doList(t *schema.List) (errs ValidationErrors) { path, _ := fieldpath.MakePath(pe) // save items on the path when we shouldExtract // but ignore them when we are removing (i.e. !w.shouldExtract) - if w.toRemove.Has(path) { - if w.shouldExtract { - newItems = append(newItems, removeItemsWithSchema(item, w.toRemove, w.schema, t.ElementType, w.shouldExtract).Unstructured()) - } else { - continue + isExactPathMatch := w.toRemove.Has(path) + isPrefixMatch := !w.toRemove.WithPrefix(pe).Empty() + if w.shouldExtract { + if isPrefixMatch { + item = removeItemsWithSchema(item, w.toRemove.WithPrefix(pe), w.schema, t.ElementType, w.shouldExtract) + } + if isExactPathMatch || isPrefixMatch { + newItems = append(newItems, item.Unstructured()) } - } - if subset := w.toRemove.WithPrefix(pe); !subset.Empty() { - item = removeItemsWithSchema(item, subset, w.schema, t.ElementType, w.shouldExtract) } else { - // don't save items not on the path when we shouldExtract. - if w.shouldExtract { + if isExactPathMatch { continue } + if isPrefixMatch { + item = removeItemsWithSchema(item, w.toRemove.WithPrefix(pe), w.schema, t.ElementType, w.shouldExtract) + } + newItems = append(newItems, item.Unstructured()) } - newItems = append(newItems, item.Unstructured()) } if len(newItems) > 0 { w.out = newItems diff --git a/typed/remove_test.go b/typed/remove_test.go index e82ce688..df59c163 100644 --- a/typed/remove_test.go +++ b/typed/remove_test.go @@ -107,6 +107,9 @@ var associativeAndAtomicSchema = `types: - name: atomicMap type: namedType: myAtomicMap + - name: atomicElementList + type: + namedType: myAtomicElementList - name: myList list: elementType: @@ -117,9 +120,20 @@ var associativeAndAtomicSchema = `types: - id - name: myAtomicMap map: + fields: + - name: id + type: + scalar: string elementType: scalar: string elementRelationship: atomic +- name: myAtomicElementList + list: + elementType: + namedType: myAtomicMap + elementRelationship: associative + keys: + - id - name: mySequence list: elementType: @@ -972,6 +986,14 @@ var extractWithKeysCases = []extractWithKeysTestCase{{ "list": []interface{}{nil}, }, }, + { + // extract atomic element from associative list + object: `{"atomicElementList":[{"id":"test-123","data":"value","extra":"field"}]}`, + set: _NS( + _P("atomicElementList", _KBF("id", "test-123")), + ), + wantOutput: typed.YAMLObject(`{"atomicElementList":[{"id":"test-123","data":"value","extra":"field"}]}`), + }, }, }} From 765e5b67912878a4c6205c909aaa2e7ad9916445 Mon Sep 17 00:00:00 2001 From: Lalit Chauhan Date: Tue, 16 Sep 2025 21:36:20 +0000 Subject: [PATCH 15/20] Add more test coverage for searialization and deserialization --- fieldpath/serialize-pe_test.go | 8 +- fieldpath/serialize_test.go | 244 ++++++++++++++++++++++++++++++++- fieldpath/set_test.go | 2 +- 3 files changed, 251 insertions(+), 3 deletions(-) diff --git a/fieldpath/serialize-pe_test.go b/fieldpath/serialize-pe_test.go index 45ef3739..6ba00387 100644 --- a/fieldpath/serialize-pe_test.go +++ b/fieldpath/serialize-pe_test.go @@ -16,7 +16,9 @@ limitations under the License. package fieldpath -import "testing" +import ( + "testing" +) func TestPathElementRoundTrip(t *testing.T) { tests := []string{ @@ -25,8 +27,10 @@ func TestPathElementRoundTrip(t *testing.T) { `f:`, `f:spec`, `f:more-complicated-string`, + `f: string-with-spaces `, `f:abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789`, `k:{"name":"my-container"}`, + `k:{"name":" name with spaces "}`, `k:{"port":"8080","protocol":"TCP"}`, `k:{"optionalField":null}`, `k:{"jsonField":{"A":1,"B":null,"C":"D","E":{"F":"G"}}}`, @@ -35,6 +39,7 @@ func TestPathElementRoundTrip(t *testing.T) { `v:"some-string"`, `v:1234`, `v:{"some":"json"}`, + `v:{"some":" some with spaces "}`, `k:{"name":"app-🚀"}`, `k:{"name":"app-💻"}`, `k:{"name":"app with-unicøde"}`, @@ -79,6 +84,7 @@ func TestDeserializePathElementError(t *testing.T) { `v:`, `k:invalid json`, `k:{"name":invalid}`, + `v:{"some":" \x41"}`, // This is an invalid JSON string because \x41 is not a valid escape sequence. } for _, test := range tests { diff --git a/fieldpath/serialize_test.go b/fieldpath/serialize_test.go index 727e0a6a..d1bd1fc2 100644 --- a/fieldpath/serialize_test.go +++ b/fieldpath/serialize_test.go @@ -21,6 +21,8 @@ import ( "fmt" "strings" "testing" + + "github.com/google/go-cmp/cmp" ) func TestSerializeV1(t *testing.T) { @@ -51,7 +53,7 @@ func TestSerializeV1GoldenData(t *testing.T) { examples := []string{ `{"f:aaa":{},"f:aab":{},"f:aac":{},"f:aad":{},"f:aae":{},"f:aaf":{},"k:{\"name\":\"first\"}":{},"k:{\"name\":\"second\"}":{},"k:{\"port\":443,\"protocol\":\"tcp\"}":{},"k:{\"port\":443,\"protocol\":\"udp\"}":{},"v:1":{},"v:2":{},"v:3":{},"v:\"aa\"":{},"v:\"ab\"":{},"v:true":{},"i:1":{},"i:2":{},"i:3":{},"i:4":{}}`, `{"f:aaa":{"k:{\"name\":\"second\"}":{"v:3":{"f:aab":{}}},"v:3":{},"v:true":{}},"f:aab":{"f:aaa":{},"f:aaf":{"k:{\"port\":443,\"protocol\":\"udp\"}":{"k:{\"port\":443,\"protocol\":\"tcp\"}":{}}},"k:{\"name\":\"first\"}":{}},"f:aac":{"f:aaa":{"v:1":{}},"f:aac":{},"v:3":{"k:{\"name\":\"second\"}":{}}},"f:aad":{"f:aac":{"v:1":{}},"f:aaf":{"k:{\"name\":\"first\"}":{"k:{\"name\":\"first\"}":{}}},"i:1":{"i:1":{},"i:3":{"v:true":{}}}},"f:aae":{"f:aae":{},"k:{\"port\":443,\"protocol\":\"tcp\"}":{"k:{\"port\":443,\"protocol\":\"udp\"}":{}},"i:4":{"f:aaf":{}}},"f:aaf":{"i:1":{"f:aac":{}},"i:2":{},"i:3":{}},"k:{\"name\":\"first\"}":{"f:aad":{"f:aaf":{}}},"k:{\"port\":443,\"protocol\":\"tcp\"}":{"f:aaa":{"f:aad":{}}},"k:{\"port\":443,\"protocol\":\"udp\"}":{"f:aac":{},"k:{\"name\":\"first\"}":{"i:3":{}},"k:{\"port\":443,\"protocol\":\"udp\"}":{"i:4":{}}},"v:1":{"f:aac":{"i:4":{}},"f:aaf":{},"k:{\"port\":443,\"protocol\":\"tcp\"}":{}},"v:2":{"f:aad":{"f:aaf":{}},"i:1":{}},"v:3":{"f:aaa":{},"k:{\"name\":\"first\"}":{},"i:2":{}},"v:\"aa\"":{"f:aab":{"f:aaf":{}},"f:aae":{},"k:{\"name\":\"first\"}":{"f:aad":{}},"i:2":{}},"v:\"ab\"":{"f:aaf":{"i:4":{}},"k:{\"port\":443,\"protocol\":\"tcp\"}":{},"k:{\"port\":443,\"protocol\":\"udp\"}":{},"v:1":{"k:{\"port\":443,\"protocol\":\"udp\"}":{}},"i:1":{"f:aae":{"i:4":{}}}},"v:true":{"k:{\"name\":\"second\"}":{"f:aaa":{}},"i:2":{"k:{\"port\":443,\"protocol\":\"tcp\"}":{}}},"i:1":{"i:3":{"f:aaf":{}}},"i:2":{"f:aae":{},"k:{\"port\":443,\"protocol\":\"tcp\"}":{"v:1":{}}},"i:3":{"f:aab":{"v:true":{"v:\"aa\"":{}}},"f:aaf":{},"i:1":{}},"i:4":{"v:\"aa\"":{"f:aab":{"k:{\"name\":\"second\"}":{}}}}}`, - `{"f:spec":{".":{},"f:apps":{".":{},"k:{\"name\":\"app-💻\"}":{".":{},"f:container":{".":{},"f:image":{},"f:name":{},"f:ports":{".":{},"k:{\"name\":\"port-🔑\"}":{".":{},"f:containerPort":{},"f:name":{}}}},"f:name":{}},"k:{\"name\":\"app-🚀\"}":{".":{},"f:container":{".":{},"f:image":{},"f:name":{},"f:ports":{".":{},"k:{\"name\":\"port-✅\"}":{".":{},"f:containerPort":{},"f:name":{}}}},"f:name":{}}}}}`, + `{"f:spec":{".":{},"f:apps":{".":{},"k:{\"name\":\" app-💻\"}":{".":{},"f:container":{".":{},"f:image":{},"f:name":{},"f:ports":{".":{},"k:{\"name\":\"port 🔑\"}":{".":{},"f:containerPort":{},"f:name":{}}}},"f:name":{}},"k:{\"name\":\" app-🚀\"}":{".":{},"f:container":{".":{},"f:image":{},"f:name":{},"f:ports":{".":{},"k:{\"name\":\"port-✅ \"}":{".":{},"f:containerPort":{},"f:name":{}}}},"f:name":{}}}}}`, } for i, str := range examples { t.Run(fmt.Sprintf("%v", i), func(t *testing.T) { @@ -71,6 +73,246 @@ func TestSerializeV1GoldenData(t *testing.T) { } } +func TestDeserializeForValidNonNormalized(t *testing.T) { + testCases := []struct { + nonNormalizedString string + normalizedString string + }{ + { + nonNormalizedString: `{ + "f:aad": {}, + "f:aaa": {}, + "f:aab": {}, + "f:aac": {}, + "f:aae": {}, + "f:aaf": {}, + + + "k:{\"name\":\"first\"}": {}, + "k:{\"name\":\"second\"}": {}, + "k:{\"port\":443,\"protocol\":\"tcp\"}": {}, + "k:{ \"protocol\":\"udp\",\"port\":443}": {}, + "v:1": {}, + + "v:2": {}, + "v: 3": {}, + "v:\"aa\"": {}, + + "v:\"ab\"": {}, + "v:true": {}, + "i:1": {}, + "i:2": {}, + "i:3": {} , + "i:4": {} +}`, + normalizedString: `{"f:aaa":{},"f:aab":{},"f:aac":{},"f:aad":{},"f:aae":{},"f:aaf":{},"k:{\"name\":\"first\"}":{},"k:{\"name\":\"second\"}":{},"k:{\"port\":443,\"protocol\":\"tcp\"}":{},"k:{\"port\":443,\"protocol\":\"udp\"}":{},"v:1":{},"v:2":{},"v:3":{},"v:\"aa\"":{},"v:\"ab\"":{},"v:true":{},"i:1":{},"i:2":{},"i:3":{},"i:4":{}}`, + }, { + nonNormalizedString: `{ + "f:aaa": { + "k:{\"name\":\"second\"}": { + "v:3": { + "f:aab": {} + } + }, + "v:3": {}, + "v:true": {} + }, + "f:aab": { + "f:aaa": {}, + "f:aaf": { + "k:{\"port\":443,\"protocol\":\"udp\"}": { + "k:{\"port\":443,\"protocol\":\"tcp\"}": {} + } + }, + "k:{\"name\":\"first\"}": {} + }, + "f:aac": { + "f:aaa": { + "v:1": {} + }, + "f:aac": {}, + "v:3": { + "k:{\"name\":\"second\"}": {} + } + }, + "f:aad": { + "f:aac": { + "v:1": {} + }, + "f:aaf": { + "k:{\"name\":\"first\"}": { + "k:{\"name\":\"first\"}": {} + } + }, + "i:1": { + "i:1": {}, + "i:3": { + "v:true": {} + } + } + }, + "f:aae": { + "f:aae": {}, + "k:{\"port\":443,\"protocol\":\"tcp\"}": { + "k:{\"port\":443,\"protocol\":\"udp\"}": {} + }, + "i:4": { + "f:aaf": {} + } + }, + "f:aaf": { + "i:1": { + "f:aac": {} + }, + "i:3": {}, + "i:2": {} + }, + "k:{\"name\":\"first\"}": { + "f:aad": { + "f:aaf": {} + } + }, + "k:{\"port\":443,\"protocol\":\"tcp\"}": { + "f:aaa": { + "f:aad": {} + } + }, + "k:{\"port\":443,\"protocol\":\"udp\"}": { + "f:aac": {}, + "k:{\"name\":\"first\"}": { + "i:3": {} + }, + "k:{\"port\":443,\"protocol\":\"udp\"}": { + "i:4": {} + } + }, + "v:2": { + "f:aad": { + "f:aaf": {} + }, + "i:1": {} + }, + "v:1": { + "f:aac": { + "i:4": {} + }, + "f:aaf": {}, + "k:{\"port\":443,\"protocol\":\"tcp\"}": {} + }, + "v:3": { + "f:aaa": {}, + "k:{\"name\":\"first\"}": {}, + "i:2": {} + }, + "v:\"aa\"": { + "f:aab": { + "f:aaf": {} + }, + "f:aae": {}, + "k:{\"name\":\"first\"}": { + "f:aad": {} + }, + "i:2": {} + }, + "v:\"ab\"": { + "f:aaf": { + "i:4": {} + }, + "k:{\"port\":443,\"protocol\":\"tcp\"}": {}, + "k:{\"port\":443,\"protocol\":\"udp\"}": {}, + "v:1": { + "k:{\"port\":443,\"protocol\":\"udp\"}": {} + }, + "i:1": { + "f:aae": { + "i:4": {} + } + } + }, + + "v:true": { + "k:{\"name\":\"second\"}": { + "f:aaa": {} + }, + "i:2": { + "k:{\"port\":443,\"protocol\":\"tcp\"}": {} + } + }, + + + "i:1": { + "i:3": { + "f:aaf": {} + } + }, + + "i:3": { + "f:aab": { + "v:true": { + "v:\"aa\"": {} + } + }, + "f:aaf": {}, + "i:1": {} + }, + + "i:2": { + "f:aae": {}, + "k:{\"port\":443,\"protocol\":\"tcp\"}": { + "v:1": {} + } + }, + + "i:4": { + "v:\"aa\"": { + "f:aab": { + "k:{\"name\":\"second\"}": {} + } + } + } +}`, + normalizedString: `{"f:aaa":{"k:{\"name\":\"second\"}":{"v:3":{"f:aab":{}}},"v:3":{},"v:true":{}},"f:aab":{"f:aaa":{},"f:aaf":{"k:{\"port\":443,\"protocol\":\"udp\"}":{"k:{\"port\":443,\"protocol\":\"tcp\"}":{}}},"k:{\"name\":\"first\"}":{}},"f:aac":{"f:aaa":{"v:1":{}},"f:aac":{},"v:3":{"k:{\"name\":\"second\"}":{}}},"f:aad":{"f:aac":{"v:1":{}},"f:aaf":{"k:{\"name\":\"first\"}":{"k:{\"name\":\"first\"}":{}}},"i:1":{"i:1":{},"i:3":{"v:true":{}}}},"f:aae":{"f:aae":{},"k:{\"port\":443,\"protocol\":\"tcp\"}":{"k:{\"port\":443,\"protocol\":\"udp\"}":{}},"i:4":{"f:aaf":{}}},"f:aaf":{"i:1":{"f:aac":{}},"i:2":{},"i:3":{}},"k:{\"name\":\"first\"}":{"f:aad":{"f:aaf":{}}},"k:{\"port\":443,\"protocol\":\"tcp\"}":{"f:aaa":{"f:aad":{}}},"k:{\"port\":443,\"protocol\":\"udp\"}":{"f:aac":{},"k:{\"name\":\"first\"}":{"i:3":{}},"k:{\"port\":443,\"protocol\":\"udp\"}":{"i:4":{}}},"v:1":{"f:aac":{"i:4":{}},"f:aaf":{},"k:{\"port\":443,\"protocol\":\"tcp\"}":{}},"v:2":{"f:aad":{"f:aaf":{}},"i:1":{}},"v:3":{"f:aaa":{},"k:{\"name\":\"first\"}":{},"i:2":{}},"v:\"aa\"":{"f:aab":{"f:aaf":{}},"f:aae":{},"k:{\"name\":\"first\"}":{"f:aad":{}},"i:2":{}},"v:\"ab\"":{"f:aaf":{"i:4":{}},"k:{\"port\":443,\"protocol\":\"tcp\"}":{},"k:{\"port\":443,\"protocol\":\"udp\"}":{},"v:1":{"k:{\"port\":443,\"protocol\":\"udp\"}":{}},"i:1":{"f:aae":{"i:4":{}}}},"v:true":{"k:{\"name\":\"second\"}":{"f:aaa":{}},"i:2":{"k:{\"port\":443,\"protocol\":\"tcp\"}":{}}},"i:1":{"i:3":{"f:aaf":{}}},"i:2":{"f:aae":{},"k:{\"port\":443,\"protocol\":\"tcp\"}":{"v:1":{}}},"i:3":{"f:aab":{"v:true":{"v:\"aa\"":{}}},"f:aaf":{},"i:1":{}},"i:4":{"v:\"aa\"":{"f:aab":{"k:{\"name\":\"second\"}":{}}}}}`, + }, + } + + for i, tc := range testCases { + t.Run(fmt.Sprintf("%v", i), func(t *testing.T) { + nonNormSet := NewSet() + err := nonNormSet.FromJSON(strings.NewReader(tc.nonNormalizedString)) + if err != nil { + t.Fatalf("Failed to deserialize non normalized string %s : %v\n%#v", tc.nonNormalizedString, err, nonNormSet) + } + + normSet := NewSet() + err = normSet.FromJSON(strings.NewReader(tc.normalizedString)) + if err != nil { + t.Fatalf("Failed to deserialize non normalized string %s : %v\n%#v", tc.normalizedString, err, normSet) + } + + nonNormSetJson, err := nonNormSet.ToJSON() + if err != nil { + t.Fatalf("Failed to serialize non-normalized set: %v\n%#v", err, nonNormSet) + + } + + normSetJson, err := normSet.ToJSON() + if err != nil { + t.Fatalf("Failed to serialize normalized set: %v\n%#v", err, normSet) + + } + + if diff := cmp.Diff(nonNormSetJson, normSetJson); diff != "" { + t.Errorf("diff should be non empty, diff %s", diff) + } + + // this should exceeds if the diff was "". + if !normSet.Equals(nonNormSet) { + t.Errorf("expected both set to be equal.") + } + + }) + } +} func TestDropUnknown(t *testing.T) { input := `{"f:aaa":{},"r:aab":{}}` expect := `{"f:aaa":{}}` diff --git a/fieldpath/set_test.go b/fieldpath/set_test.go index 63f76221..54131400 100644 --- a/fieldpath/set_test.go +++ b/fieldpath/set_test.go @@ -61,7 +61,7 @@ var randomPathMaker = randomPathAlphabet(MakePathOrDie( KeyByFields("name", "second"), KeyByFields("port", 443, "protocol", "tcp"), KeyByFields("port", 443, "protocol", "udp"), - KeyByFields("key", "value"), + KeyByFields("key", " value with spaces "), KeyByFields("lang", "en-US"), KeyByFields("unicode-key", "unicode-value-🔥"), // Values From 9c093d14f9d7f957583706ca9342e4cc60b304ea Mon Sep 17 00:00:00 2001 From: Peter Jiang Date: Thu, 2 Oct 2025 17:09:34 -0700 Subject: [PATCH 16/20] fix: RemoveItems should preserve empty list and map Signed-off-by: Peter Jiang --- typed/remove.go | 34 ++++++++++++++++++++++++++++++++-- typed/remove_test.go | 8 ++++---- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/typed/remove.go b/typed/remove.go index 78ba6f50..c1c7a517 100644 --- a/typed/remove.go +++ b/typed/remove.go @@ -75,6 +75,7 @@ func (w *removingWalker) doList(t *schema.List) (errs ValidationErrors) { } var newItems []interface{} + hadMatches := false iter := l.RangeUsing(w.allocator) defer w.allocator.Free(iter) for iter.Next() { @@ -98,12 +99,26 @@ func (w *removingWalker) doList(t *schema.List) (errs ValidationErrors) { continue } if isPrefixMatch { + // Removing nested items WITHIN this list item - preserve if it becomes empty + hadMatches = true + wasMap := item.IsMap() + wasList := item.IsList() item = removeItemsWithSchema(item, w.toRemove.WithPrefix(pe), w.schema, t.ElementType, w.shouldExtract) + // If recursive call returned null but we're removing items within (not the item itself), + // preserve the empty container structure + if item.IsNull() && !w.shouldExtract { + if wasMap { + item = value.NewValueInterface(map[string]interface{}{}) + } else if wasList { + item = value.NewValueInterface([]interface{}{}) + } + } } newItems = append(newItems, item.Unstructured()) } } - if len(newItems) > 0 { + // Preserve empty lists (non-nil) instead of converting to null when items were matched and removed + if len(newItems) > 0 || (hadMatches && !w.shouldExtract) { w.out = newItems } return nil @@ -141,6 +156,7 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors { } newMap := map[string]interface{}{} + hadMatches := false m.Iterate(func(k string, val value.Value) bool { pe := fieldpath.PathElement{FieldName: &k} path, _ := fieldpath.MakePath(pe) @@ -151,6 +167,7 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors { // save values on the path when we shouldExtract // but ignore them when we are removing (i.e. !w.shouldExtract) if w.toRemove.Has(path) { + // Exact match: removing this field itself, not items within it if w.shouldExtract { newMap[k] = removeItemsWithSchema(val, w.toRemove, w.schema, fieldType, w.shouldExtract).Unstructured() @@ -158,7 +175,19 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors { return true } if subset := w.toRemove.WithPrefix(pe); !subset.Empty() { + hadMatches = true + wasMap := val.IsMap() + wasList := val.IsList() val = removeItemsWithSchema(val, subset, w.schema, fieldType, w.shouldExtract) + // If recursive call returned null but we're removing items within (not the field itself), + // preserve the empty container structure + if val.IsNull() && !w.shouldExtract { + if wasMap { + val = value.NewValueInterface(map[string]interface{}{}) + } else if wasList { + val = value.NewValueInterface([]interface{}{}) + } + } } else { // don't save values not on the path when we shouldExtract. if w.shouldExtract { @@ -168,7 +197,8 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors { newMap[k] = val.Unstructured() return true }) - if len(newMap) > 0 { + // Preserve empty maps (non-nil) instead of converting to null when items were matched and removed + if len(newMap) > 0 || (hadMatches && !w.shouldExtract) { w.out = newMap } return nil diff --git a/typed/remove_test.go b/typed/remove_test.go index df59c163..76850652 100644 --- a/typed/remove_test.go +++ b/typed/remove_test.go @@ -281,7 +281,7 @@ var removeCases = []removeTestCase{{ quadruplets: []removeQuadruplet{{ `{"setBool":[false]}`, _NS(_P("setBool", _V(false))), - `{"setBool":null}`, + `{"setBool":[]}`, `{"setBool":[false]}`, }, { `{"setBool":[false]}`, @@ -671,7 +671,7 @@ var removeCases = []removeTestCase{{ _NS( _P("mapOfMapsRecursive", "a"), ), - `{"mapOfMapsRecursive"}`, + `{"mapOfMapsRecursive":{}}`, `{"mapOfMapsRecursive": {"a":null}}`, }, { // second-level map @@ -679,7 +679,7 @@ var removeCases = []removeTestCase{{ _NS( _P("mapOfMapsRecursive", "a", "b"), ), - `{"mapOfMapsRecursive":{"a":null}}`, + `{"mapOfMapsRecursive":{"a":{}}}`, `{"mapOfMapsRecursive": {"a":{"b":null}}}`, }, { // third-level map @@ -687,7 +687,7 @@ var removeCases = []removeTestCase{{ _NS( _P("mapOfMapsRecursive", "a", "b", "c"), ), - `{"mapOfMapsRecursive":{"a":{"b":null}}}`, + `{"mapOfMapsRecursive":{"a":{"b":{}}}}`, `{"mapOfMapsRecursive": {"a":{"b":{"c":null}}}}`, }, { // empty list From 9138e08e899f3ef992a82589f7ee549da2761c59 Mon Sep 17 00:00:00 2001 From: Peter Jiang Date: Thu, 2 Oct 2025 17:16:08 -0700 Subject: [PATCH 17/20] comments Signed-off-by: Peter Jiang --- typed/remove.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/typed/remove.go b/typed/remove.go index c1c7a517..0db1734f 100644 --- a/typed/remove.go +++ b/typed/remove.go @@ -99,12 +99,12 @@ func (w *removingWalker) doList(t *schema.List) (errs ValidationErrors) { continue } if isPrefixMatch { - // Removing nested items WITHIN this list item - preserve if it becomes empty + // Removing nested items within this list item and preserve if it becomes empty hadMatches = true wasMap := item.IsMap() wasList := item.IsList() item = removeItemsWithSchema(item, w.toRemove.WithPrefix(pe), w.schema, t.ElementType, w.shouldExtract) - // If recursive call returned null but we're removing items within (not the item itself), + // If item returned null but we're removing items within the structure(not the item itself), // preserve the empty container structure if item.IsNull() && !w.shouldExtract { if wasMap { @@ -167,7 +167,6 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors { // save values on the path when we shouldExtract // but ignore them when we are removing (i.e. !w.shouldExtract) if w.toRemove.Has(path) { - // Exact match: removing this field itself, not items within it if w.shouldExtract { newMap[k] = removeItemsWithSchema(val, w.toRemove, w.schema, fieldType, w.shouldExtract).Unstructured() @@ -179,7 +178,7 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors { wasMap := val.IsMap() wasList := val.IsList() val = removeItemsWithSchema(val, subset, w.schema, fieldType, w.shouldExtract) - // If recursive call returned null but we're removing items within (not the field itself), + // If val returned null but we're removing items within the structure (not the field itself), // preserve the empty container structure if val.IsNull() && !w.shouldExtract { if wasMap { From c760b1d1f8f9f08d3282a7b28c3f1ffc38a9469c Mon Sep 17 00:00:00 2001 From: Peter Jiang Date: Fri, 3 Oct 2025 14:54:08 -0700 Subject: [PATCH 18/20] fix merge test for new behavior Signed-off-by: Peter Jiang --- merge/multiple_appliers_test.go | 30 +++++++++++++++--------------- merge/nested_test.go | 4 ++-- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/merge/multiple_appliers_test.go b/merge/multiple_appliers_test.go index 7fad7b15..48f54cdb 100644 --- a/merge/multiple_appliers_test.go +++ b/merge/multiple_appliers_test.go @@ -1061,14 +1061,14 @@ func TestMultipleAppliersNestedType(t *testing.T) { }, }, Object: ` - mapOfMapsRecursive: - a: - c: - d: - e: - f: - g: - `, + mapOfMapsRecursive: + a: {} + c: + d: + e: + f: + g: + `, APIVersion: "v1", Managed: fieldpath.ManagedFields{ "apply-one": fieldpath.NewVersionedSet( @@ -1187,13 +1187,13 @@ func TestMultipleAppliersDeducedType(t *testing.T) { }, }, Object: ` - a: - c: - d: - e: - f: - g: - `, + a: {} + c: + d: + e: + f: + g: + `, APIVersion: "v1", Managed: fieldpath.ManagedFields{ "apply-two": fieldpath.NewVersionedSet( diff --git a/merge/nested_test.go b/merge/nested_test.go index ffdb07b8..2da4d4c9 100644 --- a/merge/nested_test.go +++ b/merge/nested_test.go @@ -554,8 +554,8 @@ func TestUpdateNestedType(t *testing.T) { }, }, Object: ` - struct: - `, + struct: {} + `, APIVersion: "v1", Managed: fieldpath.ManagedFields{ "default": fieldpath.NewVersionedSet( From 193e7fe9a92807037b3367d4265e257d09815330 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=83=A1=E7=8E=AE=E6=96=87?= Date: Fri, 24 Oct 2025 21:23:58 +0800 Subject: [PATCH 19/20] schema: add test and benchmark The test reveals the race condition and prepares for refactor. --- schema/elements_test.go | 66 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/schema/elements_test.go b/schema/elements_test.go index b097b61b..bb4b27fa 100644 --- a/schema/elements_test.go +++ b/schema/elements_test.go @@ -87,6 +87,48 @@ func TestFindField(t *testing.T) { } } +func testMap() *Map { + return &Map{ + Fields: []StructField{ + { + Name: "a", + Type: TypeRef{NamedType: strptr("aaa")}, + }, { + Name: "b", + Type: TypeRef{NamedType: strptr("bbb")}, + }, { + Name: "c", + Type: TypeRef{NamedType: strptr("ccc")}, + }, + }, + } +} + +func BenchmarkFindFieldCached(b *testing.B) { + m := testMap() + m.FindField("a") + for i := 0; i < b.N; i++ { + m.FindField("a") + } +} + +func BenchmarkFindFieldNew(b *testing.B) { + for i := 0; i < b.N; i++ { + m := testMap() + m.FindField("a") + } +} + +// As a baseline of BenchmarkFindFieldNew +func BenchmarkMakeMap(b *testing.B) { + var m *Map + for i := 0; i < b.N; i++ { + m = testMap() + } + b.StopTimer() + b.Log(m) // prevent dead code elimination +} + func TestResolve(t *testing.T) { existing := "existing" notExisting := "not-existing" @@ -177,3 +219,27 @@ func TestCopyInto(t *testing.T) { }) } } + +func TestMapCopyInto(t *testing.T) { + s := Map{ + Fields: []StructField{ + { + Name: "a", + Type: TypeRef{NamedType: strptr("aaa")}, + }, + }, + } + theCopy := Map{} + + assert := func(sf StructField, ok bool) { + if !ok || *sf.Type.NamedType != "aaa" { + t.Error("expected NamedType aaa not found") + } + } + + go func() { + s.CopyInto(&theCopy) + assert(theCopy.FindField("a")) + }() + assert(s.FindField("a")) +} From 40cac0c9a457d3fa2048901fa291d474cd8aed80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=83=A1=E7=8E=AE=E6=96=87?= Date: Fri, 24 Oct 2025 21:24:29 +0800 Subject: [PATCH 20/20] schema: fix race condition Use an additional atomic.Pointer --- schema/elements.go | 47 ++++++++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/schema/elements.go b/schema/elements.go index 5d3707a5..c8138a65 100644 --- a/schema/elements.go +++ b/schema/elements.go @@ -18,6 +18,7 @@ package schema import ( "sync" + "sync/atomic" ) // Schema is a list of named types. @@ -28,7 +29,7 @@ type Schema struct { Types []TypeDef `yaml:"types,omitempty"` once sync.Once - m map[string]TypeDef + m atomic.Pointer[map[string]TypeDef] lock sync.Mutex // Cached results of resolving type references to atoms. Only stores @@ -144,26 +145,28 @@ type Map struct { ElementRelationship ElementRelationship `yaml:"elementRelationship,omitempty"` once sync.Once - m map[string]StructField + m atomic.Pointer[map[string]StructField] } // FindField is a convenience function that returns the referenced StructField, // if it exists, or (nil, false) if it doesn't. func (m *Map) FindField(name string) (StructField, bool) { m.once.Do(func() { - m.m = make(map[string]StructField, len(m.Fields)) + mm := make(map[string]StructField, len(m.Fields)) for _, field := range m.Fields { - m.m[field.Name] = field + mm[field.Name] = field } + m.m.Store(&mm) }) - sf, ok := m.m[name] + sf, ok := (*m.m.Load())[name] return sf, ok } -// CopyInto this instance of Map into the other -// If other is nil this method does nothing. -// If other is already initialized, overwrites it with this instance -// Warning: Not thread safe +// CopyInto clones this instance of Map into dst +// +// If dst is nil this method does nothing. +// If dst is already initialized, overwrites it with this instance. +// Warning: Not thread safe. Only use dst after this function returns. func (m *Map) CopyInto(dst *Map) { if dst == nil { return @@ -175,12 +178,13 @@ func (m *Map) CopyInto(dst *Map) { dst.Unions = m.Unions dst.ElementRelationship = m.ElementRelationship - if m.m != nil { + mm := m.m.Load() + if mm != nil { // If cache is non-nil then the once token had been consumed. // Must reset token and use it again to ensure same semantics. dst.once = sync.Once{} dst.once.Do(func() { - dst.m = m.m + dst.m.Store(mm) }) } } @@ -274,12 +278,13 @@ type List struct { // if it exists, or (nil, false) if it doesn't. func (s *Schema) FindNamedType(name string) (TypeDef, bool) { s.once.Do(func() { - s.m = make(map[string]TypeDef, len(s.Types)) + sm := make(map[string]TypeDef, len(s.Types)) for _, t := range s.Types { - s.m[t.Name] = t + sm[t.Name] = t } + s.m.Store(&sm) }) - t, ok := s.m[name] + t, ok := (*s.m.Load())[name] return t, ok } @@ -352,10 +357,11 @@ func (s *Schema) Resolve(tr TypeRef) (Atom, bool) { return result, true } -// Clones this instance of Schema into the other -// If other is nil this method does nothing. -// If other is already initialized, overwrites it with this instance -// Warning: Not thread safe +// CopyInto clones this instance of Schema into dst +// +// If dst is nil this method does nothing. +// If dst is already initialized, overwrites it with this instance. +// Warning: Not thread safe. Only use dst after this function returns. func (s *Schema) CopyInto(dst *Schema) { if dst == nil { return @@ -364,12 +370,13 @@ func (s *Schema) CopyInto(dst *Schema) { // Schema type is considered immutable so sharing references dst.Types = s.Types - if s.m != nil { + sm := s.m.Load() + if sm != nil { // If cache is non-nil then the once token had been consumed. // Must reset token and use it again to ensure same semantics. dst.once = sync.Once{} dst.once.Do(func() { - dst.m = s.m + dst.m.Store(sm) }) } }