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

Notification.Tag probably shouldn't be serialized. #678

Closed
boazy opened this issue Apr 7, 2016 · 5 comments
Closed

Notification.Tag probably shouldn't be serialized. #678

boazy opened this issue Apr 7, 2016 · 5 comments

Comments

@boazy
Copy link

boazy commented Apr 7, 2016

Currently Notification.Tag on all the different platforms has no [JsonIgnore] attribute and it actually gets serialized and sent to the Push Notification Service. This leads to subtle bugs, such as when you have some non-serializable attributes which just make Json.Net crash. It can also leak out unintended data, since users don't expect it to be sent to APNS/GCM/etc.

Is there a reason why no [JsonIgnore] attribute was added to this field?

@dylanvdmerwe
Copy link
Contributor

This is actually a VERY good spot.

I actually am having an issue right now where if I provide a Tag, the GCMServiceBroker breaks and will not send messages or stop.

@Redth
Copy link
Owner

Redth commented Apr 7, 2016

This is a great catch. There is no need to serialize this property that I can think of. If it's something you absolutely want to do, you can encapsulate the notification class inside of your own object. I will be adding [JsonIgnore] to this property, thanks!

@dylanvdmerwe
Copy link
Contributor

Awesome @Redth. Was just about to work on a pull request for it. If possible can this please be released as 4.0.5?

@Redth
Copy link
Owner

Redth commented Apr 7, 2016

it will be in the very next release shortly.

And to clarify, really only GcmNotification needs this treatment, as I don't use the json serializer for the other platforms.

Eventually Json.Net may be removed as a dependency, but for now this is a good change.

@Redth Redth closed this as completed in 5abac61 Apr 7, 2016
@boazy
Copy link
Author

boazy commented Apr 7, 2016

Thanks! That was quick. :)

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