Skip to content
Merged
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
65 changes: 52 additions & 13 deletions internal/xrd/infer.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,22 @@ package xrd
import (
"fmt"
"maps"
"math"

extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"

"github.com/crossplane/crossplane-runtime/v2/pkg/errors"
)

const (
schemaTypeArray = "array"
schemaTypeBoolean = "boolean"
schemaTypeInteger = "integer"
schemaTypeNumber = "number"
schemaTypeObject = "object"
schemaTypeString = "string"
)

// InferProperties infers JSON schema properties from a map of values.
func InferProperties(spec map[string]any) (map[string]extv1.JSONSchemaProps, error) {
properties := make(map[string]extv1.JSONSchemaProps)
Expand All @@ -46,10 +56,10 @@ func InferProperties(spec map[string]any) (map[string]extv1.JSONSchemaProps, err
func inferArrayProperty(v []any) (extv1.JSONSchemaProps, error) {
if len(v) == 0 {
return extv1.JSONSchemaProps{
Type: "array",
Type: schemaTypeArray,
Items: &extv1.JSONSchemaPropsOrArray{
Schema: &extv1.JSONSchemaProps{
Type: "object",
Type: schemaTypeObject,
},
},
}, nil
Expand All @@ -61,7 +71,7 @@ func inferArrayProperty(v []any) (extv1.JSONSchemaProps, error) {
}

mergedProperties := make(map[string]extv1.JSONSchemaProps)
if firstElemSchema.Type == "object" {
if firstElemSchema.Type == schemaTypeObject {
maps.Copy(mergedProperties, firstElemSchema.Properties)
}

Expand All @@ -70,21 +80,31 @@ func inferArrayProperty(v []any) (extv1.JSONSchemaProps, error) {
if err != nil {
return extv1.JSONSchemaProps{}, err
}

// If an array contains a mix of numbers and integers, use number as the
// array type.
if elemSchema.Type == schemaTypeInteger && firstElemSchema.Type == schemaTypeNumber {
continue
}
if elemSchema.Type == schemaTypeNumber && firstElemSchema.Type == schemaTypeInteger {
firstElemSchema.Type = schemaTypeNumber
}

if elemSchema.Type != firstElemSchema.Type {
return extv1.JSONSchemaProps{}, errors.New("mixed types detected in array")
}
Comment on lines 93 to 95

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Improve mixed-array error to include conflicting types and a next step (Line 94).

Thanks for implementing numeric-type reconciliation here — one thing to tighten is the remaining mixed-type error path. Right now mixed types detected in array is pretty opaque for CLI users. Could we include both observed types and guidance (e.g., “make array element types consistent”) so users can fix their XR quickly?

Suggested patch
-		if elemSchema.Type != firstElemSchema.Type {
-			return extv1.JSONSchemaProps{}, errors.New("mixed types detected in array")
-		}
+		if elemSchema.Type != firstElemSchema.Type {
+			return extv1.JSONSchemaProps{}, errors.Errorf(
+				"cannot infer schema for array: found mixed element types (%q and %q); make all elements use a consistent type",
+				firstElemSchema.Type, elemSchema.Type,
+			)
+		}

As per coding guidelines, error messages should be meaningful to end users, include context, and suggest next steps.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if elemSchema.Type != firstElemSchema.Type {
return extv1.JSONSchemaProps{}, errors.New("mixed types detected in array")
}
if elemSchema.Type != firstElemSchema.Type {
return extv1.JSONSchemaProps{}, errors.Errorf(
"cannot infer schema for array: found mixed element types (%q and %q); make all elements use a consistent type",
firstElemSchema.Type, elemSchema.Type,
)
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/xrd/infer.go` around lines 93 - 95, The error message in the
mixed-type detection block for arrays in the inferSchema function is too generic
and doesn't provide users with actionable information. Enhance the error message
returned when elemSchema.Type differs from firstElemSchema.Type by including the
actual conflicting types being detected (extract the Type field values from both
elemSchema and firstElemSchema) and append a clear next step such as "ensure all
array elements have consistent types" to help users understand how to resolve
the issue quickly.

Source: Coding guidelines

if elemSchema.Type == "object" {
if elemSchema.Type == schemaTypeObject {
maps.Copy(mergedProperties, elemSchema.Properties)
}
}

resultSchema := firstElemSchema
if firstElemSchema.Type == "object" && len(mergedProperties) > 0 {
if firstElemSchema.Type == schemaTypeObject && len(mergedProperties) > 0 {
resultSchema.Properties = mergedProperties
}

return extv1.JSONSchemaProps{
Type: "array",
Type: schemaTypeArray,
Items: &extv1.JSONSchemaPropsOrArray{
Schema: &resultSchema,
},
Expand All @@ -94,34 +114,53 @@ func inferArrayProperty(v []any) (extv1.JSONSchemaProps, error) {
func inferProperty(value any) (extv1.JSONSchemaProps, error) {
if value == nil {
return extv1.JSONSchemaProps{
Type: "string",
Type: schemaTypeString,
}, nil
}

switch v := value.(type) {
case string:
return extv1.JSONSchemaProps{
Type: "string",
Type: schemaTypeString,
}, nil
case int, int32, int64:
return extv1.JSONSchemaProps{
Type: "integer",
Type: schemaTypeInteger,
}, nil
case float32, float64:
case float32:
// JSON doesn't have integers, so json.Unmarshal treats all numbers as
// floats. Try to detect whehter the number is actually an integer, so
// that we're more likely to infer the user's intent. This heuristic
// isn't perfect since not all integers are representable as floats, but
// it will work for common cases.
t := schemaTypeNumber
if math.Trunc(float64(v)) == float64(v) {
t = schemaTypeInteger
}

return extv1.JSONSchemaProps{
Type: t,
}, nil
case float64:
t := schemaTypeNumber
if math.Trunc(v) == v {
t = schemaTypeInteger
}

return extv1.JSONSchemaProps{
Type: "number",
Type: t,
}, nil
Comment thread
coderabbitai[bot] marked this conversation as resolved.
case bool:
return extv1.JSONSchemaProps{
Type: "boolean",
Type: schemaTypeBoolean,
}, nil
case map[string]any:
inferredProps, err := InferProperties(v)
if err != nil {
return extv1.JSONSchemaProps{}, errors.Wrap(err, "error inferring properties for object")
}
return extv1.JSONSchemaProps{
Type: "object",
Type: schemaTypeObject,
Properties: inferredProps,
}, nil
case []any:
Expand Down
72 changes: 50 additions & 22 deletions internal/xrd/infer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,25 +38,31 @@ func TestInferProperty(t *testing.T) {
"StringType": {
input: "hello",
want: want{
output: extv1.JSONSchemaProps{Type: "string"},
output: extv1.JSONSchemaProps{Type: schemaTypeString},
},
},
"IntegerType": {
input: 42,
want: want{
output: extv1.JSONSchemaProps{Type: "integer"},
output: extv1.JSONSchemaProps{Type: schemaTypeInteger},
},
},
"FloatType": {
input: 3.14,
want: want{
output: extv1.JSONSchemaProps{Type: "number"},
output: extv1.JSONSchemaProps{Type: schemaTypeNumber},
},
},
"IntegerAsFloatType": {
input: float64(1),
want: want{
output: extv1.JSONSchemaProps{Type: schemaTypeInteger},
},
},
"BooleanType": {
input: true,
want: want{
output: extv1.JSONSchemaProps{Type: "boolean"},
output: extv1.JSONSchemaProps{Type: schemaTypeBoolean},
},
},
"ObjectType": {
Expand All @@ -65,9 +71,9 @@ func TestInferProperty(t *testing.T) {
},
want: want{
output: extv1.JSONSchemaProps{
Type: "object",
Type: schemaTypeObject,
Properties: map[string]extv1.JSONSchemaProps{
"key": {Type: "string"},
"key": {Type: schemaTypeString},
},
},
},
Expand All @@ -76,9 +82,31 @@ func TestInferProperty(t *testing.T) {
input: []any{"one", "two"},
want: want{
output: extv1.JSONSchemaProps{
Type: "array",
Type: schemaTypeArray,
Items: &extv1.JSONSchemaPropsOrArray{
Schema: &extv1.JSONSchemaProps{Type: schemaTypeString},
},
},
},
},
"ArrayWithMixedNumbersIntegerFirst": {
input: []any{1, float32(3.14)},
want: want{
output: extv1.JSONSchemaProps{
Type: schemaTypeArray,
Items: &extv1.JSONSchemaPropsOrArray{
Schema: &extv1.JSONSchemaProps{Type: schemaTypeNumber},
},
},
},
},
"ArrayWithMixedNumbersFloatFirst": {
input: []any{float32(3.14), 1},
want: want{
output: extv1.JSONSchemaProps{
Type: schemaTypeArray,
Items: &extv1.JSONSchemaPropsOrArray{
Schema: &extv1.JSONSchemaProps{Type: "string"},
Schema: &extv1.JSONSchemaProps{Type: schemaTypeNumber},
},
},
},
Expand All @@ -87,17 +115,17 @@ func TestInferProperty(t *testing.T) {
input: []any{},
want: want{
output: extv1.JSONSchemaProps{
Type: "array",
Type: schemaTypeArray,
Items: &extv1.JSONSchemaPropsOrArray{
Schema: &extv1.JSONSchemaProps{Type: "object"},
Schema: &extv1.JSONSchemaProps{Type: schemaTypeObject},
},
},
},
},
"NilValue": {
input: nil,
want: want{
output: extv1.JSONSchemaProps{Type: "string"},
output: extv1.JSONSchemaProps{Type: schemaTypeString},
},
},
"ArrayWithMixedTypes": {
Expand All @@ -123,20 +151,20 @@ func TestInferProperty(t *testing.T) {
},
want: want{
output: extv1.JSONSchemaProps{
Type: "array",
Type: schemaTypeArray,
Items: &extv1.JSONSchemaPropsOrArray{
Schema: &extv1.JSONSchemaProps{
Type: "object",
Type: schemaTypeObject,
Properties: map[string]extv1.JSONSchemaProps{
"name": {Type: "string"},
"cidr": {Type: "string"},
"name": {Type: schemaTypeString},
"cidr": {Type: schemaTypeString},
"serviceEndpoints": {
Type: "array",
Items: &extv1.JSONSchemaPropsOrArray{
Schema: &extv1.JSONSchemaProps{Type: "string"},
Schema: &extv1.JSONSchemaProps{Type: schemaTypeString},
},
},
"delegation": {Type: "string"},
"delegation": {Type: schemaTypeString},
},
},
},
Expand Down Expand Up @@ -180,8 +208,8 @@ func TestInferProperties(t *testing.T) {
},
want: want{
output: map[string]extv1.JSONSchemaProps{
"key1": {Type: "string"},
"key2": {Type: "integer"},
"key1": {Type: schemaTypeString},
"key2": {Type: schemaTypeInteger},
},
},
},
Expand All @@ -194,9 +222,9 @@ func TestInferProperties(t *testing.T) {
want: want{
output: map[string]extv1.JSONSchemaProps{
"nested": {
Type: "object",
Type: schemaTypeObject,
Properties: map[string]extv1.JSONSchemaProps{
"key": {Type: "boolean"},
"key": {Type: schemaTypeBoolean},
},
},
},
Expand All @@ -211,7 +239,7 @@ func TestInferProperties(t *testing.T) {
"array": {
Type: "array",
Items: &extv1.JSONSchemaPropsOrArray{
Schema: &extv1.JSONSchemaProps{Type: "string"},
Schema: &extv1.JSONSchemaProps{Type: schemaTypeString},
},
},
},
Expand Down
Loading