Skip to content
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

trackEvent does not handle null values in JSONObject #505

Closed
markcerqueira opened this issue Oct 11, 2017 · 6 comments
Closed

trackEvent does not handle null values in JSONObject #505

markcerqueira opened this issue Oct 11, 2017 · 6 comments

Comments

@markcerqueira
Copy link

markcerqueira commented Oct 11, 2017

If I call trackEvent in MixPanelAPI with a JSONObject containing a null value the method throws an exception. This is caught internally but the result is that the event is not logged to MixPanel.

Line in question is here. properties.get(key) will throw a JSONException if the value is null. The caller to this method could validate input but it seems preferable to handle the nulls and track the event instead of swallowing the exception and not logging the event.

E/MixpanelAPI: Exception tracking event test-event
  org.json.JSONException: No value for key_with_null_value
  at org.json.JSONObject.get(JSONObject.java:354)

Oddly enough, I can only make it throw an exception on devices running Android 4.3 (I used a HTC One). Anything higher than that works, which is perplexing but maybe something changed in implementations somewhere along the way.

Thank you!

@yinfeiru
Copy link
Contributor

@markcerqueira Hey, sorry for the delay!!

If I understand it correctly, what you want is to skip the properties with nulls in our library instead of filtering them out in the caller? That makes a lot of sense. I'll get it done when I get a chance!

@markcerqueira
Copy link
Author

@yinfeiru Yep! The current behavior throws an Exception and the entire event is not logged. Unsure if the best behavior is to skip the property altogether or place in a blank String. I'll defer to you all on that call.

Thanks and cheers! 😄

@yinfeiru
Copy link
Contributor

yinfeiru commented Nov 27, 2017

@markcerqueira To me, I feel like skipping the property is the way to go because on Mixpanel you can easily filter out the skipped properties (or the null properties) with is not set filter. But I'll discuss with other folks.

Cheers!

@yinfeiru
Copy link
Contributor

@markcerqueira Hey, sorry for the late. The fix will be available in the next release. PR is here.

@markcerqueira
Copy link
Author

@yinfeiru Awesome! Thank you!

@patedit
Copy link
Contributor

patedit commented Jan 4, 2018

This has been fixed in the latest release 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants