feat: Support maxItems validation

[yongruilin]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Support maxItems validation for validation-gen

Which issue(s) this PR fixes:

Fixes

• [validation-gen] Add OpenAPI "maxItems" validation #45

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[thockin]

May want a test case that has maxItems on both the type (as in M2) and the field.

[yongruilin]

Added the test cases.

[thockin]

So, the real trick with these tests is to have as FEW cases as possible to show that the generated code covers all sorts of cases.

I think that is:

A slice field

A slice type def with tag on the type

A slice type def with no tag, used as a field with a tag

A slice type def with tag used as a field with a tag

If using the tag on a non-slice produces a compile error, you can't really include that.

Maybe repeat once for slices of strings, again for slices of struct, pointers to strings, and pointers to struct

Maybe also for slices of slices.

I would lay those out as LS []string, LPS []*string, LT []T, LPT []*T, etc.

Then manually inspect the generated code and see that it makes sense. Then write the docs_test.go to verify success and fail.

[yongruilin]

2 questions:

1. Should the constraint on the type has higher priority to the constraint on field ? e.g.

type M struct {
	TypeMeta int

	// +k8s:validation:maxItems=3
	M1 LT `json:"m1"`

	// +k8s:validation:maxItems=1
	M2 LT `json:"m2"`
}

// +k8s:validation:maxItems=2
type LT []T

Which limit should apply to M1 and M2 ?

2. Is it necessary to test string? As the validation function takes in any, shouldn't it already cover the string?

[jpbetz]

1. Should the constraint on the type has higher priority to the constraint on field ? e.g.

Today we do preorder traversal (unless I'm misreading the code), so I'd expect field to validate before the type. That said, for cases like this that short-circuit, I think I'd prefer we'd short circuit any validations on the (higher level) field if the (more deeply nested) type validation short circuits.

This gets into some really messing ordering considerations. It might be best to open a separate issue to start tracking those and leave this PR as-is?

[jpbetz]

2. Is it necessary to test string? As the validation function takes in any, shouldn't it already cover the string?

I'd include it to guarantee that future changes to this code do not break our expectations.

[thockin]

I don't think we need to do ordering here? If someone puts the same validation in two places, I think we should validate both and not try to be smart.

[thockin]

This is currently ShortCircuit. We usually want to try to return as many errors as possible in a single pass, to avoid users trying again and again and again getting new errors each time, but I guess it makes sense to not try further validation if we know the list is too long - for safety against DoS?

[thockin]

So, the real trick with these tests is to have as FEW cases as possible to show that the generated code covers all sorts of cases.

I think that is:

A slice field

A slice type def with tag on the type

A slice type def with no tag, used as a field with a tag

A slice type def with tag used as a field with a tag

If using the tag on a non-slice produces a compile error, you can't really include that.

Maybe repeat once for slices of strings, again for slices of struct, pointers to strings, and pointers to struct

Maybe also for slices of slices.

I would lay those out as LS []string, LPS []*string, LT []T, LPT []*T, etc.

Then manually inspect the generated code and see that it makes sense. Then write the docs_test.go to verify success and fail.

[thockin]

This LGTM overall, sorry to nitpick on test cases, I keep thinking up new ways I might break it :)

[thockin]

This LGTM overall, sorry to nitpick on test cases, I keep thinking up new ways I might break it :)