timehop / apns

A Go package to interface with the Apple Push Notification Service
https://godoc.org/github.com/timehop/apns
MIT License
185 stars 47 forks source link

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

Closed taylortrimble closed 9 years ago

taylortrimble commented 10 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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