-
Couldn't load subscription status.
- Fork 58
fix: flagsmith string to double conversion issue (#1557) #1617
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -180,19 +180,42 @@ private <T> ProviderEvaluation<T> buildEvaluation( | |
| * @param expectedType the type we expect for this value | ||
| * @param <T> the type we want to convert to | ||
| * @return A converted object | ||
| * @throws TypeMismatchError if the value cannot be converted to the expected type | ||
| */ | ||
| @SuppressWarnings("unchecked") | ||
| private <T> T convertValue(Object value, Class<?> expectedType) { | ||
| boolean isPrimitive = expectedType == Boolean.class | ||
| || expectedType == String.class | ||
| || expectedType == Integer.class | ||
| || expectedType == Double.class; | ||
| T flagValue = isPrimitive ? (T) value : (T) objectToValue(value); | ||
|
|
||
| if (flagValue.getClass() != expectedType) { | ||
| throw new TypeMismatchError( | ||
| "Flag value had an unexpected type " + flagValue.getClass() + ", expected " + expectedType + "."); | ||
| Object flagValue; | ||
| if (isPrimitive) { | ||
| if (expectedType == Double.class) { | ||
| if (value instanceof Double) { | ||
| flagValue = value; | ||
| } else if (value instanceof String) { | ||
| try { | ||
| flagValue = Double.parseDouble((String) value); | ||
| } catch (NumberFormatException e) { | ||
| throw new TypeMismatchError("Flag value string could not be parsed as Double: " + value); | ||
| } | ||
| } else { | ||
| throw new TypeMismatchError("Flag value had an unexpected type " | ||
| + (value != null ? value.getClass() : "null") + ", expected " + expectedType + "."); | ||
| } | ||
| } else { | ||
| flagValue = value; | ||
| } | ||
| } else { | ||
| flagValue = objectToValue(value); | ||
| } | ||
|
|
||
| if (!expectedType.isInstance(flagValue)) { | ||
| throw new TypeMismatchError("Flag value had an unexpected type " | ||
| + (flagValue != null ? flagValue.getClass() : "null" + ", expected " + expectedType + ".")); | ||
|
Comment on lines
+215
to
+216
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a minor bug in the construction of the error message here due to misplaced parentheses. When throw new TypeMismatchError("Flag value had an unexpected type "
+ (flagValue != null ? flagValue.getClass() : "null") + ", expected " + expectedType + "."); |
||
| } | ||
| return flagValue; | ||
| return (T) flagValue; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -159,6 +159,24 @@ void tearDown() throws IOException { | |||||||||||||||||||||||||||||||||||
| mockFlagsmithErrorServer.shutdown(); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| @ParameterizedTest | ||||||||||||||||||||||||||||||||||||
| @MethodSource("convertValueArguments") | ||||||||||||||||||||||||||||||||||||
| void testConvertValue(Object input, Class<?> expectedType, Object expected) throws Exception { | ||||||||||||||||||||||||||||||||||||
| var method = flagsmithProvider.getClass().getDeclaredMethod("convertValue", Object.class, Class.class); | ||||||||||||||||||||||||||||||||||||
| method.setAccessible(true); | ||||||||||||||||||||||||||||||||||||
| Object result = method.invoke(flagsmithProvider, input, expectedType); | ||||||||||||||||||||||||||||||||||||
| assertEquals(expected, result); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private static Stream<Arguments> convertValueArguments() { | ||||||||||||||||||||||||||||||||||||
| return Stream.of( | ||||||||||||||||||||||||||||||||||||
| Arguments.of(true, Boolean.class, true), | ||||||||||||||||||||||||||||||||||||
| Arguments.of("test", String.class, "test"), | ||||||||||||||||||||||||||||||||||||
| Arguments.of(123, Integer.class, 123), | ||||||||||||||||||||||||||||||||||||
| Arguments.of(3.14, Double.class, 3.14), | ||||||||||||||||||||||||||||||||||||
| Arguments.of("3.14", Double.class, 3.14)); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+171
to
+178
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To accompany the suggested change for integer conversion in
Suggested change
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| @Test | ||||||||||||||||||||||||||||||||||||
| void shouldInitializeProviderWhenAllOptionsSet() { | ||||||||||||||||||||||||||||||||||||
| HashMap<String, String> headers = new HashMap<String, String>() { | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good fix for handling string-to-double conversion. To make the provider more robust and consistent, consider applying the same logic for string-to-integer conversion. Flagsmith might also return integer values as strings, and handling this case would be a great improvement.