[WIP/DNM] feat: add inner validation support with one level nesting for field validators to validation-gen #68

aaron-prindle · 2024-12-09T08:27:47Z

This PR adds +k8s:inner tag support to validation-gen. Currently this PR only targets field based validations for inner support (vs key/elem). The PR here allows for specifying an inner validation on a field by using a tag on a struct like // +k8s:inner(InnerField)=+k8s:validation:maxItems=1.

NOTE: This PR only targets inner support for fieldValidations. Support for key & elem validations will be done in subsequent PRs. Additionally this only supports inner validation for one level nesting.

TODOs left for this PR:

Add extensive inner validation tests - eg: for all types, edge cases etc. (primitives, pointers, different nesting levels, fields that have field validation in addition to inner validation, etc.)
De-dupe shared logic looping through members and building nodes for normal pass vs inner pass
Fix pointers (PointerStruct examples currently has no validation generated)

TODOs for future PRs (will create a seperate tracking issue for these before merging):

Extend inner validation to key & elem validations
[TBD] Change +k8s:inner(field)=<validator>=<args> to instead be JSON format - eg: +k8s:inner{"field"...}

thockin

I like this approach

staging/src/k8s.io/code-generator/cmd/validation-gen/validation.go

thockin · 2024-12-11T00:42:16Z

staging/src/k8s.io/code-generator/cmd/validation-gen/validation.go

+func (td *typeDiscoverer) extractInnerValidations(subfield *types.Member, comments []string) (validators.Validations, error) {
+	var result validators.Validations
+
+	fieldTag := fmt.Sprintf("%s(%s)", innerTag, subfield.Name)


maybe comment on this "syntax" - if it gets any more complicated, we may need an actual parser. E.g. we are very whitespace sensitive (in all tag parsing).

I've added a comment explaining the syntax now:

// Currently the format for +k8s:inner tag is: // +k8s:inner(subfield-name)=<validator-tag>=<args> validator tag args..."

In the tag, is "subfield-name" the Go name or the JSON name? Which on makes more sense as an API author? If we had cross-field deps, like +ifSpecified(other-field)=+k8s:required would other-field be a Go name or a JSON name?

@jpbetz for opinion - if we use this to generate docs, it feels like JSON name makes sense? Any drawbacks?

Currently this uses the Go name. I am not sure which makes more sense as an API author as well as what aligns more across our other future tags such as +ifSpecified. From the point around docs above it seems like JSON name might make more sense? I am leaving the code + comment as is for now but am definitely open to changing this to use the JSON name.

EDIT: I changed subfield-name -> subfield-go-name for now in the comments

staging/src/k8s.io/code-generator/cmd/validation-gen/validation.go

thockin · 2024-12-11T00:56:26Z

staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/inner/validatetrue/doc.go

+	TypeMeta int `json:"typeMeta"`
+
+	// Simple struct with inner field validations
+	// +k8s:inner(Field)=+validateTrue="inner T1.Simple.Field"


You should use validateFalse so it can generate a fixture to actually run the validation test.

staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/inner/validatetrue/doc.go

staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/inner/maxitems/doc.go

staging/src/k8s.io/code-generator/cmd/validation-gen/validation.go

thockin · 2024-12-13T22:32:28Z

staging/src/k8s.io/code-generator/cmd/validation-gen/validation.go

+func (td *typeDiscoverer) extractInnerValidations(subfield *types.Member, comments []string) (validators.Validations, error) {
+	var result validators.Validations
+
+	fieldTag := fmt.Sprintf("%s(%s)", innerTag, subfield.Name)


In the tag, is "subfield-name" the Go name or the JSON name? Which on makes more sense as an API author? If we had cross-field deps, like +ifSpecified(other-field)=+k8s:required would other-field be a Go name or a JSON name?

@jpbetz for opinion - if we use this to generate docs, it feels like JSON name makes sense? Any drawbacks?

thockin · 2024-12-13T22:35:53Z

staging/src/k8s.io/code-generator/cmd/validation-gen/validation.go

+	var result validators.Validations
+
+	// Currently the format for +k8s:inner tag is:
+	// +k8s:inner(subfield-name)=<validator-tag>=<args> validator tag args..."


+k8s:ifOptionEnabled syntax is +k8s:ifOptionEnabled=FeatureX=+k8s:validateFalse="field T1.S1" which is kind of awful but works. Aesthetically I prefer your notation (as long as we have one arg, and no real parsing).

Should we convert options to `+k8s:ifOptionEnabled(FeatureX)=+k8s:validateFalse="field T1.S1" ?

@jpbetz

…alidators to validation-gen

aaron-prindle force-pushed the validation-gen-aaron-prindle-inner branch 10 times, most recently from 9b1efad to f3a3e8f Compare December 9, 2024 21:33

thockin reviewed Dec 11, 2024

View reviewed changes

aaron-prindle force-pushed the validation-gen-aaron-prindle-inner branch 3 times, most recently from 5bb6d71 to 8547537 Compare December 12, 2024 21:59

aaron-prindle changed the title ~~[WIP/DNM] feat: add inner validation support to validation-gen~~ [WIP/DNM] feat: add inner validation support with one level nesting for field validators to validation-gen Dec 13, 2024

aaron-prindle force-pushed the validation-gen-aaron-prindle-inner branch 4 times, most recently from 6177cb5 to 60e8173 Compare December 13, 2024 22:27

thockin requested changes Dec 13, 2024

View reviewed changes

aaron-prindle force-pushed the validation-gen-aaron-prindle-inner branch 3 times, most recently from cf3dbe2 to c15d955 Compare December 13, 2024 23:39

feat: add inner validation support with one level nesting for field v…

2e4d2d2

…alidators to validation-gen

aaron-prindle force-pushed the validation-gen-aaron-prindle-inner branch from c15d955 to 2e4d2d2 Compare December 13, 2024 23:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP/DNM] feat: add inner validation support with one level nesting for field validators to validation-gen #68

[WIP/DNM] feat: add inner validation support with one level nesting for field validators to validation-gen #68

aaron-prindle commented Dec 9, 2024 •

edited

Loading

thockin left a comment

thockin Dec 11, 2024

aaron-prindle Dec 12, 2024

thockin Dec 13, 2024

aaron-prindle Dec 13, 2024 •

edited

Loading

thockin Dec 11, 2024

thockin Dec 13, 2024

thockin Dec 13, 2024

[WIP/DNM] feat: add inner validation support with one level nesting for field validators to validation-gen #68

Are you sure you want to change the base?

[WIP/DNM] feat: add inner validation support with one level nesting for field validators to validation-gen #68

Conversation

aaron-prindle commented Dec 9, 2024 • edited Loading

thockin left a comment

Choose a reason for hiding this comment

thockin Dec 11, 2024

Choose a reason for hiding this comment

aaron-prindle Dec 12, 2024

Choose a reason for hiding this comment

thockin Dec 13, 2024

Choose a reason for hiding this comment

aaron-prindle Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

thockin Dec 11, 2024

Choose a reason for hiding this comment

thockin Dec 13, 2024

Choose a reason for hiding this comment

thockin Dec 13, 2024

Choose a reason for hiding this comment

aaron-prindle commented Dec 9, 2024 •

edited

Loading

aaron-prindle Dec 13, 2024 •

edited

Loading