[master] Support integer values for float fields in structured configurable variables#44567
Open
warunalakshitha wants to merge 5 commits into
Open
Conversation
📝 WalkthroughWalkthroughTOML numeric-to-Ballerina conversion now distinguishes float and decimal sources (uses Number.doubleValue() for floats and type-aware BigDecimal construction for decimals). Adds a new ChangesCricketer configurables + numeric conversion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/configurable/providers/toml/ConfigValueCreator.java`:
- Line 328: The DECIMAL_TAG branch in ConfigValueCreator currently casts
tomlValue to Double which fails when Utils.checkEffectiveTomlType allowed an
INTEGER (Long); update the DECIMAL_TAG case to handle both Long and Double (and
any existing BigDecimal) by checking tomlValue's runtime type and creating the
BigDecimal accordingly (e.g., BigDecimal.valueOf((Long) tomlValue) for Long,
BigDecimal.valueOf((Double) tomlValue) for Double, or using the value directly
if already a BigDecimal) before calling ValueCreator.createDecimalValue; also
add a test for a decimal field populated with an integer TOML literal to prevent
regressions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f61e54c1-076d-4606-ba3d-6f62c8d1ff4a
📒 Files selected for processing (5)
bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/configurable/providers/toml/ConfigValueCreator.javatests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/Config_records.tomltests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/Config_records_inline.tomltests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/configRecordType/records.baltests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/configRecordType/types.bal
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/configurable/providers/toml/ConfigValueCreator.java`:
- Around line 328-331: The decimal conversion currently loses precision by
always calling ((Number) tomlValue).doubleValue() before constructing
BigDecimal; update the TypeTags.DECIMAL_TAG branch in ConfigValueCreator (and
mirror the same change in TomlProvider.getAsDecimalAndMark) to inspect the
runtime type of tomlValue and construct the BigDecimal from the most exact
representation: if tomlValue is a Long (or Integer/Short) use
BigDecimal.valueOf(((Number) tomlValue).longValue()), if it is a BigInteger use
new BigDecimal((BigInteger) tomlValue), otherwise fall back to
BigDecimal.valueOf(((Number) tomlValue).doubleValue()) for floating inputs;
ensure you then pass that BigDecimal into ValueCreator.createDecimalValue so
large integers keep exact precision.
In
`@bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/configurable/providers/toml/TomlProvider.java`:
- Around line 243-244: The decimal conversion in TomlProvider uses
BigDecimal.valueOf(((Number) value).doubleValue()) which loses precision for
large integral Numbers; update the conversion in TomlProvider so it constructs
the BigDecimal from the integral representation instead of from double: if value
is a Long (or Integer) use BigDecimal.valueOf(((Number) value).longValue()), if
it can be a BigInteger use new BigDecimal((BigInteger) value), otherwise fall
back to new BigDecimal(value.toString()); then call
getTomlConfigValue(ValueCreator.createDecimalValue(decimalValue), key). Also add
a unit test binding a large integer TOML literal (e.g. 2^53+1) to a configurable
decimal to prevent regressions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9db79d2-cb11-4db1-ad2d-cdbea1ea6675
📒 Files selected for processing (6)
bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/configurable/providers/toml/ConfigValueCreator.javabvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/configurable/providers/toml/TomlProvider.javatests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/Config_records.tomltests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/Config_records_inline.tomltests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/configRecordType/records.baltests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/configRecordType/types.bal
✅ Files skipped from review due to trivial changes (2)
- tests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/Config_records.toml
- tests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/configRecordType/types.bal
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/Config_records_inline.toml
- tests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/configRecordType/records.bal
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44567 +/- ##
============================================
- Coverage 75.03% 75.02% -0.01%
- Complexity 58707 58711 +4
============================================
Files 3601 3601
Lines 227161 227168 +7
Branches 29581 29583 +2
============================================
+ Hits 170443 170444 +1
- Misses 47215 47218 +3
- Partials 9503 9506 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
warunalakshitha
force-pushed
the
int_values_to_float_configs
branch
from
249eed9 to
6d525b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/configurable/providers/toml/TomlProvider.java`:
- Around line 243-249: The getAsFloatAndMark() path in TomlProvider still
returns raw Long for integer TOML numbers causing inconsistent behavior with
decimal coercion; in TomlProvider.update the getAsFloatAndMark() implementation
should detect numeric values that are instances of Long (or integer types) and
convert them to Double (e.g., via Double.valueOf(((Long) value).doubleValue())
or creating a BFloat/BValue equivalent) before returning, mirroring the
BigDecimal coercion used in the decimal accessor (see
ValueCreator.createDecimalValue and getTomlConfigValue for reference); also add
a simple-provider regression test that loads a plain configurable float (e.g., x
= 12) to ensure it resolves as a float/double and include it alongside the new
TOML numeric tests.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ca3b43b-94bb-4de6-b298-3f4ee234318c
📒 Files selected for processing (11)
bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/configurable/providers/toml/ConfigValueCreator.javabvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/configurable/providers/toml/TomlProvider.javabvm/ballerina-runtime/src/test/java/io/ballerina/runtime/test/config/ConfigTest.javabvm/ballerina-runtime/src/test/java/io/ballerina/runtime/test/config/TomlProviderTest.javabvm/ballerina-runtime/src/test/resources/config_files/ArrayConfig.tomlbvm/ballerina-runtime/src/test/resources/config_files/MapTypeConfig.tomlbvm/ballerina-runtime/src/test/resources/config_files/SimpleTypesConfig.tomltests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/Config_records.tomltests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/Config_records_inline.tomltests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/configRecordType/records.baltests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/configRecordType/types.bal
✅ Files skipped from review due to trivial changes (4)
- bvm/ballerina-runtime/src/test/resources/config_files/ArrayConfig.toml
- bvm/ballerina-runtime/src/test/resources/config_files/SimpleTypesConfig.toml
- tests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/Config_records_inline.toml
- bvm/ballerina-runtime/src/test/resources/config_files/MapTypeConfig.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/Config_records.toml
- tests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/configRecordType/records.bal
- tests/jballerina-integration-test/src/test/resources/configurables/configStructuredTypesProject/configRecordType/types.bal
Comment on lines
+243
to
+249
| BigDecimal decimalValue; | ||
| if (value instanceof Long) { | ||
| decimalValue = BigDecimal.valueOf((Long) value); | ||
| } else { | ||
| decimalValue = new BigDecimal(value.toString()); | ||
| } | ||
| return getTomlConfigValue(ValueCreator.createDecimalValue(decimalValue), key); |
There was a problem hiding this comment.
Simple configurable float still misses the same int→float coercion.
This fixes the decimal accessor, but the sibling getAsFloatAndMark() path still returns the raw TOML number. Since integer TOML literals already pass float type validation, a plain configurable float with x = 12 will still resolve as Long instead of Double, so the behavior is now inconsistent between simple and structured configurables. Please normalize that path too and add a simple-provider regression case alongside the new TOML numeric tests.
Suggested follow-up
`@Override`
public Optional<ConfigValue> getAsFloatAndMark(Module module, VariableKey key) {
Object value = getPrimitiveTomlValue(module, key);
if (value == null) {
return Optional.empty();
}
- return getTomlConfigValue(value, key);
+ return getTomlConfigValue(((Number) value).doubleValue(), key);
}🤖 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
`@bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/configurable/providers/toml/TomlProvider.java`
around lines 243 - 249, The getAsFloatAndMark() path in TomlProvider still
returns raw Long for integer TOML numbers causing inconsistent behavior with
decimal coercion; in TomlProvider.update the getAsFloatAndMark() implementation
should detect numeric values that are instances of Long (or integer types) and
convert them to Double (e.g., via Double.valueOf(((Long) value).doubleValue())
or creating a BFloat/BValue equivalent) before returning, mirroring the
BigDecimal coercion used in the decimal accessor (see
ValueCreator.createDecimalValue and getTomlConfigValue for reference); also add
a simple-provider regression test that loads a plain configurable float (e.g., x
= 12) to ensure it resolves as a float/double and include it alongside the new
TOML numeric tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Fixes #44565
Approach
Samples
Remarks
Check List
This pull request updates TOML configuration parsing to accept integer numeric literals for fields declared as float in structured configurable variables and to improve decimal handling, preventing runtime type violations and increasing configuration flexibility.
Problem Addressed
Structured configurable record fields typed as float were rejected at runtime when TOML configuration provided integer literals (e.g., price = 12), causing an InherentTypeViolation.
Solution Implemented
Impact
Configurations can now use integer literals for float-typed fields and provide numeric values for decimal-typed fields without causing runtime type errors, reducing configuration failures and improving usability of structured configurables.