fix(config): preserve integer precision in Configuration unmarshaling - #277
Conversation
|
depends on Azure/ARO-HCP#6321 |
There was a problem hiding this comment.
🟡 Not ready to approve
The new UnmarshalJSON implementation can panic on valid inputs that decode to null (e.g., empty YAML), and should be guarded (with a regression test) before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR updates the types.Configuration JSON/YAML unmarshaling path to preserve integer precision when decoding into map[string]any, preventing large integer values from rendering in scientific notation during Go template execution (which can break downstream Bicep parsing).
Changes:
- Add a custom
UnmarshalJSONforConfigurationthat usesjson.Decoder.UseNumber()and normalizes numeric leaf values (json.Number→int64orfloat64). - Add recursive number normalization via
convertJSONNumbersacross nested maps and arrays. - Add unit tests covering nested/array conversion and an end-to-end YAML → template rendering case to ensure no scientific notation for large integers.
File summaries
| File | Description |
|---|---|
| config/types/configuration.go | Introduces custom JSON unmarshaling for Configuration plus recursive numeric normalization to preserve integer precision. |
| config/types/configuration_test.go | Adds tests validating numeric normalization and ensuring template rendering prints large integers without scientific notation. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
2c6d6d2 to
bf09dbc
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The current implementation can silently lose precision for large integer literals and diverges from encoding/json.Unmarshal behavior by not rejecting trailing top-level JSON values.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (2)
config/types/configuration.go:40
- UnmarshalJSON currently uses json.Decoder.Decode once and then returns nil, which can accept multiple top-level JSON values (e.g.
{...}{...}) that json.Unmarshal would reject. Add an explicit trailing-token check after the first Decode so behavior matches encoding/json.Unmarshal and malformed inputs don't get silently truncated.
dec := json.NewDecoder(bytes.NewReader(data))
dec.UseNumber()
var raw map[string]any
if err := dec.Decode(&raw); err != nil {
return err
config/types/configuration.go:70
- convertJSONNumbers falls back to json.Number.Float64() whenever Int64() fails, which will silently lose precision for large integer literals (e.g. > 2^53) and contradicts the goal of preserving integer precision. Consider only converting to float64 when the literal is actually fractional/exponent form, and otherwise keep the json.Number when it doesn’t fit in int64.
case json.Number:
if i, err := val.Int64(); err == nil {
return i
}
if f, err := val.Float64(); err == nil {
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
/assign Jan-Hendrik Boll (@janboll) |
bf09dbc to
917bfc8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
config/types/configuration.go:50
- The doc comment says this converts json.Number to int64 or float64, but the implementation can also fall back to returning a string when neither conversion succeeds. Updating the comment will keep the behavior and documentation consistent.
// convertJSONNumbers recursively walks a decoded JSON value and converts
// json.Number to int64 (whole numbers) or float64 (fractional).
| *c = nil | ||
| return nil | ||
| } | ||
| *c = Configuration(convertJSONNumbers(raw).(map[string]any)) |
There was a problem hiding this comment.
handle the cast error, don't panic
Add custom UnmarshalJSON on Configuration that uses json.Decoder with UseNumber() to prevent large integers (>= 1e6) from becoming float64 and rendering as scientific notation in Go templates (e.g. "2e+06").
917bfc8 to
09a4243
Compare
Steve Kuznetsov (stevekuznetsov)
left a comment
There was a problem hiding this comment.
/lgtm
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adalrsjr1, stevekuznetsov The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ARO-27909
What
Add custom
UnmarshalJSONonConfigurationthat usesjson.DecoderwithUseNumber(), then normalizesjson.Numbertoint64(whole numbers) orfloat64(fractional values).Why
sigs.k8s.io/yamlunmarshals YAML to JSON internally, then usesencoding/jsonto decode into Go types. When the target ismap[string]any(whichConfigurationis), all numbers becomefloat64. Go'stext/templaterendersfloat64values viafmt.Sprint, which produces scientific notation for values >= 1e6:This breaks Bicep parsing when config integers >= 1M are used in
.bicepparamtemplate files (e.g.maxActiveTimeSeries: 2000000).The fix intercepts at the unmarshal boundary —
sigs.k8s.io/yamlcallsencoding/jsonunder the hood, so the customUnmarshalJSONfires during YAML unmarshaling as well. After the fix:Testing
TestConfiguration_UnmarshalJSON_NestedAndArrays— verifiesconvertJSONNumbersrecurses into nested maps and arrays, preservingint64for integers andfloat64for fractional valuesTestConfiguration_TemplateRendering_NoScientificNotation— end-to-end: YAML unmarshal → template rendering → asserts"2000000"not"2e+06"Special notes for your reviewer
convertJSONNumbershandles all JSON shapes: maps, arrays, and leaf valuesjson.Number("1.0").Int64()fails (decimal point), so1.0correctly staysfloat64float64for integer config values need acase int64:branch — ARO-HCP has a preparatory PR for this (ARO-HCP#6321)MergeConfigurationis type-agnostic (copies values by reference), soint64types survive merges without changes