Skip to content

fix(config): preserve integer precision in Configuration unmarshaling - #277

Merged
openshift-merge-bot[bot] merged 1 commit into
Azure:mainfrom
adalrsjr1:ARO-27909
Aug 14, 2026
Merged

fix(config): preserve integer precision in Configuration unmarshaling#277
openshift-merge-bot[bot] merged 1 commit into
Azure:mainfrom
adalrsjr1:ARO-27909

Conversation

@adalrsjr1

Copy link
Copy Markdown
Contributor

ARO-27909

What

Add custom UnmarshalJSON on Configuration that uses json.Decoder with UseNumber(), then normalizes json.Number to int64 (whole numbers) or float64 (fractional values).

Why

sigs.k8s.io/yaml unmarshals YAML to JSON internally, then uses encoding/json to decode into Go types. When the target is map[string]any (which Configuration is), all numbers become float64. Go's text/template renders float64 values via fmt.Sprint, which produces scientific notation for values >= 1e6:

fmt.Sprint(float64(2000000)) → "2e+06"

This breaks Bicep parsing when config integers >= 1M are used in .bicepparam template files (e.g. maxActiveTimeSeries: 2000000).

The fix intercepts at the unmarshal boundary — sigs.k8s.io/yaml calls encoding/json under the hood, so the custom UnmarshalJSON fires during YAML unmarshaling as well. After the fix:

fmt.Sprint(int64(2000000)) → "2000000"

Testing

  • TestConfiguration_UnmarshalJSON_NestedAndArrays — verifies convertJSONNumbers recurses into nested maps and arrays, preserving int64 for integers and float64 for fractional values
  • TestConfiguration_TemplateRendering_NoScientificNotation — end-to-end: YAML unmarshal → template rendering → asserts "2000000" not "2e+06"

Special notes for your reviewer

  • convertJSONNumbers handles all JSON shapes: maps, arrays, and leaf values
  • json.Number("1.0").Int64() fails (decimal point), so 1.0 correctly stays float64
  • Downstream consumers that type-switch on float64 for integer config values need a case int64: branch — ARO-HCP has a preparatory PR for this (ARO-HCP#6321)
  • MergeConfiguration is type-agnostic (copies values by reference), so int64 types survive merges without changes

Copilot AI lite review requested due to automatic review settings July 29, 2026 20:16
@adalrsjr1

Copy link
Copy Markdown
Contributor Author

depends on Azure/ARO-HCP#6321
/hold

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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 UnmarshalJSON for Configuration that uses json.Decoder.UseNumber() and normalizes numeric leaf values (json.Numberint64 or float64).
  • Add recursive number normalization via convertJSONNumbers across 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.

Comment thread config/types/configuration.go
Comment thread config/types/configuration_test.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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.

@adalrsjr1

Copy link
Copy Markdown
Contributor Author

/assign Jan-Hendrik Boll (@janboll)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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).

Comment thread config/types/configuration.go Outdated
*c = nil
return nil
}
*c = Configuration(convertJSONNumbers(raw).(map[string]any))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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").

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci

openshift-ci Bot commented Aug 14, 2026

Copy link
Copy Markdown

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot
openshift-merge-bot Bot merged commit 45732b6 into Azure:main Aug 14, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants