Open gemmakbarlow opened 3 years ago
bsneed
commented
3 years ago That crash is a message from the OS. I suspect you're trying to store a trait that can't actually be serialized into JSON or a PList. Please reopen if needed, but be sure to provide a crash report/stack trace so we can dig further in if necessary.
bsneed
commented
3 years ago Sorry, i missed that line about "null" there. Lemme look at that and see ...
bsneed
commented
3 years ago Yup, that was it. Doh! At least we have a test for it now I guess. Lemme see what a fix might look like.
gemmakbarlow
commented
3 years ago Cheers. FWIW I've done a bit more digging and found we're providing the failing value as a custom-wrapped optional value to the traits: dictionary on SEGAnalytics.identify(::) which I'm honestly surprised the compiler is currently allowing.
We'll definitely stop doing that ASAP, but yep, thinking perhaps there's more validation checking that could be put in here somewhere.
bsneed
commented
3 years ago @gemmakbarlow ah, interesting find there!
I'm kind of perplexed how to best address this. We use JSON as storage everywhere except tvOS, which ends up being a plist. I can check upstream to insure a proper error message comes out, but it doesn't address the fact of a customer not testing that bit and assuming they can do null there. :/. I could just silently strip the null out. What are your thoughts?
gemmakbarlow
commented
3 years ago We use JSON as storage everywhere except tvOS, which ends up being a plist.
Aha, that explains why were were only encountering things on tvOS ✅
I can check upstream to insure a proper error message comes out, but it doesn't address the fact of a customer not testing that bit and assuming they can do null there. :/. I could just silently strip the null out. What are your thoughts?
If stripping the null is going to prevent a crash in Production, I think that's ideal. IMO it'd be perfectly reasonable to shout loudly in the console about the issue (if possible?) and the onus to be on the client-side engineer to figure out why a snippet of their event-data is missing, but crashes lead to a world of pain, especially for this type of stuff that only tends to show up in Prod.
Pretty late in the day for me, so will ponder other ideas overnight also.
gemmakbarlow
commented
3 years ago We use JSON as storage everywhere except tvOS, which ends up being a plist.
Aha, that explains why were were only encountering things on tvOS ✅
I can check upstream to insure a proper error message comes out, but it doesn't address the fact of a customer not testing that bit and assuming they can do null there. :/. I could just silently strip the null out. What are your thoughts?
If stripping the null is going to prevent a crash in Production, I think that's ideal. IMO it'd be perfectly reasonable to shout loudly in the console about the issue (if possible?) and the onus to be on the client-side engineer to figure out why a snippet of their event-data is missing, but crashes lead to a world of pain, especially for this type of stuff that only tends to show up in Prod.
Pretty late in the day for me, so will ponder other ideas overnight also.
Confirming that I haven't come up with a better idea here, but happy to help with reviews etc if useful / you do head down this path.
After instrumenting a new event on SDK version 4.1.4, we're seeing the following in our recent crashes -
NSInvalidArgumentException: Attempt to insert non-property list object { ... } for key SEGTraits"<null>", which we believe to be the issue.That said, it seems unexpected that the SDK would fail by crashing in this scenario; is this a known issue fixed in a newer version, or current expected behavior ? (Or somewhere in-between 😄 ?)