Skip to content

Commit cd2b883

Browse files
pohlypohly
committed
support optional map keys
Optional keys of a list map (= associative lists) keys are simply left out of the set of keys, which is different from a key with an empty value like "" for a string and obviously also different from a non-empty value. The comparison of values already supported that and the comparison of list values supported lists with different number of entries. Completely empty key field lists continue to trigger an error ("associative list with keys has an element that omits all key fields <quoted list of fields> (and doesn't have default values for any key fields)". Downgrading from a version which has support for a new optional key to a version which doesn't works as long as the optional key is not used, because the ManagedFields don't mention the new key and field and there are no list entries which have it set. It does not work when the new field and key are used because the older version doesn't know that it needs to consider the new key, as the key is not listed in the older version's OpenAPI spec. This is considered acceptable because new fields will be alpha initially and downgrades with an alpha feature enabled are not required to work. It is worth calling out in release notes, though.
1 parent 5f3bed2 commit cd2b883

File tree

4 files changed

+140
-17
lines changed

4 files changed

+140
-17
lines changed

‎fieldpath/set_test.go‎

Lines changed: 101 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -697,6 +697,58 @@ var nestedSchema = func() (*typed.Parser, string) {
697697
return parser, name
698698
}
699699

700+
var associativeListSchema = func() (*typed.Parser, string) {
701+
name := "type"
702+
parser := mustParse(`types:
703+
- name: type
704+
map:
705+
fields:
706+
- name: values
707+
type:
708+
list:
709+
elementRelationship: associative
710+
keys: ["keyAStr", "keyBInt"]
711+
elementType:
712+
map:
713+
fields:
714+
- name: keyAStr
715+
type:
716+
scalar: string
717+
- name: keyBInt
718+
type:
719+
scalar: numeric
720+
- name: value
721+
type:
722+
scalar: numeric
723+
`)
724+
return parser, name
725+
}
726+
727+
var oldAssociativeListSchema = func() (*typed.Parser, string) {
728+
name := "type"
729+
// No keyBInt yet!
730+
parser := mustParse(`types:
731+
- name: type
732+
map:
733+
fields:
734+
- name: values
735+
type:
736+
list:
737+
elementRelationship: associative
738+
keys: ["keyAStr"]
739+
elementType:
740+
map:
741+
fields:
742+
- name: keyAStr
743+
type:
744+
scalar: string
745+
- name: value
746+
type:
747+
scalar: numeric
748+
`)
749+
return parser, name
750+
}
751+
700752
func mustParse(schema typed.YAMLObject) *typed.Parser {
701753
parser, err := typed.NewParser(schema)
702754
if err != nil {
@@ -707,12 +759,15 @@ func mustParse(schema typed.YAMLObject) *typed.Parser {
707759

708760
func TestEnsureNamedFieldsAreMembers(t *testing.T) {
709761
table := []struct {
762+
schemaFn func() (*typed.Parser, string)
763+
newSchemaFn func() (*typed.Parser, string)
710764
value typed.YAMLObject
711765
expectedBefore *Set
712766
expectedAfter *Set
713767
}{
714768
{
715-
value: `{"named": {"named": {"value": 0}}}`,
769+
schemaFn: nestedSchema,
770+
value: `{"named": {"named": {"value": 0}}}`,
716771
expectedBefore: NewSet(
717772
_P("named", "named", "value"),
718773
),
@@ -723,7 +778,8 @@ func TestEnsureNamedFieldsAreMembers(t *testing.T) {
723778
),
724779
},
725780
{
726-
value: `{"named": {"a": {"named": {"value": 42}}}, "a": {"named": {"value": 1}}}`,
781+
schemaFn: nestedSchema,
782+
value: `{"named": {"a": {"named": {"value": 42}}}, "a": {"named": {"value": 1}}}`,
727783
expectedBefore: NewSet(
728784
_P("named", "a", "named", "value"),
729785
_P("a", "named", "value"),
@@ -739,7 +795,8 @@ func TestEnsureNamedFieldsAreMembers(t *testing.T) {
739795
),
740796
},
741797
{
742-
value: `{"named": {"list": [{"keyAStr": "a", "keyBInt": 1, "named": {"value": 0}}]}}`,
798+
schemaFn: nestedSchema,
799+
value: `{"named": {"list": [{"keyAStr": "a", "keyBInt": 1, "named": {"value": 0}}]}}`,
743800
expectedBefore: NewSet(
744801
_P("named", "list", KeyByFields("keyAStr", "a", "keyBInt", 1), "keyAStr"),
745802
_P("named", "list", KeyByFields("keyAStr", "a", "keyBInt", 1), "keyBInt"),
@@ -756,11 +813,46 @@ func TestEnsureNamedFieldsAreMembers(t *testing.T) {
756813
_P("named"),
757814
),
758815
},
816+
{
817+
// Generate the value using the old schema to get missing key entries,
818+
// then process with new schema which has keyBInt.
819+
schemaFn: oldAssociativeListSchema,
820+
newSchemaFn: associativeListSchema,
821+
value: `{"values": [{"keyAStr": "a", "value": 0}]}`,
822+
expectedBefore: NewSet(
823+
_P("values", KeyByFields("keyAStr", "a"), "keyAStr"),
824+
_P("values", KeyByFields("keyAStr", "a"), "value"),
825+
_P("values", KeyByFields("keyAStr", "a")),
826+
),
827+
expectedAfter: NewSet(
828+
_P("values", KeyByFields("keyAStr", "a"), "keyAStr"),
829+
_P("values", KeyByFields("keyAStr", "a"), "value"),
830+
_P("values", KeyByFields("keyAStr", "a")),
831+
_P("values"),
832+
),
833+
},
834+
{
835+
// Check that converting the value with the missing key and
836+
// the recent schema doesn't add the missing key.
837+
schemaFn: associativeListSchema,
838+
value: `{"values": [{"keyAStr": "a", "value": 1}]}`,
839+
expectedBefore: NewSet(
840+
_P("values", KeyByFields("keyAStr", "a"), "keyAStr"),
841+
_P("values", KeyByFields("keyAStr", "a"), "value"),
842+
_P("values", KeyByFields("keyAStr", "a")),
843+
),
844+
expectedAfter: NewSet(
845+
_P("values", KeyByFields("keyAStr", "a"), "keyAStr"),
846+
_P("values", KeyByFields("keyAStr", "a"), "value"),
847+
_P("values", KeyByFields("keyAStr", "a")),
848+
_P("values"),
849+
),
850+
},
759851
}
760852

761853
for _, test := range table {
762854
t.Run(string(test.value), func(t *testing.T) {
763-
parser, typeName := nestedSchema()
855+
parser, typeName := test.schemaFn()
764856
typeRef := schema.TypeRef{NamedType: &typeName}
765857
typedValue, err := parser.Type(typeName).FromYAML(test.value)
766858
if err != nil {
@@ -780,6 +872,11 @@ func TestEnsureNamedFieldsAreMembers(t *testing.T) {
780872
}
781873

782874
schema := &parser.Schema
875+
if test.newSchemaFn != nil {
876+
newParser, _ := test.newSchemaFn()
877+
schema = &newParser.Schema
878+
}
879+
783880
got := set.EnsureNamedFieldsAreMembers(schema, typeRef)
784881
if !got.Equals(test.expectedAfter) {
785882
t.Errorf("expected after EnsureNamedFieldsAreMembers:\n%v\n\ngot:\n%v\n\nmissing:\n%v\n\nsuperfluous:\n\n%v",

‎merge/default_keys_test.go‎

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,36 @@ func TestDefaultKeysFlat(t *testing.T) {
163163
),
164164
},
165165
},
166+
"apply_missing_undefaulted_defaulted_key": {
167+
Ops: []Operation{
168+
Apply{
169+
Manager: "default",
170+
APIVersion: "v1",
171+
Object: `
172+
containerPorts:
173+
- protocol: TCP
174+
name: A
175+
`,
176+
},
177+
},
178+
APIVersion: "v1",
179+
Object: `
180+
containerPorts:
181+
- protocol: TCP
182+
name: A
183+
`,
184+
Managed: fieldpath.ManagedFields{
185+
"default": fieldpath.NewVersionedSet(
186+
_NS(
187+
_P("containerPorts", _KBF("protocol", "TCP")),
188+
_P("containerPorts", _KBF("protocol", "TCP"), "name"),
189+
_P("containerPorts", _KBF("protocol", "TCP"), "protocol"),
190+
),
191+
"v1",
192+
true,
193+
),
194+
},
195+
},
166196
}
167197

168198
for name, test := range tests {
@@ -176,18 +206,6 @@ func TestDefaultKeysFlat(t *testing.T) {
176206

177207
func TestDefaultKeysFlatErrors(t *testing.T) {
178208
tests := map[string]TestCase{
179-
"apply_missing_undefaulted_defaulted_key": {
180-
Ops: []Operation{
181-
Apply{
182-
Manager: "default",
183-
APIVersion: "v1",
184-
Object: `
185-
containerPorts:
186-
- protocol: TCP
187-
`,
188-
},
189-
},
190-
},
191209
"apply_missing_defaulted_key_ambiguous_A": {
192210
Ops: []Operation{
193211
Apply{

‎typed/helpers.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,9 +217,16 @@ func keyedAssociativeListItemToPathElement(a value.Allocator, s *schema.Schema,
217217
} else if def != nil {
218218
keyMap = append(keyMap, value.Field{Name: fieldName, Value: value.NewValueInterface(def)})
219219
} else {
220-
return pe, fmt.Errorf("associative list with keys has an element that omits key field %q (and doesn't have default value)", fieldName)
220+
// Don't add the key to the key field list.
221+
// A key field list where it is set then represents a different entry
222+
// in the associate list.
221223
}
222224
}
225+
226+
if len(list.Keys) > 0 && len(keyMap) == 0 {
227+
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)
228+
}
229+
223230
keyMap.Sort()
224231
pe.Key = &keyMap
225232
return pe, nil

‎typed/validate_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,7 @@ var validationCases = []validationTestCase{{
213213
`{"list":[{"key":"a","id":1,"value":{"a":"a"}}]}`,
214214
`{"list":[{"key":"a","id":1},{"key":"a","id":2},{"key":"b","id":1}]}`,
215215
`{"atomicList":["a","a","a"]}`,
216+
`{"list":[{"key":"x","value":{"a":"a"},"bv":true,"nv":3.14}]}`,
216217
},
217218
invalidObjects: []typed.YAMLObject{
218219
`{"key":true,"value":1}`,

0 commit comments

Comments
 (0)