Merge pull request #1270 from JoelSpeed/better-handling-local-types

⚠️ Start from local type declaration when applying schema

Author: k8s-ci-robot

pkg/crd/parser.go

@@ -172,7 +172,12 @@ func (p *Parser) NeedSchemaFor(typ TypeIdent) {
 // avoid tripping recursive schemata, like ManagedFields, by adding an empty WIP schema
 p.Schemata[typ] = apiext.JSONSchemaProps{}
 
-schemaCtx := newSchemaContext(typ.Package, p, p.AllowDangerousTypes, p.IgnoreUnexportedFields)
+schemaCtx := newSchemaContext(typ.Package, p, func(typ TypeIdent) *apiext.JSONSchemaProps {
+p.NeedSchemaFor(typ)
+
+props := p.Schemata[typ]
+return &props
+}, p.AllowDangerousTypes, p.IgnoreUnexportedFields)
 ctxForInfo := schemaCtx.ForInfo(info)
 
 pkgMarkers, err := markers.PackageMarkers(p.Collector, typ.Package)

pkg/crd/schema.go

@@ -57,6 +57,9 @@ type applyFirstMarker interface {
 ApplyFirst()
 }
 
+// schemaFetcher is a function that fetches a schema for a given type.
+type schemaFetcher func(TypeIdent) *apiext.JSONSchemaProps
+
 // schemaRequester knows how to marker that another schema (e.g. via an external reference) is necessary.
 type schemaRequester interface {
 NeedSchemaFor(typ TypeIdent)
@@ -68,6 +71,7 @@ type schemaContext struct {
 info *markers.TypeInfo
 
 schemaRequester schemaRequester
+schemaFetcher schemaFetcher
 PackageMarkers markers.MarkerValues
 
 allowDangerousTypes bool
@@ -76,11 +80,12 @@ type schemaContext struct {
 
 // newSchemaContext constructs a new schemaContext for the given package and schema requester.
 // It must have type info added before use via ForInfo.
-func newSchemaContext(pkg *loader.Package, req schemaRequester, allowDangerousTypes, ignoreUnexportedFields bool) *schemaContext {
+func newSchemaContext(pkg *loader.Package, req schemaRequester, fetcher schemaFetcher, allowDangerousTypes, ignoreUnexportedFields bool) *schemaContext {
 pkg.NeedTypesInfo()
 return &schemaContext{
 pkg: pkg,
 schemaRequester: req,
+schemaFetcher: fetcher,
 allowDangerousTypes: allowDangerousTypes,
 ignoreUnexportedFields: ignoreUnexportedFields,
 }
@@ -93,6 +98,7 @@ func (c *schemaContext) ForInfo(info *markers.TypeInfo) *schemaContext {
 pkg: c.pkg,
 info: info,
 schemaRequester: c.schemaRequester,
+schemaFetcher: c.schemaFetcher,
 allowDangerousTypes: c.allowDangerousTypes,
 ignoreUnexportedFields: c.ignoreUnexportedFields,
 }
@@ -234,7 +240,9 @@ func typeToSchema(ctx *schemaContext, rawType ast.Expr) *apiext.JSONSchemaProps
 return &apiext.JSONSchemaProps{}
 }
 
-props.Description = ctx.info.Doc
+if ctx.info.Doc != "" {
+props.Description = ctx.info.Doc
+}
 
 applyMarkers(ctx, ctx.info.Markers, props, rawType)
 
@@ -270,6 +278,7 @@ func localNamedToSchema(ctx *schemaContext, ident *ast.Ident) *apiext.JSONSchema
 if aliasInfo, isAlias := typeInfo.(*types.Alias); isAlias {
 typeInfo = aliasInfo.Rhs()
 }
+
 if basicInfo, isBasic := typeInfo.(*types.Basic); isBasic {
 typ, fmt, err := builtinToType(basicInfo, ctx.allowDangerousTypes)
 if err != nil {
@@ -281,32 +290,21 @@ func localNamedToSchema(ctx *schemaContext, ident *ast.Ident) *apiext.JSONSchema
 // > Otherwise, the alias information is only in the type name, which
 // > points directly to the actual (aliased) type.
 if basicInfo.Name() != ident.Name {
-ctx.requestSchema("", ident.Name)
-link := TypeRefLink("", ident.Name)
-return &apiext.JSONSchemaProps{
-Type: typ,
-Format: fmt,
-Ref: &link,
-}
+return ctx.schemaFetcher(TypeIdent{
+Package: ctx.pkg,
+Name: ident.Name,
+})
 }
 return &apiext.JSONSchemaProps{
 Type: typ,
 Format: fmt,
 }
 }
-// NB(directxman12): if there are dot imports, this might be an external reference,
-// so use typechecking info to get the actual object
-typeNameInfo := typeInfo.(interface{ Obj() *types.TypeName }).Obj()
-pkg := typeNameInfo.Pkg()
-pkgPath := loader.NonVendorPath(pkg.Path())
-if pkg == ctx.pkg.Types {
-pkgPath = ""
-}
-ctx.requestSchema(pkgPath, typeNameInfo.Name())
-link := TypeRefLink(pkgPath, typeNameInfo.Name())
-return &apiext.JSONSchemaProps{
-Ref: &link,
-}
+
+return ctx.schemaFetcher(TypeIdent{
+Package: ctx.pkg,
+Name: ident.Name,
+})
 }
 
 // namedSchema creates a schema (ref) for an explicitly external type reference.
@@ -501,7 +499,9 @@ func structToSchema(ctx *schemaContext, structType *ast.StructType) *apiext.JSON
 } else {
 propSchema = typeToSchema(ctx.ForInfo(&markers.TypeInfo{}), field.RawField.Type)
 }
-propSchema.Description = field.Doc
+if field.Doc != "" {
+propSchema.Description = field.Doc
+}
 
 applyMarkers(ctx, field.Markers, propSchema, field.RawField)
 
@@ -513,6 +513,9 @@ func structToSchema(ctx *schemaContext, structType *ast.StructType) *apiext.JSON
 props.Properties[fieldName] = *propSchema
 }
 
+// Ensure the required fields are always listed alphabetically.
+sort.Strings(props.Required)
+
 return props
 }
 

pkg/crd/schema_test.go

@@ -64,7 +64,7 @@ func transform(t *testing.T, expr string) *apiext.JSONSchemaProps {
 pkg.NeedTypesInfo()
 failIfErrors(t, pkg.Errors)
 
-schemaContext := newSchemaContext(pkg, nil, true, false).ForInfo(&markers.TypeInfo{})
+schemaContext := newSchemaContext(pkg, nil, nil, true, false).ForInfo(&markers.TypeInfo{})
 // yick: grab the only type definition
 definedType := pkg.Syntax[0].Decls[0].(*ast.GenDecl).Specs[0].(*ast.TypeSpec).Type
 result := typeToSchema(schemaContext, definedType)

pkg/crd/testdata/cronjob_types.go

@@ -389,6 +389,30 @@ type CronJobSpec struct {
 // Test that we can add a field that can only be set to a non-default value on updates using XValidation OptionalOldSelf.
 // +kubebuilder:validation:XValidation:rule="oldSelf.hasValue() || self == 0",message="must be set to 0 on creation. can be set to any value on an update.",optionalOldSelf=true
 OnlyAllowSettingOnUpdate int32 `json:"onlyAllowSettingOnUpdate,omitempty"`
+
+// This tests that unmarkered types are handled correctly.
+// When markers are applied at the field level instead of the type level.
+// +kubebuilder:validation:Minimum=1
+// +kubebuilder:validation:Maximum=10
+LocallyBoundedInteger UnmarkeredInteger `json:"locallyBoundedInteger,omitempty"`
+
+// This tests that unmarkered types are handled correctly.
+// When markers are applied at the field level instead of the type level.
+// +kubebuilder:validation:MinLength=1
+// +kubebuilder:validation:MaxLength=10
+LocallyBoundedString UnmarkeredString `json:"locallyBoundedString,omitempty"`
+
+// This tests that field-level overrides are handled correctly
+// for local type aliases.
+// +kubebuilder:validation:MinLength=10
+// +kubebuilder:validation:MaxLength=10
+FieldLevelAliasOverride StringAliasWithValidation `json:"fieldLevelAliasOverride,omitempty"`
+
+// This tests that field-level overrides are handled correctly
+// for local type declarations.
+// +kubebuilder:validation:MinLength=10
+// +kubebuilder:validation:MaxLength=10
+FieldLevelLocalDeclarationOverride LongerString `json:"fieldLevelLocalDeclarationOverride,omitempty"`
 }
 
 type InlineAlias = EmbeddedStruct
@@ -518,6 +542,14 @@ type TotallyABool bool
 // +listType=set
 type Hosts []string
 
+// This tests that unmarkered types are handled correctly.
+// When markers are applied at the field level instead of the type level.
+type UnmarkeredInteger int32
+
+// This tests that unmarkered types are handled correctly.
+// When markers are applied at the field level instead of the type level.
+type UnmarkeredString string
+
 func (t TotallyABool) MarshalJSON() ([]byte, error) {
 if t {
 return []byte(`"true"`), nil

pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml

@@ -219,6 +219,20 @@ spec:
  This is a pointer to distinguish between explicit zero and not specified.
  format: int32
  type: integer
+ fieldLevelAliasOverride:
+ description: |-
+ This tests that field-level overrides are handled correctly
+ for local type aliases.
+ maxLength: 10
+ minLength: 10
+ type: string
+ fieldLevelLocalDeclarationOverride:
+ description: |-
+ This tests that field-level overrides are handled correctly
+ for local type declarations.
+ maxLength: 10
+ minLength: 10
+ type: string
  float64WithValidations:
  maximum: 1.5
  minimum: -0.5
@@ -8849,6 +8863,21 @@ spec:
  default: forty-two
  description: This tests that primitive defaulting can be performed.
  type: string
+ locallyBoundedInteger:
+ description: |-
+ This tests that unmarkered types are handled correctly.
+ When markers are applied at the field level instead of the type level.
+ format: int32
+ maximum: 10
+ minimum: 1
+ type: integer
+ locallyBoundedString:
+ description: |-
+ This tests that unmarkered types are handled correctly.
+ When markers are applied at the field level instead of the type level.
+ maxLength: 10
+ minLength: 1
+ type: string
  longerStringArray:
  description: This tests string alias slice item validation.
  items:
@@ -9055,10 +9084,10 @@ spec:
  and type.
  type: string
  x-kubernetes-validations:
- - message: must have good prefix
- rule: self.startsWith('good-')
  - message: must have even length
  rule: self.size() % 2 == 0
+ - message: must have good prefix
+ rule: self.startsWith('good-')
  stringWithEvenLengthAndMessageExpression:
  description: Test of the expression-based validation with messageExpression
  marker.