From 8f679c740207d1e4d5e24944df54f691e09adebb Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Thu, 17 Jul 2025 14:35:10 -0700 Subject: [PATCH] Make item validation just use a TagValidator No need for Field/TypeValidators. --- .../cmd/validation-gen/validators/each.go | 17 +- .../cmd/validation-gen/validators/item.go | 263 +++++++----------- 2 files changed, 109 insertions(+), 171 deletions(-) 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 0ed34e8a475..21a72691d33 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 @@ -53,21 +53,8 @@ func init() { 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, - }) + // Processing item tags requires the list metadata. + RegisterTagValidator(&itemTagValidator{listByPath: listMeta}) // Iterating values of lists and maps is a special tag, which can be called // directly by the code-generator logic. diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/item.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/item.go index e0bbd58d167..0f5ee76aa7c 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/item.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/item.go @@ -37,22 +37,14 @@ type keyValuePair struct { valueType codetags.ValueType } -type itemValidation struct { - // criteria contains the field(s) on which to match - criteria []keyValuePair - // valueTag is the validation tag to apply to a matched item - valueTag codetags.Tag -} - -type itemMetadata struct { - items []itemValidation -} - type itemTagValidator struct { - byPath map[string]*itemMetadata + validator Validator + listByPath map[string]*listMetadata } -func (itv *itemTagValidator) Init(cfg Config) {} +func (itv *itemTagValidator) Init(cfg Config) { + itv.validator = cfg.Validator +} func (itemTagValidator) TagName() string { return itemTagName @@ -68,9 +60,14 @@ func (itemTagValidator) ValidScopes() sets.Set[Scope] { // and listMapKey tags. func (itemTagValidator) LateTagValidator() {} +var ( + validateSliceItem = types.Name{Package: libValidationPkg, Name: "SliceItem"} +) + func (itv *itemTagValidator) GetValidations(context Context, tag codetags.Tag) (Validations, error) { // TODO: Support regular maps with syntax like: // +k8s:item("map-key")=+k8s:immutable + // This tag can apply to value and pointer fields, as well as typedefs // (which should never be pointers). We need to check the concrete type. nt := util.NonPointer(util.NativeType(context.Type)) @@ -85,16 +82,32 @@ func (itv *itemTagValidator) GetValidations(context Context, tag codetags.Tag) ( return Validations{}, fmt.Errorf("requires a validation tag as its value payload") } + // For fields, list metadata can fall back to the type. + // For types, list metadata must be defined on the type itself. + listMeta, ok := itv.listByPath[context.Path.String()] + if !ok { + if context.Scope == ScopeField { + typePath := context.Type.String() + listMeta, ok = itv.listByPath[typePath] + } + } + + // Fields inherit list metadata from typedefs, but not vice-versa. + // If we find no listMetadata then something is wrong. + if !ok || !listMeta.declaredAsMap || len(listMeta.keyFields) == 0 { + return Validations{}, fmt.Errorf("found items with no list metadata") + } + // Parse key-value pairs from named args. criteria := []keyValuePair{} processedKeys := sets.NewString() for _, arg := range tag.Args { if arg.Name == "" { - return Validations{}, fmt.Errorf("all arguments must be named (ex: fieldName:value)") + return Validations{}, fmt.Errorf("all arguments must be named (ex: fieldName: value)") } if processedKeys.Has(arg.Name) { - return Validations{}, fmt.Errorf("duplicate key %q", arg.Name) + return Validations{}, fmt.Errorf("duplicate argument: %q", arg.Name) } processedKeys.Insert(arg.Name) @@ -114,18 +127,88 @@ func (itv *itemTagValidator) GetValidations(context Context, tag codetags.Tag) ( return Validations{}, fmt.Errorf("no selection criteria was specified") } - // Store metadata for the field validator to use. - if itv.byPath[context.Path.String()] == nil { - itv.byPath[context.Path.String()] = &itemMetadata{} + if narg, nkey := len(criteria), len(listMeta.keyNames); narg != nkey { + return Validations{}, fmt.Errorf("number of arguments (%d) does not match number of listMapKey fields (%d)", narg, nkey) } - itv.byPath[context.Path.String()].items = append(itv.byPath[context.Path.String()].items, itemValidation{ - criteria: criteria, - valueTag: *tag.ValueTag, - }) + // Validate that all listMapKeys are provided and types match + foundKeys := make(map[string]bool) + sortedCriteria := make([]keyValuePair, 0, len(listMeta.keyNames)) + for _, keyName := range listMeta.keyNames { + for _, pair := range criteria { + if pair.key == keyName { + member := util.GetMemberByJSON(elemT, pair.key) + if member == nil { + return Validations{}, fmt.Errorf("no field with JSON name %q", pair.key) + } + if err := validateTypeMatch(member.Type, pair.value); err != nil { + return Validations{}, fmt.Errorf("key %q: %w", pair.key, err) + } + foundKeys[keyName] = true + sortedCriteria = append(sortedCriteria, pair) + break + } + } + } + if len(foundKeys) != len(listMeta.keyNames) { + missing := []string{} + for _, k := range listMeta.keyNames { + if !foundKeys[k] { + missing = append(missing, k) + } + } + return Validations{}, fmt.Errorf("missing required listMapKey fields: %v", missing) + } + criteria = sortedCriteria - // This tag doesn't generate validations directly, the itemFieldValidator does. - return Validations{}, nil + result := Validations{} + + // Extract validations from the stored tag + itemKey := selectorString(criteria) + itemPath := context.Path.Key(itemKey) + itemSelector := generateSelector(criteria) + subContext := Context{ + Scope: ScopeListVal, + Type: elemT, + Path: itemPath, + ListSelector: itemSelector, + ParentPath: context.Path, + } + + validations, err := itv.validator.ExtractValidations(subContext, *tag.ValueTag) + if err != nil { + return Validations{}, err + } + + result.Variables = append(result.Variables, validations.Variables...) + + // matchArg is the function that is used to select the item in new and + // old lists. + matchArg, err := createMatchFn(elemT, criteria) + if err != nil { + return Validations{}, err + } + + // equivArg is the function that is used to compare the correlated + // elements in the old and new lists, for ratcheting. + var equivArg any + + // 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(elemT))) + if directComparable { + equivArg = Identifier(validateDirectEqual) + } else { + equivArg = Identifier(validateSemanticDeepEqual) + } + + for _, vfn := range validations.Functions { + f := Function(itemTagName, vfn.Flags, validateSliceItem, matchArg, equivArg, WrapperFunction{vfn, elemT}) + result.AddFunction(f) + } + + return result, nil } // parseTypedValue parses a value based on its detected type @@ -172,138 +255,6 @@ func (itv itemTagValidator) Docs() TagDoc { return doc } -type itemValidator struct { - validator Validator - listByPath map[string]*listMetadata - itemByPath map[string]*itemMetadata -} - -func (iv *itemValidator) Init(cfg Config) { - iv.validator = cfg.Validator -} - -func (itemValidator) Name() string { - return "itemValidator" -} - -var ( - validateSliceItem = types.Name{Package: libValidationPkg, Name: "SliceItem"} -) - -func (iv itemValidator) GetValidations(context Context) (Validations, error) { - itemMeta, ok := iv.itemByPath[context.Path.String()] - if !ok { - return Validations{}, nil - } - if len(itemMeta.items) == 0 { - return Validations{}, fmt.Errorf("found item metadata with no items") - } - - // For fields, list metadata can fall back to the type. - // For types, list metadata must be defined on the type itself. - listMeta, ok := iv.listByPath[context.Path.String()] - if !ok { - if context.Scope == ScopeField { - typePath := context.Type.String() - listMeta, ok = iv.listByPath[typePath] - } - } - - // Fields inherit list metadata from typedefs, but not vice-versa. - // If we find no listMetadata then something is wrong. - if !ok || !listMeta.declaredAsMap || len(listMeta.keyFields) == 0 { - return Validations{}, fmt.Errorf("found items with no list metadata") - } - - nt := util.NonPointer(util.NativeType(context.Type)) - elemT := util.NonPointer(util.NativeType(nt.Elem)) - - result := Validations{} - - for _, item := range itemMeta.items { - if narg, nkey := len(item.criteria), len(listMeta.keyNames); narg != nkey { - return Validations{}, fmt.Errorf("number of arguments (%d) does not match number of listMapKey fields (%d)", narg, nkey) - } - - // Validate that all listMapKeys are provided and types match - foundKeys := make(map[string]bool) - sorted := make([]keyValuePair, 0, len(listMeta.keyNames)) - for _, keyName := range listMeta.keyNames { - for _, pair := range item.criteria { - if pair.key == keyName { - member := util.GetMemberByJSON(elemT, pair.key) - if member == nil { - return Validations{}, fmt.Errorf("no field with JSON name %q", pair.key) - } - if err := validateTypeMatch(member.Type, pair.value); err != nil { - return Validations{}, fmt.Errorf("key %q: %w", pair.key, err) - } - foundKeys[keyName] = true - sorted = append(sorted, pair) - break - } - } - } - if len(foundKeys) != len(listMeta.keyNames) { - missing := []string{} - for _, k := range listMeta.keyNames { - if !foundKeys[k] { - missing = append(missing, k) - } - } - return Validations{}, fmt.Errorf("missing required listMapKey fields: %v", missing) - } - item.criteria = sorted - - // Extract validations from the stored tag - itemKey := selectorString(item.criteria) - itemPath := context.Path.Key(itemKey) - itemSelector := generateSelector(item.criteria) - subContext := Context{ - Scope: ScopeListVal, - Type: elemT, - Path: itemPath, - ListSelector: itemSelector, - ParentPath: context.Path, - } - - validations, err := iv.validator.ExtractValidations(subContext, item.valueTag) - if err != nil { - return Validations{}, err - } - - result.Variables = append(result.Variables, validations.Variables...) - - // matchArg is the function that is used to select the item in new and - // old lists. - matchArg, err := createMatchFn(elemT, item.criteria) - if err != nil { - return Validations{}, err - } - - // equivArg is the function that is used to compare the correlated - // elements in the old and new lists, for ratcheting. - var equivArg any - - // 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(elemT))) - if directComparable { - equivArg = Identifier(validateDirectEqual) - } else { - equivArg = Identifier(validateSemanticDeepEqual) - } - - for _, vfn := range validations.Functions { - f := Function(itemTagName, vfn.Flags, validateSliceItem, matchArg, equivArg, WrapperFunction{vfn, elemT}) - result.AddFunction(f) - } - } - - return result, nil -} - // validateTypeMatch ensures the provided value matches the field's type func validateTypeMatch(fieldType *types.Type, value any) error { nativeType := util.NativeType(fieldType)