Make item validation just use a TagValidator

No need for Field/TypeValidators.
2026-07-18 04:54:54 +00:00 · 2025-07-17 14:35:10 -07:00
parent c047bdf58f
commit 8f679c7402
2 changed files with 109 additions and 171 deletions
--- 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.
--- 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)