-
Notifications
You must be signed in to change notification settings - Fork 49
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
Sanitize dictionaries before serialization #82
Conversation
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.
Some thoughts on how we should best handle this, thinking ahead to what the feedback will be if someone gets into this situation.
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.
This looks great functionally but feels like something that 100% needs tests
What does this PR do?
Sanitize dictionaries before serialization
Where should the reviewer start?
How should this be manually tested?
Any background context you want to provide?
I've tried multiple approaches for proper sanitization but all of them have edge cases or get overly complex.
Similar thoughts as here
posthog-ios/PostHog/Models/PostHogEvent.swift
Lines 33 to 34 in 19064a4
Codable
types only would be awesome but the interoperability with ObjC is suboptimal.Using Generics would be ideal as well but again bad interoperability with ObjC.
I guess we can improve this in the future if we see it but I'd not write tons of code right now, the older version relies on
JSONSerialization
as well.I checked https://github.com/SwiftyJSON/SwiftyJSON/blob/master/Source/SwiftyJSON/SwiftyJSON.swift
It just fails if there's a nonserializable type.
Also, https://swiftunboxed.com/stdlib/json-decoder-decodable/ but generics are not well played.
Tried to implement my own version of
AnyCodable
as well but entered the rabbit whole of https://github.com/asensei/AnyCodable/tree/masterJSONSerialization.isValidJSONObject
always requires an Object or Array, so I cannot just use it for each value of the dict.What are the relevant tickets?
Screenshots or screencasts (if UI/UX change)
Questions: