diff --git a/staging/src/k8s.io/apimachinery/pkg/api/validate/each.go b/staging/src/k8s.io/apimachinery/pkg/api/validate/each.go index 70bc5100fc4..89994c8f07e 100644 --- a/staging/src/k8s.io/apimachinery/pkg/api/validate/each.go +++ b/staging/src/k8s.io/apimachinery/pkg/api/validate/each.go @@ -18,25 +18,45 @@ package validate import ( "context" + "sort" + "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/operation" "k8s.io/apimachinery/pkg/util/validation/field" ) -// CompareFunc is a function that compares two values of the same type. -type CompareFunc[T any] func(T, T) bool +// MatchFunc is a function that compares two values of the same type, +// according to some criteria, and returns true if they match. +type MatchFunc[T any] func(T, T) bool -// EachSliceVal validates each element of newSlice with the specified -// validation function. The comparison function is used to find the -// corresponding value in oldSlice. The value-type of the slices is assumed to -// not be nilable. +// EachSliceVal performs validation on each element of newSlice using the provided validation function. +// +// For update operations, the match function finds corresponding values in oldSlice for each +// value in newSlice. This comparison can be either full or partial (e.g., matching only +// specific struct fields that serve as a unique identifier). If match is nil, validation +// proceeds without considering old values, and the equiv function is not used. +// +// For update operations, the equiv function checks if a new value is equivalent to its +// corresponding old value, enabling validation ratcheting. If equiv is nil but match is +// provided, the match function is assumed to perform full value comparison. +// +// Note: The slice element type must be non-nilable. func EachSliceVal[T any](ctx context.Context, op operation.Operation, fldPath *field.Path, newSlice, oldSlice []T, - cmp CompareFunc[T], validator ValidateFunc[*T]) field.ErrorList { + match, equiv MatchFunc[T], validator ValidateFunc[*T]) field.ErrorList { var errs field.ErrorList for i, val := range newSlice { var old *T - if cmp != nil && len(oldSlice) > 0 { - old = lookup(oldSlice, val, cmp) + if match != nil && len(oldSlice) > 0 { + old = lookup(oldSlice, val, match) + } + // If the operation is an update, for validation ratcheting, skip re-validating if the old + // value exists and either: + // 1. The match function provides full comparison (equiv is nil) + // 2. The equiv function confirms the values are equivalent (either directly or semantically) + // + // The equiv function provides equality comparison when match uses partial comparison. + if op.Type == operation.Update && old != nil && (equiv == nil || equiv(val, *old)) { + continue } errs = append(errs, validator(ctx, op, fldPath.Index(i), &val, old)...) } @@ -45,39 +65,107 @@ func EachSliceVal[T any](ctx context.Context, op operation.Operation, fldPath *f // lookup returns a pointer to the first element in the list that matches the // target, according to the provided comparison function, or else nil. -func lookup[T any](list []T, target T, cmp func(T, T) bool) *T { +func lookup[T any](list []T, target T, match MatchFunc[T]) *T { for i := range list { - if cmp(list[i], target) { + if match(list[i], target) { return &list[i] } } return nil } -// EachMapVal validates each element of newMap with the specified validation -// function and, if the corresponding key is found in oldMap, the old value. -// The value-type of the slices is assumed to not be nilable. +// EachMapVal validates each value in newMap using the specified validation +// function, passing the corresponding old value from oldMap if the key exists in oldMap. +// For update operations, it implements validation ratcheting by skipping validation +// when the old value exists and the equiv function confirms the values are equivalent. +// The value-type of the map is assumed to not be nilable. +// If equiv is nil, value-based ratcheting is disabled and all values will be validated. func EachMapVal[K ~string, V any](ctx context.Context, op operation.Operation, fldPath *field.Path, newMap, oldMap map[K]V, - validator ValidateFunc[*V]) field.ErrorList { + equiv MatchFunc[V], validator ValidateFunc[*V]) field.ErrorList { var errs field.ErrorList for key, val := range newMap { var old *V if o, found := oldMap[key]; found { old = &o } + // If the operation is an update, for validation ratcheting, skip re-validating if the old + // value is found and the equiv function confirms the values are equivalent. + if op.Type == operation.Update && old != nil && equiv != nil && equiv(val, *old) { + continue + } errs = append(errs, validator(ctx, op, fldPath.Key(string(key)), &val, old)...) } return errs } // EachMapKey validates each element of newMap with the specified -// validation function. The oldMap argument is not used. +// validation function. func EachMapKey[K ~string, T any](ctx context.Context, op operation.Operation, fldPath *field.Path, newMap, oldMap map[K]T, validator ValidateFunc[*K]) field.ErrorList { var errs field.ErrorList for key := range newMap { + var old *K + if _, found := oldMap[key]; found { + old = &key + } + // If the operation is an update, for validation ratcheting, skip re-validating if + // the key is found in oldMap. + if op.Type == operation.Update && old != nil { + continue + } // Note: the field path is the field, not the key. errs = append(errs, validator(ctx, op, fldPath, &key, nil)...) } return errs } + +// Unique verifies that each element of newSlice is unique, according to the +// match function. It compares every element of the slice with every other +// element and returns errors for non-unique items. +func Unique[T any](_ context.Context, _ operation.Operation, fldPath *field.Path, newSlice, _ []T, match MatchFunc[T]) field.ErrorList { + var dups []int + for i, val := range newSlice { + for j := i + 1; j < len(newSlice); j++ { + other := newSlice[j] + if match(val, other) { + if dups == nil { + dups = make([]int, 0, len(newSlice)) + } + if lookup(dups, j, func(a, b int) bool { return a == b }) == nil { + dups = append(dups, j) + } + } + } + } + + var errs field.ErrorList + sort.Ints(dups) + for _, i := range dups { + var val any = newSlice[i] + // TODO: we don't want the whole item to be logged in the error, just + // the key(s). Unfortunately, the way errors are rendered, it comes out + // as something like "map[string]any{...}" which is not very nice. Once + // that is fixed, we can consider adding a way for this function to + // specify that just the keys should be renderted in the error. + errs = append(errs, field.Duplicate(fldPath.Index(i), val)) + } + return errs +} + +// SemanticDeepEqual is a MatchFunc that uses equality.Semantic.DeepEqual to +// compare two values. +// This wrapper is needed because MatchFunc requires a function that takes two +// arguments of specific type T, while equality.Semantic.DeepEqual takes +// arguments of type interface{}/any. The wrapper satisfies the type +// constraints of MatchFunc while leveraging the underlying semantic equality +// logic. It can be used by any other function that needs to call DeepEqual. +func SemanticDeepEqual[T any](a, b T) bool { + return equality.Semantic.DeepEqual(a, b) +} + +// DirectEqual is a MatchFunc that uses the == operator to compare two values. +// It can be used by any other function that needs to compare two values +// directly. +func DirectEqual[T comparable](a, b T) bool { + return a == b +} diff --git a/staging/src/k8s.io/apimachinery/pkg/api/validate/each_test.go b/staging/src/k8s.io/apimachinery/pkg/api/validate/each_test.go index 2691f4c9d07..56f949d8474 100644 --- a/staging/src/k8s.io/apimachinery/pkg/api/validate/each_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/api/validate/each_test.go @@ -25,6 +25,7 @@ import ( "k8s.io/apimachinery/pkg/api/operation" "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/utils/ptr" ) type TestStruct struct { @@ -32,6 +33,32 @@ type TestStruct struct { D string } +type TestStructWithKey struct { + Key string + I int + D string +} + +type NonComparableKey struct { + I *int +} + +type NonComparableStruct struct { + I int + S []string +} + +type NonComparableStructWithKey struct { + Key string + I int + S []string +} + +type NonComparableStructWithPtr struct { + I int + P *int +} + func TestEachSliceVal(t *testing.T) { testEachSliceVal(t, "valid", []int{11, 12, 13}) testEachSliceVal(t, "valid", []string{"a", "b", "c"}) @@ -70,7 +97,7 @@ func testEachSliceVal[T any](t *testing.T, name string, input []T) { calls++ return nil } - _ = EachSliceVal(context.Background(), operation.Operation{}, field.NewPath("test"), input, nil, nil, vfn) + _ = EachSliceVal(context.Background(), operation.Operation{}, field.NewPath("test"), input, nil, nil, nil, vfn) if calls != len(input) { t.Errorf("expected %d calls, got %d", len(input), calls) } @@ -95,14 +122,98 @@ func testEachSliceValUpdate[T any](t *testing.T, name string, input []T) { old := make([]T, len(input)) copy(old, input) slices.Reverse(old) - cmp := func(a, b T) bool { return reflect.DeepEqual(a, b) } - _ = EachSliceVal(context.Background(), operation.Operation{}, field.NewPath("test"), input, old, cmp, vfn) + match := func(a, b T) bool { return reflect.DeepEqual(a, b) } + _ = EachSliceVal(context.Background(), operation.Operation{}, field.NewPath("test"), input, old, match, match, vfn) if calls != len(input) { t.Errorf("expected %d calls, got %d", len(input), calls) } }) } +func TestEachSliceValRatcheting(t *testing.T) { + testEachSliceValRatcheting(t, "ComparableStruct same data different order", + []TestStruct{ + {11, "a"}, {12, "b"}, {13, "c"}, + }, + []TestStruct{ + {11, "a"}, {13, "c"}, {12, "b"}, + }, + SemanticDeepEqual, + nil, + ) + testEachSliceValRatcheting(t, "ComparableStruct less data in new, exist in old", + []TestStruct{ + {11, "a"}, {12, "b"}, {13, "c"}, + }, + []TestStruct{ + {11, "a"}, {13, "c"}, + }, + DirectEqual, + nil, + ) + testEachSliceValRatcheting(t, "Comparable struct with key same data different order", + []TestStructWithKey{ + {Key: "a", I: 11, D: "a"}, {Key: "b", I: 12, D: "b"}, {Key: "c", I: 13, D: "c"}, + }, + []TestStructWithKey{ + {Key: "a", I: 11, D: "a"}, {Key: "c", I: 13, D: "c"}, {Key: "b", I: 12, D: "b"}, + }, + MatchFunc[TestStructWithKey](func(a, b TestStructWithKey) bool { + return a.Key == b.Key + }), + DirectEqual, + ) + testEachSliceValRatcheting(t, "Comparable struct with key less data in new, exist in old", + []TestStructWithKey{ + {Key: "a", I: 11, D: "a"}, {Key: "b", I: 12, D: "b"}, {Key: "c", I: 13, D: "c"}, + }, + []TestStructWithKey{ + {Key: "a", I: 11, D: "a"}, {Key: "c", I: 13, D: "c"}, + }, + MatchFunc[TestStructWithKey](func(a, b TestStructWithKey) bool { + return a.Key == b.Key + }), + DirectEqual, + ) + testEachSliceValRatcheting(t, "NonComparableStruct same data different order", + []NonComparableStruct{ + {I: 11, S: []string{"a"}}, {I: 12, S: []string{"b"}}, {I: 13, S: []string{"c"}}, + }, + []NonComparableStruct{ + {I: 11, S: []string{"a"}}, {I: 13, S: []string{"c"}}, {I: 12, S: []string{"b"}}, + }, + SemanticDeepEqual, + nil, + ) + testEachSliceValRatcheting(t, "NonComparableStructWithKey same data different order", + []NonComparableStructWithKey{ + {Key: "a", I: 11, S: []string{"a"}}, {Key: "b", I: 12, S: []string{"b"}}, {Key: "c", I: 13, S: []string{"c"}}, + }, + []NonComparableStructWithKey{ + {Key: "a", I: 11, S: []string{"a"}}, {Key: "b", I: 12, S: []string{"b"}}, {Key: "c", I: 13, S: []string{"c"}}, + }, + MatchFunc[NonComparableStructWithKey](func(a, b NonComparableStructWithKey) bool { + return a.Key == b.Key + }), + SemanticDeepEqual, + ) + +} + +func testEachSliceValRatcheting[T any](t *testing.T, name string, old, new []T, match, equiv MatchFunc[T]) { + t.Helper() + var zero T + t.Run(fmt.Sprintf("%s(%T)", name, zero), func(t *testing.T) { + vfn := func(ctx context.Context, op operation.Operation, fldPath *field.Path, newVal, oldVal *T) field.ErrorList { + return field.ErrorList{field.Invalid(fldPath, *newVal, "expected no calls")} + } + errs := EachSliceVal(context.Background(), operation.Operation{Type: operation.Update}, field.NewPath("test"), new, old, match, equiv, vfn) + if len(errs) > 0 { + t.Errorf("expected no errors, got %d: %s", len(errs), fmtErrs(errs)) + } + }) +} + func TestEachMapVal(t *testing.T) { testEachMapVal(t, "valid", map[string]int{"one": 11, "two": 12, "three": 13}) testEachMapVal(t, "valid", map[string]string{"A": "a", "B": "b", "C": "c"}) @@ -129,13 +240,131 @@ func testEachMapVal[T any](t *testing.T, name string, input map[string]T) { calls++ return nil } - _ = EachMapVal(context.Background(), operation.Operation{}, field.NewPath("test"), input, nil, vfn) + _ = EachMapVal(context.Background(), operation.Operation{}, field.NewPath("test"), input, nil, nil, vfn) if calls != len(input) { t.Errorf("expected %d calls, got %d", len(input), calls) } }) } +func TestEachMapValRatcheting(t *testing.T) { + testEachMapValRatcheting(t, "primitive same data", + map[string]int{"one": 11, "two": 12, "three": 13}, + map[string]int{"one": 11, "three": 13, "two": 12}, + DirectEqual, + 0, + ) + testEachMapValRatcheting(t, "primitive less data in new, exist in old", + map[string]int{"one": 11, "two": 12, "three": 13}, + map[string]int{"one": 11, "three": 13}, + DirectEqual, + 0, + ) + testEachMapValRatcheting(t, "primitive new data, not exist in old", + map[string]int{"one": 11, "two": 12, "three": 13}, + map[string]int{"one": 11, "three": 13, "two": 12, "four": 14}, + DirectEqual, + 1, + ) + testEachMapValRatcheting(t, "non comparable value, same data", + map[string]NonComparableStruct{ + "one": {I: 11, S: []string{"a"}}, + "two": {I: 12, S: []string{"b"}}, + "three": {I: 13, S: []string{"c"}}, + }, + map[string]NonComparableStruct{ + "one": {I: 11, S: []string{"a"}}, + "three": {I: 13, S: []string{"c"}}, + "two": {I: 12, S: []string{"b"}}, + }, + SemanticDeepEqual, + 0, + ) + testEachMapValRatcheting(t, "non comparable value, less data in new, exist in old", + map[string]NonComparableStruct{ + "one": {I: 11, S: []string{"a"}}, + "two": {I: 12, S: []string{"b"}}, + "three": {I: 13, S: []string{"c"}}, + }, + map[string]NonComparableStruct{ + "one": {I: 11, S: []string{"a"}}, + "three": {I: 13, S: []string{"c"}}, + }, + SemanticDeepEqual, + 0, + ) + testEachMapValRatcheting(t, "non comparable value, new data, not exist in old", + map[string]NonComparableStruct{ + "one": {I: 11, S: []string{"a"}}, + "two": {I: 12, S: []string{"b"}}, + "three": {I: 13, S: []string{"c"}}, + }, + map[string]NonComparableStruct{ + "one": {I: 11, S: []string{"a"}}, + "three": {I: 13, S: []string{"c"}}, + "two": {I: 12, S: []string{"b"}}, + "four": {I: 14, S: []string{"d"}}, + }, + SemanticDeepEqual, + 1, + ) + testEachMapValRatcheting(t, "struct with pointer field, same value different pointer", + map[string]NonComparableStructWithPtr{ + "one": {I: 11, P: ptr.To(1)}, + "two": {I: 12, P: ptr.To(2)}, + }, + map[string]NonComparableStructWithPtr{ + "one": {I: 11, P: ptr.To(1)}, + "two": {I: 12, P: ptr.To(2)}, + }, + SemanticDeepEqual, + 0, + ) + testEachMapValRatcheting(t, "nil map to empty map", + nil, + map[string]int{}, + DirectEqual, + 0, + ) + + testEachMapValRatcheting(t, "nil map to non-empty map", + nil, + map[string]int{"one": 1}, + DirectEqual, + 1, // Expect validation for new entry + ) + + testEachMapValRatcheting(t, "empty map to nil map", + map[string]int{}, + nil, + DirectEqual, + 0, + ) + + testEachMapValRatcheting(t, "non-empty map to nil map", + map[string]int{"one": 1}, + nil, + DirectEqual, + 0, + ) +} + +func testEachMapValRatcheting[K ~string, V any](t *testing.T, name string, old, new map[K]V, equiv MatchFunc[V], wantCalls int) { + t.Helper() + var zero V + t.Run(fmt.Sprintf("%s(%T)", name, zero), func(t *testing.T) { + calls := 0 + vfn := func(ctx context.Context, op operation.Operation, fldPath *field.Path, newVal, oldVal *V) field.ErrorList { + calls++ + return nil + } + _ = EachMapVal(context.Background(), operation.Operation{Type: operation.Update}, field.NewPath("test"), new, old, equiv, vfn) + if calls != wantCalls { + t.Errorf("expected %d calls, got %d", wantCalls, calls) + } + }) +} + type StringType string func TestEachMapKey(t *testing.T) { @@ -161,3 +390,103 @@ func testEachMapKey[K ~string, V any](t *testing.T, name string, input map[K]V) } }) } + +func TestEachMapKeyRatcheting(t *testing.T) { + testEachMapKeyRatcheting(t, "same data, 0 validation calls", + map[string]int{"one": 11, "two": 12, "three": 13}, + map[string]int{"one": 11, "three": 13, "two": 12}, + 0, + ) + testEachMapKeyRatcheting(t, "less data in new, exist in old, 0 validation calls", + map[string]int{"one": 11, "two": 12, "three": 13}, + map[string]int{"one": 11, "three": 13}, + 0, + ) + testEachMapKeyRatcheting(t, "new data, not exist in old, 1 validation call", + map[string]int{"one": 11, "two": 12, "three": 13}, + map[string]int{"one": 11, "three": 13, "two": 12, "four": 14}, + 1, + ) +} + +func testEachMapKeyRatcheting[K ~string, V any](t *testing.T, name string, old, new map[K]V, wantCalls int) { + t.Helper() + var zero V + t.Run(fmt.Sprintf("%s(%T)", name, zero), func(t *testing.T) { + calls := 0 + vfn := func(ctx context.Context, op operation.Operation, fldPath *field.Path, newVal, oldVal *K) field.ErrorList { + calls++ + return nil + } + _ = EachMapKey(context.Background(), operation.Operation{Type: operation.Update}, field.NewPath("test"), new, old, vfn) + if calls != wantCalls { + t.Errorf("expected %d calls, got %d", wantCalls, calls) + } + }) +} + +func TestUniqueComparableValues(t *testing.T) { + testUnique(t, "int_nil", []int(nil), 0) + testUnique(t, "int_empty", []int{}, 0) + testUnique(t, "int_uniq", []int{1, 2, 3}, 0) + testUnique(t, "int_dup", []int{1, 2, 3, 2, 1}, 2) + + testUnique(t, "string_nil", []string(nil), 0) + testUnique(t, "string_empty", []string{}, 0) + testUnique(t, "string_uniq", []string{"a", "b", "c"}, 0) + testUnique(t, "string_dup", []string{"a", "a", "c", "b", "a"}, 2) + + type isComparable struct { + I int + S string + } + + testUnique(t, "struct_nil", []isComparable(nil), 0) + testUnique(t, "struct_empty", []isComparable{}, 0) + testUnique(t, "struct_uniq", []isComparable{{1, "a"}, {2, "b"}, {3, "c"}}, 0) + testUnique(t, "struct_dup", []isComparable{{1, "a"}, {2, "b"}, {3, "c"}, {2, "b"}, {1, "a"}}, 2) +} + +func testUnique[T comparable](t *testing.T, name string, input []T, wantErrs int) { + t.Helper() + t.Run(fmt.Sprintf("%s(direct)", name), func(t *testing.T) { + errs := Unique(context.Background(), operation.Operation{}, field.NewPath("test"), input, nil, DirectEqual) + if len(errs) != wantErrs { + t.Errorf("expected %d errors, got %d: %s", wantErrs, len(errs), fmtErrs(errs)) + } + }) + t.Run(fmt.Sprintf("%s(reflect)", name), func(t *testing.T) { + errs := Unique(context.Background(), operation.Operation{}, field.NewPath("test"), input, nil, SemanticDeepEqual) + if len(errs) != wantErrs { + t.Errorf("expected %d errors, got %d: %s", wantErrs, len(errs), fmtErrs(errs)) + } + }) +} + +func TestUniqueNonComparableValues(t *testing.T) { + type nonComparable struct { + I int + S []string + } + + testUniqueByReflect(t, "noncomp_nil", []nonComparable(nil), 0) + testUniqueByReflect(t, "noncomp_empty", []nonComparable{}, 0) + testUniqueByReflect(t, "noncomp_uniq", []nonComparable{{1, []string{"a"}}, {2, []string{"b"}}, {3, []string{"c"}}}, 0) + testUniqueByReflect(t, "noncomp_dup", []nonComparable{ + {1, []string{"a"}}, + {2, []string{"b"}}, + {3, []string{"c"}}, + {2, []string{"b"}}, + {1, []string{"a"}}}, 2) +} + +func testUniqueByReflect[T any](t *testing.T, name string, input []T, wantErrs int) { + t.Helper() + var zero T + t.Run(fmt.Sprintf("%s(%T)", name, zero), func(t *testing.T) { + errs := Unique(context.Background(), operation.Operation{}, field.NewPath("test"), input, nil, SemanticDeepEqual) + if len(errs) != wantErrs { + t.Errorf("expected %d errors, got %d: %s", wantErrs, len(errs), fmtErrs(errs)) + } + }) +} diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/each.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/each.go index d17e574ddc0..f8fc1e76bc2 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/each.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/each.go @@ -43,29 +43,85 @@ var globalEachKey *eachKeyTagValidator func init() { // Lists with list-map semantics are comprised of multiple tags, which need - // to share information between them. - shared := map[string]*listMap{} // keyed by the fieldpath - RegisterTagValidator(listTypeTagValidator{shared}) - RegisterTagValidator(listMapKeyTagValidator{shared}) + // to share metadata about the list between them. + listMeta := map[string]*listMetadata{} // keyed by the field or type path - globalEachVal = &eachValTagValidator{shared, nil} + // Accumulate list metadata via tags. + RegisterTagValidator(listTypeTagValidator{byPath: listMeta}) + RegisterTagValidator(listMapKeyTagValidator{byPath: listMeta}) + + // Finish work on the accumulated list metadata. + RegisterFieldValidator(listValidator{byPath: listMeta}) + RegisterTypeValidator(listValidator{byPath: listMeta}) + + // List-map item validator uses shared listType and listMapKey information + itemMeta := make(map[string]*itemMetadata) // keyed by the fieldpath + + // Accumulate item metadata via tags. + RegisterTagValidator(&itemTagValidator{byPath: itemMeta}) + + // Finish work on the accumulated item metadata. + RegisterTypeValidator(&itemValidator{ + listByPath: listMeta, + itemByPath: itemMeta, + }) + RegisterFieldValidator(&itemValidator{ + listByPath: listMeta, + itemByPath: itemMeta, + }) + + // Iterating values of lists and maps is a special tag, which can be called + // directly by the code-generator logic. + globalEachVal = &eachValTagValidator{byPath: listMeta, validator: nil} RegisterTagValidator(globalEachVal) - globalEachKey = &eachKeyTagValidator{nil} + // Iterating keys of maps is a special tag, which can be called directly by + // the code-generator logic. + globalEachKey = &eachKeyTagValidator{validator: nil} RegisterTagValidator(globalEachKey) } // This applies to all tags in this file. var listTagsValidScopes = sets.New(ScopeAny) -// listMap collects information about a single list with map semantics. -type listMap struct { - declaredAsMap bool - keyFields []string +// listMetadata collects information about a single list with map or set semantics. +type listMetadata struct { + // These will be checked for correctness elsewhere. + declaredAsAtomic bool + declaredAsSet bool + declaredAsMap bool + keyFields []string // iff declaredAsMap + keyNames []string // iff declaredAsMap +} + +// makeListMapMatchFunc generates a function that compares two list-map +// elements by their list-map key fields. +func (lm *listMetadata) makeListMapMatchFunc(t *types.Type) FunctionLiteral { + if !lm.declaredAsMap { + panic("makeListMapMatchFunc called on a non-map list") + } + // If no keys are defined, we will throw a good error later. + + matchFn := FunctionLiteral{ + Parameters: []ParamResult{{"a", t}, {"b", t}}, + Results: []ParamResult{{"", types.Bool}}, + } + buf := strings.Builder{} + buf.WriteString("return ") + // Note: this does not handle pointer fields, which are not + // supposed to be used as listMap keys. + for i, fld := range lm.keyFields { + if i > 0 { + buf.WriteString(" && ") + } + buf.WriteString(fmt.Sprintf("a.%s == b.%s", fld, fld)) + } + matchFn.Body = buf.String() + return matchFn } type listTypeTagValidator struct { - byFieldPath map[string]*listMap + byPath map[string]*listMetadata } func (listTypeTagValidator) Init(Config) {} @@ -82,12 +138,25 @@ func (lttv listTypeTagValidator) GetValidations(context Context, tag codetags.Ta // NOTE: pointers to lists are not supported, so we should never see a pointer here. t := util.NativeType(context.Type) if t.Kind != types.Slice && t.Kind != types.Array { - return Validations{}, fmt.Errorf("can only be used on list types") + return Validations{}, fmt.Errorf("can only be used on list types (%s)", t.Kind) } switch tag.Value { - case "atomic", "set": - // Allowed but no special handling. + case "atomic": + // We don't do much with atomic, but this ensures no conflicts between + // tags on typedefs and tags on fields which use those typedefs. + if lttv.byPath[context.Path.String()] == nil { + lttv.byPath[context.Path.String()] = &listMetadata{} + } + lm := lttv.byPath[context.Path.String()] + lm.declaredAsAtomic = true + case "set": + if lttv.byPath[context.Path.String()] == nil { + lttv.byPath[context.Path.String()] = &listMetadata{} + } + lm := lttv.byPath[context.Path.String()] + lm.declaredAsSet = true + // NOTE: we validate uniqueness in the listValidator. case "map": // NOTE: maps of pointers are not supported, so we should never see a pointer here. if util.NativeType(t.Elem).Kind != types.Struct { @@ -95,11 +164,12 @@ func (lttv listTypeTagValidator) GetValidations(context Context, tag codetags.Ta } // Save the fact that this list is a map. - if lttv.byFieldPath[context.Path.String()] == nil { - lttv.byFieldPath[context.Path.String()] = &listMap{} + if lttv.byPath[context.Path.String()] == nil { + lttv.byPath[context.Path.String()] = &listMetadata{} } - lm := lttv.byFieldPath[context.Path.String()] + lm := lttv.byPath[context.Path.String()] lm.declaredAsMap = true + // NOTE: we validate uniqueness of the keys in the listValidator. default: return Validations{}, fmt.Errorf("unknown list type %q", tag.Value) } @@ -116,7 +186,7 @@ func (lttv listTypeTagValidator) Docs() TagDoc { Description: "Declares a list field's semantic type.", Payloads: []TagPayloadDoc{{ Description: "", - Docs: "map | atomic", + Docs: "atomic | map | set", }}, PayloadsType: codetags.ValueTypeString, PayloadsRequired: true, @@ -125,7 +195,7 @@ func (lttv listTypeTagValidator) Docs() TagDoc { } type listMapKeyTagValidator struct { - byFieldPath map[string]*listMap + byPath map[string]*listMetadata } func (listMapKeyTagValidator) Init(Config) {} @@ -142,7 +212,7 @@ func (lmktv listMapKeyTagValidator) GetValidations(context Context, tag codetags // NOTE: pointers to lists are not supported, so we should never see a pointer here. t := util.NativeType(context.Type) if t.Kind != types.Slice && t.Kind != types.Array { - return Validations{}, fmt.Errorf("can only be used on list types") + return Validations{}, fmt.Errorf("can only be used on list types (%s)", t.Kind) } // NOTE: lists of pointers are not supported, so we should never see a pointer here. if util.NativeType(t.Elem).Kind != types.Struct { @@ -158,11 +228,12 @@ func (lmktv listMapKeyTagValidator) GetValidations(context Context, tag codetags fieldName = memb.Name } - if lmktv.byFieldPath[context.Path.String()] == nil { - lmktv.byFieldPath[context.Path.String()] = &listMap{} + if lmktv.byPath[context.Path.String()] == nil { + lmktv.byPath[context.Path.String()] = &listMetadata{} } - lm := lmktv.byFieldPath[context.Path.String()] + lm := lmktv.byPath[context.Path.String()] lm.keyFields = append(lm.keyFields, fieldName) + lm.keyNames = append(lm.keyNames, tag.Value) // This tag doesn't generate any validations. It just accumulates // information for other tags to use. @@ -184,9 +255,116 @@ func (lmktv listMapKeyTagValidator) Docs() TagDoc { return doc } +type listValidator struct { + byPath map[string]*listMetadata +} + +func (listValidator) Init(_ Config) {} + +func (listValidator) Name() string { + return "listValidator" +} + +var ( + validateUnique = types.Name{Package: libValidationPkg, Name: "Unique"} +) + +func (lv listValidator) GetValidations(context Context) (Validations, error) { + lm := lv.byPath[context.Path.String()] + if err := lv.check(lm); err != nil { + return Validations{}, err + } + // NOTE: We don't really support list-of-list or map-of-list, so this does + // not consider the case of ScopeListVal or ScopeMapVal. If we want to + // support those, we need to look at this and make sure the paths work the + // way we need. + if context.Scope == ScopeField { + tm := lv.byPath[context.Type.String()] + if lm != nil && tm != nil { + return Validations{}, fmt.Errorf("found list metadata for both a field and its type: %s", context.Path) + } + // For the purpose of emitting validations, we can use the + // type's metadata if the field's metadata is not set. + // + // TypeValidators happen before FieldValidators, so if we end + // up here, we can rely on this having been checked already. + if lm == nil && tm != nil { + lm = tm + } + } + if lm == nil { + // TODO(thockin): enable this once the whole codebase is converted or + // if we only run against fields which are opted-in. + // if context.Type.Kind == types.Slice || context.Type.Kind == types.Array { + // return Validations{}, fmt.Errorf("found list field without a listType") + // } + return Validations{}, nil + } + + result := Validations{} + + // Generate uniqueness checks for lists with higher-order semantics. + nt := util.NativeType(context.Type) + if lm.declaredAsSet { + // Only compare primitive values when possible. Slices and maps are not + // comparable, and structs might hold pointer fields, which are directly + // comparable but not what we need. + // + // NOTE: lists of pointers are not supported, so we should never see a pointer here. + matchArg := validateSemanticDeepEqual + if util.IsDirectComparable(util.NonPointer(util.NativeType(nt.Elem))) { + matchArg = validateDirectEqual + } + f := Function("listValidator", DefaultFlags, validateUnique, Identifier(matchArg)) + result.AddFunction(f) + } + if lm.declaredAsMap { + // TODO: There are some fields which are declared as maps which do not + // enforce uniqueness in manual validation. Those either need to not be + // maps or we need to allow types to opt-out from this validation. SSA + // is also not able to handle these well. + matchArg := lm.makeListMapMatchFunc(nt.Elem) + f := Function("listValidator", DefaultFlags, validateUnique, matchArg) + result.AddFunction(f) + } + + return result, nil +} + +// make sure a given listMetadata makes sense. +func (lv listValidator) check(lm *listMetadata) error { + if lm != nil { + // Check some fundamental constraints on list tags. + decls := []string{} + if lm.declaredAsAtomic { + decls = append(decls, "atomic") + } + if lm.declaredAsSet { + decls = append(decls, "set") + } + if lm.declaredAsMap { + decls = append(decls, "map") + } + if len(decls) > 1 { + return fmt.Errorf("listType cannot have multiple types (%s)", strings.Join(decls, ", ")) + } + if lm.declaredAsMap && len(lm.keyFields) == 0 { + return fmt.Errorf("found listType=map without listMapKey") + } + if len(lm.keyFields) > 0 && !lm.declaredAsMap { + return fmt.Errorf("found listMapKey without listType=map") + } + // Check for missing listType (after the other checks so the more specific errors take priority) + if len(decls) == 0 { + return fmt.Errorf("found list metadata without a listType") + } + } + return nil +} + type eachValTagValidator struct { - byFieldPath map[string]*listMap - validator Validator + byPath map[string]*listMetadata + validator Validator } func (evtv *eachValTagValidator) Init(cfg Config) { @@ -201,30 +379,33 @@ func (eachValTagValidator) ValidScopes() sets.Set[Scope] { return listTagsValidScopes } -// LateTagValidator indicatesa that validator has to run after the listType and -// listMapKey tags. +// LateTagValidator indicates that this validator has to run AFTER the listType +// and listMapKey tags. func (eachValTagValidator) LateTagValidator() {} var ( - validateEachSliceVal = types.Name{Package: libValidationPkg, Name: "EachSliceVal"} - validateEachMapVal = types.Name{Package: libValidationPkg, Name: "EachMapVal"} + validateEachSliceVal = types.Name{Package: libValidationPkg, Name: "EachSliceVal"} + validateEachMapVal = types.Name{Package: libValidationPkg, Name: "EachMapVal"} + validateSemanticDeepEqual = types.Name{Package: libValidationPkg, Name: "SemanticDeepEqual"} + validateDirectEqual = types.Name{Package: libValidationPkg, Name: "DirectEqual"} ) func (evtv eachValTagValidator) GetValidations(context Context, tag codetags.Tag) (Validations, error) { // NOTE: pointers to lists and maps are not supported, so we should never see a pointer here. - t := util.NativeType(context.Type) - switch t.Kind { + t := context.Type + nt := util.NativeType(t) + switch nt.Kind { case types.Slice, types.Array, types.Map: default: - return Validations{}, fmt.Errorf("can only be used on list or map types") + return Validations{}, fmt.Errorf("can only be used on list or map types (%s)", t.Kind) } elemContext := Context{ - Type: t.Elem, - Parent: t, + Type: nt.Elem, + Parent: t, // possibly an alias Path: context.Path.Key("*"), } - switch t.Kind { + switch nt.Kind { case types.Slice, types.Array: elemContext.Scope = ScopeListVal case types.Map: @@ -242,12 +423,16 @@ func (evtv eachValTagValidator) GetValidations(context Context, tag codetags.Tag if len(validations.Variables) > 0 { return Validations{}, fmt.Errorf("variable generation is not supported") } + // Pass the real (possibly alias) type. return evtv.getValidations(context.Path, t, validations) } } +// t is expected to be the top-most type of the list or map. For example, if +// this is a typedef to a list, this is the alias type, not the underlying +// type. func (evtv eachValTagValidator) getValidations(fldPath *field.Path, t *types.Type, validations Validations) (Validations, error) { - switch t.Kind { + switch util.NativeType(t).Kind { case types.Slice, types.Array: return evtv.getListValidations(fldPath, t, validations) case types.Map: @@ -257,59 +442,85 @@ func (evtv eachValTagValidator) getValidations(fldPath *field.Path, t *types.Typ } // ForEachVal returns a validation that applies a function to each element of -// a list or map. +// a list or map. The type argument is expected to be the top-most type of the +// list or map. For example, if this is a typedef to a list, this is the alias +// type, not the underlying type. func ForEachVal(fldPath *field.Path, t *types.Type, fn FunctionGen) (Validations, error) { return globalEachVal.getValidations(fldPath, t, Validations{Functions: []FunctionGen{fn}}) } +// t is expected to be the top-most type of the list. For example, if this is a +// typedef to a list, this is the alias type, not the underlying type. func (evtv eachValTagValidator) getListValidations(fldPath *field.Path, t *types.Type, validations Validations) (Validations, error) { result := Validations{} result.OpaqueValType = validations.OpaqueType - var listMap *listMap - if lm, found := evtv.byFieldPath[fldPath.String()]; found { - if !lm.declaredAsMap { - return Validations{}, fmt.Errorf("found listMapKey without listType=map") + // This type is a "late" validator, so it runs after all the keys are + // registered. See LateTagValidator() above. + listMetadata := evtv.byPath[fldPath.String()] + if listMetadata == nil { + // If we don't have metadata for this field, we might have it for the + // field's type. + listMetadata = evtv.byPath[t.String()] + } + + nt := util.NativeType(t) + + // matchArg is the function that is used to lookup the correlated element in the old list. + var matchArg any = Literal("nil") + + // equivArg is the function that is used to compare the correlated elements in the old and new lists. + // It would be "nil" if the matchArg is a full comparison function. + var equivArg any = Literal("nil") + + // directComparable is used to determine whether we can use the direct + // comparison operator "==" or need to use the semantic DeepEqual when + // looking up and comparing correlated list elements for validation ratcheting. + directComparable := util.IsDirectComparable(util.NonPointer(util.NativeType(nt.Elem))) + + switch { + case listMetadata != nil && listMetadata.declaredAsMap: + // For listType=map, we use key to lookup the correlated element in the old list. + // And use equivFunc to compare the correlated elements in the old and new lists. + matchArg = listMetadata.makeListMapMatchFunc(nt.Elem) + if directComparable { + equivArg = Identifier(validateDirectEqual) + } else { + equivArg = Identifier(validateSemanticDeepEqual) } - if len(lm.keyFields) == 0 { - return Validations{}, fmt.Errorf("found listType=map without listMapKey") + case listMetadata != nil && listMetadata.declaredAsSet: + // For listType=set, matchArg is the equivalence check, so equivArg is nil. + if directComparable { + matchArg = Identifier(validateDirectEqual) + } else { + matchArg = Identifier(validateSemanticDeepEqual) } - listMap = lm + default: + // For non-map and non-set list, we don't lookup the correlated element in the old list. + // The matchArg and equivArg are both nil. } for _, vfn := range validations.Functions { - var cmpArg any = Literal("nil") - if listMap != nil { - cmpFn := FunctionLiteral{ - Parameters: []ParamResult{{"a", t.Elem}, {"b", t.Elem}}, - Results: []ParamResult{{"", types.Bool}}, - } - buf := strings.Builder{} - buf.WriteString("return ") - // Note: this does not handle pointer fields, which are not - // supposed to be used as listMap keys. - for i, fld := range listMap.keyFields { - if i > 0 { - buf.WriteString(" && ") - } - buf.WriteString(fmt.Sprintf("a.%s == b.%s", fld, fld)) - } - cmpFn.Body = buf.String() - cmpArg = cmpFn - } - f := Function(eachValTagName, vfn.Flags, validateEachSliceVal, cmpArg, WrapperFunction{vfn, t.Elem}) - result.Functions = append(result.Functions, f) + f := Function(eachValTagName, vfn.Flags, validateEachSliceVal, matchArg, equivArg, WrapperFunction{vfn, nt.Elem}) + result.AddFunction(f) } return result, nil } +// t is expected to be the top-most type of the map. For example, if this is a +// typedef to a map, this is the alias type, not the underlying type. func (evtv eachValTagValidator) getMapValidations(t *types.Type, validations Validations) (Validations, error) { result := Validations{} result.OpaqueValType = validations.OpaqueType + nt := util.NativeType(t) + equivArg := Identifier(validateSemanticDeepEqual) + if util.IsDirectComparable(util.NonPointer(util.NativeType(nt.Elem))) { + equivArg = Identifier(validateDirectEqual) + } for _, vfn := range validations.Functions { - f := Function(eachValTagName, vfn.Flags, validateEachMapVal, WrapperFunction{vfn, t.Elem}) - result.Functions = append(result.Functions, f) + f := Function(eachValTagName, vfn.Flags, validateEachMapVal, equivArg, WrapperFunction{vfn, nt.Elem}) + result.AddFunction(f) } return result, nil @@ -354,7 +565,7 @@ func (ektv eachKeyTagValidator) GetValidations(context Context, tag codetags.Tag // NOTE: pointers to lists are not supported, so we should never see a pointer here. t := util.NativeType(context.Type) if t.Kind != types.Map { - return Validations{}, fmt.Errorf("can only be used on map types") + return Validations{}, fmt.Errorf("can only be used on map types (%s)", t.Kind) } elemContext := Context{ @@ -363,6 +574,7 @@ func (ektv eachKeyTagValidator) GetValidations(context Context, tag codetags.Tag Parent: t, Path: context.Path.Child("(keys)"), } + if validations, err := ektv.validator.ExtractValidations(elemContext, *tag.ValueTag); err != nil { return Validations{}, err } else { @@ -379,7 +591,7 @@ func (ektv eachKeyTagValidator) getValidations(t *types.Type, validations Valida result.OpaqueKeyType = validations.OpaqueType for _, vfn := range validations.Functions { f := Function(eachKeyTagName, vfn.Flags, validateEachMapKey, WrapperFunction{vfn, t.Key}) - result.Functions = append(result.Functions, f) + result.AddFunction(f) } return result, nil } diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/immutable.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/immutable.go index 12be03c7924..92ae2fd2b89 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/immutable.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/immutable.go @@ -54,7 +54,7 @@ var ( func (immutableTagValidator) GetValidations(context Context, _ codetags.Tag) (Validations, error) { var result Validations - if util.NonPointer(util.NativeType(context.Type)).Kind == types.Builtin { + if util.IsDirectComparable(util.NonPointer(util.NativeType(context.Type))) { // This is a minor optimization to just compare primitive values when // possible. Slices and maps are not comparable, and structs might hold // pointer fields, which are directly comparable but not what we need.