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 diff --git a/fieldpath/element.go b/fieldpath/element.go index 1578f64c..73436912 100644 --- a/fieldpath/element.go +++ b/fieldpath/element.go @@ -18,10 +18,11 @@ package fieldpath import ( "fmt" + "iter" "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. @@ -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 05019a98..aba96b14 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) { @@ -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/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/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..cd909a50 100644 --- a/fieldpath/fromvalue_test.go +++ b/fieldpath/fromvalue_test.go @@ -19,8 +19,8 @@ package fieldpath import ( "testing" - "sigs.k8s.io/structured-merge-diff/v4/value" - yaml "sigs.k8s.io/yaml/goyaml.v2" + yaml "go.yaml.in/yaml/v2" + "sigs.k8s.io/structured-merge-diff/v6/value" ) func TestFromValue(t *testing.T) { diff --git a/fieldpath/managers_test.go b/fieldpath/managers_test.go index d06b055b..6984176c 100644 --- a/fieldpath/managers_test.go +++ b/fieldpath/managers_test.go @@ -20,13 +20,7 @@ import ( "reflect" "testing" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" -) - -var ( - // Short names for readable test cases. - _NS = fieldpath.NewSet - _P = fieldpath.MakePathOrDie + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" ) func TestManagersEquals(t *testing.T) { 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..c5eea4f0 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 ( @@ -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/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 cb18e7b1..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") @@ -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/fieldpath/serialize-pe_test.go b/fieldpath/serialize-pe_test.go index 0a6ee8f1..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,7 +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"}}}`, @@ -34,6 +39,14 @@ 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"}`, + `k:{"name":"你好世界"}`, + `k:{"name":"Привет, мир"}`, + `k:{"name":"नमस्ते दुनिया"}`, + `k:{"name":"👋"}`, } for _, test := range tests { @@ -71,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 a3bcc9e6..d1bd1fc2 100644 --- a/fieldpath/serialize_test.go +++ b/fieldpath/serialize_test.go @@ -14,13 +14,15 @@ See the License for the specific language governing permissions and limitations under the License. */ -package fieldpath +package fieldpath_test import ( "bytes" "fmt" "strings" "testing" + + "github.com/google/go-cmp/cmp" ) func TestSerializeV1(t *testing.T) { @@ -51,6 +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":{}}}}}`, } for i, str := range examples { t.Run(fmt.Sprintf("%v", i), func(t *testing.T) { @@ -70,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.go b/fieldpath/set.go index 77ae2511..d2d8c8a4 100644 --- a/fieldpath/set.go +++ b/fieldpath/set.go @@ -18,11 +18,13 @@ package fieldpath import ( "fmt" - "sigs.k8s.io/structured-merge-diff/v4/value" + "iter" "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. @@ -46,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) { @@ -385,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) @@ -454,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 { @@ -704,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 7efb374e..54131400 100644 --- a/fieldpath/set_test.go +++ b/fieldpath/set_test.go @@ -14,17 +14,17 @@ See the License for the specific language governing permissions and limitations under the License. */ -package fieldpath +package fieldpath_test import ( "bytes" "fmt" "math/rand" - "sigs.k8s.io/structured-merge-diff/v4/value" "testing" - "sigs.k8s.io/structured-merge-diff/v4/schema" - yaml "sigs.k8s.io/yaml/goyaml.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 @@ -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 with spaces "), + 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) { @@ -232,7 +253,6 @@ func TestSetIterSize(t *testing.T) { ) s2 := NewSet() - addedCount := 0 s1.Iterate(func(p Path) { if s2.Size() != addedCount { @@ -245,6 +265,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) @@ -651,80 +684,227 @@ 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 +} + +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 { panic(err) } - return sc, schema.TypeRef{NamedType: &name} + return parser } -var _P = MakePathOrDie - func TestEnsureNamedFieldsAreMembers(t *testing.T) { table := []struct { - set, expected *Set + schemaFn func() (*typed.Parser, string) + newSchemaFn func() (*typed.Parser, string) + value typed.YAMLObject + expectedBefore *Set + expectedAfter *Set }{ { - set: NewSet(_P("named", "named", "value")), - expected: NewSet( + schemaFn: nestedSchema, + 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( + schemaFn: nestedSchema, + 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"), + 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"), + _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"), ), }, + { + // 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(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 := test.schemaFn() + 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 + 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", + test.expectedAfter, got, - test.expected.Difference(got), - got.Difference(test.expected), + test.expectedAfter.Difference(got), + got.Difference(test.expectedAfter), ) } }) @@ -755,6 +935,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 dd09a8fe..f5343b69 100644 --- a/go.mod +++ b/go.mod @@ -1,11 +1,15 @@ -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 github.com/json-iterator/go v1.1.12 - github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect + go.yaml.in/yaml/v2 v2.4.2 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.23 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/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..28f81038 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" @@ -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/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..48f54cdb 100644 --- a/merge/multiple_appliers_test.go +++ b/merge/multiple_appliers_test.go @@ -22,12 +22,12 @@ 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" - yaml "sigs.k8s.io/yaml/goyaml.v2" + 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" ) func TestMultipleAppliersSet(t *testing.T) { @@ -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 eb65a5a9..2da4d4c9 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 { @@ -554,8 +554,8 @@ func TestUpdateNestedType(t *testing.T) { }, }, Object: ` - struct: - `, + struct: {} + `, APIVersion: "v1", Managed: fieldpath.ManagedFields{ "default": fieldpath.NewVersionedSet( 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..db78ff56 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 { @@ -252,3 +252,459 @@ 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: 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) + } + }) + } +} + +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) + } + }) + } +} 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/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) }) } } 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")) +} 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..8a9c0b50 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 @@ -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/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..c46e69f2 100644 --- a/typed/parser.go +++ b/typed/parser.go @@ -19,9 +19,9 @@ package typed import ( "fmt" - "sigs.k8s.io/structured-merge-diff/v4/schema" - "sigs.k8s.io/structured-merge-diff/v4/value" - yaml "sigs.k8s.io/yaml/goyaml.v2" + yaml "go.yaml.in/yaml/v2" + "sigs.k8s.io/structured-merge-diff/v6/schema" + "sigs.k8s.io/structured-merge-diff/v6/value" ) // YAMLObject is an object encoded in YAML. diff --git a/typed/parser_test.go b/typed/parser_test.go index b81a9372..9ef51cb6 100644 --- a/typed/parser_test.go +++ b/typed/parser_test.go @@ -22,8 +22,8 @@ import ( "strings" "testing" - "sigs.k8s.io/structured-merge-diff/v4/typed" - yaml "sigs.k8s.io/yaml/goyaml.v2" + yaml "go.yaml.in/yaml/v2" + "sigs.k8s.io/structured-merge-diff/v6/typed" ) func testdata(file string) string { 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..0db1734f 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 { @@ -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 } @@ -71,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() { @@ -80,24 +85,40 @@ 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 { + // 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 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 { + item = value.NewValueInterface(map[string]interface{}{}) + } else if wasList { + item = value.NewValueInterface([]interface{}{}) + } + } + } + newItems = append(newItems, item.Unstructured()) } - 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 @@ -113,6 +134,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 } @@ -131,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) @@ -148,7 +174,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 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 { + 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 { @@ -158,7 +196,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 3a959ceb..76850652 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 { @@ -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: @@ -267,7 +281,7 @@ var removeCases = []removeTestCase{{ quadruplets: []removeQuadruplet{{ `{"setBool":[false]}`, _NS(_P("setBool", _V(false))), - `{"setBool":null}`, + `{"setBool":[]}`, `{"setBool":[false]}`, }, { `{"setBool":[false]}`, @@ -657,7 +671,7 @@ var removeCases = []removeTestCase{{ _NS( _P("mapOfMapsRecursive", "a"), ), - `{"mapOfMapsRecursive"}`, + `{"mapOfMapsRecursive":{}}`, `{"mapOfMapsRecursive": {"a":null}}`, }, { // second-level map @@ -665,7 +679,7 @@ var removeCases = []removeTestCase{{ _NS( _P("mapOfMapsRecursive", "a", "b"), ), - `{"mapOfMapsRecursive":{"a":null}}`, + `{"mapOfMapsRecursive":{"a":{}}}`, `{"mapOfMapsRecursive": {"a":{"b":null}}}`, }, { // third-level map @@ -673,8 +687,40 @@ var removeCases = []removeTestCase{{ _NS( _P("mapOfMapsRecursive", "a", "b", "c"), ), - `{"mapOfMapsRecursive":{"a":{"b":null}}}`, + `{"mapOfMapsRecursive":{"a":{"b":{}}}}`, `{"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}`, }}, }} @@ -940,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"}]}`), + }, }, }} 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..92ba1416 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 { @@ -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}`, diff --git a/value/equals_test.go b/value/equals_test.go index 1f98da2f..ecfc7d03 100644 --- a/value/equals_test.go +++ b/value/equals_test.go @@ -21,8 +21,8 @@ import ( "path/filepath" "testing" - "sigs.k8s.io/structured-merge-diff/v4/value" - yaml "sigs.k8s.io/yaml/goyaml.v2" + yaml "go.yaml.in/yaml/v2" + "sigs.k8s.io/structured-merge-diff/v6/value" ) func testdata(file string) string { 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 { 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 diff --git a/value/value.go b/value/value.go index f72e5cd2..140b9903 100644 --- a/value/value.go +++ b/value/value.go @@ -23,7 +23,8 @@ import ( "strings" jsoniter "github.com/json-iterator/go" - yaml "sigs.k8s.io/yaml/goyaml.v2" + + yaml "go.yaml.in/yaml/v2" ) var ( @@ -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()) }