Page MenuHomePhabricator

[native] Warn if notif is too big after omiting messageInfo
ClosedPublic

Authored by michal on Sep 26 2022, 9:50 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 2, 7:26 AM
Unknown Object (File)
Sat, Mar 2, 7:26 AM
Unknown Object (File)
Sat, Mar 2, 7:26 AM
Unknown Object (File)
Sat, Mar 2, 7:26 AM
Unknown Object (File)
Sat, Mar 2, 7:26 AM
Unknown Object (File)
Sat, Mar 2, 7:26 AM
Unknown Object (File)
Sat, Mar 2, 7:26 AM
Unknown Object (File)
Sat, Mar 2, 7:26 AM
Subscribers

Details

Summary

ENG-1566
Now a warning should be logged if a notification exceeds size limit, even after omitting messageInfos.

Test Plan

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul added 1 blocking reviewer(s): tomek.
atul added inline comments.
keyserver/src/push/send.js
518 ↗(On Diff #17084)

super minor nit: can we change IOS to iOS

518 ↗(On Diff #17084)

Can we use ' (straight single quotes) instead of ` (backticks)?

578 ↗(On Diff #17084)

Can we use ' (straight single quotes) instead of ` (backticks)?

marcin requested changes to this revision.Sep 27 2022, 6:52 AM
marcin added inline comments.
keyserver/src/push/send.js
513 ↗(On Diff #17093)

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.

This revision now requires changes to proceed.Sep 27 2022, 6:52 AM
keyserver/src/push/send.js
513 ↗(On Diff #17093)

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 ↗(On Diff #17093)

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)

@marcin please add me as a reviewer after accepting

This revision is now accepted and ready to land.Sep 29 2022, 12:51 AM
This revision now requires review to proceed.Sep 29 2022, 12:51 AM

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.

This revision is now accepted and ready to land.Sep 29 2022, 1:59 AM
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?

Added notification id to the logs on android and ios.

tomek added a reviewer: ashoat.
In D5229#154805, @tomek wrote:

Notification id doesn't sound like a private thing, so maybe it is a good idea to log it in the message.

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

This revision is now accepted and ready to land.Sep 29 2022, 10:41 AM

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.

This revision now requires review to proceed.Sep 30 2022, 3:34 AM

Added comment

Looks great!

This revision is now accepted and ready to land.Sep 30 2022, 6:16 AM