Skip to content

Commit 70a0bfd

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #78 from apelisse/oneof-more
Integrate union in Apply/Update path
2 parents b2ed7e1 + d3d351f commit 70a0bfd

File tree

6 files changed

+231
-16
lines changed

6 files changed

+231
-16
lines changed

‎internal/fixture/state.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,11 +92,11 @@ func (s *State) Update(obj typed.YAMLObject, version fieldpath.APIVersion, manag
9292
if err != nil {
9393
return err
9494
}
95-
managers, err := s.Updater.Update(s.Live, tv, version, s.Managers, manager)
95+
newObj, managers, err := s.Updater.Update(s.Live, tv, version, s.Managers, manager)
9696
if err != nil {
9797
return err
9898
}
99-
s.Live = tv
99+
s.Live = newObj
100100
s.Managers = managers
101101

102102
return nil

‎merge/union_test.go

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
/*
2+
Copyright 2019 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package merge_test
18+
19+
import (
20+
"testing"
21+
22+
"sigs.k8s.io/structured-merge-diff/fieldpath"
23+
. "sigs.k8s.io/structured-merge-diff/internal/fixture"
24+
"sigs.k8s.io/structured-merge-diff/merge"
25+
"sigs.k8s.io/structured-merge-diff/typed"
26+
)
27+
28+
var unionFieldsParser = func() typed.ParseableType {
29+
parser, err := typed.NewParser(`types:
30+
- name: unionFields
31+
struct:
32+
fields:
33+
- name: numeric
34+
type:
35+
scalar: numeric
36+
- name: string
37+
type:
38+
scalar: string
39+
- name: type
40+
type:
41+
scalar: string
42+
union:
43+
discriminator: type
44+
fields:
45+
- fieldName: numeric
46+
discriminatedBy: Numeric
47+
- fieldName: string
48+
discriminatedBy: String`)
49+
if err != nil {
50+
panic(err)
51+
}
52+
return parser.Type("unionFields")
53+
}()
54+
55+
func TestUnion(t *testing.T) {
56+
tests := map[string]TestCase{
57+
"union_apply_owns_discriminator": {
58+
Ops: []Operation{
59+
Apply{
60+
Manager: "default",
61+
APIVersion: "v1",
62+
Object: `
63+
numeric: 1
64+
`,
65+
},
66+
},
67+
Object: `
68+
numeric: 1
69+
type: Numeric
70+
`,
71+
Managed: fieldpath.ManagedFields{
72+
"default": &fieldpath.VersionedSet{
73+
Set: _NS(
74+
_P("numeric"), _P("type"),
75+
),
76+
APIVersion: "v1",
77+
},
78+
},
79+
},
80+
"union_apply_without_discriminator_conflict": {
81+
Ops: []Operation{
82+
Update{
83+
Manager: "controller",
84+
APIVersion: "v1",
85+
Object: `
86+
string: "some string"
87+
`,
88+
},
89+
Apply{
90+
Manager: "default",
91+
APIVersion: "v1",
92+
Object: `
93+
numeric: 1
94+
`,
95+
Conflicts: merge.Conflicts{
96+
merge.Conflict{Manager: "controller", Path: _P("type")},
97+
},
98+
},
99+
},
100+
Object: `
101+
string: "some string"
102+
type: String
103+
`,
104+
Managed: fieldpath.ManagedFields{
105+
"controller": &fieldpath.VersionedSet{
106+
Set: _NS(
107+
_P("string"), _P("type"),
108+
),
109+
APIVersion: "v1",
110+
},
111+
},
112+
},
113+
"union_apply_with_null_value": {
114+
Ops: []Operation{
115+
Apply{
116+
Manager: "default",
117+
APIVersion: "v1",
118+
Object: `
119+
type: Numeric
120+
string: null
121+
numeric: 1
122+
`,
123+
},
124+
},
125+
},
126+
}
127+
128+
for name, test := range tests {
129+
t.Run(name, func(t *testing.T) {
130+
if err := test.Test(unionFieldsParser); err != nil {
131+
t.Fatal(err)
132+
}
133+
})
134+
}
135+
}
136+
137+
func TestUnionErrors(t *testing.T) {
138+
tests := map[string]TestCase{
139+
"union_apply_two": {
140+
Ops: []Operation{
141+
Apply{
142+
Manager: "default",
143+
APIVersion: "v1",
144+
Object: `
145+
numeric: 1
146+
string: "some string"
147+
`,
148+
},
149+
},
150+
},
151+
"union_apply_two_and_discriminator": {
152+
Ops: []Operation{
153+
Apply{
154+
Manager: "default",
155+
APIVersion: "v1",
156+
Object: `
157+
type: Numeric
158+
string: "some string"
159+
numeric: 1
160+
`,
161+
},
162+
},
163+
},
164+
"union_apply_wrong_discriminator": {
165+
Ops: []Operation{
166+
Apply{
167+
Manager: "default",
168+
APIVersion: "v1",
169+
Object: `
170+
type: Numeric
171+
string: "some string"
172+
`,
173+
},
174+
},
175+
},
176+
}
177+
178+
for name, test := range tests {
179+
t.Run(name, func(t *testing.T) {
180+
if test.Test(unionFieldsParser) == nil {
181+
t.Fatal("Should fail")
182+
}
183+
})
184+
}
185+
}

‎merge/update.go

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -119,16 +119,19 @@ func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpa
119119
// that you intend to persist (after applying the patch if this is for a
120120
// PATCH call), and liveObject must be the original object (empty if
121121
// this is a CREATE call).
122-
func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, manager string) (fieldpath.ManagedFields, error) {
123-
var err error
122+
func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, manager string) (*typed.TypedValue, fieldpath.ManagedFields, error) {
123+
newObject, err := liveObject.NormalizeUnions(newObject)
124+
if err != nil {
125+
return nil, fieldpath.ManagedFields{}, err
126+
}
124127
managers = shallowCopyManagers(managers)
125128
managers, err = s.update(liveObject, newObject, version, managers, manager, true)
126129
if err != nil {
127-
return fieldpath.ManagedFields{}, err
130+
return nil, fieldpath.ManagedFields{}, err
128131
}
129132
compare, err := liveObject.Compare(newObject)
130133
if err != nil {
131-
return fieldpath.ManagedFields{}, fmt.Errorf("failed to compare live and new objects: %v", err)
134+
return nil, fieldpath.ManagedFields{}, fmt.Errorf("failed to compare live and new objects: %v", err)
132135
}
133136
if _, ok := managers[manager]; !ok {
134137
managers[manager] = &fieldpath.VersionedSet{
@@ -140,18 +143,26 @@ func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldp
140143
if managers[manager].Set.Empty() {
141144
delete(managers, manager)
142145
}
143-
return managers, nil
146+
return newObject, managers, nil
144147
}
145148

146149
// Apply should be called when Apply is run, given the current object as
147150
// well as the configuration that is applied. This will merge the object
148151
// and return it.
149152
func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, manager string, force bool) (*typed.TypedValue, fieldpath.ManagedFields, error) {
150153
managers = shallowCopyManagers(managers)
154+
configObject, err := configObject.Empty().NormalizeUnions(configObject)
155+
if err != nil {
156+
return nil, fieldpath.ManagedFields{}, err
157+
}
151158
newObject, err := liveObject.Merge(configObject)
152159
if err != nil {
153160
return nil, fieldpath.ManagedFields{}, fmt.Errorf("failed to merge config: %v", err)
154161
}
162+
newObject, err = liveObject.NormalizeUnions(newObject)
163+
if err != nil {
164+
return nil, fieldpath.ManagedFields{}, err
165+
}
155166
lastSet := managers[manager]
156167
set, err := configObject.ToFieldSet()
157168
if err != nil {

‎typed/typed.go

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -159,16 +159,15 @@ func (tv TypedValue) RemoveItems(items *fieldpath.Set) *TypedValue {
159159
func (tv TypedValue) NormalizeUnions(new *TypedValue) (*TypedValue, error) {
160160
var errs ValidationErrors
161161
var normalizeFn = func(w *mergingWalker) {
162-
if err := normalizeUnion(w); err != nil {
163-
errs = append(errs, w.error(err)...)
164-
}
165-
}
166-
out, mergeErrs := merge(&tv, new, func(w *mergingWalker) {
167162
if w.rhs != nil {
168163
v := *w.rhs
169164
w.out = &v
170165
}
171-
}, normalizeFn)
166+
if err := normalizeUnion(w); err != nil {
167+
errs = append(errs, w.error(err)...)
168+
}
169+
}
170+
out, mergeErrs := merge(&tv, new, func(w *mergingWalker) {}, normalizeFn)
172171
if mergeErrs != nil {
173172
errs = append(errs, mergeErrs.(ValidationErrors)...)
174173
}
@@ -178,6 +177,11 @@ func (tv TypedValue) NormalizeUnions(new *TypedValue) (*TypedValue, error) {
178177
return out, nil
179178
}
180179

180+
func (tv TypedValue) Empty() *TypedValue {
181+
tv.value = value.Value{Null: true}
182+
return &tv
183+
}
184+
181185
func merge(lhs, rhs *TypedValue, rule, postRule mergeRule) (*TypedValue, error) {
182186
if lhs.schema != rhs.schema {
183187
return nil, errorFormatter{}.

‎typed/union.go

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package typed
1818

1919
import (
2020
"fmt"
21+
"strings"
2122

2223
"sigs.k8s.io/structured-merge-diff/schema"
2324
"sigs.k8s.io/structured-merge-diff/value"
@@ -108,7 +109,7 @@ func newFieldsSet(m *value.Map, fields []field) fieldsSet {
108109
}
109110
set := fieldsSet{}
110111
for _, f := range fields {
111-
if _, ok := m.Get(string(f)); ok {
112+
if subField, ok := m.Get(string(f)); ok && !subField.Value.Null {
112113
set.Add(f)
113114
}
114115
}
@@ -152,6 +153,14 @@ func (fs fieldsSet) Difference(o fieldsSet) fieldsSet {
152153
return n
153154
}
154155

156+
func (fs fieldsSet) String() string {
157+
s := []string{}
158+
for k := range fs {
159+
s = append(s, string(k))
160+
}
161+
return strings.Join(s, ", ")
162+
}
163+
155164
type union struct {
156165
d *discriminator
157166
dn discriminatedNames
@@ -188,7 +197,7 @@ func (u *union) Normalize(old, new, out *value.Map) error {
188197
diff := ns.Difference(os)
189198

190199
if len(ns) > 1 && len(diff) != 1 {
191-
return fmt.Errorf("unable to guess new discriminator: %v", diff)
200+
return fmt.Errorf("unable to guess new discriminator amongst new fields: %v", diff)
192201
}
193202

194203
discriminator := field("")
@@ -200,7 +209,7 @@ func (u *union) Normalize(old, new, out *value.Map) error {
200209

201210
if u.d.Get(old) != u.d.Get(new) && u.d.Get(new) != "" {
202211
if len(diff) == 1 && u.d.Get(new) != u.dn.toDiscriminated(discriminator) {
203-
return fmt.Errorf("discriminator and field changed: %v/%v", discriminator, u.d.Get(new))
212+
return fmt.Errorf("discriminator (%v) and field changed (%v)", u.d.Get(new), discriminator)
204213
}
205214
u.clear(out, u.dn.toField(u.d.Get(new)))
206215
return nil

‎typed/union_test.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,12 @@ func TestNormalizeUnions(t *testing.T) {
198198
new: `{"one": 1}`,
199199
out: `{"one": 1, "discriminator": "One"}`,
200200
},
201+
{
202+
name: "new object has three of same union set but one is null",
203+
old: `{"one": 1}`,
204+
new: `{"one": 2, "two": 1, "three": null}`,
205+
out: `{"two": 1, "discriminator": "TWO"}`,
206+
},
201207
// These use-cases shouldn't happen:
202208
{
203209
name: "one field removed, discriminator unchanged",

0 commit comments

Comments
 (0)