From 52cb155c97b86113a80c5680af1610c912dfdfae Mon Sep 17 00:00:00 2001 From: yongruilin Date: Wed, 18 Mar 2026 23:45:09 +0000 Subject: [PATCH] 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 --- .../apimachinery/pkg/api/validate/union.go | 18 ++-- .../pkg/api/validate/union_test.go | 88 +++++++++++++------ 2 files changed, 74 insertions(+), 32 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/api/validate/union.go b/staging/src/k8s.io/apimachinery/pkg/api/validate/union.go index bbc1d1d3095..753a4a9ee12 100644 --- a/staging/src/k8s.io/apimachinery/pkg/api/validate/union.go +++ b/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 } diff --git a/staging/src/k8s.io/apimachinery/pkg/api/validate/union_test.go b/staging/src/k8s.io/apimachinery/pkg/api/validate/union_test.go index a8132e3036a..d033fb95164 100644 --- a/staging/src/k8s.io/apimachinery/pkg/api/validate/union_test.go +++ b/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()) + } + }) }) } }