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/path_test.go b/fieldpath/path_test.go index 22410e5c..c5eea4f0 100644 --- a/fieldpath/path_test.go +++ b/fieldpath/path_test.go @@ -50,9 +50,66 @@ func TestPathString(t *testing.T) { _V(false), _V(3.14159), ), `.foo[="b"][=5][=false][=3.14159]`}, + { + name: "simple field", + fp: MakePathOrDie("spec"), + expect: ".spec", + }, + { + name: "app container image", + fp: MakePathOrDie( + "spec", "apps", + KeyByFields("name", "app-🚀"), + "container", "image", + ), + expect: `.spec.apps[name="app-🚀"].container.image`, + }, + { + name: "app port", + fp: MakePathOrDie( + "spec", "apps", + KeyByFields("name", "app-💻"), + "container", "ports", + KeyByFields("name", "port-🔑"), + "containerPort", + ), + expect: ".spec.apps[name=\"app-💻\"].container.ports[name=\"port-🔑\"].containerPort", + }, + { + name: "field with space", + fp: MakePathOrDie("spec", "field with space"), + expect: ".spec.field with space", + }, + { + name: "value with space", + fp: MakePathOrDie( + "spec", "apps", + _V("app with space"), + "container", "image", + ), + expect: `.spec.apps[="app with space"].container.image`, + }, + { + name: "value with quotes", + fp: MakePathOrDie( + "spec", "apps", + _V("app with \"quotes\""), + "container", "image", + ), + expect: ".spec.apps[=\"app with \\\"quotes\\\"\"].container.image", + }, + + { + name: "value with unicode", + fp: MakePathOrDie( + "spec", "apps", + _V("app-with-unicøde"), + "container", "image", + ), + expect: ".spec.apps[=\"app-with-unicøde\"].container.image", + }, } for _, tt := range table { - tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() got := tt.fp.String() diff --git a/fieldpath/serialize-pe_test.go b/fieldpath/serialize-pe_test.go index 0a6ee8f1..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 35cb6084..d1bd1fc2 100644 --- a/fieldpath/serialize_test.go +++ b/fieldpath/serialize_test.go @@ -21,6 +21,8 @@ import ( "fmt" "strings" "testing" + + "github.com/google/go-cmp/cmp" ) func TestSerializeV1(t *testing.T) { @@ -51,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_test.go b/fieldpath/set_test.go index 0685bc47..54131400 100644 --- a/fieldpath/set_test.go +++ b/fieldpath/set_test.go @@ -48,17 +48,38 @@ var randomPathMaker = randomPathAlphabet(MakePathOrDie( "aad", "aae", "aaf", + // All alphabets + "abcdefghijklmnopqrstuvwxyz", + "ABCDEFGHIJKLMNOPQRSTUVWXYZ", + // Keys + KeyByFields("name", "привет"), + KeyByFields("name", "你好"), + KeyByFields("name", "こんにちは"), + KeyByFields("name", "안녕하세요"), + KeyByFields("name", "مرحبا"), KeyByFields("name", "first"), KeyByFields("name", "second"), KeyByFields("port", 443, "protocol", "tcp"), KeyByFields("port", 443, "protocol", "udp"), + KeyByFields("key", " value 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) { diff --git a/merge/multiple_appliers_test.go b/merge/multiple_appliers_test.go index 7fad7b15..48f54cdb 100644 --- a/merge/multiple_appliers_test.go +++ b/merge/multiple_appliers_test.go @@ -1061,14 +1061,14 @@ func TestMultipleAppliersNestedType(t *testing.T) { }, }, Object: ` - mapOfMapsRecursive: - a: - c: - d: - e: - f: - g: - `, + mapOfMapsRecursive: + a: {} + c: + d: + e: + f: + g: + `, APIVersion: "v1", Managed: fieldpath.ManagedFields{ "apply-one": fieldpath.NewVersionedSet( @@ -1187,13 +1187,13 @@ func TestMultipleAppliersDeducedType(t *testing.T) { }, }, Object: ` - a: - c: - d: - e: - f: - g: - `, + a: {} + c: + d: + e: + f: + g: + `, APIVersion: "v1", Managed: fieldpath.ManagedFields{ "apply-two": fieldpath.NewVersionedSet( diff --git a/merge/nested_test.go b/merge/nested_test.go index ffdb07b8..2da4d4c9 100644 --- a/merge/nested_test.go +++ b/merge/nested_test.go @@ -554,8 +554,8 @@ func TestUpdateNestedType(t *testing.T) { }, }, Object: ` - struct: - `, + struct: {} + `, APIVersion: "v1", Managed: fieldpath.ManagedFields{ "default": fieldpath.NewVersionedSet( 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/typed/remove.go b/typed/remove.go index 86de5105..0db1734f 100644 --- a/typed/remove.go +++ b/typed/remove.go @@ -58,6 +58,10 @@ func (w *removingWalker) doList(t *schema.List) (errs ValidationErrors) { defer w.allocator.Free(l) // If list is null or empty just return if l == nil || l.Length() == 0 { + // For extraction, we just return the value as is (which is nil or empty). For extraction the difference matters. + if w.shouldExtract { + w.out = w.value.Unstructured() + } return nil } @@ -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 2cb469dc..76850652 100644 --- a/typed/remove_test.go +++ b/typed/remove_test.go @@ -107,6 +107,9 @@ var associativeAndAtomicSchema = `types: - name: atomicMap type: namedType: myAtomicMap + - name: atomicElementList + type: + namedType: myAtomicElementList - name: myList list: elementType: @@ -117,9 +120,20 @@ var associativeAndAtomicSchema = `types: - id - name: myAtomicMap map: + fields: + - name: id + type: + scalar: string elementType: scalar: string elementRelationship: atomic +- name: myAtomicElementList + list: + elementType: + namedType: myAtomicMap + elementRelationship: associative + keys: + - id - name: mySequence list: elementType: @@ -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"}]}`), + }, }, }}