Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/crd/flatten.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ func flattenAllOfInto(dst *apiextensionsv1.JSONSchemaProps, src apiextensionsv1.
flattenAllOfInto(dstProps.Schema, *srcProps.Schema, errRec)
case "XPreserveUnknownFields":
dstField.Set(srcField)
case "XMapType":
case "XMapType", "XListType", "XListMapKeys":
dstField.Set(srcField)
case "XValidations":
dstField.Set(reflect.AppendSlice(srcField, dstField))
Expand Down
55 changes: 35 additions & 20 deletions pkg/crd/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ import (
const (
// defPrefix is the prefix used to link to definitions in the OpenAPI schema.
defPrefix = "#/definitions/"

// arrayType is the JSON schema type for arrays.
arrayType = "array"
)

// byteType is the types.Type for byte (see the types documention
Expand Down Expand Up @@ -203,7 +206,7 @@ func applyMarkers(ctx *schemaContext, markerSet markers.MarkerValues, props *api
}

for _, schemaMarker := range itemsMarkers {
if props.Type != "array" || props.Items == nil || props.Items.Schema == nil {
if props.Type != arrayType || props.Items == nil || props.Items.Schema == nil {
err := fmt.Errorf("must apply %s to an array value, found %s", schemaMarker.Name, props.Type)
ctx.pkg.AddError(loader.ErrFromNode(err, node))
} else {
Expand Down Expand Up @@ -314,26 +317,30 @@ func localNamedToSchema(ctx *schemaContext, ident *ast.Ident) *apiextensionsv1.J
ctx.requestSchema(pkgPath, typeNameInfo.Name())
link := TypeRefLink(pkgPath, typeNameInfo.Name())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
resolvedType := typeInfoRaw
if aliasInfo, isAlias := typeInfoRaw.(*types.Alias); isAlias {
resolvedType = aliasInfo.Rhs()
}

I think we need to handle the case where Go 1.23+ represents type aliases as *types.Alias instead of *types.Named. Right now if someone has type Foo = []T in an external package, isNamed is false, typ stays empty and +listType still fails — same as before this fix. Could you add the alias unwrap before the switch? That seems like a blocker for merging this one.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sivchari did you check this one ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, addressed with types.Unalias() so slice/map aliases resolve correctly and +listType/+mapType apply

// 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 {
props := &apiextensionsv1.JSONSchemaProps{
Ref: &link,
}

// Set the type for slice and map aliases so field-level markers like +listType
// and +mapType can work. Structs are skipped — they use a $ref only.
// Go 1.23+ (gotypesalias=1) represents `type Foo = []T` as *types.Alias, so
// unalias first to reach the real underlying type.
switch types.Unalias(typeInfo.(types.Type)).Underlying().(type) {
case *types.Slice:
props.Type = arrayType
case *types.Map:
//nolint:goconst
props.Type = "object"
case *types.Basic:
// 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
}
namedTypeInfo := ctx.schemaRequester.LookupType(ctx.pkg, typeNameInfo.Name())
namedSchema := infoToSchema(ctx.ForInfo(namedTypeInfo))
props.Type = namedSchema.Type
props.Format = namedSchema.Format
}

return &apiextensionsv1.JSONSchemaProps{
Type: typ,
Format: fmt,
Ref: &link,
}
return props
default:
ctx.pkg.AddError(loader.ErrFromNode(fmt.Errorf("unsupported type %T for identifier %s", typeInfo, ident.Name), ident))
return &apiextensionsv1.JSONSchemaProps{}
Expand All @@ -356,10 +363,19 @@ func namedToSchema(ctx *schemaContext, named *ast.SelectorExpr) *apiextensionsv1
nonVendorPath := loader.NonVendorPath(typeNameInfo.Pkg().Path())
ctx.requestSchema(nonVendorPath, typeNameInfo.Name())
link := TypeRefLink(nonVendorPath, typeNameInfo.Name())
return &apiextensionsv1.JSONSchemaProps{
// Set the type for slice and map aliases so field-level markers like +listType
// and +mapType can work. Structs are skipped — they use a $ref only.
props := &apiextensionsv1.JSONSchemaProps{
Ref: &link,
}
switch typeInfoRaw.Underlying().(type) {
case *types.Slice:
props.Type = arrayType
case *types.Map:
props.Type = "object"
}
// NB(directxman12): we special-case things like resource.Quantity during the "collapse" phase.
return props
}

// arrayToSchema creates a schema for the items of the given array, dealing appropriately
Expand All @@ -378,7 +394,7 @@ func arrayToSchema(ctx *schemaContext, array *ast.ArrayType) *apiextensionsv1.JS
items := typeToSchema(ctx.ForInfo(&markers.TypeInfo{}), array.Elt)

return &apiextensionsv1.JSONSchemaProps{
Type: "array",
Type: arrayType,
Items: &apiextensionsv1.JSONSchemaPropsOrArray{Schema: items},
}
}
Expand Down Expand Up @@ -436,7 +452,6 @@ func mapToSchema(ctx *schemaContext, mapType *ast.MapType) *apiextensionsv1.JSON
return &apiextensionsv1.JSONSchemaProps{}
}

//nolint:goconst
return &apiextensionsv1.JSONSchemaProps{
Type: "object",
AdditionalProperties: &apiextensionsv1.JSONSchemaPropsOrBool{
Expand Down
25 changes: 25 additions & 0 deletions pkg/crd/testdata/cronjob_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,10 +215,27 @@ type CronJobSpec struct {
// This tests that associative lists work via a nested type.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR adds props.Type = "object" for map aliases to make +mapType work at the field level, but there's no test covering that path. Could you add a field with a map alias type and +mapType on it? Without it a regression there would go unnoticed. Could you please add ?

NestedAssociativeList NestedAssociativeList `json:"nestedassociativeList"`

// This tests that +listType can be applied at the field level for type aliases (issue #988).
// +listType=map
// +listMapKey=name
// +listMapKey=secondary
FieldLevelListType ConditionsWithoutMarker `json:"fieldLevelListType"`

// This tests that a field-level +listType combined with a type-level +listType
// is preserved at the top level instead of being wiped into allOf (issue #988).
// +listType=map
// +listMapKey=name
// +listMapKey=secondary
ListTypeFromTypeAndField NestedAssociativeList `json:"listTypeFromTypeAndField"`

// A map that allows different actors to manage different fields
// +mapType=granular
MapOfInfo map[string][]byte `json:"mapOfInfo"`

// This tests that +mapType can be applied at the field level for type aliases (issue #988).
// +mapType=granular
FieldLevelMapType MapWithoutMarker `json:"fieldLevelMapType"`

// A map that allows different actors to manage different fields via a nested type.
NestedMapOfInfo NestedMapOfInfo `json:"nestedMapOfInfo"`

Expand Down Expand Up @@ -562,6 +579,14 @@ type NestedAssociativeList []AssociativeType
// +mapType=granular
type NestedMapOfInfo map[string][]byte

// ConditionsWithoutMarker is a type alias to a slice without type-level markers.
// This tests that +listType can be applied at the field level (issue #988).
type ConditionsWithoutMarker []AssociativeType

// MapWithoutMarker is a type alias to a map without type-level markers.
// This tests that +mapType can be applied at the field level (issue #988).
type MapWithoutMarker map[string][]byte

// +kubebuilder:validation:MinLength=4
// This tests that markers that are allowed on both fields and types are applied to types
type LongerString string

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I was looking at flattenAllOfInto and noticed XListType and XListMapKeys don't have their own case there. If both the type and the field have +listType set, they'll hit default: and the annotation gets wiped from the top level of the CRD.

Could it not be a silent failure? XMapType has its own case already so it handles this fine, could we do the same for these two?

Expand Down
54 changes: 54 additions & 0 deletions pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,27 @@ spec:
This tests that field-level overrides are handled correctly
for local type aliases.
type: string
fieldLevelListType:
description: 'This tests that +listType can be applied at the field
level for type aliases (issue #988).'
items:
properties:
foo:
type: string
name:
type: string
secondary:
type: integer
required:
- foo
- name
- secondary
type: object
type: array
x-kubernetes-list-map-keys:
- name
- secondary
x-kubernetes-list-type: map
fieldLevelLocalDeclarationOverride:
allOf:
- minLength: 4
Expand All @@ -244,6 +265,14 @@ spec:
for local type declarations.
maxLength: 10
type: string
fieldLevelMapType:
additionalProperties:
format: byte
type: string
description: 'This tests that +mapType can be applied at the field
level for type aliases (issue #988).'
type: object
x-kubernetes-map-type: granular
float64WithValidations:
maximum: 1.5
minimum: -0.5
Expand Down Expand Up @@ -9429,6 +9458,28 @@ spec:
default: forty-two
description: This tests that primitive defaulting can be performed.
type: string
listTypeFromTypeAndField:
description: |-
This tests that a field-level +listType combined with a type-level +listType
is preserved at the top level instead of being wiped into allOf (issue #988).
items:
properties:
foo:
type: string
name:
type: string
secondary:
type: integer
required:
- foo
- name
- secondary
type: object
type: array
x-kubernetes-list-map-keys:
- name
- secondary
x-kubernetes-list-type: map
locallyBoundedInteger:
description: |-
This tests that unmarkered types are handled correctly.
Expand Down Expand Up @@ -9752,6 +9803,8 @@ spec:
- explicitlyRequiredK8s
- explicitlyRequiredKubebuilder
- explicitlyRequiredKubernetes
- fieldLevelListType
- fieldLevelMapType
- float64WithValidations
- floatWithValidations
- foo
Expand All @@ -9764,6 +9817,7 @@ spec:
- kubernetesDefaultedObject
- kubernetesDefaultedSlice
- kubernetesDefaultedString
- listTypeFromTypeAndField
- mapOfInfo
- nestedMapOfInfo
- nestedStructWithSeveralFields
Expand Down