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

Not sending FailedNotif if identifier not specified. #19

Open nathany opened 9 years ago

nathany commented 9 years ago

This relates to #10, as I try to build a synchronous API around Send and FailedNotif.

To get an error from Apple, I am changing a single digit in an otherwise valid device token. I've added logging to timehop/apns.

On this push (with invalid data) nothing comes back from Apple:

2014/12/17 11:32:26 Apple gateway at gateway.sandbox.push.apple.com:2195
2014/12/17 11:32:28 pushing payload...
2014/12/17 11:32:28 json: {"aps":{"alert":{"body":"hello robots"}}}
2014/12/17 11:32:29 Timed out waiting for errors (FailedNotifs). Must be good.

That's not great, but okay. On this push, errrors are coming back from Apple, but the identifiers don't match up so FailedNotifs are never sent.

2014/12/17 11:32:31 pushing payload...
2014/12/17 11:32:31 json: {"aps":{"alert":{"body":"hello robots"}}}
2014/12/17 11:32:31 handleError n.Identifier=0 err.Identifier=2 err=&apns.Error{Command:0x8, Status:0x8, Identifier:0x2, ErrStr:"Invalid token"}
2014/12/17 11:32:31 handleError n.Identifier=0 err.Identifier=2 err=&apns.Error{Command:0x8, Status:0x8, Identifier:0x2, ErrStr:"Invalid token"}
2014/12/17 11:32:32 Timed out waiting for errors (FailedNotifs). Must be good.

If Identifier is set, then I do see an identifier match and the error is sent via FailedNotif:

2014/12/17 11:35:02 pushing payload...
2014/12/17 11:35:02 json: {"aps":{"alert":{"body":"hello robots"}}}
2014/12/17 11:35:02 handleError n.Identifier=1234 err.Identifier=1234 err=&apns.Error{Command:0x8, Status:0x8, Identifier:0x4d2, ErrStr:"Invalid token"}
2014/12/17 11:35:02 reportFailedPush apns.Notification{ID:"user:timestamp", DeviceToken:"30c4afb...", Identifier:0x4d2, Expiration:(*time.Time)(0xc2080bc018), Priority:10, Payload:(*apns.Payload)(0xc2082488c0)} true
2014/12/17 11:35:02 Notif user:timestamp failed with {8 8 1234 Invalid token}
taylortrimble commented 9 years ago

Gross. I expect this to get cleared up with #17 (synchronous Send, no requeueing?), but it's good to keep an eye on it.

nathany commented 9 years ago

I'd be happy just to remove the "set an identifier if not set" logic and always require it from the caller.

nathany commented 9 years ago

I still think removing determineIdentifier entirely would be the best bet. Just require the identifier from the client.

taylortrimble commented 9 years ago

I do like the idea of "transparent defaults", but I do see your point as well. I wouldn't be heartbroken either way. :wink:

nathany commented 9 years ago

When replaying notifications, should it use the same IDs or call determineIdentifier for new ones?