Added BaseEppoClient.getTypedAssignmentResult + nullability #149
Conversation
hbordersTwitch
force-pushed
the
hbordersTwitch/valued_flag_evaluation_result
branch
2 times, most recently
from
b297bd6 to
39d894e
Compare
| @NotNull String flagKey, | ||
| @NotNull String subjectKey, | ||
| @NotNull Attributes subjectAttributes, | ||
| @NotNull EppoValue defaultValue, |
There was a problem hiding this comment.
I couldn't actually find a requirement that defaultValue be @NotNull, but it makes a lot of things a lot simpler if it is, and I suspect that EppoValue.nullValue() wouldn't make sense if defaultValue could be null.
| this.getTypedAssignment( | ||
| flagKey, | ||
| subjectKey, | ||
| subjectAttributes, | ||
| EppoValue.valueOf(defaultValue.toString()), | ||
| VariationType.JSON); | ||
| return parseJsonString(value.stringValue()); | ||
| @Nullable final JsonNode jsonValue = parseJsonString(value.stringValue()); | ||
| return jsonValue != null ? jsonValue : defaultValue; |
There was a problem hiding this comment.
This is actually a behavior change because previously it would return null, but returning defaultValue makes more sense based on the behavior of the rest of the SDK.
| } | ||
|
|
||
| @NotNull | ||
| public FlagConfig getFlag() { | ||
| return flag; |
There was a problem hiding this comment.
Having access to the flag is helpful so I can know the index of the current variant. I need to know the variant index because Crashlytics has limited property space, so we pack all feature flags into a indexed string with one character per feature flag.
| } else { | ||
| @NotNull final String stringValue = notNullBase64Decode(variation.getValue().stringValue()); | ||
| @Nullable final VariationType variationType = flag.getVariationType(); | ||
| if (variationType == null) { |
There was a problem hiding this comment.
This was a bug. variationType could be null, which can cause a NPE when evaluating the switch statement.
| new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'", Locale.US); | ||
| dateFormat.setTimeZone(java.util.TimeZone.getTimeZone("UTC")); | ||
| return dateFormat; | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| public static void throwIfEmptyOrNull(String input, String errorMessage) { | ||
| @NotNull | ||
| public static String throwIfEmptyOrNull(@Nullable String input, @NotNull String errorMessage) { |
There was a problem hiding this comment.
It's helpful to return the value in case we use this inside of a chained this() constructor call where this() has to be the first instance method call.
| throw new UnsupportedOperationException( | ||
| "Cannot stringify Eppo Value type " + this.type.name()); | ||
| } | ||
| return type.toString(boolValue, doubleValue, stringValue, stringArrayValue); |
There was a problem hiding this comment.
Moving this into type removed the default case.
| @@ -136,21 +197,4 @@ public boolean equals(Object otherObject) { | |||
| public int hashCode() { | |||
| return Objects.hash(type, boolValue, doubleValue, stringValue, stringArrayValue); | |||
| } | |||
|
|
|||
| /** This method is to allow for Android 21 support; String.join was introduced in API 26 */ | |||
| private static String joinStringArray(List<String> stringArray) { | |||
There was a problem hiding this comment.
Moved this into EppoValueType
| }, | ||
| ; | ||
|
|
||
| public abstract boolean isNull(); |
There was a problem hiding this comment.
Making these abstract lets us remove the default case from EppoValue.toString().
| public static VariationType fromString(String value) { | ||
| for (VariationType type : VariationType.values()) { | ||
| @Nullable | ||
| public static VariationType fromString(@NotNull String value) { |
There was a problem hiding this comment.
This was a source of a bug. Code assumed this was method returned not-null, but it can return null
| String key = jsonNode.get("key").asText(); | ||
| @NotNull | ||
| private FlagConfig deserializeFlag(@NotNull JsonNode jsonNode) { | ||
| @NotNull final String key = jsonNode.get("key").asText(); |
There was a problem hiding this comment.
Since jsonNode.asText() returns an empty string:
Method that will return a valid String representation of the container value, if the node is a value node (method isValueNode returns true), otherwise empty String.
we can assume that every call with .asText() is @NotNull
Added minimum nullability necessary to add required fields. Also added `sortedVariationKeys` to `FlagConfig` to make finding the variation index cheaper
hbordersTwitch
force-pushed
the
hbordersTwitch/valued_flag_evaluation_result
branch
from
39d894e to
295d752
Compare
|
Does it help if I separate the nullability from the |
Eppo Internal:
🎟️ Fixes issue
📜 Design Doc: link if applicable
Motivation and Context
When evaluating a feature flag, I want to know if the evaluation failed and why. We report the state of all of our feature flags into Crashlytics as we evaluate them, and if a feature flag failed to load, we want to update its state accordingly.
Description
Added minimum nullability necessary to add required fields. Also added
sortedVariationKeystoFlagConfigto make finding the variation index cheaper.If you strongly oppose the nullability annotations, I can remove them, but it made reasoning about the code much simpler, and I found a couple of missing null checks because of them.
How has this been documented?
Added documentation to new classes.
How has this been tested?
Added unit tests. Not sure how to unit test no allocations inside
BaseEppoClientTest