Merge pull request #298 from jpbetz/test-key-changes
Add associative list key schema evolution changes
diff --git a/fieldpath/set_test.go b/fieldpath/set_test.go
index 0254e2d..0685bc4 100644
--- a/fieldpath/set_test.go
+++ b/fieldpath/set_test.go
@@ -697,6 +697,58 @@
return parser, name
}
+var associativeListSchema = func() (*typed.Parser, string) {
+ name := "type"
+ parser := mustParse(`types:
+- name: type
+ map:
+ fields:
+ - name: values
+ type:
+ list:
+ elementRelationship: associative
+ keys: ["keyAStr", "keyBInt"]
+ elementType:
+ map:
+ fields:
+ - name: keyAStr
+ type:
+ scalar: string
+ - name: keyBInt
+ type:
+ scalar: numeric
+ - name: value
+ type:
+ scalar: numeric
+`)
+ return parser, name
+}
+
+var oldAssociativeListSchema = func() (*typed.Parser, string) {
+ name := "type"
+ // No keyBInt yet!
+ parser := mustParse(`types:
+- name: type
+ map:
+ fields:
+ - name: values
+ type:
+ list:
+ elementRelationship: associative
+ keys: ["keyAStr"]
+ elementType:
+ map:
+ fields:
+ - name: keyAStr
+ type:
+ scalar: string
+ - name: value
+ type:
+ scalar: numeric
+`)
+ return parser, name
+}
+
func mustParse(schema typed.YAMLObject) *typed.Parser {
parser, err := typed.NewParser(schema)
if err != nil {
@@ -707,12 +759,15 @@
func TestEnsureNamedFieldsAreMembers(t *testing.T) {
table := []struct {
+ schemaFn func() (*typed.Parser, string)
+ newSchemaFn func() (*typed.Parser, string)
value typed.YAMLObject
expectedBefore *Set
expectedAfter *Set
}{
{
- value: `{"named": {"named": {"value": 0}}}`,
+ schemaFn: nestedSchema,
+ value: `{"named": {"named": {"value": 0}}}`,
expectedBefore: NewSet(
_P("named", "named", "value"),
),
@@ -723,7 +778,8 @@
),
},
{
- value: `{"named": {"a": {"named": {"value": 42}}}, "a": {"named": {"value": 1}}}`,
+ schemaFn: nestedSchema,
+ value: `{"named": {"a": {"named": {"value": 42}}}, "a": {"named": {"value": 1}}}`,
expectedBefore: NewSet(
_P("named", "a", "named", "value"),
_P("a", "named", "value"),
@@ -739,7 +795,8 @@
),
},
{
- value: `{"named": {"list": [{"keyAStr": "a", "keyBInt": 1, "named": {"value": 0}}]}}`,
+ schemaFn: nestedSchema,
+ value: `{"named": {"list": [{"keyAStr": "a", "keyBInt": 1, "named": {"value": 0}}]}}`,
expectedBefore: NewSet(
_P("named", "list", KeyByFields("keyAStr", "a", "keyBInt", 1), "keyAStr"),
_P("named", "list", KeyByFields("keyAStr", "a", "keyBInt", 1), "keyBInt"),
@@ -756,11 +813,46 @@
_P("named"),
),
},
+ {
+ // Generate the value using the old schema to get missing key entries,
+ // then process with new schema which has keyBInt.
+ schemaFn: oldAssociativeListSchema,
+ newSchemaFn: associativeListSchema,
+ value: `{"values": [{"keyAStr": "a", "value": 0}]}`,
+ expectedBefore: NewSet(
+ _P("values", KeyByFields("keyAStr", "a"), "keyAStr"),
+ _P("values", KeyByFields("keyAStr", "a"), "value"),
+ _P("values", KeyByFields("keyAStr", "a")),
+ ),
+ expectedAfter: NewSet(
+ _P("values", KeyByFields("keyAStr", "a"), "keyAStr"),
+ _P("values", KeyByFields("keyAStr", "a"), "value"),
+ _P("values", KeyByFields("keyAStr", "a")),
+ _P("values"),
+ ),
+ },
+ {
+ // Check that converting the value with the missing key and
+ // the recent schema doesn't add the missing key.
+ schemaFn: associativeListSchema,
+ value: `{"values": [{"keyAStr": "a", "value": 1}]}`,
+ expectedBefore: NewSet(
+ _P("values", KeyByFields("keyAStr", "a"), "keyAStr"),
+ _P("values", KeyByFields("keyAStr", "a"), "value"),
+ _P("values", KeyByFields("keyAStr", "a")),
+ ),
+ expectedAfter: NewSet(
+ _P("values", KeyByFields("keyAStr", "a"), "keyAStr"),
+ _P("values", KeyByFields("keyAStr", "a"), "value"),
+ _P("values", KeyByFields("keyAStr", "a")),
+ _P("values"),
+ ),
+ },
}
for _, test := range table {
t.Run(string(test.value), func(t *testing.T) {
- parser, typeName := nestedSchema()
+ parser, typeName := test.schemaFn()
typeRef := schema.TypeRef{NamedType: &typeName}
typedValue, err := parser.Type(typeName).FromYAML(test.value)
if err != nil {
@@ -780,6 +872,11 @@
}
schema := &parser.Schema
+ if test.newSchemaFn != nil {
+ newParser, _ := test.newSchemaFn()
+ schema = &newParser.Schema
+ }
+
got := set.EnsureNamedFieldsAreMembers(schema, typeRef)
if !got.Equals(test.expectedAfter) {
t.Errorf("expected after EnsureNamedFieldsAreMembers:\n%v\n\ngot:\n%v\n\nmissing:\n%v\n\nsuperfluous:\n\n%v",
diff --git a/merge/default_keys_test.go b/merge/default_keys_test.go
index 24b202d..28f8103 100644
--- a/merge/default_keys_test.go
+++ b/merge/default_keys_test.go
@@ -163,6 +163,36 @@
),
},
},
+ "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 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/schema_change_test.go b/merge/schema_change_test.go
index d733407..db78ff5 100644
--- a/merge/schema_change_test.go
+++ b/merge/schema_change_test.go
@@ -252,3 +252,459 @@
})
}
}
+
+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/typed/helpers.go b/typed/helpers.go
index f10ac6e..8a9c0b5 100644
--- a/typed/helpers.go
+++ b/typed/helpers.go
@@ -217,9 +217,16 @@
} else if def != nil {
keyMap = append(keyMap, value.Field{Name: fieldName, Value: value.NewValueInterface(def)})
} else {
- return pe, fmt.Errorf("associative list with keys has an element that omits key field %q (and doesn't have default value)", fieldName)
+ // Don't add the key to the key field list.
+ // A key field list where it is set then represents a different entry
+ // in the associate list.
}
}
+
+ if len(list.Keys) > 0 && len(keyMap) == 0 {
+ return pe, fmt.Errorf("associative list with keys has an element that omits all key fields %q (and doesn't have default values for any key fields)", list.Keys)
+ }
+
keyMap.Sort()
pe.Key = &keyMap
return pe, nil
diff --git a/typed/validate_test.go b/typed/validate_test.go
index b4c177a..92ba141 100644
--- a/typed/validate_test.go
+++ b/typed/validate_test.go
@@ -213,6 +213,7 @@
`{"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}`,
