xrd: Opportunistically infer integer types when generating from an XR#108
Conversation
When generating an XRD from an example XR, we have to infer the type of each field from the example input. We do this by unmarshalling into a `map[string]any` and then observing the actual type of each value. Since the JSON specification has only a single numerical type, which is floating point, `json.Unmarshal` treats all numbers as floats. This means a field like `replicas: 3` in an example XR produces a `number` field in the OpenAPI spec when an `integer` field would be more appropriate. Detect integers by comparing the truncation of the value to the original value. This is an imperfect heuristic since not all integers are representable as floats, but it will work for common cases (small integers) and produce less surprising behavior for users. Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
📝 WalkthroughWalkthroughThe ChangesJSON schema type inference improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/xrd/infer_test.go (1)
56-61: ⚡ Quick winPlease add one regression case for mixed numeric floats in arrays.
Great addition for scalar inference. Could you also add a case that exercises
[]any{float64(1), float64(1.5)}(or equivalent viaInferProperties) so we lock in intended numeric-array behavior and prevent silent regressions here?🤖 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_test.go` around lines 56 - 61, Add a new regression test case as a sibling to the IntegerAsFloatType test case in the same test structure. The new test case should exercise mixed numeric floats in an array input, such as []any{float64(1), float64(1.5)}, either by passing it directly as input or using InferProperties, and should specify the expected JSONSchemaProps output to lock in the intended numeric-array behavior. This ensures silent regressions in array numeric type inference are prevented.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@internal/xrd/infer.go`:
- Around line 111-133: The heuristic change now causes array type inference to
fail when arrays contain both whole-number floats (inferred as "integer") and
decimal floats (inferred as "number"). Fix the type compatibility logic in the
inferArrayProperty function (referenced at Line 74) to treat "integer" and
"number" types as compatible rather than conflicting. When merging array element
types, if both "integer" and "number" types are present, promote the merged
result to "number" type to allow numeric arrays with mixed whole and decimal
numbers.
---
Nitpick comments:
In `@internal/xrd/infer_test.go`:
- Around line 56-61: Add a new regression test case as a sibling to the
IntegerAsFloatType test case in the same test structure. The new test case
should exercise mixed numeric floats in an array input, such as
[]any{float64(1), float64(1.5)}, either by passing it directly as input or using
InferProperties, and should specify the expected JSONSchemaProps output to lock
in the intended numeric-array behavior. This ensures silent regressions in array
numeric type inference are prevented.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a22cff0d-cfa0-4202-859c-7852f75c86b0
📒 Files selected for processing (2)
internal/xrd/infer.gointernal/xrd/infer_test.go
If an array in an example XR contains a mix of integers and floats, use "number" as the array's element type rather than returning an error. Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/xrd/infer_test.go (1)
180-183: ⚡ Quick winSwitch error assertions to
cmp.Diff+cmpopts.EquateErrors()for guideline compliance.Nice coverage additions here. Could we align the error checks with the repo’s test pattern? Comparing
err.Error()strings is brittle and bypasses the prescribedcmpopts.EquateErrors()flow.Suggested patch
import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ - if err != nil || tc.want.err != nil { - if err == nil || tc.want.err == nil || err.Error() != tc.want.err.Error() { - t.Errorf("inferProperty() error = %v, wantErr %v", err, tc.want.err) - } - return - } + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { + t.Errorf("inferProperty() -wantErr, +gotErr:\n%s", diff) + return + } @@ - if err != nil || tc.want.err != nil { - if err == nil || tc.want.err == nil || err.Error() != tc.want.err.Error() { - t.Errorf("InferProperties() error = %v, wantErr %v", err, tc.want.err) - } - return - } + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { + t.Errorf("InferProperties() -wantErr, +gotErr:\n%s", diff) + return + }As per coding guidelines for
**/*_test.go, error testing should usecmp.Diffwithcmpopts.EquateErrors().Also applies to: 263-266
🤖 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_test.go` around lines 180 - 183, Replace the error string comparison logic in the inferProperty test function with the repository's prescribed testing pattern. At lines 180-183 and also at lines 263-266, replace the manual error comparison using err.Error() string comparison with cmp.Diff combined with cmpopts.EquateErrors(). Instead of checking if err == nil, tc.want.err == nil, and comparing error strings, use cmp.Diff to compare the errors directly with EquateErrors option to properly validate error values according to the repo's testing guidelines for test files.Source: Coding guidelines
🤖 Prompt for all review comments with 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.
Inline comments:
In `@internal/xrd/infer.go`:
- Around line 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.
---
Nitpick comments:
In `@internal/xrd/infer_test.go`:
- Around line 180-183: Replace the error string comparison logic in the
inferProperty test function with the repository's prescribed testing pattern. At
lines 180-183 and also at lines 263-266, replace the manual error comparison
using err.Error() string comparison with cmp.Diff combined with
cmpopts.EquateErrors(). Instead of checking if err == nil, tc.want.err == nil,
and comparing error strings, use cmp.Diff to compare the errors directly with
EquateErrors option to properly validate error values according to the repo's
testing guidelines for test files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 00bee349-8dc3-4de3-8407-21c7c1cd9339
📒 Files selected for processing (2)
internal/xrd/infer.gointernal/xrd/infer_test.go
| if elemSchema.Type != firstElemSchema.Type { | ||
| return extv1.JSONSchemaProps{}, errors.New("mixed types detected in array") | ||
| } |
There was a problem hiding this comment.
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.
| 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
|
Successfully created backport PR for |
2 participants
Description of your changes
When generating an XRD from an example XR, we have to infer the type of each field from the example input. We do this by unmarshalling into a
map[string]anyand then observing the actual type of each value.Since the JSON specification has only a single numerical type, which is floating point,
json.Unmarshaltreats all numbers as floats. This means a field likereplicas: 3in an example XR produces anumberfield in the OpenAPI spec when anintegerfield would be more appropriate.Detect integers by comparing the truncation of the value to the original value. This is an imperfect heuristic since not all integers are representable as floats, but it will work for common cases (small integers) and produce less surprising behavior for users.
I have:
./nix.sh flake checkto ensure this PR is ready for review.- [ ] Linked a PR or a docs tracking issue to document this change.backport release-x.ylabels to auto-backport this PR.