-
Notifications
You must be signed in to change notification settings - Fork 3
fix: serialize booleans as native JSON in precomputed request body #243
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
Changes from all commits
e04bd79
3b057a1
e5d0fb8
927430b
501238e
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 |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| package cloud.eppo.android.util; | ||
|
|
||
| import cloud.eppo.api.Attributes; | ||
| import cloud.eppo.api.EppoValue; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| /** | ||
| * Utility class for serializing subject and action attributes to the wire format expected by the | ||
| * precomputed flags API. | ||
| * | ||
| * <p>The API expects attributes to be separated into: | ||
| * | ||
| * <ul> | ||
| * <li>numericAttributes: numbers (integers and doubles) | ||
| * <li>categoricalAttributes: strings, booleans, and other non-numeric types | ||
| * </ul> | ||
| * | ||
| * <p>This matches the behavior of the JS SDK and ensures consistent wire format across all Eppo | ||
| * SDKs. | ||
| */ | ||
| public final class ContextAttributesSerializer { | ||
|
|
||
| private ContextAttributesSerializer() { | ||
| // Prevent instantiation | ||
| } | ||
|
|
||
| /** | ||
| * Serializes attributes into the context attributes format expected by the API. | ||
| * | ||
| * @param attributes The attributes to serialize (can be null) | ||
| * @return A map containing "numericAttributes" and "categoricalAttributes" keys | ||
| */ | ||
| public static Map<String, Object> serialize(Attributes attributes) { | ||
| Map<String, Object> result = new HashMap<>(); | ||
| Map<String, Number> numericAttrs = new HashMap<>(); | ||
| Map<String, Object> categoricalAttrs = new HashMap<>(); | ||
|
|
||
| if (attributes != null) { | ||
| for (String key : attributes.keySet()) { | ||
| EppoValue value = attributes.get(key); | ||
| if (value != null && !value.isNull()) { | ||
| if (value.isNumeric()) { | ||
|
Contributor
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. I wonder if the newly-added
Member
Author
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. Thanks, good suggestion, hadn't explore the common java repo.
Member
Author
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. It doesn't simplify the code for us because we need to know the variation type, would look like this: Is that what you were trying to simplify?
Member
Author
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. It would be more helpful to augment the Then the serializer becomes: |
||
| numericAttrs.put(key, value.doubleValue()); | ||
| } else if (value.isBoolean()) { | ||
| // Booleans should be serialized as native JSON booleans, not strings | ||
| categoricalAttrs.put(key, value.booleanValue()); | ||
| } else { | ||
| categoricalAttrs.put(key, value.stringValue()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| result.put("numericAttributes", numericAttrs); | ||
| result.put("categoricalAttributes", categoricalAttrs); | ||
| return result; | ||
| } | ||
| } | ||
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.
Should we be adding
nullas the value if it is null?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.
We explicitly skip null in swift and javascript so I was matching that behavior.