Repository navigation
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 7e5ab28 | Docs | View more details | Give us feedback! |
aarsilv
added
the
crucible: reviewing
Read any numeric flag as Int when it is a whole number within Int range. Return TYPE_MISMATCH for fractional or out-of-range values, keep PARSE_ERROR for values that can't be parsed, and keep Double reads. Ref: FFL-3565
3a64335 to
6c1181d
Compare
aarsilv
changed the title
Cap decimal parsing at 64 characters, add non-numeric, boundary and single-fit tests, and correct the README and KDoc. Unparseable values of every numeric type, including "float", return PARSE_ERROR. Ref: FFL-3565
Add 64- and 65-character boundary rows for the Int parse length limit, describe the limit precisely in the README and resolveIntValue KDoc, and reword the zero-scale comment to match observed Android behavior. Ref: FFL-3565
aarsilv
removed
the
crucible: reviewing
There was a problem hiding this comment.
🟡 Changes recommended
The boundary tests lack a regression case for long zero-padded integers, leaving the documented length-check exemption unprotected.
1 open issue
What changed in this PR
This PR makes numeric flag reads as Int lossless across "integer", "number" and "float" values, while leaving Double reads unchanged.
Changes:
- Accept whole numbers within
Intrange; distinguish type mismatches from parsing errors. - Bound decimal parsing to 64 characters, with an exception for plain integers within range.
- Document the behavior and extend conversion, logging and telemetry tests.
| File | Description |
|---|---|
reliability/single-fit/flags/src/test/kotlin/com/datadog/android/flags/integration/FlagsEvaluationTest.kt |
Adds cross-type numeric resolution checks. |
reliability/single-fit/flags/src/test/kotlin/com/datadog/android/flags/integration/EvaluationTelemetryTest.kt |
Checks numeric type-mismatch telemetry. |
features/dd-sdk-android-flags/src/test/kotlin/com/datadog/android/flags/internal/DatadogFlagsClientTest.kt |
Covers conversion boundaries, errors and warnings. |
features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/FlagValueConverter.kt |
Implements bounded, lossless numeric-to-Int conversion. |
features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/FlagsClient.kt |
Documents Int resolution and length limits. |
features/dd-sdk-android-flags/README.md |
Explains numeric reads and error classifications. |
detekt_custom_unsafe_calls.yml |
Registers the potentially throwing trailing-zero operation. |
detekt_custom_safe_calls_third_party.yml |
Registers safe numeric parsing and conversion calls. |
Review effort: Balanced
Add a 65-character zero-padded integer row so moving the length check before toIntOrNull() fails a test. Ref: FFL-3565
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
aarsilv
added
the
ready-for-human-review
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
No actionable conversion defect was reported, including in the intentional length-limit exemption for zero-padded plain integers. Gradle suites and Android runtime checks were not rerun.
🤖 Bits Code Review · Commit c90d1c3
| Double::class -> | ||
| variationType == VariationType.NUMBER.value || variationType == VariationType.FLOAT.value || | ||
| variationType == VariationType.INTEGER.value | ||
| Int::class, Double::class -> isNumeric(variationType) |
There was a problem hiding this comment.
🤖 Context from Claude: Int reads now accept all three numeric types. A "number" value can hold a whole or a fractional number. OpenFeature defines one Number type. So the served type no longer decides an Int read. The parsed value does: a whole value in Int range succeeds. Double reads already accepted all three types.
| Boolean::class -> variationValue.lowercase(Locale.US).toBooleanStrictOrNull() as? T | ||
| String::class -> variationValue as T | ||
| Int::class -> variationValue.toIntOrNull() as? T | ||
| Int::class -> parseLosslessInt(variationValue, variationType) as? T |
There was a problem hiding this comment.
🤖 Context from Claude: The Int conversion now runs inside runCatching. If the decimal path throws an unexpected exception, the client returns the default value with PARSE_ERROR. The exception does not reach the host app.
| * [MAX_DECIMAL_LENGTH] characters. | ||
| */ | ||
| private fun parseLosslessInt(variationValue: String, variationType: String): Int? = variationValue.toIntOrNull() | ||
| ?: variationValue.takeIf { it.length <= MAX_DECIMAL_LENGTH } |
There was a problem hiding this comment.
🤖 Context from Claude: Plain integers take the fast toIntOrNull() path. Other values longer than 64 characters skip the BigDecimal parse and report PARSE_ERROR. Without this limit, one long number string can take seconds on each read. On the JVM, a value of 1 followed by 200,000 zeros and .0 took about 10 s in stripTrailingZeros().
| } | ||
| if (problem != null) { | ||
| @Suppress("ThrowingInternalException") // Caught by runCatching in convert() | ||
| throw TypeMismatchException("Flag has type '$variationType' with $problem but Int was requested") |
There was a problem hiding this comment.
🤖 Context from Claude: Parsing succeeded before this branch. A fractional value, or a whole value outside Int range, cannot be an Int without loss. So it is a type mismatch, not a parse error. runCatching in convert() catches the exception. The client then reports TYPE_MISMATCH.
| * [BigDecimal.stripTrailingZeros]. | ||
| */ | ||
| private fun isFractional(decimal: BigDecimal): Boolean { | ||
| if (decimal.scale() <= 0 || decimal.signum() == 0) return false |
There was a problem hiding this comment.
🤖 Context from Claude: BigDecimal tells whole values from fractional ones exactly, with no rounding. The scale check comes first: stripTrailingZeros() throws ArithmeticException on scale overflow for input such as 100e2147483647. The zero check is for older Android libcore, which returns a zero such as 0.0 unchanged from stripTrailingZeros(). The detekt config now lists stripTrailingZeros() as a throwing call.
| "number, 5.00000000000000000000000000000000000000000000000000000000000000, 5", | ||
| "number, 00000000000000000000000000000000000000000000000000000000000000005, 5" | ||
| ) | ||
| fun `M return value W resolve() { numeric flag, whole value, Int default }`( |
There was a problem hiding this comment.
🤖 Context from Claude: These tests protect lossless Int conversion for all numeric flag types. After parsing succeeds, a whole value in Int range succeeds. A parsed fractional or out-of-range value returns TYPE_MISMATCH. A value that cannot be parsed returns PARSE_ERROR. If direct Int parsing fails, an input over 64 characters also returns PARSE_ERROR. The -2147483649 row catches a removed lower bound. The -2147483648.0 row catches <= instead of <. The 100e2147483647 row catches a removed scale check. The 67-character row catches a removed length limit.
| Arguments.of("enabled", "fallback", ErrorCode.TYPE_MISMATCH, optedOut), | ||
| Arguments.of("malformed", false, ErrorCode.PARSE_ERROR, optedOut) | ||
| Arguments.of("malformed", false, ErrorCode.PARSE_ERROR, optedOut), | ||
| Arguments.of("number", 0, ErrorCode.TYPE_MISMATCH, optedOut) |
There was a problem hiding this comment.
🤖 Context from Claude: This end-to-end row reads the fixture's "number" value 2.5 as Int through the SDK's JSON response handling. It checks the default value, TYPE_MISMATCH, the evaluation count and that the SDK sends no exposure. With the previous converter, the read reports PARSE_ERROR, so this row fails.
| val discountPercentage = client.resolveDoubleValue("discount-rate", 0.0) | ||
| ``` | ||
|
|
||
| Any numeric flag can be read with either method. `resolveIntValue` returns the value only when it is a whole number within `Int` range, such as `5` or `5.0`. Otherwise it returns the default value: `resolve()` reports `TYPE_MISMATCH` when the value is fractional, such as `1.2`, or outside `Int` range, such as `2147483648`, and `PARSE_ERROR` when the value can't be parsed. To bound parsing work, a value longer than 64 characters is not parsed unless it is a plain integer within `Int` range, so it reports `PARSE_ERROR`. |
There was a problem hiding this comment.
🤖 Context from Claude: This paragraph documents the Int-read and Double-read rules for app developers. The README definition of TYPE_MISMATCH, and the resolveIntValue and resolveDoubleValue KDoc, match it.
aarsilv
removed
the
ready-for-human-review
|
Question - is it possible to use |
| } | ||
| if (problem != null) { | ||
| @Suppress("ThrowingInternalException") // Caught by runCatching in convert() | ||
| throw TypeMismatchException("Flag has type '$variationType' with $problem but Int was requested") |
There was a problem hiding this comment.
worth to mark with method with @throws annotation to no forget to wrap this method in a future changes. Then suppressing wouldn't be needed
There was a problem hiding this comment.
🤖 Reply from Claude, working with Aaron:
Good idea. 7e5ab28 adds @Throws(TypeMismatchException::class) to parseLosslessInt, requireLosslessInt, parseLosslessDouble and requireLosslessDouble, as FlagsJson does.
The @Suppress has to stay. The ThrowingInternalException rule reports every throw in internal code and does not read @Throws (tools/detekt/src/main/kotlin/com/datadog/tools/detekt/rules/sdk/ThrowingInternalException.kt). We checked: with @Throws and without the suppression, detekt reports both throw lines.
Return TYPE_MISMATCH for a whole value at 2^53 or more that matches neither the Double nor its printed form. Return PARSE_ERROR for NaN, Infinity and text over 64 characters at that magnitude. Update docs. Ref: FFL-3565
aarsilv
changed the title
| * a value whose parsed Double is 2^53 or more in magnitude, when the value is longer than [MAX_DECIMAL_LENGTH] | ||
| * characters or [BigDecimal] can't parse it. | ||
| */ | ||
| private fun parseLosslessDouble(variationValue: String, variationType: String): Double? = |
There was a problem hiding this comment.
🤖 Context from Claude: This makes Double reads match the shared fixtures for large whole values. 9007199254740993 now gives TYPE_MISMATCH instead of a rounded value. Below 2^53 in magnitude, Double holds every whole value exactly, so the parsed value returns at once. At 2^53 or more, an input of at most 64 characters goes to BigDecimal. A longer input gives PARSE_ERROR. A whole value passes if it equals the Double or its printed form (see isExactDouble).
| } | ||
|
|
||
| @Suppress("UnsafeThirdPartyFunctionCall") // Safe: BigDecimal(Double) and valueOf() only run for a finite Double | ||
| private fun isExactDouble(decimal: BigDecimal, double: Double): Boolean = |
There was a problem hiding this comment.
🤖 Context from Claude: A whole value also passes when the Double prints as that value, such as 1e300. So a double written in its shortest printed form reads back as itself. Without this check, a test that reads random doubles fails.
| "integer, 9007199254740993", | ||
| "float, 9007199254740993" | ||
| ) | ||
| fun `M return default with TYPE_MISMATCH W resolve() { whole value Double can't hold, Double default }`( |
There was a problem hiding this comment.
🤖 Context from Claude: These rows detect regressions in Double rejection. They cover shared fixture cases and edge cases such as 1e400. All 8 rows fail without the exactness check.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61f82932ac
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Int::class -> variationValue.toIntOrNull() as? T | ||
| Double::class -> variationValue.toDoubleOrNull() as? T | ||
| Int::class -> parseLosslessInt(variationValue, variationType) as? T | ||
| Double::class -> parseLosslessDouble(variationValue, variationType) as? T |
There was a problem hiding this comment.
When an existing numeric flag contains a value such as 9007199254740993, this change makes a Double read return the application default with TYPE_MISMATCH; NaN, Infinity, and sufficiently long values similarly change to errors. The parent implementation returned every value accepted by toDoubleOrNull(), so routing Double reads through parseLosslessDouble() silently changes public resolution and exposure behavior even though this change is scoped to lossless Int reads. Keep the direct Double conversion here, or move this separately as an explicit behavior change.
AGENTS.md reference: AGENTS.md:L270-L273
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Reply from Claude, working with Aaron:
This change is intentional. The PR title and description now cover Double reads. The shared mobile fixtures in DataDog/ffe-system-test-data#65 expect TYPE_MISMATCH for a Double read of 9007199254740993, because the lossless rule does not allow a rounded result. The README and PR description document NaN, Infinity and the 64-character limit. The resolveDoubleValue KDoc describes the conversion rules and parse limits.
| val discountPercentage = client.resolveDoubleValue("discount-rate", 0.0) | ||
| ``` | ||
|
|
||
| Any numeric flag can be read with either method. `resolveIntValue` returns the value only when it is a whole number within `Int` range, such as `5` or `5.0`. Otherwise it returns the default value: `resolve()` reports `TYPE_MISMATCH` when the value is fractional, such as `1.2`, or outside `Int` range, such as `2147483648`, and `PARSE_ERROR` when the value can't be parsed. To bound parsing work, a value longer than 64 characters is not parsed unless it is a plain integer within `Int` range, so it reports `PARSE_ERROR`. `resolveDoubleValue` returns a fractional value as the nearest `Double`, such as `1.2`. It returns a whole value only when the `Double` has exactly that value, or prints as that value, such as `1e300`. Otherwise `resolve()` reports `TYPE_MISMATCH`, for example for `9007199254740993`. `NaN` and `Infinity` report `PARSE_ERROR`. So does a value longer than 64 characters whose `Double` is 2^53 or more in magnitude. |
There was a problem hiding this comment.
🤖 Context from Claude: This paragraph documents the Int-read and Double-read rules for app developers. The README definition of TYPE_MISMATCH, and the resolveIntValue and resolveDoubleValue KDoc, match it.
Co-authored-by: May Lee <may.lee@datadoghq.com>
Add @throws to the lossless Int and Double helpers, note why Int reads do not use intValueExact(), and fix two README slips from the applied suggestions. Ref: FFL-3565
|
🤖 Reply from Claude, working with Aaron:
It is possible, but it is slow on older Android, so this PR keeps the explicit checks.
On Android 9, |
What does this PR do?
This PR makes Int reads of numeric flags lossless, and checks whole values in Double reads (FFL-3565). The served numeric type (
"integer","number"or"float") does not change the result:TYPE_MISMATCHwhen the parsed value is fractional or out of Int range.PARSE_ERRORwhen the SDK cannot parse the value. The SDK does not parse a value longer than 64 characters unless it is a plain integer in Int range.Double, such as1.2.Doublehas exactly that value, or prints as that value, such as1e300. Otherwise it returnsTYPE_MISMATCH, for example for9007199254740993.PARSE_ERRORforNaNandInfinity, and for a value longer than 64 characters whoseDoubleis 2^53 or more in magnitude.Int reads, compared with
develop:develop:"integer","number"develop:"float"5TYPE_MISMATCH5.0,1e3PARSE_ERRORTYPE_MISMATCH1.2PARSE_ERRORTYPE_MISMATCHTYPE_MISMATCH2147483648PARSE_ERRORTYPE_MISMATCHTYPE_MISMATCHnot-a-numberPARSE_ERRORTYPE_MISMATCHPARSE_ERRORDouble reads, compared with
develop(all numeric types):develop1.2,5,9007199254740992,1e3009007199254740993,92233720368547758079.007199254740992E15TYPE_MISMATCH1e400InfinityTYPE_MISMATCHNaN,Infinity,1e2147483649NaNorInfinityPARSE_ERRORDoubleis 2^53 or more in magnitudeDoublePARSE_ERROREach error returns the default value.
Motivation
The OpenFeature spec defines a Number as "a numeric value of unspecified type or size". This PR uses lossless Int conversion and checks whole values in Double reads, for all three numeric types. Requirement 1.3.4 recommends the default value when a typed read gets a value of an unexpected type. The flagd numeric coercion ADR describes lossless numeric coercion, with a separate limit of 2^53−1. This PR does not add that limit.
Additional Notes
Release notes
resolve()and the OpenFeature provider do not logTYPE_MISMATCH. For parsed fractional or out-of-range"integer"or"number"values, these paths no longer log aPARSE_ERRORwarning.resolveIntValuelogs aTYPE_MISMATCHwarning."float"values that the SDK cannot parse now log aPARSE_ERRORwarning on all paths."integer"or"number"values, OpenFeature error hooks now getTYPE_MISMATCHinstead ofPARSE_ERROR."float"value in Int range now returns the value. SoresolveIntValueno longer logs aTYPE_MISMATCHwarning for it.TYPE_MISMATCHmessages include the value. Example:Flag has type 'number' with fractional value '1.2' but Int was requested.Doublenor its printed form, such as9007199254740993, now returns the default value withTYPE_MISMATCHinstead of a rounded value.NaNorInfinitynow returnPARSE_ERROR.1e400now returnsTYPE_MISMATCHinstead ofInfinity.resolveDoubleValuelogs aTYPE_MISMATCHwarning, and every read path logs aPARSE_ERRORwarning. Failed Double reads send no exposure or RUM evaluation events.BigDecimalrejects, such as one with surrounding spaces, now returnsPARSE_ERROR. This comes from reading the source; no test checks it.Implementation
parseLosslessInt()in therunCatchingblock ofconvert().BigDecimalfinds fractional values and checks the Int range. It does not round.isFractional()checks the scale before it callsstripTrailingZeros(). This prevents a scale overflow for values such as100e2147483647.toIntOrNull(). If that fails, a value longer than 64 characters returnsPARSE_ERROR. This limit bounds theBigDecimalwork.parseLosslessDouble(). Below 2^53 in magnitude, every whole value is exact, so the parsedDoublereturns with no other check. At 2^53 or more,BigDecimalchecks that a whole value equals theDoubleor its printed form (BigDecimal.valueOf). The 64-character limit applies there too.Documentation
FlagsClient.resolveIntValueandFlagsClient.resolveDoubleValueKDoc and the Flags README describe the Int-read and Double-read rules and the 64-character limit.TYPE_MISMATCHas a flag value type that does not match the expected type. Its example is a fractional value read withresolveIntValue.Validation
ktlintFormat,detektandlintReleasepass.testDebugUnitTest: 856 tests, 0 failures.testDebugUnitTest: 84 tests, 0 failures.:reliability:single-fit:flags:testReleaseUnitTest: 155 tests, 0 failures. The new single-fit checks fail with thedevelopconverter (3 failures).checkGeneratedFilespasses."string"as numeric, remove the length limit, the lower Int bound or the scale check, or move the length limit by one or two characters.develop. The fixtures omitextraLogging, which the Android model requires, so the test copy adds an empty object to each assignment. Of 95 runs, 88 pass, 3 are skipped and 4 fail. The 4 failures expectFLAG_NOT_FOUNDfor a flag with an unsupportedvariationType, which FFL-3568: Share precomputed fixtures and lossless numeric policy #4101 handles. Without the Double change, 12 more runs fail: the Double reads of whole values above 2^53.BigDecimal("0.0").stripTrailingZeros()returns scale 0 on API 28 (buildPSR1.210301.009.B1) and API 36 emulators. The AOSP libcore source for Android 6 to 9 returns a zero unchanged, with its scale, so the API 28 emulator differs from that source. API 23 to 27 were not checked at runtime, so the zero guard inisFractional()stays."number". Later changes do not affect these values.20011.2PARSE_ERRORTYPE_MISMATCH1.2Review checklist (to be filled by reviewers)