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

Alert is serialized to JSON even if all fields are blank. #5

Closed
taylortrimble opened this issue Oct 24, 2014 · 5 comments · Fixed by #14
Closed

Alert is serialized to JSON even if all fields are blank. #5

taylortrimble opened this issue Oct 24, 2014 · 5 comments · Fixed by #14

Comments

@taylortrimble
Copy link
Contributor

I think it's because it's a struct. One thought is to make it a pointer, but that adds some complexity to the user.

If we wanted to avoid complexity for the user, we could do our own serialization entirely to make sure the alert isn't serialized unless it needs to be. We could probably do that as a method on the Alert itself, which would be neat. That adds complexity to the lib though.

@nathany
Copy link
Contributor

nathany commented Nov 26, 2014

The use case for this is for Passbook and Newstand notifications. Timehop APNS currently sends:

{"aps":{"alert":{}}}

{"aps":{"alert":{},"content-available":1}}

Whereas Grocer doesn't include "Alert":

{"aps": {}}

{"aps": {"content-available":1}}

It looks like omitting the Alert is the intended behaviour:

Alert            Alert  `json:"alert,omitempty"`

But apparently omitempty doesn't do this for structs.
https://github.com/timehop/apns/blob/master/notification.go#L49

@nathany
Copy link
Contributor

nathany commented Nov 26, 2014

mgo/bson does an IsZero check on all the fields of a struct to decide whether to omit it:
https://github.com/go-mgo/mgo/blob/v2/bson/encode.go#L164

Making Alert into a pointer would be more efficient. NewPayload could allocate Alert, so it would need to be explicitly set to nil by the caller to skip it.

An alternative would be to have different payloads/notification types for different things (like Grocer), such as a PassbookNotification with no payload.

Before going down either of these roads, has anyone confirmed that Apple doesn't accept {"aps":{"alert":{}}} for passbook notifications?

@taylortrimble
Copy link
Contributor Author

Regarding your suggestions: I think it would be awesome if we could avoid reflection, and definitely better if we could avoid application-presumptive code. For example, my use case for empty alerts has nothing to do with passbook, but I still don't want an alert.

Apple does accept empty alert structs in my use case. However, the payload size is highly restricted at 255 bytes, and "alert":{} takes up ~4% of the payload. Not really a big deal to me, just waste I noted.

I'd advocate for some custom implementation of MarshalJSON that can do non-reflective checks, if someone does care to implement this.

@nathany
Copy link
Contributor

nathany commented Nov 26, 2014

I need to verify if this is necessary for the Passbook case before making any change. It might not be a big deal for us either (I don't think we have the same 255 byte limit here, not since the v2 api).

I'm also trying to find out how to fix the tests #8 before proceeding.

@nathany
Copy link
Contributor

nathany commented Dec 10, 2014

I think we can do this without reflection. I'll give it a shot.

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

Successfully merging a pull request may close this issue.

2 participants