Use IsZero instead of IsNil for union ratcheting check

- Use reflect.ValueOf(oldObj).IsZero() instead of IsNil() so union
  validation works with non-nilable T (e.g. value types)
- Remove hasOldValue guard from inner loop conditionals; only check
  at the final ratcheting skip point
- Add doc comments explaining T is "any" rather than "comparable"
  because union members can be slices
- Add value-type subtests for Union and DiscriminatedUnion

Co-authored-by: Tim Hockin <thockin@google.com>

staging/src/k8s.io/apimachinery/pkg/api/validate/union.go

@@ -61,6 +61,10 @@ type UnionValidationOptions struct {
 //		)...)
 //		return errs
 //	}
+//
+// Note that T is "any", rather than "comparable", because union-members can be
+// slices, meaning T might be a struct with a slice, meaning it is not
+// comparable.
 func Union[T any](_ context.Context, op operation.Operation, fldPath *field.Path, obj, oldObj T, union *UnionMembership, isSetFns ...ExtractorFn[T, bool]) field.ErrorList {
 	options := UnionValidationOptions{
 		ErrorForEmpty: func(fldPath *field.Path, allFields []string) *field.Error {
@@ -99,6 +103,10 @@ func Union[T any](_ context.Context, op operation.Operation, fldPath *field.Path
 //
 // It is not an error for the discriminatorValue to be unknown.  That must be
 // validated on its own.
+//
+// Note that T is "any", rather than "comparable", because union-members can be
+// slices, meaning T might be a struct with a slice, meaning it is not
+// comparable.
 func DiscriminatedUnion[T any, D ~string](_ context.Context, op operation.Operation, fldPath *field.Path, obj, oldObj T, union *UnionMembership, discriminatorExtractor ExtractorFn[T, D], isSetFns ...ExtractorFn[T, bool]) (errs field.ErrorList) {
 	if len(union.members) != len(isSetFns) {
 		return field.ErrorList{
@@ -107,10 +115,10 @@ func DiscriminatedUnion[T any, D ~string](_ context.Context, op operation.Operat
 					len(isSetFns), len(union.members))),
 		}
 	}
-	hasOldValue := !reflect.ValueOf(oldObj).IsNil()
+	hasOldValue := !reflect.ValueOf(oldObj).IsZero() // because T is any, rather than comparable
 	var changed bool
 	discriminatorValue := discriminatorExtractor(obj)
-	if op.Type == operation.Update && hasOldValue {
+	if op.Type == operation.Update {
 		oldDiscriminatorValue := discriminatorExtractor(oldObj)
 		changed = discriminatorValue != oldDiscriminatorValue
 	}
@@ -119,7 +127,7 @@ func DiscriminatedUnion[T any, D ~string](_ context.Context, op operation.Operat
 		member := union.members[i]
 		isDiscriminatedMember := string(discriminatorValue) == member.discriminatorValue
 		newIsSet := fieldIsSet(obj)
-		if op.Type == operation.Update && hasOldValue && !changed {
+		if op.Type == operation.Update && !changed {
 			oldIsSet := fieldIsSet(oldObj)
 			changed = changed || newIsSet != oldIsSet
 		}
@@ -197,12 +205,12 @@ func unionValidate[T any](op operation.Operation, fldPath *field.Path,
 		}
 	}
 
-	hasOldValue := !reflect.ValueOf(oldObj).IsNil()
+	hasOldValue := !reflect.ValueOf(oldObj).IsZero() // because T is any, rather than comparable
 	var specifiedFields []string
 	var changed bool
 	for i, fieldIsSet := range isSetFns {
 		newIsSet := fieldIsSet(obj)
-		if op.Type == operation.Update && hasOldValue && !changed {
+		if op.Type == operation.Update && !changed {
 			oldIsSet := fieldIsSet(oldObj)
 			changed = changed || newIsSet != oldIsSet
 		}

staging/src/k8s.io/apimachinery/pkg/api/validate/union_test.go

@@ -67,18 +67,34 @@ func TestUnion(t *testing.T) {
 				members = append(members, NewUnionMember(f))
 			}
 
-			// Create mock extractors that return predefined values instead of
-			// actually extracting from the object.
-			extractors := make([]ExtractorFn[*testMember, bool], len(tc.fieldValues))
-			for i, val := range tc.fieldValues {
-				extractors[i] = func(_ *testMember) bool { return val }
-			}
+			t.Run("pointer", func(t *testing.T) {
+				// Create mock extractors that return predefined values instead of
+				// actually extracting from the object.
+				extractors := make([]ExtractorFn[*testMember, bool], len(tc.fieldValues))
+				for i, val := range tc.fieldValues {
+					extractors[i] = func(_ *testMember) bool { return val }
+				}
 
-			got := Union(context.Background(), operation.Operation{}, nil, &testMember{}, nil,
-				NewUnionMembership(members...), extractors...)
-			if !reflect.DeepEqual(got, tc.expected) {
-				t.Errorf("got %v want %v", got, tc.expected)
-			}
+				got := Union(context.Background(), operation.Operation{}, nil, &testMember{}, nil,
+					NewUnionMembership(members...), extractors...)
+				if !reflect.DeepEqual(got, tc.expected) {
+					t.Errorf("got %v want %v", got, tc.expected)
+				}
+			})
+			t.Run("value", func(t *testing.T) {
+				// Create mock extractors that return predefined values instead of
+				// actually extracting from the object.
+				extractors := make([]ExtractorFn[testMember, bool], len(tc.fieldValues))
+				for i, val := range tc.fieldValues {
+					extractors[i] = func(_ testMember) bool { return val }
+				}
+
+				got := Union(context.Background(), operation.Operation{}, nil, testMember{}, testMember{},
+					NewUnionMembership(members...), extractors...)
+				if !reflect.DeepEqual(got, tc.expected) {
+					t.Errorf("got %v want %v", got, tc.expected)
+				}
+			})
 		})
 	}
 }
@@ -131,26 +147,44 @@ func TestDiscriminatedUnion(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
+		members := []UnionMember{}
+		for _, f := range tc.fields {
+			members = append(members, NewDiscriminatedUnionMember(f[0], f[1]))
+		}
+
 		t.Run(tc.name, func(t *testing.T) {
-			members := []UnionMember{}
-			for _, f := range tc.fields {
-				members = append(members, NewDiscriminatedUnionMember(f[0], f[1]))
-			}
+			t.Run("pointer", func(t *testing.T) {
+				discriminatorExtractor := func(_ *testMember) string { return tc.discriminatorValue }
 
-			discriminatorExtractor := func(_ *testMember) string { return tc.discriminatorValue }
+				// Create mock extractors that return predefined values instead of
+				// actually extracting from the object.
+				extractors := make([]ExtractorFn[*testMember, bool], len(tc.fieldValues))
+				for i, val := range tc.fieldValues {
+					extractors[i] = func(_ *testMember) bool { return val }
+				}
 
-			// Create mock extractors that return predefined values instead of
-			// actually extracting from the object.
-			extractors := make([]ExtractorFn[*testMember, bool], len(tc.fieldValues))
-			for i, val := range tc.fieldValues {
-				extractors[i] = func(_ *testMember) bool { return val }
-			}
+				got := DiscriminatedUnion(context.Background(), operation.Operation{}, nil, &testMember{}, nil,
+					NewDiscriminatedUnionMembership(tc.discriminatorField, members...), discriminatorExtractor, extractors...)
+				if !reflect.DeepEqual(got, tc.expected) {
+					t.Errorf("got %v want %v", got.ToAggregate(), tc.expected.ToAggregate())
+				}
+			})
+			t.Run("value", func(t *testing.T) {
+				discriminatorExtractor := func(_ testMember) string { return tc.discriminatorValue }
 
-			got := DiscriminatedUnion(context.Background(), operation.Operation{}, nil, &testMember{}, nil,
-				NewDiscriminatedUnionMembership(tc.discriminatorField, members...), discriminatorExtractor, extractors...)
-			if !reflect.DeepEqual(got, tc.expected) {
-				t.Errorf("got %v want %v", got.ToAggregate(), tc.expected.ToAggregate())
-			}
+				// Create mock extractors that return predefined values instead of
+				// actually extracting from the object.
+				extractors := make([]ExtractorFn[testMember, bool], len(tc.fieldValues))
+				for i, val := range tc.fieldValues {
+					extractors[i] = func(_ testMember) bool { return val }
+				}
+
+				got := DiscriminatedUnion(context.Background(), operation.Operation{}, nil, testMember{}, testMember{},
+					NewDiscriminatedUnionMembership(tc.discriminatorField, members...), discriminatorExtractor, extractors...)
+				if !reflect.DeepEqual(got, tc.expected) {
+					t.Errorf("got %v want %v", got.ToAggregate(), tc.expected.ToAggregate())
+				}
+			})
 		})
 	}
 }