-
Notifications
You must be signed in to change notification settings - Fork 31
fix!: Correctly type user and group properties #312
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
Conversation
99f940f to
dd70e0e
Compare
posthog/src/main/java/com/posthog/internal/PostHogLocalEvaluationModels.kt
Outdated
Show resolved
Hide resolved
| groups: Map<String, String>?, | ||
| personProperties: Map<String, String>?, | ||
| groupProperties: Map<String, String>?, | ||
| personProperties: Map<String, Any?>?, |
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.
will this break the public API if someone declare and pass a Map<String, String>? should we raise a major?
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.
Yeah, this is technically a major version bump in both packages. Hopefully this should be the last for a while.
| groups: Map<String, String>?, | ||
| personProperties: Map<String, String>?, | ||
| groupProperties: Map<String, String>?, | ||
| personProperties: Map<String, Any?>?, |
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 prob should normalize all params with Any since non-serialized values can break serialization (confirm if GSON does this automatically?)
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.
GSON should be able to do this automatically but I'll include a test to verify
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.
Does this address your concerns? I'd started looking at normalizing into primitive JSON types, but it seems GSON's default behavior may suffice.
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.
probably yes, we do this on iOS https://github.com/PostHog/posthog-ios/blob/ba5dd65d2341ab8ea1949f072594ef67c490af3a/PostHog/Utils/DictUtils.swift#L10
if GSON behaves the same way, we are good.
It's important that GSON either stringifies any non-serializable object or just does not set it, rather than failing the serialization.
I'd suggest using a different type of object, which is not serializable eg a Context, Activity, View classes in Android, I suspect it might break the serialization but i am not 100% sure
696b74d to
3bb5b82
Compare
3bb5b82 to
886d3a2
Compare
This allows properties to be safely deserialized without raising exceptions. If an exception is raised while deserializing a property, the property will simply be ignored and an error logged.
Instructions and example for changelogPlease add an entry to the appropriate changelog:
Make sure the entry includes this PR's number. ## Next
- Correctly type user and group properties ([#312](https://github.com/PostHog/posthog-android/pull/312))If none of the above apply, you can opt out of this check by adding |
💡 Motivation and Context
groupPropertiesis typed incorrectly in many cases.This PR changes many instances of
groupProperties: Map<String, String>togroupProperties: Map<String, Map<String, Any?>>Similarly, many instances of
userPropertieshave changed fromMap<String, String>toMap<String, Any?>to match other SDKs.This changes the shape of interfaces both in core and server-side libraries. This is a major version bump for both.
Resolves #310
💚 How did you test it?
Existing tests are updated
📝 Checklist