This differential removes messageInfos fields from iOS notification if it is too large. Size is compared to thresholds from apple/google docs
Details
Details
Build iOS app without changes, open web app and send message large enough that notification is not generated. The introduce changes in the mobile app and send the same message. Ensure notification appears
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-1323
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
I may have not enough context about the notifications (maybe it would be good to add linear task reference in diff description), so adding @tomek as blocking reviewer, but I have some comments and questions below:
First, it's good to always add related revisions when some changes depend on other diffs.
I see, that the diff uses variables from D4600, but in D4600 also validateAndroidNotificationByteSize is defined - was it just moved, or is it duplicated? Maybe push/utils.js is a better place for these functions?
I'm not sure if validate... is a good name for the function that mutates an object. Maybe something more like validateAndReduceExceededNotificationSize... would better describe what it does?
What happens if notifications after removing messageInfos still exceeds limits? Shouldn't the size be checked again somewhere after reducing notification and such situation be handled somehow? Or is it impossible to happen?
Comment Actions
I see, that the diff uses variables from D4600, but in D4600 also validateAndroidNotificationByteSize is defined - was it just moved, or is it duplicated? Maybe push/utils.js is a better place for these functions?
This is definitely a mistake of mine. I was also thinking about introducing those functions in utils.js and initially placed them there. mBut when submitting a differential I decided to move those functions to send.js. It is definitely my oversight.
I decided not to place those functions in utils.js since they rely on internal structure of notification object. This structure is know only from send.js file. If we decide to modify notification structure somehow in send.js we would need to modify probably utils.js as well. This increases coupling so I thought it is better to implement functions that modify notification structure in the very same file it is defined. I will update D4600 not to include redundant definition.
I'm not sure if validate... is a good name for the function that mutates an object. Maybe something more like validateAndReduceExceededNotificationSize... would better describe what it does?
What about normalizeNotificationByteSize? Comparably informative but less verbose.
What happens if notifications after removing messageInfos still exceeds limits? Shouldn't the size be checked again somewhere after reducing notification and such situation be handled somehow? Or is it impossible to happen?
In my opinion it highly unlikely that we exceed 4000 characters with just data we send apart from messageInfos. If it happens notification will simply not get delivered. The purpose of this diff stack is to be able to deliver notification even if actual message content needs to be downloaded from the server.
| keyserver/src/push/send.js | ||
|---|---|---|
| 193–203 ↗ | (On Diff #14804) | We already have a single point for each notification - prepareXNotification. Maybe we should introduce this logic there? It's more maintainable, because we wouldn't need to remember that normalize... function has to be called after every prepare... call. Regarding the name, normalize is better than validate, but maybe we can have e.g. ensureNotificationSizeIsNotExceeded? This function could take and return trimmed notification and avoid mutating its parameter. It is always better to avoid the mutation, because functions that are pure are easier to reason about. |
Comment Actions
- Please set the stack correctly so we can see where these variables come from!
- Agree that we should do this in prepare*Notification to decide whether to include messageInfos, rather than deciding later and using delete
- Have we checked if the libraries we use for APNs and FCM can help us with this calculation? As @jacek suggested this would perhaps make it possible to check for the global payload size (instead of just messageInfos)
Comment Actions
Ok, I will make those changes once I am back on Monday. I created spearate functions since I thought we might want to have functionality to prepare full (not trimmed) notifications. But I agree that prepareXNotification is alse reasonable place.
- Have we checked if the libraries we use for APNs and FCM can help us with this calculation?
I did the research. On Android we use just Object type for notification so there is probably no other way to calculate its JSON byte size. On iOS we use apn.Notification. It provides length() function. However this function is implemented in such a way that it firstly calls compile() method which sets property compiled once and forever. compiled property of apn.Notification is then used for sending. I tested it by replacing my Buffer.byteLength() with just notification.length(). Large enough notification was not sent. https://github.com/node-apn/node-apn/blob/38a357ed0c153aad09c2857e48a710527e685bfc/lib/notification/index.js#L73
As @jacek suggested this would perhaps make it possible to check for the global payload size (instead of just messageInfos)
Those functions calculate global payload size.
Comment Actions
It seems reasonable to use notification.length()... if we need to unset compiled flag, maybe instead we can just clone the compiled notification into a new one that isn't compiled? Not sure if that's possible
Those functions calculate global payload size.
Oops! Yeah you're right. I'm still not convinced we're calculating the size correctly, though...
| keyserver/src/push/send.js | ||
|---|---|---|
| 515 ↗ | (On Diff #14804) | I think APNs payload limit doesn't apply to the whole notification, since some of it is set as HTTP headers. Even if the size limit includes HTTP headers, you are likely estimating it wrong by using JSON.stringify. Does APNs even use JSON.stringify for the payload? |
| 587 ↗ | (On Diff #14804) | Is JSON.stringify really what FCM uses? Even if it is, it seems like there are some quirks, eg. "FCM adds the prefix gcm.notification. for every key in notification payload." from here. |
This comment was removed by marcin.
Comment Actions
It seems reasonable to use notification.length()... if we need to unset compiled flag, maybe instead we can just clone the compiled notification into a new one that isn't compiled? Not sure if that's possible
compiled is not a flag. It is stringified JSON representation of the notification that actually gets sent. When we call length() on apn.Notification instance it firstly computes compiled attribute and then calls exactly Buffer.byteLength(...) to calculate and return its length. If we call length() on notification any change we further introduce ( delete notification.payload.messageInfos) will not be reflected in the JSON that is sent to the device. If we want to use length() function for iOS notification we must create new notification instance or modify compiled property. Modifying compiled property sound easiest for now, but I am afraid that future releases of this library might rename this field to something else since is is not expected to be accessed directly by developers. Additionally we know that compile() is called further by node-apn library, so by calling length() we might compile too early. So I think the best is to create deep-copy of notification for length calculation purpose
Comment Actions
Use length() method to calculate iOS notification size. Remove calculations for Android since it might not give accurate results
| keyserver/src/push/send.js | ||
|---|---|---|
| 508 ↗ | (On Diff #14837) | Should we use structuedClone() instead? Link to docs, I learned about it from this StackOverflow answer |
| 508 ↗ | (On Diff #14837) | Are we definitely sure that notification.length() is the right thing to be comparing? I want to make sure that it corresponds to the payload size that Apple talks about being a limit. I'm not sure if it includes the length of HTTP headers or not. Can you clarify the details of what notification.length() counts and what it doesn't count, and also the details of how Apple applies the limit? |
| 509 ↗ | (On Diff #14837) | I thought the plan was to move this to prepareIOSNotification so we could avoid the delete. Has this plan changed because of the need for a clone? |
| keyserver/src/push/send.js | ||
|---|---|---|
| 509 ↗ | (On Diff #14837) | Actually I do no understand why moving this operation to prepareIOSNotification implies that we do not use delete. Why is it an issue that we use delete here? |
| keyserver/src/push/send.js | ||
|---|---|---|
| 508 ↗ | (On Diff #14837) | I understand that apple explicitly states that limit applies to JSON notification payload (not including headers) https://developer.apple.com/documentation/usernotifications/setting_up_a_remote_notification_server/generating_a_remote_notification?language=objc. What about length() function? It includes only aps dictionary and our custom key (payload): https://github.com/node-apn/node-apn/blob/38a357ed0c153aad09c2857e48a710527e685bfc/lib/notification/index.js#L97. It does not include headers for calculation. they are calculated separately in a different function: https://github.com/node-apn/node-apn/blob/38a357ed0c153aad09c2857e48a710527e685bfc/lib/notification/index.js#L38 Therefore I think length() function will give correct value. |
| 508 ↗ | (On Diff #14837) | This function work in browser. When I tried to use it inside keyserver code I received 'not found' error. What is the case against _cloneDeep from 'lodash`? |
| keyserver/src/push/send.js | ||
|---|---|---|
| 508 ↗ | (On Diff #14837) | RE notification.length() – great!! RE structuredClone(), the reason I prefer it to Lodash is that it is a native API. I think it is generally better to use native APIs than Lodash. That said, it seems like the native API is not supported on our version of Node... so it makes sense to use _cloneDeep. |
| 509 ↗ | (On Diff #14837) | Instead of creating a payload and then removing part of it, I think it would be better if we cloned the notification without messageInfos, then add messageInfos to the clone and check its length. That way we could avoid delete. I think it's better to avoid delete because it's better to simply not add something than to add and then delete it. It's not a big deal, but I think is more clear. |
Comment Actions
Add to copy to check size and then add to original instead of removing from original.
Comment Actions
We agreed not to use structuredClone since it is not present in Node environment. It can only be accessed from browser.
| keyserver/src/push/send.js | ||
|---|---|---|
| 509–511 ↗ | (On Diff #14919) | Do we really need _merge here? If we can find a way to do it without Lodash, that would be best. If we really need _merge, we could consider setting up a _flow, eg.: const copyWithMessageInfos =_flow( _cloneDeep, _merge({ payload: { messageInfos }}), )(notification); |
| keyserver/src/push/send.js | ||
|---|---|---|
| 509–511 ↗ | (On Diff #14919) | We do not need merge. We can use basic assignments: const copyWithMessageInfos = _cloneDeep(notification); copyWithMessageInfos.payload = { ...copyWithMessageInfos.payload, messageInfos, }; (code above is already formatted)
I can probably assume that the version above is preferred, right? |
Comment Actions
Yes, the modern view is that it's best to avoid Lodash. Here is an article with some context.