This differential removes messageInfos fields from Android notification if it is too large. Size is compared to thresholds from google docs
Details
Details
Build Android app, launch Logcat in Android studio. Kill the app. Send short message from web client. Ensure notification is displayed and "db path:" count in logs increases (messageInfos is stored in SQLite). Send very large notification, ensure notification is displayed but "db path:" count in logs does not increase (messageInfos field is not present so nothing is stored in the database)
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-1323
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| keyserver/src/push/send.js | ||
|---|---|---|
| 577 ↗ | (On Diff #15006) | This JSON.parse does not seem necessary since we define custom_notification above. Can we avoid it somehow? To be honest, codeVersion < 31 is ancient... we could probably ignore that case... Separately, we might consider bumping the version in verifyClientSupported to something higher. Then we could delete all the code that handles old versions. |
| 589 ↗ | (On Diff #15006) | Still wondering about this question from my last review:
|
| keyserver/src/push/send.js | ||
|---|---|---|
| 541 ↗ | (On Diff #14968) | Actually you are right, direct assignment to notification.data is fine |
| 595 ↗ | (On Diff #14968) | I am sure what do you mean in this question:
What do you mean by asking this question? Are our libraries converting the object we get from prepareAndroidNotification to the instance of some class or is FCM converting this JSON to something else for the purpose of sending through the wire? |
Comment Actions
I was just looking to confirm that FCM really just takes notification.data and converts it to JSON. It seems like you've confirmed that, so we're all good!
(The reason I am surprised is that JSON is not really an efficient way to pack data. But probably FCM does some compression after converting to JSON, and there is probably some good reason for the JSON conversion.)
Comment Actions
One experiment we can do is to verify that a notification with fcmMaxNotificationPayloadByteSize bytes can be sent, but the one with fcmMaxNotificationPayloadByteSize + 1 can't.
| keyserver/src/push/send.js | ||
|---|---|---|
| 571 ↗ | (On Diff #15006) | At this point the notification doesn't contain message infos, so it's a misleading name |
| 577 ↗ | (On Diff #15006) | Agree, we should either create custom_notification as an object and then stringify it to put into notification.data and use here, or drop the support for this version. |
| 589 ↗ | (On Diff #15006) | Do we know which encoding is used by FCM to send this string? If it's something different than UTF-8 we would need to provide an additional parameter |
Comment Actions
I will prepare and share results of such experiment
| keyserver/src/push/send.js | ||
|---|---|---|
| 589 ↗ | (On Diff #15006) | I was not able to find straightforward answer but found two articles:
|
Comment Actions
Drop support for custom notification structure for codeVersion 31.
Moving this into separate diff would make it a lot easier to review this diff.
| keyserver/src/push/send.js | ||
|---|---|---|
| 556–560 ↗ | (On Diff #15267) | Is there a good reason for using cloneDeep now? Would the code from suggestion work correctly? |
Comment Actions
I did the experiment and below I am sharing my results:
- On debug build I was able to send up to 4169 bytes from web client to android emulator client.
- On release build I was able to send up to 4155 bytes from web to android emulator
- On release build I was able to send up to 4158 bytes from web to android device
All above were same messages between same users (except the first case). Therefore me and @tomek came to conclusion that probably Google guarantees delivery up to 4000 bytes but there is some buffer space left that enables sending larger messages but its precise size is not documented. We agreed sticking to 4000 bytes from docs is a wise option.
Comment Actions
Close but the comment needs to be revised now
| keyserver/src/push/send.js | ||
|---|---|---|
| 543–547 | This comment is no longer accurate and is now confusing after being moved | |
| keyserver/src/push/send.js | ||
|---|---|---|
| 543–547 | In my opinion it is not inaccurate. We dropped support for code version below 32, but we still support anything between and 69. Previously this comment was explaining why return notification without badgeOnly and notifID if badgeOnly && codeVersion < 69). Now being in this place it explains why we need the opposite !badgeOnly || codeVersion >= 69 to be satisfied to include badgeOnly and notifID in notification payload. If I were to make decision I would keep this comment here, but I understand it might be confusing from a short glimpse, so I can refactor the code so that the condition in the if is the same as in previous version. Then there will be no place for confusion. | |
| keyserver/src/push/send.js | ||
|---|---|---|
| 543–547 | The part I thought was inaccurate is this:
Referring to "this payload" is inaccurate here because the payload below is not missing an ID. Instead, this is referring to the other payload (the one that is actually missing an ID) | |
| keyserver/src/push/send.js | ||
|---|---|---|
| 542–546 ↗ | (On Diff #15711) | This is better, but I still think this comment should be moved to line 554. It's not clear to me why it is here... we are talking about older clients, but the condition handles newer clients. @tomek pushed back when I suggested moving the comment... @tomek, if you feel strongly can you clarify why? Otherwise, let's just move it |
| keyserver/src/push/send.js | ||
|---|---|---|
| 548–552 ↗ | (On Diff #15786) | Sorry, I realized now that I was confusing myself... there is no else block here, so the new location of the comment is even more confusing. Let's do this: // The reason we only include `badgeOnly` for newer clients is because older // clients don't know how to parse it. The reason we only include `id` for // newer clients is that if the older clients see that field, they assume // the notif has a full payload, and then crash when trying to parse it. // By skipping `id` we allow old clients to still handle in-app notifs and // badge updating. if (!badgeOnly || codeVersion >= 69) { |