Merge pull request #1310 from JoelSpeed/revert-1270

⚠️  Revert local override breaking behavioural change

Author: k8s-ci-robot

pkg/crd/parser.go

@@ -172,12 +172,7 @@ 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, func(typ TypeIdent) *apiext.JSONSchemaProps {
-		p.NeedSchemaFor(typ)
-
-		props := p.Schemata[typ]
-		return &props
-	}, p.AllowDangerousTypes, p.IgnoreUnexportedFields)
+	schemaCtx := newSchemaContext(typ.Package, p, p.AllowDangerousTypes, p.IgnoreUnexportedFields)
 	ctxForInfo := schemaCtx.ForInfo(info)
 
 	pkgMarkers, err := markers.PackageMarkers(p.Collector, typ.Package)

pkg/crd/schema.go

@@ -57,12 +57,10 @@ 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)
+	LookupType(pkg *loader.Package, name string) *markers.TypeInfo
 }
 
 // schemaContext stores and provides information across a hierarchy of schema generation.
@@ -71,7 +69,6 @@ type schemaContext struct {
 	info *markers.TypeInfo
 
 	schemaRequester schemaRequester
-	schemaFetcher   schemaFetcher
 	PackageMarkers  markers.MarkerValues
 
 	allowDangerousTypes    bool
@@ -80,12 +77,11 @@ 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, fetcher schemaFetcher, allowDangerousTypes, ignoreUnexportedFields bool) *schemaContext {
+func newSchemaContext(pkg *loader.Package, req schemaRequester, allowDangerousTypes, ignoreUnexportedFields bool) *schemaContext {
 	pkg.NeedTypesInfo()
 	return &schemaContext{
 		pkg:                    pkg,
 		schemaRequester:        req,
-		schemaFetcher:          fetcher,
 		allowDangerousTypes:    allowDangerousTypes,
 		ignoreUnexportedFields: ignoreUnexportedFields,
 	}
@@ -98,7 +94,6 @@ 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,
 	}
@@ -240,9 +235,7 @@ func typeToSchema(ctx *schemaContext, rawType ast.Expr) *apiext.JSONSchemaProps
 		return &apiext.JSONSchemaProps{}
 	}
 
-	if ctx.info.Doc != "" {
-		props.Description = ctx.info.Doc
-	}
+	props.Description = ctx.info.Doc
 
 	applyMarkers(ctx, ctx.info.Markers, props, rawType)
 
@@ -278,7 +271,6 @@ 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 {
@@ -290,21 +282,50 @@ 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 {
-			return ctx.schemaFetcher(TypeIdent{
-				Package: ctx.pkg,
-				Name:    ident.Name,
-			})
+			ctx.requestSchema("", ident.Name)
+			link := TypeRefLink("", ident.Name)
+			return &apiext.JSONSchemaProps{
+				Type:   typ,
+				Format: fmt,
+				Ref:    &link,
+			}
 		}
 		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())
+
+	// In cases where we have a named type, apply the type and format from the named schema
+	// to allow markers that need this information to apply correctly.
+	var typ, fmt string
+	if namedInfo, isNamed := typeInfo.(*types.Named); isNamed {
+		// We don't want/need to do this for structs, maps, or arrays.
+		// These are already handled in infoToSchema if they have custom marshalling.
+		if _, isBasic := namedInfo.Underlying().(*types.Basic); isBasic {
+			namedTypeInfo := ctx.schemaRequester.LookupType(ctx.pkg, namedInfo.Obj().Name())
+
+			namedSchema := infoToSchema(ctx.ForInfo(namedTypeInfo))
+			typ = namedSchema.Type
+			fmt = namedSchema.Format
+		}
+	}
 
-	return ctx.schemaFetcher(TypeIdent{
-		Package: ctx.pkg,
-		Name:    ident.Name,
-	})
+	return &apiext.JSONSchemaProps{
+		Type:   typ,
+		Format: fmt,
+		Ref:    &link,
+	}
 }
 
 // namedSchema creates a schema (ref) for an explicitly external type reference.
@@ -504,9 +525,7 @@ func structToSchema(ctx *schemaContext, structType *ast.StructType) *apiext.JSON
 		} else {
 			propSchema = typeToSchema(ctx.ForInfo(&markers.TypeInfo{}), field.RawField.Type)
 		}
-		if field.Doc != "" {
-			propSchema.Description = field.Doc
-		}
+		propSchema.Description = field.Doc
 
 		applyMarkers(ctx, field.Markers, propSchema, field.RawField)
 

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, nil, true, false).ForInfo(&markers.TypeInfo{})
+	schemaContext := newSchemaContext(pkg, 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/testdata.kubebuilder.io_cronjobs.yaml

@@ -226,18 +226,23 @@ spec:
                 format: int32
                 type: integer
               fieldLevelAliasOverride:
+                allOf:
+                - maxLength: 255
+                  minLength: 1
+                - maxLength: 10
+                  minLength: 10
                 description: |-
                   This tests that field-level overrides are handled correctly
                   for local type aliases.
-                maxLength: 10
-                minLength: 10
                 type: string
               fieldLevelLocalDeclarationOverride:
+                allOf:
+                - minLength: 4
+                - minLength: 10
                 description: |-
                   This tests that field-level overrides are handled correctly
                   for local type declarations.
                 maxLength: 10
-                minLength: 10
                 type: string
               float64WithValidations:
                 maximum: 1.5
@@ -9098,10 +9103,10 @@ spec:
                   and type.
                 type: string
                 x-kubernetes-validations:
-                - message: must have even length
-                  rule: self.size() % 2 == 0
                 - message: must have good prefix
                   rule: self.startsWith('good-')
+                - message: must have even length
+                  rule: self.size() % 2 == 0
               stringWithEvenLengthAndMessageExpression:
                 description: Test of the expression-based validation with messageExpression
                   marker.