fix(memprovider): use int64 type in IntEvaluation to match API#464
Conversation
Summary of ChangesHello @leoromanovsky, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical type mismatch bug within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a type mismatch issue in IntEvaluation by ensuring int64 is used consistently, which aligns with the OpenFeature API specification. The changes are logical and the updated tests properly verify the fix. I've included a couple of suggestions to enhance code readability by replacing magic numbers with constants from the math package.
The IntEvaluation method was using genericResolve[int] which caused
TYPE_MISMATCH errors when users stored int64 values in Variants.
The FeatureProvider interface defines IntEvaluation with int64:
IntEvaluation(ctx, flag string, defaultValue int64, ...) IntResolutionDetail
The type assertion in genericResolve should use int64 to match.
Before this fix:
- User stores: Variants{"max": int64(100)}
- genericResolve[int] tries: value.(int)
- Type assertion fails because int64 != int
- Returns default value with TYPE_MISMATCH error
After this fix:
- User stores: Variants{"max": int64(100)}
- genericResolve[int64] tries: value.(int64)
- Type assertion succeeds
- Returns correct value
Updated tests to use explicit int64 types in Variants to match the
expected usage pattern for the IntEvaluation API.
Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com>
793a5c1 to
4ec983d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #464 +/- ##
==========================================
+ Coverage 83.11% 83.23% +0.11%
==========================================
Files 27 27
Lines 2097 2111 +14
==========================================
+ Hits 1743 1757 +14
Misses 305 305
Partials 49 49
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com>
erka
left a comment
There was a problem hiding this comment.
these changes doesn't resolve the issue, now int has the same problem.
|
I would suggest to modify // genericResolve is a helper to extract type verified evaluation and fill openfeature.ProviderResolutionDetail
func genericResolve[T comparable](value any, defaultValue T, detail *openfeature.ProviderResolutionDetail) T {
switch v := value.(type) {
case int8:
value = int64(v)
case int16:
value = int64(v)
case int32:
value = int64(v)
case int:
value = int64(v)
case float32:
value = float64(v)
}
v, ok := value.(T)
if ok {
return v
}
detail.Reason = openfeature.ErrorReason
detail.ResolutionError = openfeature.NewTypeMismatchResolutionError("incorrect type association")
return defaultValue
}We need to have tests for Please revert the changes to the |
Address review feedback by making genericResolve more forgiving.
Instead of requiring explicit int64 values, the function now coerces
smaller integer types (int, int8, int16, int32) to int64 and float32
to float64.
This allows users to write natural Go code without explicit casts:
Variants: map[string]any{"value": 42} // works now
Instead of requiring:
Variants: map[string]any{"value": int64(42)}
Added test coverage for both int and int64 variants.
Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com>
Add test coverage for all coerced integer types (int8, int16, int32, int) in addition to int64. Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com>
Revert the int64 cast in testprovider_test.go since genericResolve now coerces int to int64 automatically. This avoids breaking changes for existing users. Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com>
Thanks for the thoughtful feedback; I understand your perspective. |
…riants Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
|
@leoromanovsky thank you |
fix(memprovider): add type conversion for int8/int16/int32/float32 va…
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
|
@sahidvelji are you still okay with this PR? |
|
Do we need to account for unsigned integers also? It won't be possible to coerce a uint64 to int64 safely. Maybe we could document that only signed ints are supported? What do you think? |
|
To be honest, I don’t have a solid answer. I agree that adding that only signed ints are supported to the docs is the best option right now. @sahidvelji |
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Motivation
When using the InMemoryProvider for testing with integer flags, users encounter unexpected TYPE_MISMATCH errors even when storing values as int64:
This makes the InMemoryProvider unusable for integer flag testing.
Root Cause
The IntEvaluation method calls genericResolve[int] (line 98), but the FeatureProvider interface specifies int64:
The genericResolve helper performs a type assertion value.(T). When T is int but the stored value is int64, the assertion fails because int64 != int in Go's type system, even on 64-bit platforms.
Compare with FloatEvaluation which correctly uses genericResolve[float64].
Changes
Design Decision
This fix requires users to explicitly type integer variants as int64:
This is consistent with:
An alternative would be to handle both int and int64 in genericResolve, but this would add complexity and deviate from the pattern used by other evaluation methods.