ENG-1566
Now a warning should be logged if a notification exceeds size limit, even after omitting messageInfos.
Details
Test on both Android and IOS. Change apnMaxNotificationPayloadByteSize/ fcmMaxNotificationPayloadByteSize to a small number and send a notification to the app. You should see a warning in the logs. Revert the changed values and try to send a notification again, no warning should be logged.
Diff Detail
- Repository
- rCOMM Comm
- Branch
- eng1566
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
keyserver/src/push/send.js | ||
---|---|---|
513 | If this condition is true then the second is bound to be false. Therefore it would be logical to make the second check somehow dependent on the first. We can either return notifications in this if statement or use else if for the second. |
keyserver/src/push/send.js | ||
---|---|---|
513 | I would like to change it to a return to keep it consistent with the android function. Is there any reason why we assign to .messageInfos and then return it outside the if statement, instead of just returning copyWithMessageInfos? |
keyserver/src/push/send.js | ||
---|---|---|
513 | Yes - it is a safety measure. copyEithMessageInfos is not just JSON object - it is apn.Notification instance. Calling length() on apn.Notification instance compiles it - makes is immutable. Even if we assign it some properties those changes will not be reflected in plaintext data that will be sent as notification. We decided not to return copyEithMessageInfos since we cannot guarantee that it does not change further when being send as notification. |
Added explicit return in response to review. Also based on the notes about .length() which makes the notification immutable, added a defensive _cloneDeep before checking the notification size without the messageInfos.
Based on the source code of the node-apn library that we are using, calling .length() is equivalent to calling Buffer.byteLength(JSON.stringify(notif), notif.encoding || "utf8") which is how we check android's notifications sizes currently. We could potentially get rid of .length() and use byteLength directly but then we would be depending on an implementation detail. (regardless this should probably be a separate diff/task)
I am accepting.this revision since it implements functionality expected by ENG-1566 in a way that does not conflict with our previous work (copying message and calling length() on a copy). Nevertheless I can see that we are deliberately sending a notifications that will not be delivered. Are we ok with this in long term? Usually we are very careful when it comes to iOS notifications. Perhaps we could modify this function to return undefined in such a case so that apnPush utility method can choose not to send this notification? Or send it to blob service? Those two are of course beyond the scope of this differential and are rather a material for Linear task. @ashoat what do you think?
Notification id doesn't sound like a private thing, so maybe it is a good idea to log it in the message.
keyserver/src/push/send.js | ||
---|---|---|
517–518 ↗ | (On Diff #17141) | At this point it is a good idea to introduce a helper method to compute the length so that we avoid repeated logic. The method would take a notification, create its copy and return the length. It might be a new diff before this one, or could be a followup task. |
keyserver/src/push/send.js | ||
---|---|---|
517–518 ↗ | (On Diff #17141) | But in the first case, we modify the copy before computing length. Would you prefer that we set the .messageInfos, and check the length, then if it's too big we remove the messageInfos from the payload and check the length again? |
Just to make sure: @ashoat can we safely log a notification id?
keyserver/src/push/send.js | ||
---|---|---|
517–518 ↗ | (On Diff #17141) | Good point. Let's keep it as is, but please add a comment explaining why we're doing this. This is important, because in the future someone might want to simplify the code and that could introduce a bug. |
Just to make sure: @ashoat can we safely log a notification id?
Yes, from keyserver code this is okay. We have much higher standards for services though
It might be a good idea to have some guidelines for what can be logged where. I created a task to track this https://linear.app/comm/issue/ENG-1926/create-a-doc-describing-our-logging-policy.